-
Notifications
You must be signed in to change notification settings - Fork 7.7k
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
Add support for replacing class synopses based on stubs #7340
Conversation
I've just submitted php/doc-en#839 for reference as well |
d44992f
to
abc313c
Compare
cf39c05
to
02da8a9
Compare
a3720a6
to
4354a32
Compare
$docComparator->loadXML($xml1); | ||
$xml1 = $docComparator->saveXML(); | ||
|
||
$originalSynopsis->parentNode->replaceChild($newSynopsis, $originalSynopsis); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this be in here? This looks unrelated to the comparison.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, even though it makes the function have a side-effect, it's still absolutely required (I tried to clone $originalSynopsis
before in order to prevent this, but it didn't work) :/
The reason why we need this code is that it attaches the $newSynopsis
element to $doc
by replacing $originalSynopsis
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hm, would using DOMDocument::importNode work? Maybe it's possible to directly import it to $docComparator even?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've just tried this:
$docComparator = new DOMDocument();
$docComparator->preserveWhiteSpace = false;
$docComparator->formatOutput = true;
$docComparator->loadXML('<root></root>');
$originalSynopsis = $docComparator->importNode($originalSynopsis, true);
$docComparator->documentElement->appendChild($originalSynopsis);
$xml1 = $docComparator->saveXML($originalSynopsis);
$newSynopsis = $docComparator->importNode($newSynopsis, true);
$docComparator->documentElement->appendChild($newSynopsis);
$newSynopsis->setAttribute("xmlns", "http://docbook.org/ns/docbook");
$xml2 = $docComparator->saveXML($newSynopsis);
$xml2 = getReplacedSynopsisXml($xml2);
return $xml1 === $xml2;
But it still doesn't disregard whitespace changes - I guess because $originalSynopsis
keeps the whitespaces. As far as I see that's why we have to first convert the 2 DOMElement
s to strings, and then load them to the comparator.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay, let's stick with what you have then. Maybe rename the function to make it clear that it also modifies, something like replaceAndCompareXml()
? Can't think of a great name...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
replaceAndCompareXml()
is not that bad of a name - I don't have any better suggestion.
$docComparator->loadXML($xml1); | ||
$xml1 = $docComparator->saveXML(); | ||
|
||
$originalSynopsis->parentNode->replaceChild($newSynopsis, $originalSynopsis); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay, let's stick with what you have then. Maybe rename the function to make it clear that it also modifies, something like replaceAndCompareXml()
? Can't think of a great name...
No description provided.