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
The LinkGenerator calls drupal_render() so has an implicit and undeclared dependency on the render service
Proposed resolution
Inject the service in the constructor and remove the deprecated function call
Remaining tasks
make patch, fix tests that need to construct a LinkGenerator
User interface changes
n/a
API changes
n/a
Beta phase evaluation
Issue category | Bug because the link generator has an implicit and undeclared dependency on the render service, and requires several extra method calls because it uses the deprecated function |
---|---|
Issue priority | Normal |
Prioritized changes | The main goal of this issue is performance. |
Disruption | Not disruptive for core/contributed and custom modules/themes |
Comment | File | Size | Author |
---|---|---|---|
#15 | interdiff-15.txt | 1.3 KB | joshi.rohit100 |
#15 | 2505669-15.patch | 4.61 KB | joshi.rohit100 |
#10 | increment.txt | 736 bytes | pwolanin |
#10 | 2505669-10.patch | 4.61 KB | pwolanin |
#8 | inject_render_service-2505669-8.patch | 4.6 KB | jcloys |
Comments
Comment #1
pwolanin CreditAttribution: pwolanin at Acquia commentedFirst pass to see what tests fail.
Comment #4
willzyx CreditAttribution: willzyx commentedComment #6
pwolanin CreditAttribution: pwolanin at Acquia commentedremaining work is pretty novice - needs fixes in 2 test classes.
Comment #7
jcloys CreditAttribution: jcloys commentedWorking on the test.
Comment #8
jcloys CreditAttribution: jcloys commentedComment #9
larowlanThis is a performance boost too, less calls to the static
Comment #10
pwolanin CreditAttribution: pwolanin at Acquia commentedMinor - missed the variable name in @param
Comment #11
kgoel CreditAttribution: kgoel at Forum One commentedI applied the patch, and looked at the patch in PhpStorm. I had a question about injected service into the LinkGenerator constructor and pwolanin said it's not considered as an API change and it wont break BC.
#9 stated that it would improve performance so I am doing next. I will post performance result shortly.
Comment #13
dawehnerAt some point we need to talk about that. Its worth to see the talk fabpot about the deprecation note, its never as easy as you think.
For now though, this is certainly the right thing to do, especially given how you would override the link generator.
Comment #14
Wim LeersA bunch of nits. I'd feel bad for un-RTBC'ing it, but the beta evaluation is missing here too. Plus, it's an easy fix.
Nit: we usually write "The renderer."
Nit: The middle line can be deleted.
Comment #15
joshi.rohit100Comment #16
Wim LeersYou can omit the "service" but, but that'd be okay too.
Now let's add the beta evaluation still.
Comment #17
pwolanin CreditAttribution: pwolanin at Acquia commentedComment #18
pwolanin CreditAttribution: pwolanin at Acquia commentedadded beta eval
Comment #19
dawehnerLet's get it in, we have all we need.
Comment #21
xjmSo, #2393329: Replace all drupal_render calls with the service, and inject it, if possible. was postponed to 8.1.x, and we also agreed there to make all these changes in one single patch rather than in dozens of small issues. It'd be good going forward to keep in mind the recommendation made on the parent issue, and also note the part of the beta policy prioritized changes that says:
(Emphasis added.)
drupal_render() is deprecated for 9.0.0.
However, since this keeps coming up with regard to
drupal_render()
in particular, I discussed this more with @Wim Leers and @berdir to understand the importance. @Wim Leers said that replacing a use of the static service call (whichdrupal_render()
wraps) with an injected service does actually make a noteworthy performance difference relative to that bit of code, and since LinkGenerator is very much in the critical path, this issue in partiular is worth treating as a performance improvement.In general, however, replacing code deprecated for 9.x is not a prioritized change during the beta, nor is general refactoring for dependency injection. Please help communicate this on other issues that you see in the queue along those lines.
So this patch individually is a good improvement during the beta. I'll follow up more on #2393329: Replace all drupal_render calls with the service, and inject it, if possible. (basically, replacing it with an injected service in frequently used code is worthwhile, but simply converting the procedural call to a static method call isn't really in scope for the beta). Thanks everyone for your work here! Committed and pushed to 8.0.x.
Comment #22
xjm(Note that I reverted and re-committed this to add proper reviewer credit.)
Comment #24
xjm