Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
[DomCrawler] Revert previous restriction, allow selection of every DOMNode object#17035
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
stof commentedDec 16, 2015
you need to update the phpdoc of Crawler in a few place talking about DomElement though |
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.
Enclose the message in single quotes please.
EdgarPE commentedDec 21, 2015
Rebased changes. DomCrawler tests still pass. |
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.
Why are you adding a test without any assertion here ?
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.
I wanted to test the deprecation notice, but it's out of scope of this PR, so just removed it.
stof commentedDec 22, 2015
👍 |
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.
Imo the message is a bit misleading as you only check the type of the first element.
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.
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 DOMElementThere 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.
Yes, I think that's better.
…lection of every DOMNode object#17021
jakzal commentedDec 23, 2015
status: reviewed |
jakzal commentedDec 23, 2015
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#17021Commits-------d2872a3 [DomCrawler] Revert previous restriction, allow selection of every DOMNode object
jakzal commentedDec 23, 2015
Looks like gh didn't close this PR automatically. |
This is a backport of PR#17021