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
There are a few situations in core where links are generated when the system is running outside of the primary front controller or from a special route. These include
- update.php
- core/authorize.php
- core/rebuild.php
- core/install.php (?)
When links are generated, there are two scenarios. One, a link needs to be generated relative to the entry point (eg, update.php). Two, a link needs to be generated relative to the site root. When this happens, the base_url gets passed into Url() as an option, but to get the base_url you either need to rely on the global or perform some shenanigans to construct it on your own. This is bad.
Proposed resolution
Options for discussion:
- Add an option to Url() to force links to be relative to the site root and not the current entry point.
- Refactor the link generator to always assume routes are relative to the primary front controller unless told otherwise
- Use #2529170: Remove DrupalKernel::initializeRequestGlobals and replace base_root, base_url and base_path with a service, and inject the App service into UrlGenerator, MetadataBubblingUrlGenerator, and UnroutedUrlAssembler to use instead of the globals.
Remaining tasks
Discuss, do it.
User interface changes
None.
API changes
Url() and/or the link generator may have a new option.
Data model changes
Should be none.
| Comment | File | Size | Author |
|---|---|---|---|
| #36 | add_option_to_url_to-2548095-36.patch | 4.46 KB | vprocessor |
| #24 | interdiff-20-24.txt | 3.6 KB | mpdonadio |
| #24 | add_option_to_url_to-2548095-24.patch | 5.16 KB | mpdonadio |
| #20 | interdiff.txt | 552 bytes | dawehner |
| #20 | 2548095-20.patch | 2.35 KB | dawehner |
Comments
Comment #2
mpdonadioActually, I am starting to think this is a bug. If you are passing a route to Url(), then I don't see why links should ever be relative to anything other than the primary front controller for the site (adjusted for language, etc).
?
Comment #3
David_Rothstein CreditAttribution: David_Rothstein as a volunteer commentedI agree. In Drupal 7 this just worked - seems like a regression if in Drupal 8 you have to mess with the base URL (and force the link to be absolute) yourself.
Comment #4
stefan.r CreditAttribution: stefan.r commentedyes, right now I think the only way to work around this is by using toAbsolute()
Comment #5
mpdonadioFugly patch, but I think it shows the general problem. The base_url is being set up properly by the kernel. The problem is that UrlGenerator::generateFromRoute() grabs the base_url from the RequestContext when it isn't explicitly specified. And, when the RequestContext context is created in the UrlGenerator, it doesn't set up the base_url; it will be derived from the Request object, which will grab it from the path to the front controller that was the entry point. And this last part is in Symfony land, and out of our control.
When I apply this on top of the patch from #1885564: theme.maintenance.inc (authorize.php) - Convert theme_ functions to Twig, and then comment out the 'absolute' and 'base_url' options from Module::postInstallTasks(), then I get the proper URLs in the "Authorize file system changes" page.
Not sure how best to handle this, but I suspect we need to inject the base_url into the UrlGenerator service for use with routes instead of grabbing them from the context?
Comment #7
znerol CreditAttribution: znerol commentedRelated? #2529170: Remove DrupalKernel::initializeRequestGlobals and replace base_root, base_url and base_path with a service
Comment #8
dawehnerThe more I think about the issue the more I think we should actually make it that the URL is generated like we would have visited index.php. It is what people expected.
If you still want to generate one relative to the current front controller we could make that behaviour opt in.
Comment #9
mpdonadioauthorize.php has a @todo and update.php will have the @todo referencing this.
rebuild.php does
I would really be surprised if install.php doesn't do something to work around this, too, for the post installation links, but I didn't look into it too much.
Later today I'll fix as many places in code that use shenanigans so we can have a failing patch to test with. I may wait to see if the update one gets in today.
Comment #10
catchBumping to major, this would have simplified #2540416: Move update.php back to a front controller.
Comment #11
mpdonadioOK, first pass at fixing Url usage. Didn't look at installer yet (I totally forget how the post installation redirect works). Also didn't address the actual problem. So, this should at least start to show some failures (or show where we are lacking test coverage).
No interdiff; the patch in #5 was just a demo.
Comment #12
mpdonadioIf the site has been properly installed, then this should work when we fix the underlying problem.
Comment #13
catchActually bumping this time.
Comment #16
mpdonadioComment #17
mpdonadioComment #18
dawehnerWhat about something like this? I mean what we want is a different thing, so we should also name it different.
Comment #20
dawehnerMuh.
Comment #22
Crell CreditAttribution: Crell as a volunteer commentedWe normally return static, not $this, in the docs.
Ibid.
I'm a bit unclear on how this patch is supposed to help, though...
Comment #23
tim.plunkettPlease disregard #22.1.
We use
@return $thiswhen it's the same instance.@return staticis for when it's a factory method or similar.Comment #24
mpdonadioFixed a problem in #20 and updated most of the @todo that reference this issue, except the ones in DbUpdateController. Unit tests pass locally, let's see what a full run does now.
Comment #26
tim.plunkettSure it's ugly, but this isn't a major bug.
Comment #27
joelpittetJust out of curiosity is there a better name than
mainBaseUrl? Since it's referring to a front controller, could it be named as such?mainFrontControllerUrl? assuming there are more than one front controllers(naive, but is there?)Alternatives:
whoIsOnFirstBaseUrlstarKillerBaseUrlAllYourMainBaseRbelongsToUrlComment #30
seanBI just ran in to this issue. Is there another way around this? Using Url() in my update hook always adds /update.php/ to all URLs. If there is no way to fix this, it is probably a bug right?
Comment #31
joelpittet@seanB yes that sounds like a separate issue, please have a search if someone has created it first and then if not create a new one
Comment #32
seanBOk thnx, I created issue https://www.drupal.org/node/2824581
Comment #33
Wim LeersBecause this is not yet fixed, I had to add a hideous work-around to #2830326-5: Broken link to 'Put your site into maintenance mode' on update.php results in WSOD.
Comment #34
mpdonadioNo real surprise, but #24 doesn't apply, so this needs a reroll. Probably needs to be a hand re-roll; since #1475510: Remove external dependencies from the core repo and let Composer manage the dependencies instead I haven't really successfully rebased against 8.0.x.
Comment #35
vprocessor CreditAttribution: vprocessor at Skilld commentedComment #36
vprocessor CreditAttribution: vprocessor at Skilld commentedreroll for 8.3.x
Comment #37
vprocessor CreditAttribution: vprocessor at Skilld commentedComment #38
vprocessor CreditAttribution: vprocessor at Skilld commentedComment #39
vprocessor CreditAttribution: vprocessor at Skilld commented