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
We added Drupal::l() as a helper shortcut for generating a link.
l() takes the path, Drupal::l() uses a route name and route params.
However, since the introduction of the Url object, we now have more code using the object than the route names directly.
Proposed resolution
Switch Drupal::l() to expect a Url object
Remaining tasks
N/A
User interface changes
N/A
API changes
Drupal::l() now is a wrapper for \Drupal\Core\Utility\LinkGeneratorInterface::generateFromUrl() instead of \Drupal\Core\Utility\LinkGeneratorInterface::generate().
Comment | File | Size | Author |
---|---|---|---|
#45 | 2277103.45.patch | 1.6 KB | alexpott |
#41 | interdiff.txt | 13.95 KB | Wim Leers |
#41 | 2277103-drupal-l-41.patch | 89.8 KB | Wim Leers |
#38 | interdiff.txt | 38.83 KB | tim.plunkett |
#38 | 2277103-l-generator-38.patch | 90.08 KB | tim.plunkett |
Comments
Comment #1
tim.plunkettComment #3
cilefen CreditAttribution: cilefen commentedComment #4
hotpizzas CreditAttribution: hotpizzas commentedModified conflicts in the following files.
CONFLICT (content): Merge conflict in core/themes/seven/seven.theme
CONFLICT (content): Merge conflict in core/modules/menu_ui/src/MenuForm.php
CONFLICT (modify/delete): core/modules/menu_link/menu_link.module
CONFLICT (modify/delete): core/modules/contact/src/CategoryForm.php
CONFLICT (content): Merge conflict in core/modules/comment/src/CommentBreadcrumbBuilder.php
CONFLICT (content): Merge conflict in core/modules/comment/src/Controller/AdminController.php
CONFLICT (content): Merge conflict in core/modules/block_content/src/BlockContentTypeForm.php
CONFLICT (content): Merge conflict in core/modules/block_content/block_content.pages.inc
CONFLICT (content): Merge conflict in core/includes/menu.inc
CONFLICT (content): Merge conflict in core/includes/common.inc
Deleted files: core/modules/contact/src/CategoryForm.php
core/modules/menu_link/menu_link.module
Modified core/includes/menu.inc the function in conflict had been moved to core/lib/Drupal/Core/Render/Element/Link.php
The method name is preRenderLink
Comment #6
cilefen CreditAttribution: cilefen commentedComment #7
xjmComment #8
xjmWe discussed in #2343651: Remove most remaining l() calls the impact this switch would have on links to entities, which currently have a couple ugly flavors in that issue. When you have the full entity object:
When you don't:
Switching
l()
to take aUrl
object would definitely cut down the verbosity for entities (needing to chain the link generator and the generation method) and bring entity links more in line with (then other uses of)\Drupal::l()
generally, but what about the extra tedium of generating your Url object all the time?Also, we'd need to consider the DX of having
\Drupal::l()
and\Drupal::url()
be parallel and do the "same thing" for URL creation, vs. taking different types of arguments.Comment #9
tim.plunkettAfter #2343651: Remove most remaining l() calls goes in, there will be a ton of new calls to Drupal::l().
#2345793: Entities should provide a method for a properly generated link to the entity really covers a LOT of them, since so much of this is entity-based.
I think we should repurpose this to add a linkFromUrl() or something to LinkGeneratorTrait instead. It won't benefit .module code, but I think Drupal::l() is becoming too useful to switch it to take Url.
Any thoughts before I update the title/issue summary?
Here's an example patch (deviates from the previous patches completely)
Comment #12
tim.plunkettTraits can't conflict with property definitions.
Comment #14
tim.plunkettThat patch didn't have the interdiff applied. Maybe this 18 hour day is a bit too much for me.
Comment #15
dawehnerSeems valid.
Comment #16
Wim LeersIMHO that's a non-issue, because going from
to:
really isn't painful; it actually shows clearly that those two parameters that used to be there are really a pair (some programming languages allow to pass pairs of values!); by encapsulating them in a
Url
object, which is not painful at all, as the example demonstrates, just makes that more explicit.+1. The question is: do we address that here or in a follow-up?
Is it really that bad to change
to:
IMHO that's okay. Same argument as the one at the beginning of this comment.
The other way around :P
Comment #17
dawehnerAt least the link generator itself should support both, it is written to support different usecases anyway. I would kind of prefer the first one,
because it adds less coupling between the various systems.
Well, this is quite some API change in that case. A lot of contrib projects might break again here. On other point is that people
might start to use Url::createFromPath() which then would be horrible again.
Comment #18
xjmBumping to beta-blocking based on #2339219: [meta] Finalize URL generation API (naming, docs, deprecation).
Comment #19
Wim LeersWe (effulentsia, xjm, pwolanin and I) discussed this today at length as part of #2339219, and we concluded this: #2339219-55: [meta] Finalize URL generation API (naming, docs, deprecation) (please go and read that comment).
From that conclusion, the following is what applies to this issue:
Url
object\Drupal::l()
should also accept aUrl
object, not the route name, parameters, options trifecta, for consistency, and it's then only a procedural alias for the only way to callLinkGenerator
, which is conceptually very easy to understandAlso checked with dawehner, and he can live with this also.
This patch does not build on top of previous patches in this issue. Drupal installs, all unit tests pass, so I'm fairly hopeful this one will be green :)
Comment #20
Wim LeersComment #22
dawehnerSo we basically drop that helpful information what options could be about?
I mean really, those could be jumped over
But this is \Drupal::l() right?
\Drupal::l()
Meh, you could have tried to make the diff smaller by not changing that.
Did you really killed some test coverage? Just curious.
Comment #23
pwolanin CreditAttribution: pwolanin commentedDiscussed with dawehner in person - since the doc changes are done, maybe leave them. Converted 2 more calls to Drupal::l() and fixed a bunch of missing use statements causing fatal.
Comment #24
pwolanin CreditAttribution: pwolanin commentedComment #26
dawehnerLet me try to figure out the remaining failures.
Comment #27
dawehnerGive Wim some sleep, seriously, @random guy out there, don't burn your people. This is horrible if you want to release this Drupal 8 thing.
Comment #28
dawehner.
Comment #29
Wim LeerssetRequest()
calls. Without them, the comments no longer made sense. It took me a long time to figure out why the hell this was testing seemingly identical things multiple times… until I realized that's exactly what it was doing: it was testing the same thing multiple times. Hence I just removed the duplicates and updated the comments to be correct.Comment #31
pwolanin CreditAttribution: pwolanin commentedI think dawehner and I had some miscommunication in git - some use statements went missing again in his last patch.
Let's see if this is working.
Comment #33
tim.plunkettPHPStorm:
Run inspection by name
Undefined class
Custom Scope: Changed files
Yay.
Comment #35
larowlan/me takes a look
Comment #36
tim.plunkettI didn't realize we were *removing* the other methods from LinkGenerator!
The issue title says "switch", which is fine...
And if we do go ahead with removing generate(), we should really just rename generateFromUrl.
I don't think we should have removed generateFromLink without removing Link.
Comment #37
larowlanor not
Comment #38
tim.plunkettOkay, I decided to make those changes. I also ensured we weren't using $this->getLinkGenerator()->generate( where we could be using $this->l(
If this is green, I'd consider it RTBC.
Comment #39
pwolanin CreditAttribution: pwolanin commentedthis is basically RTBC, but there is a small conflict with #2343759: Provide an API function to replace url()/l() for external urls, so let's put that in first and then fix this and get it in.
Comment #40
xjmUnpostponing. Neither blocks the other. :)
Comment #41
Wim LeersI reviewed all interdiffs since my patch in #19.
LinkGenerator::generateFromLink
LinkGenerator::generateFromUrl()
/LinkGenerator::generate()
/\Drupal::linkGenerator()->generate()
calls with\Drupal::l()
calls, for consistency. (At dawehner's request :)) These are trivial changes.Sadly the #38 interdiff was incomplete, it doesn't list the
$this->l()
changes. So I had to review the entire 90K patch after all :PWhile doing that, @dawehner discovered that
LinkGenerator::generate()
doesn't callsetUrlGenerator()
on the passed inUrl
object. It should, because it avoids three method callsUrl::toString()
call… which can be quite a lot. The optimization simply didn't exist onLinkGenerator::generateFromUrl()
before (probably because it was undocumented). It was approved by both pwolanin and myself.Since there are only simple changes and we really need to get this done: assuming it comes back green: RTBC.
Comment #42
alexpottCommitted 4f49087 and pushed to 8.0.x. Thanks!
Comment #44
mradcliffeHEAD is broken on standard install and install with drush with standard install.
Here's a trace when going to core/install.php on a fresh git clone.
Comment #45
alexpottThe problem is we have other objects called Url in different namespaces -
Drupal\views\Plugin\views\field
,Drupal\Core\Render\Element
Not sure why this was not caught earlier.
Comment #46
mradcliffeI applied the patch and was able to load the installation screen manually. Waiting on report from testbot_ci which also reproduced the issue with standard install profile using Drush.
Comment #47
jaredsmith CreditAttribution: jaredsmith commentedTesting the patch 44 using the new testbot infrastructure using the drush installer. The installation worked as expected.
Comment #48
webchickGreat catch.
Committed and pushed to 8.x. Thanks!
Comment #50
ArlaFound and updated this change record: https://www.drupal.org/node/2189619 ("Use the \Drupal\Core\Url object in place of arrays of route info").
IS API changes might need an update as well.