fixed DOMNode comparator for canonical comparison

[julianseeger]

7dadedf introduced with Release 4.0.18 a bug when comparing two DOMNodes.

Previously, the comparison was always canonical (thanks to C14N), but now it no longer is. I would call that a BC break. This PR is basically a revert of that commit to make the DOMNode comparator canonical again. The test should make the problem pretty clear:

    public function testAssertEqualsNormalizesAttributes()
    {
        $comparator = new PHPUnit_Framework_Comparator_DOMNode();

        $document1 = new DOMDocument();
        $document1->loadXML("<a x='' a=''/>");

        $document2 = new DOMDocument();
        $document2->loadXML("<a a='' x=''/>");

        $comparator->assertEquals($document1, $document2);    // this fails since 4.0.18
    }

Realworld example: paratestphp/paratest#90

[whatthejeff]

I wasn't sure about 7dadedf either, @sebastianbergmann. Maybe it makes sense to look at the $canonicalize argument?

[sebastianbergmann]

I agree with @whatthejeff that the $canonicalize should be taken into account.

The problem I wanted to fix with 7dadedf was that the comparison could detect an inequality but then the diff would show no differences.

[julianseeger]

I wasn't sure about 7dadedf either, @sebastianbergmann. Maybe it makes sense to look at the $canonicalize argument?

I decided to keep the change as small as possible. Actually it makes sense for the method signature but from an XML point of view, it's problematic. $canonicalize defaults to false what makes sense for most stuff like arrays. But the default usecase for XML is probably the $canonical=true. If we would take $canonical into account, we had to pull this argument down to all methods like Assert::assertXmlStringEqualsXmlString, Assert::assertXmlStringNotEqualsXmlString etc. They would all need an additional argument "$canonical".

Also a key point is the backwards compatibility. Up to 4.0.17, the default behavior for the DOMNode Comparator was $canonical=true. But to keep up with the interface, the default behavior is $canonical=false. And at least the docblock only mentions arrays. So even if we would adjust every assertion method that results in DOMNode::assertEquals, we would brake other projects that depend on that.

I think there are 2 ways to see a fix that uses the $canonical correctly: either it's a bugfix that the $canonical was ignored before (like someone had opened an issue "$canonical argument is ignored") or it introduces new behavior when used the "old" way (BC break).

The problem I wanted to fix with 7dadedf was that the comparison could detect an inequality but then the diff would show no differences.

Can you give an example for that? Canonicalization of equal xml should keep it equal...

[sebastianbergmann]

Can you please have a look at 5934d89?

[julianseeger]

Maybe you could add the unittests. Then we would know if it produces the same result.
Otherwise I can only test it tonight (in about 7h).

[sebastianbergmann]

Yes, I will add the unit tests to the Comparator component.

[julianseeger]

Well, you could have copied my test and execute it. Thats what I would have done to "have a look at [that commit]"