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

Reference: https://www.drupal.org/core/beta-changes
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
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

pwolanin’s picture

Status: Active » Needs review
FileSize
2.5 KB

First pass to see what tests fail.

Status: Needs review » Needs work

The last submitted patch, 1: 2505669-1.patch, failed testing.

Status: Needs work » Needs review

pwolanin queued 1: 2505669-1.patch for re-testing.

willzyx’s picture

Fatal error: Uncaught exception 'PDOException' with message 'SQLSTATE[HY000]: General error: 13 database or disk is full' in /var/lib/drupaltestbot/sites/default/files/checkout/core/lib/Drupal/Core/Database/StatementPrefetch.php:175

Status: Needs review » Needs work

The last submitted patch, 1: 2505669-1.patch, failed testing.

pwolanin’s picture

Issue tags: +Needs beta evaluation, +Novice

remaining work is pretty novice - needs fixes in 2 test classes.

jcloys’s picture

Working on the test.

jcloys’s picture

Status: Needs work » Needs review
FileSize
1.82 KB
4.6 KB
larowlan’s picture

This is a performance boost too, less calls to the static

pwolanin’s picture

FileSize
4.61 KB
736 bytes

Minor - missed the variable name in @param

kgoel’s picture

Issue tags: +Performance

I 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.

pwolanin queued 10: 2505669-10.patch for re-testing.

dawehner’s picture

Status: Needs review » Reviewed & tested by the community

I 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.

At 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.

Wim Leers’s picture

Status: Reviewed & tested by the community » Needs work

A 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.

  1. +++ b/core/lib/Drupal/Core/Utility/LinkGenerator.php
    @@ -37,16 +38,26 @@ class LinkGenerator implements LinkGeneratorInterface {
    +   * The render service.
    ...
    +   *   The render service.
    
    +++ b/core/tests/Drupal/Tests/Core/Utility/LinkGeneratorTest.php
    @@ -43,6 +43,14 @@ class LinkGeneratorTest extends UnitTestCase {
    +   * The mocked render service.
    

    Nit: we usually write "The renderer."

  2. +++ b/core/tests/Drupal/Tests/Core/Utility/LinkGeneratorTest.php
    @@ -43,6 +43,14 @@ class LinkGeneratorTest extends UnitTestCase {
    +   * @var \PHPUnit_Framework_MockObject_MockObject|\Drupal\Core\Render\RendererInterface
    +   *
    +   */
    

    Nit: The middle line can be deleted.

joshi.rohit100’s picture

Status: Needs work » Needs review
FileSize
4.61 KB
1.3 KB
Wim Leers’s picture

You can omit the "service" but, but that'd be okay too.

Now let's add the beta evaluation still.

pwolanin’s picture

Issue summary: View changes
pwolanin’s picture

Issue tags: -Needs beta evaluation

added beta eval

dawehner’s picture

Status: Needs review » Reviewed & tested by the community

Let's get it in, we have all we need.

  • xjm committed 8d848f2 on 8.0.x
    Issue #2505669 by pwolanin, jcloys, joshi.rohit100: Inject render...
xjm’s picture

Issue summary: View changes
Priority: Normal » Major
Status: Reviewed & tested by the community » Fixed

So, #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:

code already marked for removal by 8.0.0

(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 (which drupal_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.

xjm’s picture

(Note that I reverted and re-committed this to add proper reviewer credit.)

  • xjm committed 0b10469 on 8.0.x
    Revert "Issue #2505669 by pwolanin, jcloys, joshi.rohit100: Inject...
  • xjm committed 0e2ca03 on 8.0.x
    Issue #2505669 by pwolanin, jcloys, joshi.rohit100, dawehner, Wim Leers...
xjm’s picture

Issue tags: +D8 Accelerate London

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.