[DomCrawler] Revert previous restriction, allow selection of every DOMNode object #17035
Conversation
|
you need to update the phpdoc of Crawler in a few place talking about DomElement though |
| @@ -709,6 +705,10 @@ public function link($method = 'get') | |||
|
|
|||
| $node = $this->getNode(0); | |||
|
|
|||
| if (!$node instanceof \DOMElement) { | |||
| throw new \InvalidArgumentException(sprintf("The current node list should contain only DOMElement instances, '%s' found.", get_class($node))); | |||
There was a problem hiding this comment.
Enclose the message in single quotes please.
|
Rebased changes. DomCrawler tests still pass. |
| $crawler = new Crawler(); | ||
| $crawler->add(new \DOMNode()); | ||
| $crawler = $this->createTestCrawler(); | ||
| $crawler->addHtmlContent('<html><div class="foo"></html>', 'UTF-8'); |
There was a problem hiding this comment.
Why are you adding a test without any assertion here ?
There was a problem hiding this comment.
I wanted to test the deprecation notice, but it's out of scope of this PR, so just removed it.
|
👍 |
| @@ -709,6 +705,10 @@ public function link($method = 'get') | |||
|
|
|||
| $node = $this->getNode(0); | |||
|
|
|||
| if (!$node instanceof \DOMElement) { | |||
| throw new \InvalidArgumentException(sprintf('The current node list should contain only DOMElement instances, "%s" found.', get_class($node))); | |||
There was a problem hiding this comment.
Imo the message is a bit misleading as you only check the type of the first element.
There was a problem hiding this comment.
What do you suggest on this? Because only the first node is used my suggestion would be to reword the Exception like this:
throw new \InvalidArgumentException(sprintf('The selected node should be instance of DOMElement, "%s" found.', get_class($node)));
For the corresponding PHPDoc:
@throws \InvalidArgumentException If the current node list is empty or the selected node is not instance of DOMElement
…lection of every DOMNode object #17021
|
status: reviewed |
|
Thanks @EdgarPE for your excellent work! |
…ion of every DOMNode object (EdgarPE) This PR was squashed before being merged into the 2.8 branch (closes #17035). Discussion ---------- [DomCrawler] Revert previous restriction, allow selection of every DOMNode object | Q | A | ------------- | --- | Bug fix? | no | New feature? | yes, revert to previous behaviour | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | #16933 | License | MIT | Doc PR | This is a backport of PR #17021 Commits ------- d2872a3 [DomCrawler] Revert previous restriction, allow selection of every DOMNode object
|
Looks like gh didn't close this PR automatically. |
This is a backport of PR #17021