Skip to content
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

DomCrawler parents only on first node of selection #18723

Closed
flip111 opened this issue May 7, 2016 · 11 comments
Closed

DomCrawler parents only on first node of selection #18723

flip111 opened this issue May 7, 2016 · 11 comments

Comments

@flip111
Copy link
Contributor

flip111 commented May 7, 2016

The DomCrawler seems to take the first node and traverse up the tree until the root node. But i think that the selection can have different parents, when i saw the function description i would expect it gets the parent of each node in the selection. So with 5 nodes selected i would get 5 parents, of which some could be the same parent (an array of 5 references of which some references to the same object). My suggestion would be to introduce another method with the functionality just described. About naming the methods i'm not sure yet.'

/**
* Returns the parents nodes of the current selection.
*
* @return Crawler A Crawler instance with the parents nodes of the current selection
*
* @throws \InvalidArgumentException When current node is empty
*/
public function parents()
{
if (!$this->nodes) {
throw new \InvalidArgumentException('The current node list is empty.');
}
$node = $this->getNode(0);
$nodes = array();
while ($node = $node->parentNode) {
if (XML_ELEMENT_NODE === $node->nodeType) {
$nodes[] = $node;
}
}
return $this->createSubCrawler($nodes);
}

@xabbuh
Copy link
Member

xabbuh commented May 19, 2016

All other methods only work on the first node of the current selection. Adding a method that would work with all nodes from the selection might be confusing.

So I am 👎 on this suggestion.

@flip111
Copy link
Contributor Author

flip111 commented May 24, 2016

@xabbuh i did a quick count, seems that 13 of the 53 methods have getNode(0) in them. Also the responsibility or "category" of the methods seem to be varying quite a bit. Maybe better look at the use of the method then worrying about it being confusing related to other methods? That's what the API doc is for, no?

@xabbuh
Copy link
Member

xabbuh commented May 25, 2016

@flip111 Well, we can of course update docblocks if there are some which are not clear enough.

@jakzal
Copy link
Contributor

jakzal commented Jun 27, 2016

Anything to do here since the related PR was rejected?

@flip111
Copy link
Contributor Author

flip111 commented Jun 27, 2016

Accept or reject the issue on principle, we can think of implementation later.

@carsonbot
Copy link

Thank you for this suggestion.
There has not been a lot of activity here for a while. Would you still like to see this feature?

@carsonbot
Copy link

Hello? This issue is about to be closed if nobody replies.

@flip111
Copy link
Contributor Author

flip111 commented Jan 3, 2021

.

@carsonbot carsonbot removed the Stalled label Jan 3, 2021
@carsonbot
Copy link

Thank you for this suggestion.
There has not been a lot of activity here for a while. Would you still like to see this feature?

@carsonbot
Copy link

Just a quick reminder to make a comment on this. If I don't hear anything I'll close this.

@carsonbot
Copy link

Hey,

I didn't hear anything so I'm going to close it. Feel free to comment if this is still relevant, I can always reopen!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants