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
Part of #2339219: [meta] Finalize URL generation API (naming, docs, deprecation). url()
needs to be deprecated and removed from Drupal 8, because it circumvents the routing system for internal URLs.
Proposed resolution
Convert as many straightforward url()
calls as possible.
- For internal URLs,
\Drupal::url()
(which takes a route name) is usually the correct replacement in procedural code. UrlGenerationTrait
is available for use in classes.- For now, uses of
url()
for external URLs (if any) could be converted to directly useUrlGenerator::generateFromPath()
Remaining tasks
In a followup issue, we will rename url()
to _url()
to make the BC break explicit, and mark it deprecated. After that, the remaining uses of _url()
will be removed before 8.0.0
User interface changes
None.
API changes
None yet (in other issues).
Comment | File | Size | Author |
---|---|---|---|
#60 | interdiff.txt | 1.53 KB | tim.plunkett |
#60 | 2340251-url-60.patch | 224.57 KB | tim.plunkett |
Comments
Comment #1
dawehnerJust a start to show in which direction it could lead.
Comment #2
Wim LeersComment #3
Wim LeersComment #4
iMiksuComment #5
xjmSo if we remove all calls to
url()
, but don't removeurl()
, I don't think that makes sense. With this issue, we're basically saying that usingurl()
is not best practice (as it sort of circumvents the routing system, I guess), but we're offering users no documented replacements. It's not simply a matter of using\Drupal::url()
, because that only works for routes, whereasurl()
could take either a system path or an external URL. And #2339219: [meta] Finalize URL generation API (naming, docs, deprecation) seems to suggest thatUrlGenerator::generateFromPath()
, which is whaturl()
currently wraps, should not be used for internal paths.Comment #6
iMiksuHere's an re-roll.
aggregator.theme.inc
failed asurl()
was replaced by usage ofcheck_plain()
in #2261425: Streamline aggregator's entities rendering with rest of core.Comment #7
dawehnerI think we should convert all INTERNAL calls away from url() and convert all EXTERNAL (not necesassary external URL (see other issue)) to use the new method on the url generator
and drop the long documentation on url() saying: use this in case you want to do X and use that if you want to do Y.
Comment #8
xjmThanks @dawehner. I read the other issue again but still confused about what an "external call" is, if it doesn't mean "a call to create a url from http://example.com". Internal or external to what code?
Comment #9
tim.plunkettSee #2342925: Simplify url() in order for the installer to function without it, which you'll need for the installer.
Comment #10
Wim LeersI'll push this one forward.
Comment #11
Wim LeersI did another 60. There should be enough examples now.
Comment #13
Wim LeersWe'll be able to remove
PathProcessorFront::processOutbound()
once all internal URL generation is route-based. There already is a<front>
route.Comment #14
dawehner@Wim Leers
Maybe we should keep it, just for consistence, but yeah this would help people to find the bad calls.
Comment #16
xjmCritical based on #2339219: [meta] Finalize URL generation API (naming, docs, deprecation).
For this issue, let's start by converting as many straightforward
url()
calls as possible. If there are a few calls that are difficult to convert, we will leave them in for now and renameurl()
to_url()
. I'll file issues for the next steps.Comment #17
xjmComment #18
xjmComment #19
xjmComment #20
iMiksuChecking invalid PHP syntax.
Comment #21
iMiksuThere you go.
Comment #22
iMiksuWorking on this slightly further.
Comment #23
Upchuk CreditAttribution: Upchuk commented@iMiksu,
Does your patch include all occurrences of old url()?
Comment #24
iMiksu@Upchuk no it does not, just couple more, if you interested you may continue from here, I'll get some sleep.
Comment #25
Upchuk CreditAttribution: Upchuk commentedSame here. will check back tomorrow and will try to help out if I can still find anything. There's also #2343651: Remove most remaining l() calls I can take a look at.
Good night.
Comment #26
xjmThanks @iMiksu!
Let's get some test results. :)
Comment #27
larowlanNote #2342949: Cleanup BlockContentBlock also touches one of these and is RTBC, if that helps
Comment #30
alexpottGonna take this on for a bit
Comment #31
alexpottInterim work to see where I'm at
Some points of interest - generateFromRoute has less options than generateFromPath so things like adding a base path don't work. Also the way in which symfony's url generator encodes url's is different from Drupal's use of rawurlencode so had to override it in our generator.
Comment #32
Wim Leers#31: interesting! I found another, related, problem with using the Symfony URL generator, described over at #2343651-15: Remove most remaining l() calls: Drupal likes
http://example.com?destination=admin/people
, but Symfony doeshttp://example.com?destination=admin%2Fpeople
. There's no way to override that though, unless we override all ofdoGenerate()
.Comment #34
alexpottSo I think we should create a follow up for the non clean url tests in DownloadTest and ImageStylesPathAndUrlTest. We can't make the changes to \Drupal\user\EventSubscriber\MaintenanceModeSubscriber - this is just too hard for this issue - we need to split this subscriber up - atm it occurs before the request context that is set - moving it after has issues to - let's address this in #2294571: Redirect anonymous users to login page from an exception listener instead of in MaintenanceModeSubscriber and restrict access to the my-account link to authenticated users and #2288911: Use route name instead of system path in user maintenance mode subscriber.
Someone else's turn to take this through to green and rtbc.
Comment #36
tim.plunkettMy turn!
Comment #37
tim.plunkettPer discussion with @dawehner, NullGenerator needs to hardcode for <front>.
Also I fixed the taxonomy changes, and made a few other conversions.
Two patterns we need to address:
url(NULL, array('absolute' => TRUE))
is used for #field_prefix in many places, just to get the http://drupal8/subdir/ part.url(current_path())
used in various places, like Views.Comment #38
dawehnerWorking on that.
Comment #40
dawehnerSome more.
Comment #42
alexpottTurns out that this is not so novice after all - sometimes when a replacement fails it can be really hard to work why and which one is causing the issue
Comment #43
tim.plunkettA major problem I'm seeing is that the request context, when retrieved from the container, is not updated with the info from the request.
Unless someone knows how to get the request passed to fromRequest in the container, I added a Drupal\Core\Routing wrapper class with fromRequestStack.
This should fix some of the fails from #37, I haven't looked into the new fails in #40.
Still working on this.
Comment #45
tim.plunkettI had some trouble with LanguageUILanguageNegotiationTest, \Drupal::url() seems to be skipping the language processing?!
I'll have to revisit that. Used generateFromPath() for now.
I didn't *need* to change all of the
assertEqual($this->getUrl(),
toassertUrl(
, but I'm glad I did because it found a couple bugs. I only touched lines covered by this patch anyway.Comment #47
tim.plunkettDurpal!
Comment #49
tim.plunkettComment #51
tim.plunkettI'll be back in 8-9 hours to work on this more, but I can't get Drupal\aggregator\Tests\AggregatorRenderingTest to fail locally, help needed there.
Comment #53
dawehnerWoorking on fixing the test and cleanups
Comment #54
dawehner@tim.plunkett
It is incredible helpful to have a apache configuration for
localhost/d8
and one ford8.dev
This should be green now.
Comment #55
Wim LeersFixing of remaining
url()
calls, analysis of the ones that we can't convertI'm still working on this.
I'm not working on anything in the current patch, so rerolling to address the review below can happen in parallel while I'm fixing remaining calls.
Review
(I reviewed the entire patch, looked at every hunk.)
What if somebody alters the route to not point at
/
, but at/llamas/are/fun
? Won't this then be incorrect?It's not clear to me when
NullGenerator
is used; if this is only used in tests, then this would be okay.Wow, that seems like a pretty major oversight on Symfony's side?
Looks like a regression. I'm guessing this is because the values in
baseFieldDefinitions()
must be static, and therefore the URL could be incorrect in a multisite setup?We need to fix this before RTBC I think?
Same.
Huh? Why doesn't it?\Drupal::url()
is an alias forUrlGenerator::generateFromRoute()
, which callsUrlGenerator::processPath()
; the language prefixing is done bylanguage.module
'sOutboundPathProcessor
s.This is the rewriting of the *domain*, which indeed only happens in
UrlGenerator::generateFromPath()
. Also applies toDrupal\language\Tests\LanguageUrlRewritingTest
I think something like
system.private_files_url_generator
or preferably evensystem.private_file_download
would be much better. The current route name suggests this can be used for all file downloads, which is not the case.Unfortunate, but this is at least pretty harmless. This @todo we could just commit as-is, IMO.
As you can tell: very, very little to complain about for a >200K patch.
Comment #56
tim.plunkett#55.1 That is only ever used by the early installer. By the time system.module is installed, the NullGenerator is swapped out.
#55.2 RequestContext is in symfony/routing, Request and RequestStack is in symfony/http-foundation. So that does seem like an oversight, maybe @dawehner can open an issue upstream?
#55.3 I didn't remove this. I don't know if its a problem, or just that it seemed like overkill
#55.4 and #55.5 These I didn't comment out either, but unless there is a good reason from @dawehner, I agree we should fix here
#55.6 This seems like a pre-existing bug, and it seems to me that it deserves its own issue.
#55.7 Either name is fine by me.
This patch is big, it removes more than half of the url() calls. #2344487: Provide special route names for <current> and <none> will help us remove even more.
I don't know if we should continue to expand this patch to include more conversions...
Comment #57
Wim Leersurl()
so we can keep (not comment) the test coverage.system.private_file_download
.I've stopped converting additional ones, this reroll includes the ones I already did. From my POV, #54 is RTBC as long as we revert #55.4, #55.5 and #55.6. If you can review and approve the attached interdiff, and attach a reroll with the revert, then we can get this big step forward committed.
There will be 131 calls to
url()
left.Comment #59
tim.plunkettThe last hunk of the interdiff is what fixes #55.4 and #55.5. That may need a follow-up to always update the request context when requests are pushed on or off the stack.
Otherwise the rest are reverts. I'd consider this to be RTBC if it passes.
Comment #60
tim.plunkettOh now I feel extra good about that change, see the interdiff. They were called together before, now its always done.
Comment #61
Wim LeersLooks great. RTBC per #56, #57 and #59.
Comment #62
dawehnerGreat teamwork!
Comment #63
catchHaven't gone through the whole patch yet, mostly repeating previous reviews here:
I do think we need an upstream issue, that looks like a straight bug to me.
Same question here for the upstream issue, again it looks like a straight bug (or a very poor choice if not).
Might be worth a novice follow-up to rename that display.
Comment #64
dawehnerNot sure here, the symfony one provides nicer URLs.
I am probably the only person on earth who hates renaming displays. Once you do that, especially when people don't use at least proper prefixes, it can be pretty annoying to figure out which kind of display this is.
Comment #65
xjmWe should probably create a followup issue for removing our code if/when the upstream issue is resolved, and put a @todo referencing it in the codebase. Doesn't need to block commit.
But since this would be a core VDC patch we get the final say in what the name would be? :)
Comment #66
tim.plunkettRegarding the
+ '%2F' => '/',
hunk:Upstream UrlGenerator includes decoding:
We only need the / one, so we're overriding. It's not that they need to change something upstream, just that our URLs are "uglier" because they expect less decoding.
And to clarify the naming thing.
Core's policy is to have the displays named
$display_type . '_' . $i
, starting with 1. Asking for more descriptive names means no easy rule, more bikeshedding, and risking the naming getting stale if the purpose of something changes. So let's stick with feed_1 here.Comment #67
webchickOk, to keep pre-beta things moving along, since timezones suck, and it looks like most of catch's review is addressed, committed and pushed to 8.x. I did add the following @todo to core/lib/Drupal/Core/Routing/RequestContext.php in the class definition PHPDoc:
Comment #69
webchickComment #70
xjm