Replace usages of deprecated method drupal_render_root()

Uploading a patch.

Many of those should have the service injected rather than calling \Drupal:Service()

here is the updated patch as per #4.
Thanks @NickWilde

@ieguskiza: ah yes I see that, didn't notice that before. Can't say it makes sense to me but I won't argue with @AlexPott without pretty good reason - generally I'd assume he knows better than me.

@ieguskiza: I was not notice you are working on it, my bad!! Please carry on.

Use Patch #3
If drupal_render_root() replace with \Drupal::service('renderer')->renderRoot(); than remove
-
function drupal_render_root(&$elements) {
return \Drupal::service('renderer')->renderRoot($elements);
}
in common.inc file .

@amit.drupal: defnitely not in this patch - it is deprecated but will be left until Drupal 9 for backwards compatibility.

Reuploading #3 -- if the final patch on the issue is not the correct patch, it causes problems for our testing infrastructure and also confuses people. Thanks!

Thanks @ieguskiza and @NickWilde for being thoughtful about the issue's scope here.

@alexpott and I discussed his feedback in #2704871-20: Replace usages of deprecated method drupal_render() as compared to this issue. For both these functions, we have two overlapping goals:

1. Remove the use of code that is already deprecated so that core is correct.
2. Remove uses of globals from classes for better decoupling, unit testing, etc.

The differences between this issue and that one were:

1. The reason he asked for the issue to be rescoped is that it was originally only scoped to the node module, which is incorrect for the reasons described in https://www.drupal.org/core/scope#files. In this issue, we're already correctly updating all of core -- good work. :)
2. drupal_render() is used in everything from global includes, to procedural module code, to tests and properly decoupled OO code. That means that the different uses will eventually need different patterns of fix -- so it makes sense to make the most straightforward change first for reviewability and then go from there. However, for this issue, all uses of drupal_render_root() are already in classes, so there is a single conceptual change.
3. There are over 100 uses of drupal_render() to be updated. In contrast, drupal_render_root() only has seven plus a documentation reference.

It's not that it's wrong to simply replace all the usages with Drupal::service() -- that does make it better, because it solves the first problem of having deprecated code usages. It is still a partial fix, though, and when every single usage will need to be changed again in another way in another issue, and there are fewer than 10 of them, it also doesn't save us too much to do it in two steps.

So let's go ahead and continue with injecting the service.

Thanks everyone!

Thanks @poornima,
Can you please provide an interdiff?

Please suggest :

If drupal_render_root() replace with \Drupal::service('renderer')->renderRoot();

than remove function in " core/lib/Drupal/Core/Render/MainContent/AjaxRenderer.php " path

protected function drupalRenderRoot(&$elements) {
return drupal_render_root($elements);
}

@sidharthap interdiff for (2818677_15.patch) patch

@amit.drupal: adjusted the message/added deprecated notice. Not removing so as to maintain BC. Thanks :)

New patch attached. Fully injects the service via the relevant method (container->get or service call) per class.

One note (mostly for xjm or other reviewers):

