Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Honor $canonicalize parameter in DOMNodeComparator::assertEquals #105

Open
Ste4890 opened this issue Apr 3, 2023 · 0 comments
Open

Honor $canonicalize parameter in DOMNodeComparator::assertEquals #105

Ste4890 opened this issue Apr 3, 2023 · 0 comments

Comments

@Ste4890
Copy link

Ste4890 commented Apr 3, 2023

The private method DOMNodeComparator::nodeToText can receive a $canonicalize flag to avoid re-loading a node.
This function is only called twice in assertEquals, and the class is final, so no descendants could override this behavior.

In DOMNodeComparator::assertEquals, nodeToText is called with an hardcoded true argument, rather than making use of the $canonicalize parameter that could be passed to it.

Moreover, \SebastianBergmann\Comparator\DOMNodeComparatorTest::testAssertEqualsSucceeds only tests the method with the default false value for the parameter.

Is there a practical reason for this? I tried searching in other issues and in the git history for the assertEquals method, but I found nothing useful.

As per a possible solution, I think it would be rather straightforward to change this current code in DOMNodeComparator::assertEquals

  $expectedAsString = $this->nodeToText($expected, true, $ignoreCase);
  $actualAsString   = $this->nodeToText($actual, true, $ignoreCase);

with

  $expectedAsString = $this->nodeToText($expected, $canonicalize, $ignoreCase);
  $actualAsString   = $this->nodeToText($actual, $canonicalize, $ignoreCase);

A new test case or a modification for the old one would be needed.

I could open a pull request if this is deemed the right solution and there is a need for it; I am not sure if this is correct behavior for a design choice, though.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

1 participant