search

sebastianbergmann / comparator

Provides the functionality to compare PHP values for equality.
BSD 3-Clause "New" or "Revised" License
6.99k stars 68 forks source link

Honor $canonicalize parameter in `DOMNodeComparator::assertEquals` #105

Open Ste4890 opened 1 year ago

Ste4890 commented 1 year ago

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.