+++ b/core/modules/rest/src/Tests/Views/StyleSerializerTest.php
@@ -682,8 +690,6 @@ public function testFieldapiField() {
   public function testGroupRows() {
-    /** @var \Drupal\Core\Render\RendererInterface $renderer */
-    $renderer = $this->container->get('renderer');
     $this->drupalCreateContentType(['type' => 'page']);
     // Create a text field with cardinality set to unlimited.
     $field_name = 'field_group_rows';
@@ -726,7 +732,7 @@ public function testGroupRows() {

@@ -726,7 +732,7 @@ public function testGroupRows() {
     // Check if the field_group_rows field is grouped.
     $expected = [];
     $expected[] = [$field_name => implode(', ', $grouped_field_values)];
-    $this->assertEqual($serializer->serialize($expected, 'json'), (string) $renderer->renderRoot($build));
+    $this->assertEqual($serializer->serialize($expected, 'json'), (string) $this->renderer->renderRoot($build));

This could be done in a separate issue but since it was quite minor it seemed to me that it should just be done now/while that file is being edited. (Couple other instances of the equivalent change as well)

:( missed semicolon.

harrumph I clearly should have tested locally first.

Perhaps I shouldn't have been doing *anything* yesterday... went and didn't include one of the changes after local testing.

phew! that should be all.

protip: Run *all* tests locally not just the classes you think need to be run to avoid issue spam and embarrassment.
confession: I didn't do that.

The latest patch no longer applies against 8.4.x
Rerolled #31

Rerolled again

I don't know what happened in the last patch, anyway #34 apply correctly against 8.4.x

I raise the patch again (#34), to prevent the bot from putting this task back in "Need work". I think that needs review.

This patch #41 does not apply to 8.6.x.

Here is a rerolled patch.

Here is updated patch. Hopefully this time all the tests will pass.

Updated patch with fix reported by test bot. Hopefully this time all the tests will pass.

I have 2 very small nitpicks, but otherwise this is looking great!

1. +++ b/core/lib/Drupal/Core/Render/MainContent/AjaxRenderer.php
   @@ -75,12 +86,13 @@ public function renderResponse(array $main_content, Request $request, RouteMatch
   -   * Wraps drupal_render_root().
   +   * Wraps renderRoot().
   

   Not sure if this makes a lot of sense now.

2. +++ b/core/lib/Drupal/Core/Render/MainContent/AjaxRenderer.php
   @@ -75,12 +86,13 @@ public function renderResponse(array $main_content, Request $request, RouteMatch
   +   * @deprecated as of Drupal 8.3.x, will be removed before Drupal 9.0.0. Use
   

   incorrect version, should be 8.7.x.

This patch #53 does not apply anymore on 8.6.x. Needs reroll.

@borisson_ Thanks for the review.

Not sure if this makes a lot of sense now.

Yes, this still makes sense. The more appropriate version can be Wraps \Drupal\Core\Render\RendererInterface::renderRoot()

Here is the updated patch.

Awesome, thanks for fixing my nits.

+++ b/core/lib/Drupal/Core/Render/MainContent/AjaxRenderer.php
@@ -75,12 +86,13 @@ public function renderResponse(array $main_content, Request $request, RouteMatch
   /**
-   * Wraps drupal_render_root().
+   * Wraps \Drupal\Core\Render\RendererInterface::renderRoot().
    *
-   * @todo Remove as part of https://www.drupal.org/node/2182149.
+   * @deprecated as of Drupal 8.7.x, will be removed before Drupal 9.0.0. Use
+   *   $this->renderer or the 'renderer' service instead.
    */
   protected function drupalRenderRoot(&$elements) {
-    return drupal_render_root($elements);
+    return $this->renderer->renderRoot($elements);
   }

This need to trigger the deprecation. Something like

@trigger_error('\Drupal\Core\Render\MainContent\AjaxRenderer::drupalRenderRoot() is deprecated as of Drupal 8.7.x, will be removed before Drupal 9.0.0. Use $this->renderer or the 'renderer' service instead. See https://www.drupal.org/node/NID', E_USER_DEPRECATED);

Where NID is the change record that tells people about this. An @see needs to be added too. See https://www.drupal.org/core/deprecation

We also need to add the same @trigger_error() to drupal_render_root() to prove we've replaced all usages.

Here is the updated patch.

* @deprecated \Drupal\Core\Render\MainContent\AjaxRenderer::drupalRenderRoot()

In the above line, the line length is exceeding 80 characters. @alexpott I am not sure if the line length is allowed to exceed 80 characters if FQN of a method is too long in the @deprecated tag.

I wonder why @trigger_error() is not added to drupal_render() function when #2704871: Replace usages of deprecated method drupal_render() got fixed.

@msankhala

I wonder why @trigger_error() is not added to drupal_render() function when...

because the deprecation policy and support for testing was only implemented during the 8.4.x cycle.

Now this @trigger_error() added in \Drupal\Core\Render\MainContent\AjaxRenderer::drupalRenderRoot() is making the test fail. Should uses of \Drupal\Core\Render\MainContent\AjaxRenderer::drupalRenderRoot() in all test cases be replaced with rendere service?

@msankhala yep or the function is not deprecated.

Here is updated patch.

Ahh.. Missed the actual patch

I had a look at the error thrown by test bot. This is the error:

Drupal\Tests\Core\Controller\AjaxRendererTest
exception: [Other] Line 0 of sites/default/files/simpletest/phpunit-22.xml:
PHPunit Test failed to complete; Error: PHPUnit 6.5.10 by Sebastian Bergmann and contributors.

Testing Drupal\Tests\Core\Controller\AjaxRendererTest
E                                                                   1 / 1 (100%)

Time: 206 ms, Memory: 4.00MB

There was 1 error:

1) Drupal\Tests\Core\Controller\AjaxRendererTest::testRenderWithFragmentObject
Error: Call to a member function getCurrentRequest() on null

/var/www/html/core/lib/Drupal/Core/Render/Renderer.php:601
/var/www/html/core/lib/Drupal/Core/Render/Renderer.php:578
/var/www/html/core/lib/Drupal/Core/Render/Renderer.php:140
/var/www/html/core/lib/Drupal/Core/Render/MainContent/AjaxRenderer.php:73
/var/www/html/core/tests/Drupal/Tests/Core/Controller/AjaxRendererTest.php:65

By looking at the stack trace it is clear that issue is with \Drupal\Core\Render\Renderer::getCurrentRenderContext()

  protected function getCurrentRenderContext() {
    $request = $this->requestStack->getCurrentRequest();
    return isset(static::$contextCollection[$request]) ? static::$contextCollection[$request] : NULL;
  }

where $this->requestStack is null. I am not sure why requestStack is null.

Or the issue is with \Drupal\Tests\Core\Controller\TestAjaxRenderer class which is extending \Drupal\Core\Render\MainContent\AjaxRenderer
Or the issue is with the mock being created in \Drupal\Tests\Core\Controller\AjaxRendererTest::setUp() something like

$renderer = $this->getMockBuilder('Drupal\Core\Render\Renderer')
      ->disableOriginalConstructor()
      ->setMethods(NULL)
      ->getMock();

Any help is appreciated.

@msankhala we need to mock the renderer so we can test the \Drupal\Core\Render\MainContent\AjaxRenderer class in isolation.

1. +++ b/core/lib/Drupal/Core/Render/MainContent/AjaxRenderer.php
   @@ -75,12 +86,17 @@ public function renderResponse(array $main_content, Request $request, RouteMatch
   +   * is deprecated as of Drupal 8.7.x, will be removed before Drupal 9.0.0. Use
   +   * $this->renderer or the 'renderer' service instead.
   

   This deprecation message doesn't need to say or the 'renderer' service - any class that extends this one and calls this method should use $this->renderer. Fix in attached patch.

2. +++ b/core/tests/Drupal/Tests/Core/Controller/AjaxRendererTest.php
   @@ -1,16 +1,11 @@
   -/**
   - * @file
   - * Contains \Drupal\Tests\Core\Controller\AjaxRendererTest.
   - */
   -
   

   This is removed here because the PHPCS sniff that tests for these not being there doesn't work on files with more that one class. This file now only has a single class so fails PHPCS without this change.

Thanks @alexpott for the explanation and patch. I learned many things while working on this issue like deprecation policy and support, use of @trigger_error() in the context of deprecation, PHPCS rule for file docblock and mocking a service.

The patch #73 is not applying anymore needs reroll.

git apply -v 2818677-73.patch
Checking patch core/core.services.yml...
Checking patch core/includes/common.inc...
Checking patch core/lib/Drupal/Core/Render/MainContent/AjaxRenderer.php...
Checking patch core/lib/Drupal/Core/Render/MainContent/DialogRenderer.php...
Checking patch core/lib/Drupal/Core/Render/MainContent/ModalRenderer.php...
Checking patch core/lib/Drupal/Core/Render/MainContent/OffCanvasRenderer.php...
Checking patch core/modules/filter/tests/src/Kernel/FilterAPITest.php...
error: while searching for:

  public static $modules = ['system', 'filter', 'filter_test', 'user'];

  protected function setUp() {
    parent::setUp();

    $this->installConfig(['system', 'filter', 'filter_test']);
  }

  /**

error: patch failed: core/modules/filter/tests/src/Kernel/FilterAPITest.php:21
error: core/modules/filter/tests/src/Kernel/FilterAPITest.php: patch does not apply
Checking patch core/modules/rest/tests/src/Functional/Views/StyleSerializerTest.php...
Checking patch core/modules/system/tests/modules/dialog_renderer_test/dialog_renderer_test.services.yml...
Checking patch core/modules/system/tests/modules/dialog_renderer_test/src/Render/MainContent/WideModalRenderer.php...
Checking patch core/modules/views/src/Plugin/views/display/Feed.php...
Checking patch core/tests/Drupal/Tests/Core/Controller/AjaxRendererTest.php...

Here is the rerolled patch.

Now Patch #76 is no more valid. Patch #73 is valid again because of revert issue #2822382: Make every $modules property protected on classes extending BrowserTestBase and KernelTestBase

See https://cgit.drupalcode.org/drupal/commit/core/modules/filter/tests/src/...

I can confirm patch #73 applies cleanly and remove all the uses of drupal_render_root() function. After applying this patch i verified with grep.

grep -rn "drupal_render_root" core
core/includes/common.inc:867:function drupal_render_root(&$elements) {
core/includes/common.inc:868:  @trigger_error('drupal_render_root() is deprecated in Drupal 8.0.x and will be removed before Drupal 9.0.0. Use the "renderer" service instead.');

Setting it to rtbc.

The rtbc patch need to be the latest on the issue

1. +++ b/core/includes/common.inc
   @@ -865,6 +865,7 @@ function drupal_pre_render_links($element) {
   +  @trigger_error('drupal_render_root() is deprecated in Drupal 8.0.x and will be removed before Drupal 9.0.0. Use the "renderer" service instead.');
   

   s/8.0.x/8.7.x/

2. +++ b/core/lib/Drupal/Core/Render/MainContent/AjaxRenderer.php
   @@ -75,12 +86,16 @@ public function renderResponse(array $main_content, Request $request, RouteMatch
   +   * @deprecated in Drupal 8.7.x and will be removed before Drupal 9.0.0. Use
   

   Here it is correct.

@Wim Leers drupal_render_root() has been deprecated for ever.

/**
 * Renders final HTML given a structured array tree.
 *
 * @deprecated as of Drupal 8.0.x, will be removed before Drupal 9.0.0. Use the
 *   'renderer' service instead.
 *
 * @see \Drupal\Core\Render\RendererInterface::renderRoot()
 * @see https://www.drupal.org/node/2912696
 */
function drupal_render_root(&$elements) {
  return \Drupal::service('renderer')->renderRoot($elements);
}

D'oh, right. I was blinded by the fact that we're adding that trigger_error() now. But of course, we simply weren't adding that back in the 8.0 days!

Sorry for the distraction. 😅

1. +++ b/core/includes/common.inc
   @@ -865,6 +865,7 @@ function drupal_pre_render_links($element) {
   +  @trigger_error('drupal_render_root() is deprecated in Drupal 8.0.x and will be removed before Drupal 9.0.0. Use the "renderer" service instead.');
   

   This needs to reference a change record

2. +++ b/core/lib/Drupal/Core/Render/MainContent/AjaxRenderer.php
   @@ -29,14 +30,24 @@ class AjaxRenderer implements MainContentRendererInterface {
   +    $this->renderer = $renderer;
   

   i have quite a few classes in client projects that extend these renderers, we need a change record for this (Even though its allowed under our BC policy)

@larowlan made the deprecation slightly easier on projects that extend the renderers by doing a proper parameter add using the deprecation system :)

I've also added test coverage since the added deprecation forgot to add E_USER_DEPRECATION plus it is good practice to have some test coverage.

looks ok to me.

1. +++ b/core/includes/common.inc
   @@ -867,6 +867,7 @@ function drupal_pre_render_links($element) {
    function drupal_render_root(&$elements) {
   +  @trigger_error('drupal_render_root() is deprecated in Drupal 8.0.x and will be removed before Drupal 9.0.0. Use the "renderer" service instead. See https://www.drupal.org/node/2912696.', E_USER_DEPRECATED);
   

   8.0.0, and with the now officially accepted new rules, "drupal:8.0.0"?

2. +++ b/core/lib/Drupal/Core/Render/MainContent/DialogRenderer.php
   @@ -21,14 +22,28 @@ class DialogRenderer implements MainContentRendererInterface {
   +  public function __construct(TitleResolverInterface $title_resolver, RendererInterface $renderer = NULL) {
        $this->titleResolver = $title_resolver;
   +    $this->renderer = $renderer;
   +    if ($this->renderer === NULL) {
   +      @trigger_error('The $renderer parameter is added in Drupal 8.7.0 and will be required before Drupal 9.0.0. See https://www.drupal.org/node/3009400.', E_USER_DEPRECATED);
   +      $this->renderer = \Drupal::service('renderer');
   +    }
   

   maybe use the new approach that checks $renderer, above the assignment and updates that?

3. +++ b/core/modules/filter/tests/src/Kernel/FilterAPITest.php
   @@ -291,7 +299,7 @@ public function testProcessedTextElement() {
          '#format' => 'element_test',
        ];
   -    drupal_render_root($build);
   +    $this->renderer->renderRoot($build);
    
   

   we use $this->rendered only in a single place, lets not assign this to $this->renderer?

4. +++ b/core/modules/rest/tests/src/Functional/Views/StyleSerializerTest.php
   @@ -63,6 +70,7 @@ protected function setUp($import_test_views = TRUE) {
    
        $this->enableViewsTestModule();
   +    $this->renderer = $this->container->get('renderer');
      }
   

   this one does use it in 3 different methods, but maybe switch to \Drupal::service()?

5. +++ b/core/tests/Drupal/Tests/Core/Controller/AjaxRendererTest.php
   @@ -45,15 +42,25 @@ protected function setUp() {
          ]);
   -    $this->ajaxRenderer = new TestAjaxRenderer($element_info_manager);
   -
   -    $this->renderer = $this->getMockBuilder('Drupal\Core\Render\Renderer')
   +    $renderer = $this->getMockBuilder(Renderer::class)
          ->disableOriginalConstructor()
   -      ->setMethods(NULL)
          ->getMock();
   

   why are we using a mockBuilder here on the actual class, can't we mock with \Drupal\Core\Render\RendererInterface?

6. +++ b/core/tests/Drupal/Tests/Core/Controller/AjaxRendererTest.php
   @@ -78,24 +85,19 @@ public function testRenderWithFragmentObject() {
   +  public function testConstructorRendererArgument() {
   +    $element_info_manager = $this->getMock(ElementInfoManagerInterface::class);
   +    $container = $this->createMock(ContainerInterface::class);
   +    $container->expects($this->once())
   +      ->method('get')
   +      ->with('renderer')
   +      ->willReturn(NULL);
   +    \Drupal::setContainer($container);
   +    new AjaxRenderer($element_info_manager);
      }
   

   I personally find unit test for constructor deprecations overkill, but if it's already here..

Addressed my review as nobody else seemed very eager to do so :p

Fixed a left-over getMock() call.

Updating the deprecation messages (again) and their tests.

Looks all reviews addressed

1. +++ b/core/lib/Drupal/Core/Render/MainContent/AjaxRenderer.php
   @@ -22,14 +23,28 @@ class AjaxRenderer implements MainContentRendererInterface {
        $this->elementInfoManager = $element_info_manager;
   +    $this->renderer = $renderer;
   +    if ($this->renderer === NULL) {
   

   Why not do the same as we do here (and elsewhere in core), the slightly different logic meant I had to reread this a couple of times to see why it was special;

2. +++ b/core/lib/Drupal/Core/Render/MainContent/DialogRenderer.php
   @@ -21,14 +22,28 @@ class DialogRenderer implements MainContentRendererInterface {
        $this->titleResolver = $title_resolver;
   +    if ($renderer === NULL) {
   +      @trigger_error('The renderer service must be passed to ' . __METHOD__ . ' and will be required before Drupal 9.0.0. See https://www.drupal.org/node/3009400', E_USER_DEPRECATED);
   +      $renderer = \Drupal::service('renderer');
   +    }
   

   Like here.

1. Yeah, no reason, just old code, updated the other one but not this.

back to rtbc!

Committed b5238ac and pushed to 8.8.x. Thanks!

Credited @xjm, @ieguskiza, @borisson_, @catch, @larowlan for issue management and review.

publish the change record