Deprecate loading multiple documents in the same crawler #16057
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| @@ -20,7 +20,10 @@ public function testConstructor() | |||
| $crawler = new Crawler(); | |||
| $this->assertCount(0, $crawler, '__construct() returns an empty crawler'); | |||
|
|
|||
| $crawler = new Crawler(new \DOMNode()); | |||
| $doc = new \DOMDocument(); | |||
| $node = $doc->createElement('test'); | |||
There was a problem hiding this comment.
this change should actually be done in older branches too. Passing an orphan DOMNode creates a broken crawler:
- we actually only accept
DOMElement(andDOMDocumentwhere we get the documentElement in it), not anyDOMNode(you get errors in some cases if you inject such nodes in a crawler) - we don't accept nodes not attached to a document (you would get an
Invalid state errorfrom the DOM library when accessingownerDocument, which we already do when filtering the crawler)
|
the failure with deps=low in the Security component seems related to a bug in PHPUnit 5.0 Edit: reported as sebastianbergmann/phpunit#1886 |
|
and for deps=high, it is because 2.8 needs to be merged into master to add the new class |
|
Once this PR is merged, I will work on the update of the component for 3.0 to avoid using SplObjectStorage (I prefer waiting until this PR is merged to avoid useless conflicts) |
| @@ -307,6 +313,14 @@ public function addNodes(array $nodes) | |||
| */ | |||
| public function addNode(\DOMNode $node) | |||
| { | |||
| if (null !== $this->document && $this->document !== $node->ownerDocument) { | |||
| @trigger_error('Attaching DOM nodes from multiple documents in a Crawler is deprecated as of 2.8 and will be forbidden in 3.0.'); | |||
5a8e2ad to
a78caa9
Compare
|
@fabpot it would be great to merge 2.8 into master to fix the deps=high builds (this is painful because of the descriptor fixture changes btw) |
a78caa9 to
0d1cb3b
Compare
|
@symfony/deciders this one is ready |
|
Thank you @stof. |
fabpot
added a commit
that referenced
this pull request
Oct 2, 2015
…er (stof) This PR was merged into the 2.8 branch. Discussion ---------- Deprecate loading multiple documents in the same crawler | Q | A | ------------- | --- | Bug fix? | no | New feature? | no | BC breaks? | no | Deprecations? | yes | Tests pass? | yes | Fixed tickets | #15849 | License | MIT | Doc PR | n/a Note that loading multiple documents in the same crawler already creates weird things when working with namespaces (the list of mapping of aliases to namespaces is shared between documents, which was flawed). As said in the issue, this opens the door to optimizations in the future (sharing the DOMXpath instance for instance, including with subcrawler) Commits ------- 0d1cb3b Deprecate loading multiple documents in the same crawler
fabpot
added a commit
that referenced
this pull request
Oct 2, 2015
This PR was merged into the 2.8 branch. Discussion ---------- Fix the DomCrawler tests | Q | A | ------------- | --- | Bug fix? | yes | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | n/a | License | MIT | Doc PR | n/a This fixes the tests by applying changes from #16057 and #16058 in the right order in ``addNode`` Commits ------- e6feed2 Fix the DomCrawler tests
Merged
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Note that loading multiple documents in the same crawler already creates weird things when working with namespaces (the list of mapping of aliases to namespaces is shared between documents, which was flawed).
As said in the issue, this opens the door to optimizations in the future (sharing the DOMXpath instance for instance, including with subcrawler)