Problem/Motivation

drupal_render() was marked as deprecated, though its still called in quite some places.

Proposed resolution

All the child issues are closed as duplicates of this issue. Create a single patch for this issue instead of many sub-issues.

  1. Avoid rendering manually by letting the template who is printing the variable render it.
  2. Inject the renderer service into service, which uses drupal_render()
  3. Use \Drupal::service('renderer')->render() for old prodecural code.
  4. Replace drupal_render_root() with $renderer->renderRoot().

Remaining tasks

In 8.1.x, begin with a single patch to replace drupal_render() with use of the renderer service. Rest TBD.

User interface changes

None

API changes

None

Comments

pjonckiere’s picture

Issue summary: View changes

I'm getting a lot of hits on dupal_render() (393 in code, 538 total). Shouldn't we make this a meta issue?

Also, the comment above the function reads:

 @deprecated as of Drupal 8.0.x, will be removed before Drupal 9.0.0.

Should the version of this issue be updated?

joelpittet’s picture

Component: other » theme system
Issue summary: View changes
Issue tags: +Twig

@pjonckiere I'm not sure a meta as I hope we can do it all here, but maybe... Tagged with Twig so it shows up on our http://drupaltwig.org/

Also the first step is to avoid calling it all together preventing early rendering, then inject, then procedural service call.

I've updated the IS and added a script to help find them all.

@pjonckiere is this similar to what you used?

List:

ack 'drupal_render\(' core

Count

ack 'drupal_render\(' core | wc -l
joelpittet’s picture

A few will be cleaned up by means of #2348381: [META-0 theme functions left] Convert/refactor core theme functions, and lots are tests.

It may be worth injecting the renderer onto WebTestBase for all the tests to call $this->render();

pjonckiere’s picture

I let PhpStorm do the searching for me, but it comes down to the same as your regex command.

I did search on the whole project, and not just core, so there are slightly less hits then what I mentioned. But it still might be somewhat early to start working on this.

Cottser’s picture

@joelpittet in #3: The render thingy exists today as a trait, see #2257519: Move content assertion methods into a trait, so DUTB can consume it, too (\Drupal\simpletest\AssertContentTrait), available in WebTestBase and KernelTestBase.

If we try to evaluate these and remove early rendering, this is probably going to stall out. If we want to replace the drupal_render() calls we should make it scriptable. See also "how to meta your metas".

cilefen’s picture

cilefen’s picture

I think that the need to inject the service in some cases but not in others automatically makes this a meta because it isn't scriptable. A human has to review each replacement.

cilefen’s picture

Issue summary: View changes
Issue tags: +Novice
heddn’s picture

Assigned: Unassigned » heddn
Issue summary: View changes
heddn’s picture

Issue summary: View changes
cilefen’s picture

Title: Replace all drupal_render calls with the service, and inject it, if possible. » [META] Replace all drupal_render calls with the service, and inject it, if possible.
Issue summary: View changes
heddn’s picture

Assigned: heddn » Unassigned
cilefen’s picture

Issue summary: View changes
cilefen’s picture

nghai’s picture

Issue summary: View changes
Noe_’s picture

Issue summary: View changes
Mile23’s picture

heddn’s picture

Issue summary: View changes
edysmp’s picture

Issue summary: View changes
Adita’s picture

Issue summary: View changes
cilefen’s picture

Please make sure new child issues are set to "Active" unless there is a patch posted.

keopx’s picture

Issue summary: View changes
keopx’s picture

Issue summary: View changes
keopx’s picture

Issue summary: View changes
Mile23’s picture

Should we be replacing drupal_render_root() as well?

Mile23’s picture

Issue summary: View changes
mitrpaka’s picture

Issue summary: View changes
Mile23’s picture

Fabianx’s picture

#25: drupal_render_root() is replaced with $renderer->renderRoot().

Wim Leers’s picture

How was I not yet following this issue?!?

Crell’s picture

Wim: The same reason I wasn't. Welcome to my life. :-) (subscribe!)

willzyx’s picture

Issue summary: View changes
willzyx’s picture

Issue summary: View changes
willzyx’s picture

Issue summary: View changes
willzyx’s picture

Issue summary: View changes
willzyx’s picture

Issue summary: View changes
mitrpaka’s picture

Issue summary: View changes
willzyx’s picture

Issue summary: View changes
xjm’s picture

Version: 8.0.x-dev » 8.1.x-dev
Status: Active » Postponed

Thanks everyone for your work here. drupal_render() was not deprecated before the beta, and is marked for removal by 9.0.0. Per the allowed beta changes policy, only the removal of functions deprecated for removal 8.0.0 is a prioritized change. So the normal task of removing uses of drupal_render() isn't in scope during this phase of the release cycle, since we have to keep drupal_render() around until 9.x anyway.

It would be okay to resume simple conversions that preserve BC after 8.0.0, so postponing this to 8.1.x.

It would be great to have everyone's help instead in #2348381: [META-0 theme functions left] Convert/refactor core theme functions and #2205673: [META] Remove all @deprecated functions marked "remove before 8.0". Also see the other open Twig issues:
https://www.drupal.org/project/issues/search/drupal?project_issue_follow...

Cottser’s picture

Agreed, thanks @xjm.

xjm’s picture

Title: [META] Replace all drupal_render calls with the service, and inject it, if possible. » Replace all drupal_render calls with the service, and inject it, if possible.
Issue summary: View changes

Also, discussed with @alexpott and @webchick. When we do convert these, we should do it in a single patch rather than many small issues since it's actually a single conceptual change. So let's not file any additional child issues, and re-scope this to not be a meta.

Wim Leers’s picture

Tagging DX because it'd be better for DX if zero calls to drupal_render() remained: then contrib/custom code will be less tempted to call drupal_render(), thus encouraging better code, thus leading to a more consistent overall (core + contrib + custom) D8 code base, thus a better DX.

Fabianx’s picture

We can still justify some of those conversions in D8 with performance however.

- If something is called on every page and uses drupal_render() within an injectable service => convert it
- If something is not called on every page but just on some obscure admin page or within some legacy function => leave it

So we should split at least one performance issue for conversions that have a performance impact.

(and yes the indirection of drupal_render -> \Drupal::service('renderer') -> $container->get('renderer') -> $container->getRenderer() -> render() does matter.)

xjm’s picture

Issue tags: -Novice, -php-novice

Removing the novice tags as it's not good to have these on issues that are targeted for 8.1.x or that do not have consensus in one way or another.

If there is an actual non-negligible performance gain, let's demonstrate that where it happens and file related issues. Thanks!

Edit: Agreed that it is better DX for sure; just that alone isn't enough to justify a change at this point.

hass’s picture

Are you guys aware that render - both service and old render function is completly broken? #2474865: drupal_render() and service no longer attach

Mile23’s picture

Issue summary: View changes
Status: Postponed » Active

Updating issue summary.

Mile23’s picture

Version: 8.1.x-dev » 8.2.x-dev

Drupal 8.1.0-beta1 was released on March 2, 2016, which means new developments and disruptive changes should now be targeted against the 8.2.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.2.x-dev » 8.3.x-dev

Drupal 8.2.0-beta1 was released on August 3, 2016, which means new developments and disruptive changes should now be targeted against the 8.3.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.3.x-dev » 8.4.x-dev

Drupal 8.3.0-alpha1 will be released the week of January 30, 2017, which means new developments and disruptive changes should now be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.