[DomCrawler] Revert previous restriction, allow selection of every DOMNode object #17021

EdgarPE · 2015-12-15T16:49:45Z

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

Update: no BC break here.

Related Pull request: #16058
Mostly reverted previous commits: f416e70 9e60980

The linked PR above introduced a serious limitation in the DomCrawler component, namely only allows DOMElement and DOMDocument objects to be selected/added to the Crawler. Previously it was possible to completely parse, process, and also modify any Html, and XML documents.

Text nodes, attribute nodes and comment nodes are now impossible to search or add using the Crawler. I think these changes greatly reduce the usage possibilities of DomCrawler. So, my suggestion is:

Let's revert two previous changes and allow selecting every possible DOMNode object. There were fatal errors in some cases, witch were fixed with the changed type hints. Instead of type hints, now \InvalidArgumentException is thrown if needed in these methods: Crawler::link(), Crawler::links() and Crawler::form(), because these methods add special treatment for specific nodes.

I am new to this community so I'm sorry if my commit or Pull Request have some flaws or miss something. In that case please let me know what should I do to fix it.

stof · 2015-12-15T16:55:14Z

src/Symfony/Component/DomCrawler/Form.php

     * @param string      $currentUri The URI of the page where the form is embedded
     * @param string      $method     The method to use for the link (if null, it defaults to the method defined by the form)
     * @param string      $baseHref   The URI of the <base> used for relative links, but not for empty action
     *
     * @throws \LogicException if the node is not a button inside a form tag
     */
-    public function __construct(\DOMElement $node, $currentUri, $method = null, $baseHref = null)
+    public function __construct(\DOMNode $node, $currentUri, $method = null, $baseHref = null)


This should not be changed back, as this class cannot work with anything else than a DOMElement (it triggers fatal errors for others)

stof · 2015-12-15T16:55:49Z

All changes in Form, Link and fields should be reverted

stof · 2015-12-15T16:57:49Z

src/Symfony/Component/DomCrawler/Crawler.php

@@ -830,14 +842,9 @@ private function filterRelativeXPath($xpath)

        $crawler = $this->createSubCrawler(null);

