Skip to content

Conversation

@bor0
Copy link
Contributor

@bor0 bor0 commented Sep 3, 2014

https://bugs.php.net/bug.php?id=67948

Add handling for DOMNodeList parameter as well

@JanTvrdik
Copy link

👍

@datibbaw
Copy link
Contributor

datibbaw commented Sep 4, 2014

Nice work, you may wish to consider porting it to 5.6 as well :)

@bor0
Copy link
Contributor Author

bor0 commented Sep 4, 2014

Thanks. @datibbaw, I will create a PR for 5.6 once this gets approved (in case there are some remarks, so that I don't have to update both PRs).

@datibbaw
Copy link
Contributor

datibbaw commented Sep 4, 2014

Hang on, the expected result is that the parent tag is NOT included in the saved HTML, so it should yield:

<span>first</span>
<span>second</span>

@bor0
Copy link
Contributor Author

bor0 commented Sep 4, 2014

But it does include the parent node. $div->childNodes includes <div> as well. It just does not include the whole document (doc param is NULL)

@datibbaw
Copy link
Contributor

datibbaw commented Sep 4, 2014

Yeah, what I meant to say is that the parent node should NOT be there; if you look closely at the example given in the bug report you will see that the parent <div> is only included because it's a child of <body>.

@bor0
Copy link
Contributor Author

bor0 commented Sep 4, 2014

Yup, I figured that the example in the bug report could be wrong. They probably meant "div->childNodes" instead of "body->childNodes".

@datibbaw
Copy link
Contributor

datibbaw commented Sep 4, 2014

No, @DaveRandom meant $body->childNodes ... otherwise, what would be the difference between:

$dom->saveHTML($div->childNodes); // with your patch
$dom->saveHTML($div); // already possible

@DaveRandom
Copy link
Contributor

@datibbaw is correct, the point of this is to export only the nodes in the NodeList, in the order in which they appear.

Exporting the containing tag is already possible (as shown above) and, more importantly, not every NodeList has a single parent:

$html = <<<HTML
<header>
    <div>This is in the header</div>
</header>
<section>
    <div>This is some section content</div>
</section>
HTML;

$doc = new DOMDocument;
$doc->loadHTML($html);
$xpath = new DOMXPath($doc);
$nodes = $xpath->query('//div');
$data = $doc->saveHTML($nodes);
var_dump($data);
/* expected:
    <div>This is in the header</div>
    <div>This is some section content</div>
*/

@bor0
Copy link
Contributor Author

bor0 commented Sep 4, 2014

PR updated with the remarks! Thanks guys! Please re-check when you have the time.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The output of htmlNodeDump() should be checked at each iteration.

@datibbaw
Copy link
Contributor

datibbaw commented Sep 4, 2014

Looking better now; I'm just wondering whether we should extend this behaviour to ->saveXML() as well, since their method signatures are so closely related. @DaveRandom what do you reckon?

@DaveRandom
Copy link
Contributor

(As discussed in room 11 yesterday) I agree that saveXML() should exhibit the same signature, although I have not looked into how feasible this is - I imagine it's not far off copy/paste though.

https://bugs.php.net/bug.php?id=67948

Add handling for DOMNodeList parameter as well
@bor0
Copy link
Contributor Author

bor0 commented Sep 5, 2014

Updated PR with saveXML() change.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This code is nearly identical to the other one, except for htmlNodeDump() vs xmlNodeDump() ... I think we can carve out a static inline xmlBufferPtr *dumpNodeList(...) :)

@krakjoe
Copy link
Member

krakjoe commented Jan 5, 2017

Since this has merge conflicts, targets a security fix only branch, and since a patch against a supported branch would have to look different, I'm closing this PR.

Please take this action as encouragement to open a clean PR against a supported branch.

@krakjoe krakjoe closed this Jan 5, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants