Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
[DomCrawler] Add a way to filter direct children#28221
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
[DomCrawler] Add a way to filter direct children#28221
Uh oh!
There was an error while loading.Please reload this page.
Conversation
0bc09b8 to8c9e3c7Compare
nicolas-grekas left a comment
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.
thanks, here are some comments.
| * @throws \RuntimeException if the CssSelector Component is not available and $selector is provided | ||
| */ | ||
| publicfunctionchildren() | ||
| publicfunctionchildren($selector =null) |
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.
Actually, this is a BC break: child classes overriding this method would break with this change.
What we do in these situations is fetching the argument using \func_get_arg(), see e.g. theFinder::sortByName() method.public function children(/* string $selector = null */) should be the new signature
| } | ||
| /** | ||
| * @param bool $html Whether HTML support should be enabled. Disable it for XML documents |
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.
this argument should be removed: read$this->isHtml instead.
| /** | ||
| * @param bool $html Whether HTML support should be enabled. Disable it for XML documents | ||
| * | ||
| * @return CssSelectorConverter A CssSelectorConverter instance |
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 be removed and replaced by a real return type on the method
| * | ||
| * @return CssSelectorConverter A CssSelectorConverter instance | ||
| * | ||
| * @throws \RuntimeException if the CssSelector Component is not available |
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.
uppercase "If" (same in other places)
einenlum commentedAug 19, 2018
Thank you very much for your feedbacks@nicolas-grekas :). |
nicolas-grekas left a comment
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.
(with minor comments)
| /** | ||
| * Returns the children nodes of the current selection. | ||
| * | ||
| * @param $selector string|null An optional CSS selector to filter children |
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.
swap needed:@param string|null $selector
| thrownew \InvalidArgumentException('The current node list is empty.'); | ||
| } | ||
| if ($selector) { |
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.
if (null !== $selector) {?
| } | ||
| /** | ||
| * @throws \RuntimeException if the CssSelector Component is not available |
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.
uppercaseIf
einenlum commentedAug 19, 2018
Thanks@nicolas-grekas :). Fixed. |
| $converter =$this->createCssSelectorConverter($this->isHtml); | ||
| $xpath =$converter->toXPath($selector,'child::*/'); | ||
| return$this->filterXPath($xpath); |
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.
this could be made much more efficient by usingchild:: as prefix when converting CSS and then usingfilterRelativeXPath instead of going throughrelativize to modify the XPath again
einenlum commentedAug 20, 2018
@stof Thank you for your feedback. I fixed it. |
| * @throws \RuntimeException If the CssSelector Component is not available and $selector is provided | ||
| */ | ||
| publicfunctionchildren() | ||
| publicfunctionchildren(/* string $selector = null */) |
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.
@nicolas-grekas shouldn't we also detect cases wheregetclass($this) !== __CLASS__ (plus the special handling of common mock libraries) to trigger a deprecation warning if they don't have the argument in the child class ? Otherwise, we don't have a continous migration path.
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.
Looking at the source, we forgot such deprecations in many places.
Maybe merge as is (the continuous upgrade path would be justnot adding this argument in v5)
and fix all the code base at once next?
6f303ec tof634afdComparenicolas-grekas commentedAug 24, 2018
Thank you@einenlum. |
…nlum)This PR was squashed before being merged into the 4.2-dev branch (closes#28221).Discussion----------[DomCrawler] Add a way to filter direct children| Q | A| ------------- | ---| Branch? | master| Bug fix? | no| New feature? | yes| BC breaks? | no| Deprecations? | no| Tests pass? | yes| Fixed tickets |#28171| License | MIT| Doc PR | -The Dom-Crawler component only has a `filter()` method (to filter the node and all its children) and a `children()` method to return direct children.**There is currently no way to easily filter (thanks to a selector) the direct children of a node, like jQuery allows so (with a selector passed to the `.children([selector])` method).****This PR adds a way to optionally filter direct children thanks to a CSS selector**. Here is an example of the usage:```php$html = <<<'HTML'<html> <body> <div> <p></p> <p></p> <div> <p></p> </div> </div> </body></html>HTML;$crawler = new Crawler($html);$foo = $crawler->filter('#foo');$foo->children() // will select `#p1`, `#p2` and `#nested`$foo->children('p') // will select `#p1` and `p2`$foo->children('.lorem') // will select `#p1` and `p2````This PR adds only an optional parameter and adds no BC break.Commits-------f634afd [DomCrawler] Add a way to filter direct children
einenlum commentedAug 24, 2018
Thank you very much@nicolas-grekas! :) |
javiereguiluz commentedSep 7, 2018
@einenlum thanks for this feature! We've createdsymfony/symfony-docs#10288 to not forget about documenting this new feature. It'd be great if you could provide the docs for it. If you need any help doing that, ask us in the Symfony Docs repository. Thanks! |
einenlum commentedSep 7, 2018
@javiereguiluz Thank you for your comment. I will create a PR on Symfony Docs in the following days, no problem! |
The Dom-Crawler component only has a
filter()method (to filter the node and all its children) and achildren()method to return direct children.There is currently no way to easily filter (thanks to a selector) the direct children of a node, like jQuery allows so (with a selector passed to the
.children([selector])method).This PR adds a way to optionally filter direct children thanks to a CSS selector. Here is an example of the usage:
This PR adds only an optional parameter and adds no BC break.