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 #17
Problem/Motivation
It should be as easy as possible to have something like url() but using route names and parameters on controllers.
Proposed resolution
Add a url() method to FormBase adn ControllerBase so devs can call $this->url() without needing to use the DIC
Remaining tasks
None
User interface changes
None
API changes
No breaking changes.
New url() method added to 2 classes.
Related Issues
#2078285: Add short-cut methods to the \Drupal class for generating URLs and links from routes
#2073811: Add a url generator twig extension
#2078287: [meta] Fix the DX of the link generator
Comment | File | Size | Author |
---|---|---|---|
#24 | url-2073813-24.patch | 9.29 KB | twistor |
#21 | url-2073813-21.patch | 9.25 KB | pwolanin |
#21 | 2073813-20-21.increment.txt | 2.04 KB | pwolanin |
#20 | url-2073813-20.patch | 9.23 KB | pwolanin |
#20 | 2073813-16-20.increment.txt | 9.74 KB | pwolanin |
Comments
Comment #1
dawehnerComment #2
tim.plunkettWhoo! This is nice.
I don't think we need further test coverage here, we either have the coverage for the form or we don't, and the API we're using has coverage.
The docs are excellent, especially all of the @throws part, that's helpful.
Comment #3
dawehnerJust realized that array() is optional.
Comment #4
tim.plunkettComment #5
tstoecklerCan we name this $route_name in FormBase and ControllerBase? In UrlGenerator::generatorFromRoute() it's clear what $name means, but here it would be useful to be more explicit IMO.
Comment #6
pwolanin CreditAttribution: pwolanin commentedMakes sense to me - also makes it consistent with the redirect() method. Just changing the variable names, so leaving at rtbc.
Comment #7
dawehnerAt least in the link generator we also use #route_parameters.
Comment #8
tstoecklerNoticed some strange whitespace, so quickly re-rolled for that. Leaving RTBC.
This actually indicates that we have missing test coverage for adding a shortcut inline. Opened #2075557: Missing test coverage for adding a shortcut link inline for that. (See also the attached screenshot.)
Comment #9
alexpottSo we've had 4 patches since the rtbc in #2. Setting back to needs review so we can ensure that the current patch should be rtbc.
Comment #10
pwolanin CreditAttribution: pwolanin commentedAs a follow-up we should add another redirect() method (?) that takes a route and params?
Comment #11
dawehnerRedirect already works like that:
Comment #12
dawehnerI would be fine to set this as RTBC ... ;)
Comment #13
pwolanin CreditAttribution: pwolanin commentedYes, this looks good.
Comment #14
alexpottNeeds a reroll
Comment #15
pwolanin CreditAttribution: pwolanin commentedAdded Drupal:url() back to #2078285: Add short-cut methods to the \Drupal class for generating URLs and links from routes, since it's not in the patch and seems better suited there?
Comment #16
pwolanin CreditAttribution: pwolanin commentedtrivial conflict in the use statements due to the patch that added renamed to Drupal\Core\DependencyInjection\ContainerInjectionInterface;
Comment #17
pwolanin CreditAttribution: pwolanin commentedupdated the issue summary a bit
Comment #17.0
pwolanin CreditAttribution: pwolanin commentedUpdated issue summary.
Comment #17.1
pwolanin CreditAttribution: pwolanin commentedUpdated issue summary.
Comment #18
webchickLet's not add all of those docs for url() multiple times, but rather put a @see in there, similar to how we do t().
Comment #19
tim.plunkettAlso while rerolling, be sure to say
URL
in comments, noturl
(when its not the class name)Comment #20
pwolanin CreditAttribution: pwolanin commentedhow about this?
Comment #21
pwolanin CreditAttribution: pwolanin commenteddawehner points out that the docs are actually on the interfaces.
Comment #22
dawehnerThank you.
Comment #23
alexpottPatch no longer applies.
Comment #24
twistor CreditAttribution: twistor commentedComment #25
alexpottCommitted 95c2e17 and pushed to 8.x. Thanks!
Comment #26
catch#2083415: [META] Write up all outstanding change notices before release
Comment #27
catch#2083415: [META] Write up all outstanding change notices before release
Comment #28
dawehnerhttps://drupal.org/node/2076011/revisions/view/2825237/2832719
https://drupal.org/node/2060189/revisions/view/2829611/2832723
Comment #29
h3rj4n CreditAttribution: h3rj4n commentedI think the text on the pages is sufficient. Example code isn't needed for these functions as long as there is at least one occurrence in core as reference I guess.
Comment #30
dawehnerThank you, ... let's mark it was fixed.
Comment #31
xjmUntagging. Please remove the "Needs change notification" tag when the change notice task is complete.
Comment #32
xjmComment #33
xjmd.o--
So we need to reference this issue on those two change notices.
Comment #34
xjm**sigh**
Comment #34.0
xjmUpdated issue summary.
Comment #35
star-szrI added this issue to the two change notices, if that's all that's needed on top of @dawehner's edits this issue can be closed now.
https://drupal.org/node/2060189
https://drupal.org/node/2076011