Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
Deprecate loading multiple documents in the same crawler#16057
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
Uh oh!
There was an error while loading.Please reload this page.
Conversation
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 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)
stof commentedOct 1, 2015
the failure with deps=low in the Security component seems related to a bug in PHPUnit 5.0 Edit: reported assebastianbergmann/phpunit#1886 |
stof commentedOct 1, 2015
and for deps=high, it is because 2.8 needs to be merged into master to add the new class |
stof commentedOct 1, 2015
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) |
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.
missingE_USER_DEPRECATED
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.
fixed
5a8e2ad toa78caa9Comparestof commentedOct 2, 2015
@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 to0d1cb3bComparestof commentedOct 2, 2015
@symfony/deciders this one is ready |
fabpot commentedOct 2, 2015
Thank you@stof. |
…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/aNote 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
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/aThis fixes the tests by applying changes from#16057 and#16058 in the right order in ``addNode``Commits-------e6feed2 Fix the DomCrawler tests
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)