DOMNodeComparator::nodeToText Refactoring

[Ovsyanka]

1. I removed

   $document->formatOutput = true; $document->normalizeDocument();

Because the unit-tests not failed after that and I don't see why it needed. 2. I removed a lot of checks instanceof DOMDocument because in fact there is always DOMDocument in the $node variable in case if canonicalize is true (and it is always true). 3. I removed $canonicalize because in fact it is always true and the unused argument just make the code maintenance harder. Especially without corresponding unit-tests.

[codecov-io]

Codecov Report

Merging #56 into master will increase coverage by 0.48%. The diff coverage is 22.22%.

@@             Coverage Diff              @@
##             master      #56      +/-   ##
============================================
+ Coverage     79.14%   79.62%   +0.48%     
+ Complexity      122      120       -2     
============================================
  Files            15       15              
  Lines           326      324       -2     
============================================
  Hits            258      258              
+ Misses           68       66       -2

Impacted Files	Coverage Δ	Complexity Δ
src/DOMNodeComparator.php	63.63% <22.22%> (+5.3%)	8 <3> (-2)	:arrow_down:

---

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 48a0188...133b441. Read the comment docs.

[sebastianbergmann]

$canonicalize is an optional parameter of PHPUnit\Framework\TestCase::assertEquals() (see https://github.com/sebastianbergmann/phpunit/blob/master/src/Framework/Assert.php#L546). From there, it is passed to PHPUnit\Framework\Constraint\IsEqual (see https://github.com/sebastianbergmann/phpunit/blob/master/src/Framework/Constraint/IsEqual.php#L56). Finally, it is passed to the Comparator object in https://github.com/sebastianbergmann/phpunit/blob/master/src/Framework/Constraint/IsEqual.php#L102.

SebastianBergmann\Comparator\DOMNodeComparator::assertEquals() should pass $canonicalize on to SebastianBergmann\Comparator\DOMNodeComparator::nodeToText() -- but it does not. So, indeed, inside that private method it is always true.

I think that $canonicalize should be passed to nodeToText(). I did not remember the details about why it is not passed on until I did some digging in the Git history:

https://github.com/sebastianbergmann/phpunit/pull/1236#issuecomment-41763665

At least I agree with myself from four years ago :-)

I think the right thing to do here is to release a new major version of this component (that passes $canonicalize to nodeToText()) alongside PHPUnit 8.0. This will be backward compatibility break.

Maybe feeec2d001bd16dc199183fc54b80c1aeba57c54 needs to be reverted until that time, too.

[sebastianbergmann]

$document->formatOutput = true; and $document->normalizeDocument(); are needed to make the output in case of a failure readable.

[Ovsyanka]

I investigated that history too, but decided to not interfere with that at this point.

I think that $canonicalize should be passed to nodeToText()

I believe I agree with you that it is point. But I think it is a good idea to aplly that refactoring untill we solve the issue whith $canonicalize passing in the next major version. Because with that refactoring it will be simpler to solve issue #21 and maybe some other if they would appear.

Besides that I think that it is complex question how the DOMComparator should act with different arguments and needs some discussions or, at leat, better documentaion. I haven't good understanding about intent of $canonicalize=false and $ignoreCase=true at this moment. I believe that $canonicalize=true should made nesessary changes in the xml to make it comparable as xml structure and it is the only thing I expect from DOMNodeComparator. But what should $canonicalize=false do?

And I don't see the intent of $ignoreCase=true. It is contradictory with the $canonicalize=true because <A /> and <a /> will be considered as equal but in terms of xml structure it is not true.

$document->formatOutput = true; and $document->normalizeDocument(); are needed to make the output in case of a failure readable.

Could you give some examples? I am sure that there should be tests for that and I could write some, but I didn't get the issue yet.

[Ovsyanka]

Maybe feeec2d needs to be reverted until that time, too.

Personally I am not sure what I think about that. The error was implemented in the c42cbbec and I don't know how much users relay on the new behavior. In my case I updated from the PHPUnit5 to the PHPUnit6 and instantly faced with that issue (uexpected case conversion) and came here. I saw existed PR #53 about that and decided to wait.

So, my point is - maybe it is a good idea to leave this PR #53 merged because that was obviously an error and in case you didn't get issues about that - the most of users just didn't notice that error and they will not notice the fix, but it is better to have fixed code.

On other hand, if it is like this from start of the current major version - maybe you shouldn't change it. If someone was hurted about that - he wrote the workaround I believe. I did =)

$this->assertEquals($expectedDom, $xmlDom, "", 0.0, 10, true, true);