Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.Updated: Comment #N
Problem/Motivation
Currently the URL utility \Drupal\Component\Utility\Url just can validate (Url::isValid()) URL's with a defined scheme. However, an URL isn't required to explicitly define a scheme:
http://tools.ietf.org/html/rfc3986#section-4.2
http://url.spec.whatwg.org/#concept-scheme-relative-url
This functionality can become quite handy and we should support such URL structures.
Proposed resolution
Make sure the URL utility properly validates scheme relative URL's. To do so I suggest to adjust Url::isValid() to not explicitly check for schemes (https / ftp / feed) but instead check the pattern defined by RFC 3986 section 3.1 and allow scheme relative URLs at the same time.
Checking the scheme should be fully upon Url::stripDangerousProtocols() instead.
Remaining tasks
Reviews needed
User interface changes
None
API changes
None
| Comment | File | Size | Author |
|---|---|---|---|
| #51 | 2195983-43.patch | 10.5 KB | quietone |
| #51 | 2195983-43-fail.patch | 4.88 KB | quietone |
| #41 | 2195983-40.patch | 10.47 KB | quietone |
| #41 | diff-29-40.txt | 13.05 KB | quietone |
| #29 | support_scheme_relative-2195983-29.patch | 12.56 KB | Nitesh Sethia |
Comments
Comment #1
das-peter CreditAttribution: das-peter commentedComment #4
das-peter CreditAttribution: das-peter commentedHad to adjust url validation in some locations.
Comment #5
sunThis looks great to me.
That said, do we actually know why
Url::isValid()does not usefilter_var()?If there's a particular reason for why we're not able to use it, then I think it would be a good idea to document that in its phpDoc, so we're able to double-check whether the custom preg_match() approach is obsolete with a new PHP version in the future.
Comment #6
das-peter CreditAttribution: das-peter commentedDuring my test's I figured out that
filter_var($value, FILTER_VALIDATE_URL)won't work with schema relative URLs. The function only validates if there's any kind of schema present - I thought about introducing something like(filter_var($value, FILTER_VALIDATE_URL) || filter_var("schema:" . $value, FILTER_VALIDATE_URL))to make schema relative paths validating. However that seems pretty ugly.Comment #7
das-peter CreditAttribution: das-peter commentedAdded comment to clarify why
preg_match()insteadfilter_var()is used.Comment #8
sunThanks!
Comment #9
hass CreditAttribution: hass commentedCan you explain what
'h+-.:'is made for, please?Comment #10
sunThat's an allowed/supported character test.
Comment #11
alexpottd8-add-support-for-schema-relative-urls-2195983-7.patch no longer applies.
Comment #12
das-peter CreditAttribution: das-peter commentedRe-roll
Comment #13
tim.plunkettI think this is actually a dupe of the older #1783278: Scheme-relative URL rejected by validation.
Can you compare the two approaches?
If this one is preferred, we can just mark that back to D7.
Comment #14
sunThis patch here definitely looks preferable, because it also removes that apparently hard-coded list of schemes in the preg_match() of isValid(), which has absolutely no business in there. :)
Due to that change, however, this patch cannot be backported, so we might as well keep the other issue to consider a D7/D6 compatible feature addition (which will probably look completely different to this patch here).
Comment #15
Dave ReidComment #16
sunBtw, marginally related:
PHP 5.4.7+ properly parses schema/protocol-relative URLs: http://3v4l.org/aPX44
Bummer, we only require 5.4.2 right now.
Comment #17
Wim LeersYAY! Thanks for working on this :)
Only found nitpicks:
No actual code changes.
Comment #18
Wim LeersAlso, I'm wondering if this shouldn't be considered a bug report instead of a feature request?
Comment #19
alexpottd8-add-support-for-schema-relative-urls-2195983-17.patch no longer applies.
Comment #20
martin107 CreditAttribution: martin107 commentedReroll.
Comment #22
balintcsaba CreditAttribution: balintcsaba commentedrerolled
Comment #23
jhedstromComment #24
Nitesh Sethia CreditAttribution: Nitesh Sethia as a volunteer commentedRerolled the patch successfully.
Comment #26
Nitesh Sethia CreditAttribution: Nitesh Sethia as a volunteer commentedRerolled the patch.
Comment #28
Nitesh Sethia CreditAttribution: Nitesh Sethia as a volunteer commentedComment #29
Nitesh Sethia CreditAttribution: Nitesh Sethia as a volunteer commentedRerolled
Comment #30
Nitesh Sethia CreditAttribution: Nitesh Sethia as a volunteer commentedComment #40
quietone CreditAttribution: quietone as a volunteer commentedThis looks like a duplicate of an older issue #1783278: Scheme-relative URL rejected by validation, and despite that being the older issue in #14 sun states that this is the preferred solution "because it also removes that apparently hard-coded list of schemes in the preg_match() of isValid(), which has absolutely no business in there. :)" Based on that the way forward is to close the other as a duplicate and merge the two patches.
Any objections?
Comment #41
quietone CreditAttribution: quietone as a volunteer commentedI did some work to merge the two patches. The changes for \Drupal\Component\Utility\UrlHelper::isExternal from this patch were not added, just didn't get to it.
Here is the work so far. There is no expectation that tests will pass.
Comment #42
Wim LeersNice, exciting to see this move forward again, @quietone! 🥳
Comment #51
quietone CreditAttribution: quietone as a volunteer commentedOK, I have looked at isExternal and I believe the current version in HEAD because the new tests for relative schemes pass without any changes.
Being hopeful, I am uploading a fail patch as well.
And I have moved credit from the other issue to this one. Let me know if I made a mistake on that.