[DomCrawler] fixed handling of schemes by Link::getUri() #7214
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
| array(' | ||
| /foo', 'http://localhost/bar/foo/', 'http://localhost/foo'), | ||
| array('/foo | ||
| ', 'http://localhost/bar/foo', 'http://localhost/foo'), |
There was a problem hiding this comment.
Thos two tests should be kept and must work. If not, this is a BC break. And indeed, I think your change does not change these.
|
Ok, I will update the pull request |
| @@ -89,7 +89,7 @@ public function getUri() | |||
| $uri = trim($this->getRawUri()); | |||
|
|
|||
| // absolute URL? | |||
| if (0 === strpos($uri, 'http')) { | |||
| if (!is_null(parse_url($uri, PHP_URL_SCHEME))) { | |||
There was a problem hiding this comment.
we don't use is_null in Symfony but null !== instead.
A link (anchor tag with an href attr) in crawled by the Crawler can contain any valid URI, including mailto: links. Currently this is not correctly supported by Link::getUri. Schemes that do not start with 'http' are treated as relative URIs and appenden to the base URI. This leads to strange URIs like this: http://foo.com/mailto:foo@bar.com Fixed Link::getUri to treat any URI with a schema part as an absolute URL. Updated the unit tests to test for this.
|
Fixed |
fabpot
added a commit
that referenced
this pull request
Feb 28, 2013
…(PR #7214) This PR was merged into the 2.1 branch. Commits ------- 8f8ba38 [DomCrawler] fix handling of schemes by Link::getUri() Discussion ---------- [DomCrawler] fixed handling of schemes by Link::getUri() A link (anchor tag with an href attr) in pages crawled by the Crawler can contain any valid URI, including mailto: links. Currently this is not correctly supported by `Link::getUri`. Schemes that do not start with 'http' are treated as relative URIs and appenden to the base URI. This leads to strange URIs like this: http://foo.com/mailto:foo@bar.com Fixed `Link::getUri` to treat any URI with a schema part as an absolute URL. Updated the unit tests to test for this. --------------------------------------------------------------------------- by matthijsvandenbos at 2013-02-28T11:55:18Z Ok, I will update the pull request --------------------------------------------------------------------------- by matthijsvandenbos at 2013-02-28T12:25:45Z Fixed
|
It's still wrong for URLs starting with // (double slash). |
|
@tomasz-ulanowski the support of scheme-relative uris has been added by a separate PR |
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.
A link (anchor tag with an href attr) in pages crawled by the Crawler
can contain any valid URI, including mailto: links.
Currently this is not correctly supported by
Link::getUri.Schemes that do not start with 'http' are treated as relative URIs
and appenden to the base URI. This leads to strange URIs like this:
http://foo.com/mailto:foo@bar.com
Fixed
Link::getUrito treat any URI with a schema part as anabsolute URL. Updated the unit tests to test for this.