Replace usages of deprecated method drupal_render()

Changing all ocurrences on node module files to \Drupal::service('renderer')->render().

1. +++ b/core/modules/node/node.module
   @@ -578,9 +578,9 @@ function template_preprocess_node(&$variables) {
   +  $variables['author_name'] = \Drupal::service('renderer')->render($variables['elements']['uid']);
   

   Let's store the service in a variable so we don't have to call it twice.

2. +++ b/core/modules/node/src/NodeListBuilder.php
   @@ -110,7 +110,7 @@ public function buildRow(EntityInterface $entity) {
   +      '#suffix' => ' ' . \Drupal::service('renderer')->render($mark),
   

   Let's inject the service instead.

3. +++ b/core/modules/node/src/Plugin/Action/UnpublishByKeywordNode.php
   @@ -24,7 +24,7 @@ class UnpublishByKeywordNode extends ConfigurableActionBase {
   +      if (strpos(\Drupal::service('renderer')->render($elements), $keyword) !== FALSE || strpos($node->label(), $keyword) !== FALSE) {
   

   Let's inject the service instead.

+++ b/core/modules/node/src/Plugin/Action/UnpublishByKeywordNode.php
@@ -24,7 +24,7 @@ class UnpublishByKeywordNode extends ConfigurableActionBase {
+      if (strpos(\Drupal::service('renderer')->render($elements), $keyword) !== FALSE || strpos($node->label(), $keyword) !== FALSE) {

Let's inject the service instead.

To inject the service on this point, I needed to modify the ExecutableInterface, but maybe the best solution would be to call the renderer service from the execute method implemented on UnpublishByKeywordNode class.

1. +++ b/core/lib/Drupal/Core/Executable/ExecutableInterface.php
   @@ -12,6 +12,6 @@
   -  public function execute();
   +  public function execute(RendererInterface $renderer);
   

   We cannot do API changes at this point, the service must be injected in the constructor.

2. +++ b/core/modules/node/src/Plugin/Action/UnpublishByKeywordNode.php
   @@ -21,10 +22,10 @@ class UnpublishByKeywordNode extends ConfigurableActionBase {
   -  public function execute($node = NULL) {
   +  public function execute(RendererInterface $renderer, $node = NULL) {
   

   Same here.

You need to do the same you already did in NodeListBuilder

Try to fix #8. Hope it works!

Try to fix previous failed test! Hope it works for now

diff --git a/core/CHANGELOG.txt b/core/CHANGELOG.txt
index ae0f620..609fd4a 100644
--- a/core/CHANGELOG.txt
+++ b/core/CHANGELOG.txt
@@ -11,7 +11,7 @@ Drupal 8.1.0, 2016-04-20
     * Added Symfony Polyfill Iconv 1.1.0.
     * Added paragonie/random_compat 1.4.1.
 - Updated vendor libraries:
-    * Updated to the last 2.x minor version of Symfony: 2.8 (2.8.4).
+    * Updated to Symfony 2.8.4.
     * Updated to CKEditor 4.5.8.
     * Updated to Modernizr 3.3.1.
 - Added modules:
@@ -34,13 +34,12 @@ Drupal 8.1.0, 2016-04-20
       reducing the code needed to extend them.
     * Simplified Migrate API by replacing migration configuration entities with
       migration plugins.
-    * Various improvements for defining entity types:
-      * Added support for entity types to specify translatable plural labels.
-      * Added a revision log interface and trait for revisionable entity types.
-      * Added key field definitions to ContentEntityBase, reducing code from
-        child classes.
-      * Added generic route providers for add-page and add-form entity routes,
-        reducing the code needed to define an entity type.
+    * Added support for entity types to specify translatable plural labels.
+    * Added a revision log interface and trait for revisionable entity types.
+    * Added key field definitions to ContentEntityBase, reducing code from
+      child classes.
+    * Added generic route providers for add-page and add-form entity routes,
+      reducing the code needed to define an entity type.
 - Testing improvements:

I'm pretty sure we shouldn't be making changes to the CHANGELOG.txt ;-)

Thanks #16 apologies for my mistake. Should be fine now!

Looks good to me and it's a good improvement, thanks for working on this one.

The only thing I'm not really sure it's if constructor arguments are considered an API change. I'm RTBCing this one so we get feedback from core committers.

Can we just have one patch to do a find and replace on the entire core to replace drupal_render() with \Drupal::service('renderer')->render().

And then we can have followup issues to inject the renderer service where necessary.

See https://www.drupal.org/core/scope

+++ b/core/modules/node/src/NodeListBuilder.php
@@ -44,11 +52,13 @@ class NodeListBuilder extends EntityListBuilder {
+  public function __construct(EntityTypeInterface $entity_type, EntityStorageInterface $storage, DateFormatterInterface $date_formatter, RedirectDestinationInterface $redirect_destination,
+                              RendererInterface $renderer) {

Also, now irrelevant because I think we should handle injection separately, this should all go on one line.

I would say this is a Novice task, steps are described in the IS.

Done as suggested by Alex.
Used following command.
grep -rl 'drupal_render()' core/ | xargs sed -i 's/drupal_render()/\\Drupal::service('renderer')->render()/g'
I hope that is ok.

This patch applies the needed change in the core directory.

Let @Alex to jump into this.

Patch in #25 applies and converts most calls to drupal_render() that aren't in documentation. There's still one in core/includes/common.inc.

Added follow-ups to issue summary.

+++ b/core/lib/Drupal/Core/Theme/ThemeManager.php
@@ -303,7 +303,7 @@ public function render($hook, array $variables) {
-        drupal_render($preprocess_bubbleable);
+        \Drupal::service('renderer')->render($preprocess_bubbleable);

This is in a service. In services, the renderer service should be injected.

@wim leers: Check out #20. The service will still work with \Drupal, and we'll poke through and inject usages appropriately in a follow-up. This way we at least have deprecated drupal_render() with an easy-to-review patch.

Here's the beginnings of that follow-up: #2729597: [meta] Replace \Drupal with injected services where appropriate in core

Looks like a failing test in Drupal\views_ui\Tests\PreviewTest:

fail: [Fatal error] Line 0 of :
[19-May-2016 17:02:00 Australia/Sydney] Uncaught PHP Exception LogicException: "Render context is empty, because render() was called outside of a renderRoot() or renderPlain() call. Use renderPlain()/renderRoot() or #lazy_builder/#pre_render instead." at /var/www/html/core/lib/Drupal/Core/Render/Renderer.php line 241

I'm still learning Drupal 8 so i read a lot of code from core and core modules. Solutions like these only teaches people bad practices.
You already see lots of contrib modules using \Drupal::service('foo') outside of legacy procedural code and that's sad imho.

You already see lots of contrib modules using \Drupal::service('foo') outside of legacy procedural code and that's sad imho.

Absolutely.

However, this patch is big and will be hard to review and commit if we don't do that. drupal_render() is officially deprecated whereas \Drupal is not.

We'll do the IoC injection work on #2729597: [meta] Replace \Drupal with injected services where appropriate in core

If we could get those two tests passing in #30... :-)

I think it's the change in the interdiff. That might need to be the bare_html_page_renderer service instead of renderer.

Hi Mile23,

Done changes as per your suggestions.

Please review.

Thanks!!

+1 for RTBC.

Hi,
I fixed the problem with the latest patch but I'm unsure if it is inside of the scope of the issue since it involves another deprecated function: drupal_render_root.

I can remove this fix and move it into an entirely separate issue if needed, but in the meantime, the patch is ready to review.

Thanks @ieguskiza for working on this!

1. +++ b/core/includes/common.inc
   @@ -21,6 +21,7 @@
   +use Drupal\Core\Render\BareHtmlPageRenderer;
   
   @@ -896,7 +897,7 @@ function drupal_render_children(&$element, $children_keys = NULL) {
   -      $output .= drupal_render($element[$key]);
   +      $output .= \Drupal::service('bare_html_page_renderer')->render($element[$key]);
   

   Should we be using this here?

2. +++ b/core/modules/views/src/Plugin/views/display/Feed.php
   @@ -92,7 +92,7 @@ public function preview() {
   -        '#plain_text' => drupal_render_root($output),
   +        '#plain_text' => \Drupal::service('renderer')->renderRoot($output),
   

   Should this be part of this patch or should we split this off into a different issue?

Patch looks good RTBC to me, but I'm guessing we should clarify these two questions before we RTBC this... or perhaps RTBC it to get answers to these qeustions?

Hi @Manuel Garcia,
thanks for your review.
To be fair I only tried to fix the patch provided in the previous comments so I'm at a loss on why we are using bare_html_page_renderer in that case. Maybe @Mile23 can give us some insight since it was him who proposed it in the first place.

About your second comment, I agree we should just stick to the scope of the issue and only replace the drupal_render calls here. I can create a separate related issue for the replacing of drupal_render_root() with \Drupal::service('renderer')->renderRoot().

Reading comments #46 and #47 I upload this patch to apply the change commented in the second suggestion of comment #46.

On the other hand, I'm not sure if we should add the 'bare_html_page_renderer', as discussed in the first suggestion of comment #46.

Thanks @dimaro for that, RTBCing to get committers feedback

Yep, looks like that one bare_html_page_renderer is needed for this to pass.

There are still tons of documentation and string references to drupal_render(), but I think that also makes sense to address in a followup issue. I've created #2834889: Replace documentation and string references to drupal_render() for that.

After I filter out all the string and docs references, as well as references to drupal_render_root() which already has a related issue, here's what's left:

[mandelbrot:maintainer | Mon 12:32:55] $ grep -r "drupal_render" * | grep -v "*" | grep -v "//" | grep -v "drupal_render_root" | grep -v "drupal_render()"
core/includes/common.inc:function drupal_render(&$elements, $is_recursive_call = FALSE) {
core/includes/common.inc:function drupal_render_children(&$element, $children_keys = NULL) {
core/modules/system/tests/modules/common_test/common_test.module:function common_test_drupal_render_printing_pre_render($elements) {
core/tests/Drupal/Tests/Core/Render/RendererBubblingTest.php:        'keys' => ['simpletest', 'drupal_render', 'children_attached'],
core/tests/Drupal/Tests/Core/Render/RendererBubblingTest.php:    $element = ['#cache' => ['keys' => ['simpletest', 'drupal_render', 'children_attached']]];
core/tests/Drupal/Tests/Core/Render/RendererPlaceholdersTest.php:    $cached_element = $this->memoryCache->get('simpletest:drupal_render:children_placeholders')->data;
core/tests/Drupal/Tests/Core/Render/RendererPlaceholdersTest.php:        'keys' => ['simpletest', 'drupal_render', 'children_placeholders'],

The definitions of the deprecated functions, a test module function name, and then a couple of keys for something. I'll check on what those last are.

I had to fix one coding standards error:

diff --git a/core/includes/common.inc b/core/includes/common.inc
index d81fd13..29a645a 100644
--- a/core/includes/common.inc
+++ b/core/includes/common.inc
@@ -21,7 +21,6 @@
 use Drupal\Core\StringTranslation\PluralTranslatableMarkup;
 use Drupal\Core\Render\BubbleableMetadata;
 use Drupal\Core\Render\Element;
-use Drupal\Core\Render\BareHtmlPageRenderer;
 
 /**
  * @defgroup php_wrappers PHP wrapper functions

The use statement is not actually needed since we are using the service. :)

For everyone who raised concerns on this issue about not doing proper DI yet, we will definitely do that in followups. I encourage you to help in #2729597: [meta] Replace \Drupal with injected services where appropriate in core! The goal for this first step is to not have any reliance on the deprecated procedural function.

Also, let's create a specific followup as a child of #2729597: [meta] Replace \Drupal with injected services where appropriate in core for the DI for these and other instances in core.

+++ b/core/includes/common.inc
@@ -896,7 +897,7 @@ function drupal_render_children(&$element, $children_keys = NULL) {
   $output = '';
   foreach ($children_keys as $key) {
     if (!empty($element[$key])) {
-      $output .= drupal_render($element[$key]);
+      $output .= \Drupal::service('bare_html_page_renderer')->render($element[$key]);

This seems very wrong. This should definitely use the renderer service.

@Manuel Garcia also raised this in #46.1.

I don't even understand how this can possibly pass, since \Drupal\Core\Render\BareHtmlPage(RendererInterface)::render() does not exist! :O

Alrighty, reverted since it looks like this does need more discussion. Thanks @wimleers.

[…] here's what's left:

…
RendererBubblingTest.php
…
RendererPlaceholdersTest.php
…

The occurrences of the string drupal_render in those tests only exist because they're used as the cache keys (and hence cache IDs) for drupal_render()-related test coverage. Feel free to change all those occurrences to renderer!

I don't even understand how this can possibly pass, since \Drupal\Core\Render\BareHtmlPage(RendererInterface)::render() does not exist!

0_o

Alright then, let's see what the bot says... addressing #54 and #57.

Followup for proper IoC injection of renderer service: #2834982: Replace non-test usages of \Drupal::service('renderer') with IoC injection

Reroll of #58. There are still a ton of comment usages.

Try again...

Looks otherwise good, but this patch should not change any documentation: Now the patch makes a change to a comment in core/lib/Drupal/Core/Menu/menu.api.php, which is in scope for #2834889: Replace documentation and string references to drupal_render() . So basically, if that's removed this should be ok. Changing back to Needs work.

There were a couple other doc changes, as well. Removed those.

Sorry, I missed one more thing in the previous review: In core/modules/views_ui/src/Form/BreakLockForm.php the array syntax has been changed from the short syntax to array(). AFAIK this is against the D8 coding standards, was there a specific reason for this? If not, the syntax should probably be changed back and only the called function changed.

The few other documentation changes were a good catch, didn't notice them on the first run! Thanks for the quick update of the patch. Changing back to Needs work pending the array syntax thing. I think we should be done after clearing that up.

Edit: Just noticed that it has been previously array() also in the core code, so the array syntax thing was actually probably only a re-roll mistake, and there wasn't any reason for it. Let's fix that, sorry for not noticing it on the first review.

If you look at the testbot results for the last patch, you'll see that it agrees with you about the array syntax. :-)

Amended here.

I couldn't find any other occurrences of array() in the patch.

That's true, the testbot apparently also noticed it :)

Everything seems to be in order now, so marking the issue RTBC. Thanks for all the hard work!

I searched for usages of drupal_render() and all of the existing instances are mentions in the documentation, exception messages or api.php files so this looks good for me.

Committed 7f27bba and pushed to 8.4.x. Thanks!