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.
Problem/Motivation
#2417647: Add leading slash to paths within 'user-path:' URIs, to allow 'user-path:' URIs to point to the <none> route altered the URI scheme for user-path to use a leading slash, but the code doesn't actually check for the leading slash. This can cause very wierd behavior, eg
Url::fromUri('user-path:foo/bar')->toString();
will just swallow the 'f' and produce '/oo/bar'
Proposed resolution
Check the the slash, otherwise throw an exception.
Remaining tasks
None.
User interface changes
None.
API changes
None.
Comment | File | Size | Author |
---|---|---|---|
#20 | Screen Shot 2015-02-13 at 11.34.34 AM.png | 35.97 KB | webchick |
#18 | interdiff-2423579-18.txt | 801 bytes | heilop |
#18 | 2423579-18.patch | 4.44 KB | heilop |
#15 | interdiff-13-15.txt | 1.48 KB | mpdonadio |
#15 | 2423579-15.patch | 4.44 KB | mpdonadio |
Comments
Comment #1
mpdonadioFirst pass at a patch w/ a test.
Comment #2
Wim LeersI independently cross-rolled this patch yesterday.
Near identical.
May the reviewers choose which they prefer :)
Comment #3
Wim LeersClarifying title; no leading slash is fine if no path component is specified in the URI. I.e. these are all valid:
user-path:#foo
user-path:?foo=bar#baz
And none of them have a leading slash.
Comment #4
xjmYep we added the validation for that in #2417567: Rename user-path: scheme to internal:, and I was also thinking it should happen with the scheme generally, rather than at the wrapper level.
Comment #5
xjmComment #6
xjmAdding that coverage.
Comment #7
mpdonadioWim's exception message is better, and will point the developer to how to actually fix the problem.
Comment #8
xjmHere's that extended test coverage. Note that I've done this:
...in the hope of reusing the data providers for tests for
fromUserInput()
(see #2417567: Rename user-path: scheme to internal:).Comment #9
xjmThe docblock should say "valid" here rather than "invalid". :P
Comment #12
mpdonadioI should have time to look at this tonight, though it wasn't apparent from a quick look where those exceptions are really coming from.
Comment #13
mpdonadioHere is a patch. It makes it green. However, I think this is really a bug in LinkWidget.
Read LinkWidget::validateUriElement() after the patch has been applied. I have verified that the `if` on line 155 evaluates to TRUE, but since there isn't a return, it happily continues onward, and then barfs on the `Url::fromUri($uri)` on line 165. However, the exception is coming from the one that we throw in this patch.
If people agree, I will post an issue and fix it.
Comment #14
Wim LeersAgreed:
That's a great, great question. Let's do that.
These changes are also being made over at #2419693: Move URI access validation from widget to field constraint — you discovered exactly the same problem here.
Comment #15
mpdonadioOK, if we add the return we don't need the try/catch, and LinkFieldTest passes locally for me b/c ::setError() keeps the first error message. Is this still in scope for this issue?
Comment #16
Wim LeersAwesome! Just one super silly typo :P But marking RTBC, because that can easily be fixed on commit.
s/entity:/user-path:/
Comment #17
heilop CreditAttribution: heilop commentedI am working and fixing #16
Comment #18
heilop CreditAttribution: heilop commentedI replaced entity: with user-path: Tests the fromUri() in this two methods documents.
Comment #19
Wim LeersThanks! :)
Comment #20
webchickMy one worry with this patch was that this developer-oriented exception message would leak its way out into "user space" but that doesn't seem to be the case:
Therefore, looks good! Committed and pushed to 8.0.x. Thanks!
Comment #22
mpdonadioAnd congrats to @heilop, as this looks like his first Drupal 8 commit!
Comment #23
Wim Leers#20: Indeed it doesn't, that would indeed be unacceptable. Validation callback + constraints are responsible for what you see in the UI. Do we ever really show PHP-level exception to the end user? I can't think of any place where we do that. I sure hope we don't :P
Comment #24
xjmMinor followup: #2431085: Validation error for manually entered paths in the link widget is a little confusing