Movatterモバイル変換

[0]ホーム
URL:

Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

 Sign up
Appearance settings

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

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

Closed
EdgarPE wants to merge8 commits intosymfony:masterfromEdgarPE:domcrawler-select-domnodes
Closed

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

EdgarPE wants to merge8 commits intosymfony:masterfromEdgarPE:domcrawler-select-domnodes

Conversation

@EdgarPE
Copy link
Contributor

QA
Bug fix?no
New feature?yes, revert to previous behaviour
BC breaks?no
Deprecations?no
Tests pass?yes
Fixed tickets#16933
LicenseMIT
Doc PR

Update: no BC break here.

Related Pull request:#16058
Mostly reverted previous commits:f416e709e60980

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() andCrawler::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.

@EdgarPEEdgarPE changed the title[DomCrawler] Revert previous restriction, allow selection of every DOMNode pbject[DomCrawler] Revert previous restriction, allow selection of every DOMNode objectDec 15, 2015
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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

@stof
Copy link
Member

All changes in Form, Link and fields should be reverted

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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

@EdgarPE
Copy link
ContributorAuthor

@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
Copy link
ContributorAuthor

@stof based on your input I kept all type hints inForm,Link andFields/*, also related tests. But removed theDOMElement restriction form Crawler on all nodes. Nodes now can containDOMNode-s, butCrawler::link().Crawler::links() andCrawler::forms() throw an\InvalidArgumentException if needed.

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

@EdgarPE
Copy link
ContributorAuthor

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

missing space before the opening parenthesis.

@stof
Copy link
Member

should we merge this in older branches as a bugfix ?

@xabbuh
Copy link
Member

Does it work the same when applied to older branches?

@EdgarPE
Copy link
ContributorAuthor

This change set reverts/rewrites9e60980 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
Copy link
ContributorAuthor

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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

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

@jakzal
Copy link
Contributor

@EdgarPE great job!

👍 once my comments are addressed

@EdgarPE
Copy link
ContributorAuthor

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

this could be only@expectedExceptionMessage (no regexp)

@EdgarPE
Copy link
ContributorAuthor

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
Copy link
ContributorAuthor

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

@stof
Copy link
Member

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

@xabbuh
Copy link
Member

I agree with@stof.

…MNode object.Squashed commit of the following:commit 69c97511c99e761dfeb612bb1d3b8a83b8984810Author: EdgarPE <hello@edgarpe.hu>Date:   Tue Dec 15 17:01:10 2015 +0100    Add CHANGELOGcommit 19a171af813e83112fb404a1b86985215c83346aAuthor: EdgarPE <hello@edgarpe.hu>Date:   Tue Dec 15 16:48:46 2015 +0100    Test coverage for InvalidArgumentExceptions in Crawler.phpcommit 8b050b780222e8c09c1b5ac2776584cd0d42d807Author: EdgarPE <hello@edgarpe.hu>Date:   Tue Dec 15 15:43:13 2015 +0100    Reverted [DomCrawler] Changed typehints form DomNode to DomElementf416e70commit 52ff5af0c13417277b4e3b122f35536ed43d0668Author: 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
Copy link
ContributorAuthor

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

@jakzaljakzal closed thisDec 23, 2015
jakzal added a commit that referenced this pull requestDec 23, 2015
…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
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

7 participants

@EdgarPE@stof@xabbuh@jakzal@fabpot@nicolas-grekas@carsonbot
[8]ページ先頭
©2009-2025 Movatter.jp