Early Bird Registration for DrupalCon Portland 2024 is open! Register by 23:59 UTC on 18 March 2024, to get $100 off your ticket.
This is a child issue of #2339219: [meta] Finalize URL generation API (naming, docs, deprecation). See that issue's summary for additional context.
Problem/Motivation
- In Drupal 7,
url()
andl()
could be passed one of:- an internal Drupal path (e.g.,
node/1
) - a relative URL to a resource on the same machine but that's not an internal Drupal path (e.g.,
/robots.txt
) - an absolute URL (e.g.,
http://example.com/foo/bar
)
- an internal Drupal path (e.g.,
- In Drupal 8, internal Drupal paths are a deprecated concept, replaced by route names and parameters.
- The Drupal 8 replacements to url() and l() are methods of the same name within
UrlGeneratorTrait
andLinkGeneratorTrait
. However, these, by design, only accept route names and parameters; they do not accept non-Drupal-routed URLs (relative or absolute) as arguments. - #2343661: Rename l() to _l() and url() to _url(), and document replacements will remove the global url() and l() functions.
- So, what are D7 modules that are currently calling url()/l() for non-Drupal-routed URLs supposed to do when they port to D8?
Proposed resolution
- Create a new service that handles building a URL given either a relative or absolute URL. It can add query string, fragment, etc, will validate the scheme, and pre-pend the base path. It will mostly be copying the code from UrlGenerator::generateFromPath(), but omitting the processPath() call since we don't want to try to alias or do language processing
- Have separate methods for the validating/sanitizing the scheme vs handling the options, and a convenience one that calls both
- Replace calls in core to url() with actual URLs to calls to this service
- Inject this service into the UrlGenerator to avoid code duplication
- Remove UrlGenerator::generateFromPath()
Remaining tasks
User interface changes
API changes
Comment | File | Size | Author |
---|---|---|---|
#86 | interdiff.txt | 1.94 KB | tim.plunkett |
#86 | 2343759-url-from-methods-86.patch | 57.94 KB | tim.plunkett |
#84 | interdiff.txt | 24.9 KB | tim.plunkett |
#84 | 2343759-url-from-methods-84.patch | 57.94 KB | tim.plunkett |
#79 | 2343759-url-79.patch | 49.58 KB | tim.plunkett |
Comments
Comment #1
effulgentsia CreditAttribution: effulgentsia commentedComment #2
dawehnerHad some idea in mind:
I really like the fact that standard based approach, though the question is what do people aspect. The question is which way, do we want to go.
What do you think about the domain path bit (other ideas, I think it is though needed)
100% agreed
Comment #3
effulgentsia CreditAttribution: effulgentsia commentedIs there a reason to split generateFromUriReference() into generateFromExternalPath() and generateFromDomainInternalPath()? I was envisioning generateFromUriReference() as being able to support both, since "URI references" can be either relative to hostname or absolute. Or, do you see
/update.php
andhttp://example.com/foo/bar
as being sufficiently different to warrant separate methods?Comment #4
effulgentsia CreditAttribution: effulgentsia commentedOops. Of course, I had to pick as an example something that we recently changed from a non-Drupal-path to a Drupal-path (#2250119: Run updates in a full environment). Updated the issue summary to use
/robots.txt
as the example of an "external" path on the same host.Comment #5
catchinstall.php would be another example.
Comment #6
dawehner@effulgentsia
This wasn't clear for me at least from your IS. I would be fine with both, the question is what people would expect.
In general it helps for me as a developer if I have one method which just does one thing, but this one thing might be using a UriReference() so I think having one would be fine.
Comment #7
dawehnerAdding a related issue
Comment #8
martin107 CreditAttribution: martin107 commentedJust a minor thing..
Yesterday, in IRC I saw lots of frustration and gnashing of teeth over PHP and its handling of _toString() errors.
This made me feel a little guilty because, under the 'Probably bug' section of my lint tool I had seen pile of lint error in this regard and done nothing
\Drupal\core\Url is on the list.
From batch.inc
Comment #9
dawehner@martin107
\Drupal\Core\Url provides toString() but not __toString()
Comment #10
martin107 CreditAttribution: martin107 commentedI see that, my thinking was flawed in other ways ....
the fix is trivial, but I thought it was going to be washed out with the changes here and in others in the works.
but what ever way I cut this function the change will remain relatively constant
So sorry for the noise the thing to do it put it forward as a quickfix child of #2344799: [Meta issue] Clear _toString is not implemented errors.
Comment #11
dawehner@martin107
In HEAD $url is a string.
Comment #12
xjmSo I thought I posted a comment on this issue Wednesday, but it seems to have been lost in the ether.
I'd prefer to have separate methods for internal, non-routed vs. external URLs. Here's why: As a module author, I can easily understand what an external URL is. I can also fairly easily understand what a "path processed by Drupal" is. The third sub-category of "what url() used to do"--these internal, non-routed paths like
/robots.txt
--is a bit less obvious, but I can figure it out from the first two.However, if the first and third things are lumped together, that makes it a lot more confusing to understand what I'm supposed to use when, and it would be easy to try to misuse that method for internal, routed paths.
Comment #13
effulgentsia CreditAttribution: effulgentsia commentedOk, so if we do #12, what names?
How about
generateFromExternalPath()
for things like/install.php
and/robots.txt
? It's "external" in the sense of being external to Drupal/Symfony routing, which is where UrlGeneratorInterface lives. And the word "path" means internal to the domain, per http://en.wikipedia.org/wiki/Uniform_resource_locator#Syntax.And for passing a URL rather than just a path, how about either
generateFromUrl()
orsanitizeUrl()
? Neither name is great, because it's kind of weird to "generate a URL from a URL", but the function does more than just sanitize (e.g., $options can include query and fragment additions).Better ideas for either welcome.
Comment #14
pwolanin CreditAttribution: pwolanin commentedDiscussing with dawehner and effulgentsia - let's take a different approach and create a tiny service that just does URL "tweaking". Will update the summary to reflect that.
Comment #15
pwolanin CreditAttribution: pwolanin commentedComment #16
pwolanin CreditAttribution: pwolanin commentedHere's a quick first pass at building a class and interface so we can argue about naming, etc with something to look at.
Comment #17
dawehnerI wonder whether we should still have some protected against accidentally passing in a url object?
Comment #18
xjmI don't understand what
buildFromPath()
is supposed to do from the name (with no docs or explanation on-isue), so maybe that's an indication that it needs a different name?I'm assuming
buildLocalUrl()
isrobots.txt
andbuildExternalUrl()
ishttp://othersite.example.com/foo
?Comment #19
pwolanin CreditAttribution: pwolanin commentedHere's a more functional patch after more discussion with dawehner and Wim Leers - we need a way to indicate that the Drupal based path should be inserted - use a special case scheme "base://"
Let's see what the tests say.
Comment #20
pwolanin CreditAttribution: pwolanin commentedAnd a few more tweaks for better security.
Comment #21
dawehnerI really like the simplification needed here.
Let's ensure to not forget to fix this.
I really wonder whether we should try to performance-optimize here.
I am curious whether we want to support this automatic extraction on the longrun. Was is the point in here? (Sure this is just a copy of the old code).
I think we should recommend actively people to use the other method, if they know what they want to do.
Comment #24
xjmIs this supposed to be in the routing component? As far as I know it explicitly does not depend on the routing system, no? Unless we're going to merge this with something that supports routes, or...
From this, I have absolutely no idea why there is a UrlBuilder and a UrlGenerator. That is going to cause a lot of frustration, especially since people need to know about both of them for porting
url()
from D7."the Drupal base path will be included in the generated URL," I guess? Still on the fence about this.
We need some documentation near the top here (similar to in the original patch from #2339219: [meta] Finalize URL generation API (naming, docs, deprecation)) that emphasizes that this should not be used for pages generated by Drupal (i.e. handled by the routing system). Also with @see to the UrlGenerator for those.
Still not sure about this name. I guess
buildLocalOrExternalUrl()
is too verbose?Or, it's already called
UrlBuilder
. Could we do instead:UrlBuilder::localUrl()
UrlBuilder::externalUrl()
UrlBuilder::localOrExternalUrl()
Comment #25
xjm@pwolanin, have you reviewed UrlGenerator? I don't understand why we are putting these methods on a different, almost identically named class.
Comment #26
pwolanin CreditAttribution: pwolanin commentedSee the meta issue for details until the issue summary is updated here.
Updated patch that will hopefully run at least. No interdiff, since it's a rather different approach. Note that this includes dawehner's work on #2346361: Add a UnroutedUrlAssembler and put it into the container
Comment #28
pwolanin CreditAttribution: pwolanin commentedFix missing use
Comment #29
pwolanin CreditAttribution: pwolanin commentedComment #31
pwolanin CreditAttribution: pwolanin commentedoops, missed converting a method call.
Comment #33
larowlan/me takes the baton
timezones++ (for once)
Comment #34
larowlanphpunit passing locally, expecting fails but not sure how many - let the bot do its thing
Comment #35
larowlanfixes shortcut
Comment #36
larowlanMore fixes
Comment #40
larowlanscrewy stuff going on in batch redirect for installer, hoping this is enough to sort it.
got to look at some other stuff will check back later
install test stuff passing locally, but not sure what else this will break
Comment #41
larowlanlast patch wasn't merged against HEAD
Comment #43
larowlanGreen!
Comment #44
pwolanin CreditAttribution: pwolanin commentedminor doc clean-ups and add one useful method (but not used currently)
Comment #45
xjmI think we should have one or more of
isRouted()
,isUnrouted()
,isUri()
, and/orisInternal()
methods rather than doing string parsing magic. @Crell pointed out that they maybe should not be onUrlHelper
, but that is out of scope here; for now, we should put them on the same class asisExternal()
.Comment #46
xjmSo this is... suspicious. If we need to concatenate
base://
on here, are other uses ofinstall_redirect_url()
elsewhere broken?Comment #47
larowlanthere are no other uses that also use batch_process which in turn passes it to ::unrouted
this fixes linkformatter changes, no patch
Comment #48
xjmWe had some reservations about where we're putting these classes and the dependencies we're adding, so I filed: #2346683: Decide where to put URL and link generation classes for decoupling/dependencies.
We should be able to clean it up in a mostly-BC way later, so we don't need to worry about it now.
Comment #49
Wim LeersI've also been working on a full review. I have a bunch of fixed nitpicks, a bug that I added test coverage for and I discovered the same problem as #46.
Reroll coming very soon.
Comment #50
Wim LeersTurns out that "domain/install.php" automatically redirects to "domain/core/install.php", so this wouldn't cause any problems, hence it would only slow down the installation unnecessary. So this was a regression, but a fairly harmless one. Nevertheless, reverted.
unrouted URL assembler (Fixed!)
Yes! :)
s/URL/Url/ (Fixed!)
s/encapselate/encapsulate/ (Fixed!)
s/uri/URI/ (Fixed!)
Isn't this wrong? Unrouted URIs that aren't external (e.g.
base://robots.txt
) would still be mapped to the code path for routes.Updated the test coverage, confirmed the bug, fixed it.
Here it is updated correctly :)
s/External URLs/Unrouted URIs/ (Fixed!)
s/URL builder/unrouted URL assembler/ (Fixed!)
This means that
base://
URLs (likebase://robots.txt
) won't work for menu links.Furthermore,
PathValidator::getUrlIfValid()
(called just before this snippet of code) doesn't handlebase://
URLs.Should that be a follow-up?
The last point is the only one that still needs to be answered before this is RTBC IMO.
Comment #51
Wim LeersComment #52
pwolanin CreditAttribution: pwolanin commentedWe've been working in parallel. Here's our added fixes on top of What Wim added.
Comment #53
pwolanin CreditAttribution: pwolanin commented@Wim Leers - links like 'base://robots.txt' are not expected to work for menu link. I don't think they should. We can debate as a follow-up, but honestly, I think it would be horribly abused.
Comment #57
pwolanin CreditAttribution: pwolanin commentedmissed a rename in Wim's patch
diff --git a/core/tests/Drupal/Tests/Core/ExternalUrlTest.php b/core/tests/Drupal/Tests/Core/UnroutedUrlTest.php
similarity index 100%
rename from core/tests/Drupal/Tests/Core/ExternalUrlTest.php
rename to core/tests/Drupal/Tests/Core/UnroutedUrlTest.php
piper:drupal-8 pwolanin$ git ci -am"rename file"
Comment #59
xjmDreditor started going crazy so pasting my notes here. I will incorporate these docs changes into the sandbox. Review was based on #44 and I was in the middle of reviewing Url.
This also is questionable, but @dawehner says @pwolanin is fixing it. :)
"The URL assembler"
So, better would be:
Because URIs are not routes.
Nitpick: uri should be caps.
We should add a separate paragraph here that says something like:
Then we add an @see to unrouted().
An associative array of route parameter names and values.
An associative array of additional URL options
So, the object is not routed. That would be weird. How about:
and
Same for the docblocks I guess.
Similar to the above, add a paragraph:
and a similar @see.
Spelling: encapsulate.
URI in caps (here and elsewhere).
Should we just change the name of the parameter and constructor argument to (e.g.) $route_or_uri? Could be in a followup since this is a pre-existing problem that just becomes weirder.
This is another place we want the new method (as above) instead of the strpos; could be a followup.
Other name idea: isBaseRelativeUri()....
How about "..of the unrouted URL."
Comment #60
dawehnerAddressing my own points.
Comment #61
webchickAccording to https://qa.drupal.org/pifr/test/875118 this failed testbot. :(
Comment #63
Crell CreditAttribution: Crell commentedI know there's still work happening but this looks mostly fine to me. Just a few smaller critiques below.
(I would still favor fromUnrouted() and fromRouted() or such.)
Extra ;
Coding standards: These should be lowerCamel.
I'm unclear from context why we're moving this to an array of urls? Is there a @dataProvider on it? If not,should there be?
Comment #64
xjmAttached is just some docs cleanups that I've pushed to the sandbox. It does not address #63 or the test failure, so uploading as a do-not-test patch since we know it would fail.
Comment #65
dawehnerI'm STUPID
Comment #67
dawehnerThis is using a a @depends annotation so this is passed along from the previous tests etc.
Comment #68
Wim LeersIndeed, it uses
@depends
. I also was confused by that, but this is 1) the pre-existing pattern, 2) exactly the same technique (array ofUrl
objects) is used in theUrlTest
sibling-test.Comment #69
xjmWe need to throw an exception here (either in Url::unrouted() or Url::setUnrouted()) if someone tries to pass a string that does not have a scheme. The exception should tell the developer that Url::unrouted() is only for non-routed, non-Drupal URLs, that routed URLs should use a route name with Url::routed(), and that non-routed base-relative URIs should begin with the
base://
prefix. See the second paragraph of the doblock onUrl::unrouted()
for a starting point.Edit: Also we need test coverage for throwing said exception.
Comment #70
Wim LeersRight, we forgot that! Great catch! :)
Comment #71
dawehnerAdd tests for the points in #2343759-69: Provide an API function to replace url()/l() for external urls
Comment #73
dawehnerLet's reroll.
Comment #74
Wim Leerss/schema/scheme/
Comment #75
webchickI can fix that on commit. :) Any other feedback, or is this RTBC?
Comment #76
effulgentsia CreditAttribution: effulgentsia commentedBoth of these return a URI to a consumer that isn't handling a URI successfully, but that's an edge case, and makes more sense to address in follow ups, so filed those and added @todos. Also fixed #74 and another text change Wim found and told me about as I was rolling this.
Comment #77
Wim LeersWonderful! :)
Comment #79
tim.plunkettMinor conflict with the follow-up in #2277103: Switch Drupal::l() and LinkGenerator to expect a Url object.
Comment #80
tim.plunkettI was not involved in any of the discussions about naming, but I really don't like that "unrouted" is exposed as a public API.
We have two static factory methods. One is when you have a route name, and the other a URI.
Why not call them ::fromRoute() or ::fromRouteName(), and ::fromUri()? Internally they can be routed/unrouted.
Comment #81
catchOK this looks great but after discussion with Alex Pott we're both not keen on routed vs. unrouted. The idea came up to use fromUri() and fromRoute() instead - now that we have the base::// scheme that seems fine. We could also improve the exception method in fromUri() to point to fromRoute() when there's no scheme passed in.
Discussed this with webchick, xjm and effulgentsia as well and they agree with that change. So sorry to CNW but unless there's a strong objection could we rename the methods?
Comment #82
catchCrossposted with Tim, but yes, exactly.
Comment #83
tim.plunkettWorking on this.
Comment #84
tim.plunkettMade the changes. I also expanded the exception message in fromUri().
Finally, cleaned up the @covers annotations in the tests, they were referencing old methods. In doing so I noticed we had a redundant test method, which I've removed.
Comment #85
larowlan%s/controller/controlled
%s/An/A
this drops the test right? think that needs to go back in some form
Comment #86
tim.plunkettComment #87
larowlanback to rtbc unless bot says otherwise
Comment #88
larowlanUpdated change record to reflect new method names ::fromRoute and ::fromUri
Comment #89
pwolanin CreditAttribution: pwolanin commentedbetter names, thanks, +1
Comment #90
Crell CreditAttribution: Crell commentedfromUri()/fromRoute()++
Comment #91
alexpottCommitted 3102292 and pushed to 8.0.x. Thanks!
Comment #93
geerlingguy CreditAttribution: geerlingguy commentedI updated the change record and added two examples for D7/D8. Also opened a follow-up; this change breaks the ability to create internal links during hook_install() (and potentially other situations where routes might not be available yet): #2348503: Drupal::l() can't be used for internal paths in hook_install() - Symfony throws "Route does not exist".