-        foreach ($this->nodes as $node) {
+        foreach ($this as $node) {


this change should be reverted. There is no reason to iterate on the crawler rather than iterator directly on the internal array

EdgarPE · 2015-12-15T17:12:54Z

@stof Thank you for your comments. Later today I'll update my patch to incorporate everything you asked, and also fix fabpot.io errors. The Travis CI fail seems to be irrelevant, or at least caused by something else.

EdgarPE · 2015-12-15T18:42:11Z

@stof based on your input I kept all type hints in Form, Link and Fields/*, also related tests. But removed the DOMElement restriction form Crawler on all nodes. Nodes now can contain DOMNode-s, but Crawler::link(). Crawler::links() and Crawler::forms() throw an \InvalidArgumentException if needed.

This way there is no need for a second BC BREAK.

EdgarPE · 2015-12-15T18:45:10Z

Also reworded the exceptions and made the changes in test cases. Now, this is a much smaller PR, only 2 files changed.

fabpot · 2015-12-15T20:02:54Z

src/Symfony/Component/DomCrawler/Crawler.php

@@ -706,6 +702,10 @@ public function link($method = 'get')

        $node = $this->getNode(0);

+        if(!$node instanceof \DOMElement) {


missing space before the opening parenthesis.

stof · 2015-12-16T08:14:31Z

should we merge this in older branches as a bugfix ?

xabbuh · 2015-12-16T09:55:36Z

Does it work the same when applied to older branches?

EdgarPE · 2015-12-16T13:44:53Z

This change set reverts/rewrites 9e60980 which was introduced in 2.8 Sadly, this exact change set as a patch does not apply to 2.8. Should I work on a 2.8 version of these changes?

EdgarPE · 2015-12-16T16:53:27Z

I don't know what is the policy of backports, so I made a different PR for it.

jakzal · 2015-12-17T16:41:33Z

src/Symfony/Component/DomCrawler/Crawler.php

@@ -706,6 +702,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)));


For consistency, we should use single quotes to enclose the message, and double quotes inside the message:

throw new \InvalidArgumentException(sprintf('The current node list should contain only DOMElement instances, "%s" found.', get_class($node)));

jakzal · 2015-12-17T16:45:08Z

@EdgarPE great job!

👍 once my comments are addressed

EdgarPE · 2015-12-17T20:23:46Z

@jakzal Thank you! Let me know if there is anything I should fix.

nicolas-grekas · 2015-12-18T09:41:04Z

src/Symfony/Component/DomCrawler/Tests/CrawlerTest.php

@@ -745,6 +735,26 @@ public function testLink()
        }
    }

+    /**
+     * @expectedException \InvalidArgumentException
+     * @expectedExceptionMessageRegExp /^The current node list should contain only DOMElement instances/


this could be only @expectedExceptionMessage (no regexp)

EdgarPE · 2015-12-18T20:45:47Z

It looks like the tests are failing because of the new commit is master. Let me know if there is anything I should fix, or if I need to do a rebase now.

EdgarPE · 2015-12-21T23:46:31Z

Rebased my changes to latest master, DomCrawler tests still pass. I'm looking forward for your update on this.

stof · 2015-12-22T09:27:28Z

I suggest merging #17035 instead, which does the same changes in 2.8

xabbuh · 2015-12-22T09:32:46Z

I agree with @stof.

…MNode object. Squashed commit of the following: commit 69c97511c99e761dfeb612bb1d3b8a83b8984810 Author: EdgarPE <hello@edgarpe.hu> Date: Tue Dec 15 17:01:10 2015 +0100 Add CHANGELOG commit 19a171af813e83112fb404a1b86985215c83346a Author: EdgarPE <hello@edgarpe.hu> Date: Tue Dec 15 16:48:46 2015 +0100 Test coverage for InvalidArgumentExceptions in Crawler.php commit 8b050b780222e8c09c1b5ac2776584cd0d42d807 Author: EdgarPE <hello@edgarpe.hu> Date: Tue Dec 15 15:43:13 2015 +0100 Reverted [DomCrawler] Changed typehints form DomNode to DomElement f416e70 commit 52ff5af0c13417277b4e3b122f35536ed43d0668 Author: EdgarPE <hello@edgarpe.hu> Date: Tue Dec 15 15:33:58 2015 +0100 Reverted: feature #16058 Prevent adding non-DOMElement elements in DomCrawler (stof) 9f362a1

EdgarPE · 2015-12-22T12:04:24Z

@xabbuh @stof 👍 Merging #17035 into master causes some conflicts but it's easy to solve

…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

carsonbot added the Status: Needs Review label Dec 15, 2015

EdgarPE changed the title ~~[DomCrawler] Revert previous restriction, allow selection of every DOMNode pbject~~ [DomCrawler] Revert previous restriction, allow selection of every DOMNode object Dec 15, 2015

EdgarPE mentioned this pull request Dec 15, 2015

Impossible to select non-DOMElements #16933

Closed

stof reviewed Dec 15, 2015
View reviewed changes

fabpot reviewed Dec 15, 2015
View reviewed changes

EdgarPE mentioned this pull request Dec 16, 2015

[DomCrawler] Revert previous restriction, allow selection of every DOMNode object #17035

Closed

jakzal added the DomCrawler label Dec 17, 2015

jakzal reviewed Dec 17, 2015
View reviewed changes

jakzal added Status: Reviewed and removed Status: Needs Review labels Dec 17, 2015

nicolas-grekas reviewed Dec 18, 2015
View reviewed changes

EdgarPE added 8 commits December 22, 2015 12:58

[DomCrawler] Re-add DOMDocument type hints in Form, Link and fields

1d0d5ea

Coding standard fixes.

408d0bd

Remove extra blank line.

1ebb09c

[DomCrawler] Spelling and clarification.

e403289

CS fix

5d99e77

Grammar fix.

f5bdb38

Remove expectedExceptionMessageRegExp from tests.

a8f1c90

jakzal closed this Dec 23, 2015

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[DomCrawler] Revert previous restriction, allow selection of every DOMNode object #17021

[DomCrawler] Revert previous restriction, allow selection of every DOMNode object #17021

EdgarPE commented Dec 15, 2015

stof Dec 15, 2015

stof commented Dec 15, 2015

stof Dec 15, 2015

EdgarPE commented Dec 15, 2015

EdgarPE commented Dec 15, 2015

EdgarPE commented Dec 15, 2015

fabpot Dec 15, 2015

stof commented Dec 16, 2015

xabbuh commented Dec 16, 2015

EdgarPE commented Dec 16, 2015

EdgarPE commented Dec 16, 2015

jakzal Dec 17, 2015

jakzal commented Dec 17, 2015

EdgarPE commented Dec 17, 2015

nicolas-grekas Dec 18, 2015

EdgarPE commented Dec 18, 2015

EdgarPE commented Dec 21, 2015

stof commented Dec 22, 2015

xabbuh commented Dec 22, 2015

EdgarPE commented Dec 22, 2015

		@@ -706,6 +702,10 @@ public function link($method = 'get')

		$node = $this->getNode(0);

		if(!$node instanceof \DOMElement) {

[DomCrawler] Revert previous restriction, allow selection of every DOMNode object #17021

[DomCrawler] Revert previous restriction, allow selection of every DOMNode object #17021

Conversation

EdgarPE commented Dec 15, 2015

stof Dec 15, 2015

Choose a reason for hiding this comment

stof commented Dec 15, 2015

stof Dec 15, 2015

Choose a reason for hiding this comment

EdgarPE commented Dec 15, 2015

EdgarPE commented Dec 15, 2015

EdgarPE commented Dec 15, 2015

fabpot Dec 15, 2015

Choose a reason for hiding this comment

stof commented Dec 16, 2015

xabbuh commented Dec 16, 2015

EdgarPE commented Dec 16, 2015

EdgarPE commented Dec 16, 2015

jakzal Dec 17, 2015

Choose a reason for hiding this comment

jakzal commented Dec 17, 2015

EdgarPE commented Dec 17, 2015

nicolas-grekas Dec 18, 2015

Choose a reason for hiding this comment

EdgarPE commented Dec 18, 2015

EdgarPE commented Dec 21, 2015

stof commented Dec 22, 2015

xabbuh commented Dec 22, 2015

EdgarPE commented Dec 22, 2015