Implement a Renderer service; reduces drupal_render / _theme service container calls

This seems highly sensible to me. I also agree with pushing off most of the refactoring other than just the injection here; the patch is big enough and moving code around inside the Renderer class is not even remotely BC-breaking. Just the clearer injected dependencies is a win already for the classes that are calling drupal_render().

There's only a few tweaks I'd still make cleanup-wise for now:

1. +++ b/core/lib/Drupal/Core/Render/Renderer.php
   @@ -0,0 +1,327 @@
   +    static $stack;
   

   This can be moved to an object property. Then the closures can reference it either by assigning to a temp variable or binding the closures to $this before calling them (which you can do as of PHP 5.4 so that $this works inside a closure).

   Of course, those closures maybe can just turn into methods on this class then. That's probably a bit more refactoring than we want to bother with right now.

2. +++ b/core/lib/Drupal/Core/Render/Renderer.php
   @@ -0,0 +1,327 @@
   +   * @see drupal_render()
   +   * @see drupal_render_collect_post_render_cache
   

   @see needs updating.

3. +++ b/core/lib/Drupal/Core/Theme/ThemeManager.php
   @@ -109,6 +113,295 @@ public function setActiveTheme(ActiveTheme $active_theme) {
   +   * _theme() is an internal function. Do not call this function directly as it
   +   * will prevent the following items from working correctly:
   +   * - Render caching.
   +   * - JavaScript and CSS asset attachment.
   +   * - Pre / post render hooks.
   +   * - Defaults provided by hook_element_info(), including attached assets.
   +   * Instead, build a render array with a #theme key, and either return the
   +   * array (where possible) or call drupal_render() to convert it to HTML.
   

   We can eliminate this part of the docblock as the method is protected.

4. +++ b/core/modules/views/src/Plugin/views/field/FieldPluginBase.php
   @@ -1718,6 +1718,14 @@ public static function trimText($alter, $value) {
   +  protected function getRenderer() {
   +    if (!$this->renderer) {
   +      $this->renderer = \Drupal::service("renderer");
   +    }
   +
   +    return $this->renderer;
   +  }
   

   Since we're introducing it here, let's go ahead and just inject it rather than calling out to the container. It's a plugin so it should still be injectable with the create() method.

This is a duplicate, pretty sure dawehner had separate issues for both

This is the issue larowlan mentioned, but seriously, let's be pragmatic here, let's try to work in babysteps.

This can be moved to an object property. Then the closures can reference it either by assigning to a temp variable or binding the closures to $this before calling them (which you can do as of PHP 5.4 so that $this works inside a closure).

Of course, those closures maybe can just turn into methods on this class then. That's probably a bit more refactoring than we want to bother with right now.

Babysteps, here is a follow up: #2354691: Move static variables and closures out of Renderer

We can eliminate this part of the docblock as the method is protected.

Decdided to eliminate it completly and point to self::render() as it contains the same documentation already anyway.

Since we're introducing it here, let's go ahead and just inject it rather than calling out to the container. It's a plugin so it should still be injectable with the create() method.

I am pretty sure you don't know what you are talking about :) We are talking here about the following few limited set of subclasses:

• Status (core/modules/file/src/Plugin/views/field/Status.php)
• Numeric (core/modules/views/src/Plugin/views/field/Numeric.php)
• Score (core/modules/search/src/Plugin/views/field/Score.php)
• NodeNewComments (core/modules/comment/src/Plugin/views/field/NodeNewComments.php)
• Url (core/modules/views/src/Plugin/views/field/Url.php)
• LanguageField (core/modules/views/src/Plugin/views/field/LanguageField.php)
• Counter (core/modules/views/src/Plugin/views/field/Counter.php)
• Links (core/modules/views/src/Plugin/views/field/Links.php)
• Dropbutton (core/modules/views/src/Plugin/views/field/Dropbutton.php)
• Custom (core/modules/views/src/Plugin/views/field/Custom.php)
• Xss (core/modules/views/src/Plugin/views/field/Xss.php)
• Xss (core/modules/aggregator/src/Plugin/views/field/Xss.php)
• FileSize (core/modules/views/src/Plugin/views/field/FileSize.php)
• FieldTest (core/modules/views/tests/modules/views_test_data/src/Plugin/views/field/FieldTest.php)
• StatisticsLastCommentName (core/modules/comment/src/Plugin/views/field/StatisticsLastCommentName.php)
• Depth (core/modules/comment/src/Plugin/views/field/Depth.php)
• Field (core/modules/field/src/Plugin/views/field/Field.php)
• BulkForm (core/modules/system/src/Plugin/views/field/BulkForm.php)
• NodeBulkForm (core/modules/node/src/Plugin/views/field/NodeBulkForm.php)
• UserBulkForm (core/modules/user/src/Plugin/views/field/UserBulkForm.php)
• UserData (core/modules/user/src/Plugin/views/field/UserData.php)
• Link (core/modules/user/src/Plugin/views/field/Link.php)
• LinkCancel (core/modules/user/src/Plugin/views/field/LinkCancel.php)
• LinkEdit (core/modules/user/src/Plugin/views/field/LinkEdit.php)
• ContactLink (core/modules/contact/src/Plugin/views/field/ContactLink.php)
• EntityLabel (core/modules/views/src/Plugin/views/field/EntityLabel.php)
• DblogMessage (core/modules/dblog/src/Plugin/views/field/DblogMessage.php)
• Link (core/modules/comment/src/Plugin/views/field/Link.php)
• LinkApprove (core/modules/comment/src/Plugin/views/field/LinkApprove.php)
• LinkReply (core/modules/comment/src/Plugin/views/field/LinkReply.php)
• LinkEdit (core/modules/comment/src/Plugin/views/field/LinkEdit.php)
• LinkDelete (core/modules/comment/src/Plugin/views/field/LinkDelete.php)
• Taxonomy (core/modules/taxonomy/src/Plugin/views/field/Taxonomy.php)
• Language (core/modules/taxonomy/src/Plugin/views/field/Language.php)
• LinkEdit (core/modules/taxonomy/src/Plugin/views/field/LinkEdit.php)
• File (core/modules/file/src/Plugin/views/field/File.php)
• Uri (core/modules/file/src/Plugin/views/field/Uri.php)
• FileMime (core/modules/file/src/Plugin/views/field/FileMime.php)
• Path (core/modules/node/src/Plugin/views/field/Path.php)
• Node (core/modules/node/src/Plugin/views/field/Node.php)
• Language (core/modules/node/src/Plugin/views/field/Language.php)
• Type (core/modules/node/src/Plugin/views/field/Type.php)
• Revision (core/modules/node/src/Plugin/views/field/Revision.php)
• HistoryUserTimestamp (core/modules/history/src/Plugin/views/field/HistoryUserTimestamp.php)
• User (core/modules/user/src/Plugin/views/field/User.php)
• Mail (core/modules/user/src/Plugin/views/field/Mail.php)
• Language (core/modules/user/src/Plugin/views/field/Language.php)
• Name (core/modules/user/src/Plugin/views/field/Name.php)
• Boolean (core/modules/views/src/Plugin/views/field/Boolean.php)
• Date (core/modules/views/src/Plugin/views/field/Date.php)
• StatisticsLastUpdated (core/modules/comment/src/Plugin/views/field/StatisticsLastUpdated.php)
• LastTimestamp (core/modules/comment/src/Plugin/views/field/LastTimestamp.php)
• Username (core/modules/comment/src/Plugin/views/field/Username.php)
• Comment (core/modules/comment/src/Plugin/views/field/Comment.php)
• TitleLink (core/modules/aggregator/src/Plugin/views/field/TitleLink.php)
• ContextualLinks (core/modules/contextual/src/Plugin/views/field/ContextualLinks.php)
• DblogOperations (core/modules/dblog/src/Plugin/views/field/DblogOperations.php)
• TimeInterval (core/modules/views/src/Plugin/views/field/TimeInterval.php)
• TranslationLink (core/modules/content_translation/src/Plugin/views/field/TranslationLink.php)
• Extension (core/modules/file/src/Plugin/views/field/Extension.php)
• Link (core/modules/node/src/Plugin/views/field/Link.php)
• LinkEdit (core/modules/node/src/Plugin/views/field/LinkEdit.php)
• LinkDelete (core/modules/node/src/Plugin/views/field/LinkDelete.php)
• RevisionLink (core/modules/node/src/Plugin/views/field/RevisionLink.php)
• RevisionLinkRevert (core/modules/node/src/Plugin/views/field/RevisionLinkRevert.php)
• RevisionLinkDelete (core/modules/node/src/Plugin/views/field/RevisionLinkDelete.php)
• MachineName (core/modules/views/src/Plugin/views/field/MachineName.php)
• Bundle (modules/entity_views/src/Plugin/views/field/Bundle.php)
• Serialized (core/modules/views/src/Plugin/views/field/Serialized.php)
• Markup (core/modules/views/src/Plugin/views/field/Markup.php)
• NodeComment (core/modules/comment/src/Plugin/views/field/NodeComment.php)
• EntityLink (core/modules/comment/src/Plugin/views/field/EntityLink.php)
• Link (modules/entity_views/src/Plugin/views/field/Link.php)
• LinkDelete (modules/entity_views/src/Plugin/views/field/LinkDelete.php)
• LinkEdit (modules/entity_views/src/Plugin/views/field/LinkEdit.php)
• Entity (modules/entity_views/src/Plugin/views/field/Entity.php)
• PrerenderList (core/modules/views/src/Plugin/views/field/PrerenderList.php)
• Permissions (core/modules/user/src/Plugin/views/field/Permissions.php)
• Roles (core/modules/user/src/Plugin/views/field/Roles.php)
• TaxonomyIndexTid (core/modules/taxonomy/src/Plugin/views/field/TaxonomyIndexTid.php)
• Broken (core/modules/views/src/Plugin/views/field/Broken.php)
• Standard (core/modules/views/src/Plugin/views/field/Standard.php)

+++ b/core/modules/views/src/Plugin/views/field/FieldPluginBase.php
@@ -1718,6 +1718,14 @@ public static function trimText($alter, $value) {
+    if (!$this->renderer) {

Wrong programming language, things aren't always that easy!

yeah, sucker!

Doc blocks are always broken when copying procedural to classes. Also there were references to old procedural functions.

One more small catch. Interdiff is still against #8.

I tried a little benchmark and it seems to be indeed a bit slower, maybe actually triggered by the problem that object method calls are slower than ordinary function calls.
It would though be sad if such things would play such a big role ... http://www.lionsad.de/xhprof-kit/xhprof/xhprof_html/?run1=543bf339ee873&...

Method calls are slower than function calls on the order of nanoseconds from the last time I saw that benchmarked. That shouldn't make a difference. Has the number of calls changed? That's usually the culprit when OOP looks slower than procedural code: The number of stack calls goes up as code gets broken up more.

@crell
No, this patch actually drops a couple of function calls ... see the link.

Reroll ...

@WimLeers
Would you be interested in actually getting this in? This could allow future improvements for drupal_render potentially easier.

That patch looks really nice. The more code refactored out of common.inc the better.

To bad that noone reviewed it in all that time, anyway.

Tipping this might fail because of htmlfragment/htmlpage issue.
But will be lazy and get the bot to test it.

Reviews and more re-rolls to come.

doh

Fixes some minor coding standards, added an interface, injected it into \Drupal\Core\Render\MainContent\HtmlRenderer

I've reviewed this as part of the re-roll, and I think its a great cleanup, I think it gives us more options later in the cycle too.

fixing fails now, missed a core.services change

actually, no I didn't - NodeController has a concrete new call to NodeViewController
and unit tests, should have run those first

this should be good

1. +++ b/core/lib/Drupal/Core/Render/Renderer.php
   @@ -0,0 +1,582 @@
   +        // Since #pre_render callbacks may be used for generating a render array's
   +        // content, and we might be rendering the main content for the page, it is
   

   more then 80 chars.

2. +++ b/core/lib/Drupal/Core/Theme/ThemeManager.php
   @@ -110,6 +114,257 @@ public function setActiveTheme(ActiveTheme $active_theme) {
   +        // exists and renders an empty string and a hook that is not implemented.
   ...
   +      // Include files required by the base hook, since its variable preprocessors
   ...
   +      // it like the #attached property on render arrays. In Drupal 8, this is the
   ...
   +      // template_preprocess() run (for example, if the default implementation is
   ...
   +      // these cases, a template should still be able to expect to have access to
   ...
   +      // is reasonably safe, and allows us to save on the overhead of adding some
   

   more then 80 chars.

I am pretty sure you don't know what you are talking about :) We are talking here about the following few limited set of subclasses: […]

dawehner++ for the delightful sarcasm :D

I was already following #2239457: Move main complexity of drupal_render() into Drupal\Core\Render\Render, didn't know about this one yet.

Overall: +1, just like I said on that other issue. Another thing I said there: Oh, how I've wished that RenderTest was a PHPUnit test! This will make it possible :).

Concerns:

1. the keeping of drupal_render_root(), but then requiring Renderer::render($e, TRUE) when using the Renderer service — we intentionally added a new function to make it easier to discern root calls. I think we should hence keep that for Renderer also, or at the very least have a very good reason to omit it.
2. the IS say It also doesn't introduce an interface yet, as the methods might change in the future once the conversion is done., but the patch does introduce an interface. I think we indeed don't want to add an interface (yet)?
3. The profiling in #13 is concerning. An 8% increase in memory usage… that seems very significant. So I redid the profiling. Did you pick the fastest run out of a few, to account for I/O slowness/CPU L1+L2 cache? I'm seeing that it's essentially exactly the same performance, as we'd expect.
   

   	HEAD	patch	Diff	Diff%
   Number of Function Calls	61,580	61,159	-421	-0.7%
   Incl. Wall Time (microsec)	216,573	214,457	-2,116	-1.0%
   Incl. MemUse (bytes)	22,208,608	22,215,880	7,272	0.0%
   Incl. PeakMemUse (bytes)	22,275,168	22,282,152	6,984	0.0%

   (Tested using the default frontpage with one extra block and a node.)

Other than that, I only found nitpicks.

1. +++ b/core/lib/Drupal/Core/Render/Renderer.php
   @@ -0,0 +1,383 @@
   + * Converts a render array into a string.
   
   +++ b/core/lib/Drupal/Core/Render/RendererInterface.php
   @@ -0,0 +1,223 @@
   + * Defines an interface for turning a render array into a string.
   

   s/string/HTML string/?

   Also: "converts" versus "turning into", should be consistent.

2. +++ b/core/lib/Drupal/Core/Render/RendererInterface.php
   @@ -0,0 +1,223 @@
   +   * @code
   +   *     array(
   +   *       '#theme' => 'image',
   +   *       '#attributes' => array('class' => array('foo')),
   +   *       '#theme_wrappers' => array('container'),
   +   *     );
   +   * @endcode
   +   *     and we need to pass the class 'bar' as an attribute for 'container', we
   +   *     can rewrite our element thus:
   +   * @code
   +   *     array(
   +   *       '#theme' => 'image',
   +   *       '#attributes' => array('class' => array('foo')),
   +   *       '#theme_wrappers' => array(
   +   *         'container' => array(
   +   *           '#attributes' => array('class' => array('bar')),
   +   *         ),
   +   *       ),
   +   *     );
   +   * @endcode
   

   Indentation for these @code tags seems off.

3. +++ b/core/lib/Drupal/Core/Theme/ThemeManager.php
   @@ -110,6 +114,257 @@ public function setActiveTheme(ActiveTheme $active_theme) {
   +   * @see ::render for the full documentation.
   

   Not ::render(), but \…\Renderer::render().

the keeping of drupal_render_root(), but then requiring Renderer::render($e, TRUE) when using the Renderer service — we intentionally added a new function to make it easier to discern root calls. I think we should hence keep that for Renderer also, or at the very least have a very good reason to omit it.

Valid point!

the IS say It also doesn't introduce an interface yet, as the methods might change in the future once the conversion is done., but the patch does introduce an interface. I think we indeed don't want to add an interface (yet)?

As said on IRC, in case we don't add an interface people also have to adapt the method renames ... so there is not a big gain, in not adding the interface now.
Let's keep it, we probably want one in the future anyway.

The profiling in #13 is concerning. An 8% increase in memory usage… that seems very significant. So I redid the profiling. Did you pick the fastest run out of a few, to account for I/O slowness/CPU L1+L2 cache? I'm seeing that it's essentially exactly the same performance, as we'd expect.

A one percent improvement is not too bad, given that there is maybe potential for a little bit more.

I would not trust my old crappy computer, to be honest, I really need a new one which actually works.

Found also a couple of things.

As said on IRC, in case we don't add an interface people also have to adapt the method renames ... so there is not a big gain, in not adding the interface now.

Good point.

A one percent improvement is not too bad, given that there is maybe potential for a little bit more.

Remember the situation msonnabaum described in the IS: a View with 100 nodes and 11 columns. That's a lot more drupal_render() calls right there. I think such a use case would indeed show a bigger improvement.

Rebased (straight reroll).

Only fixing a few docs indentation problems.

1. +++ b/core/includes/theme.inc
   @@ -157,307 +157,6 @@ function list_themes($refresh = FALSE) {
   -function _theme($hook, $variables = array()) {
   

   :) :) :)

2. +++ b/core/tests/Drupal/Tests/Core/Entity/Controller/EntityViewControllerTest.php
   @@ -67,7 +67,7 @@ public function testView() {
   +    $controller = new EntityViewController($entity_manager, $this->getMock('Drupal\Core\Render\RendererInterface'));
   

   :)

RTBC + 1 - this is great!

Just one of potential many follow ups #2378883: Convert existing drupal_render() KernelTestBase tests to PHPUnit tests

This is now a critical task as it blocks a critical bug that could only be solved sanely by creating a service like this:

#2364381: Exception thrown in drupal_render() causes an exception during the rendering of exceptions

After this issue is in, adding a resetStack method becomes trivial.

+++ b/core/includes/common.inc
@@ -2380,494 +2380,17 @@ function drupal_pre_render_links($element) {
-  $stack->push(new RenderStackFrame());

Can remove use for RenderStackFrame.

+++ b/core/includes/common.inc
@@ -2380,494 +2380,17 @@ function drupal_pre_render_links($element) {
+ * @deprecated as of Drupal 8.0.x. Use the 'renderer' service instead.
  */
 function drupal_render_root(&$elements) {
...
+/**
+ * @deprecated as of Drupal 8.0.x. Use the 'renderer' service instead.
+ */
+function drupal_render(&$elements, $is_recursive_call = FALSE) {

We should be saying that they will be removed before 9.0.0

Great looking patch btw. sorry for the nits.

Nits fixed.

+++ b/core/includes/common.inc
@@ -2363,31 +2362,14 @@ function drupal_pre_render_links($element) {
+ * @deprecated as of Drupal 9.0.0. Use the 'renderer' service instead.
...
+ * @deprecated as of Drupal 9.0.0. Use the 'renderer' service instead.

Oops I should have give you the exact thing to write...
@deprecated as of Drupal 8.0.x, will be removed before Drupal 9.0.0. Use the 'renderer' service instead.

Will fix on commit unless this goes back to needs work - leaving at rtbc for a bit to get additional feedback.

This issue is a critical task and is allowed per https://www.drupal.org/core/beta-changes. Committed da8ea3b and pushed to 8.0.x. Thanks!

diff --git a/core/includes/common.inc b/core/includes/common.inc
index ac02bc0..d694df1 100644
--- a/core/includes/common.inc
+++ b/core/includes/common.inc
@@ -2362,14 +2362,24 @@ function drupal_pre_render_links($element) {
 }
 
 /**
- * @deprecated as of Drupal 9.0.0. Use the 'renderer' service instead.
+ * 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()
  */
 function drupal_render_root(&$elements) {
   return \Drupal::service('renderer')->renderRoot($elements);
 }
 
 /**
- * @deprecated as of Drupal 9.0.0. Use the 'renderer' service instead.
+ * Renders 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::render()
  */
 function drupal_render(&$elements, $is_recursive_call = FALSE) {
   return \Drupal::service('renderer')->render($elements, $is_recursive_call);
diff --git a/core/lib/Drupal/Core/Render/RendererInterface.php b/core/lib/Drupal/Core/Render/RendererInterface.php
index d9594ae..a6ec2ed 100644
--- a/core/lib/Drupal/Core/Render/RendererInterface.php
+++ b/core/lib/Drupal/Core/Render/RendererInterface.php
@@ -2,7 +2,7 @@
 
 /**
  * @file
- * Contains \Drupal\Core\Render\Renderer.
+ * Contains \Drupal\Core\Render\RendererInterface.
  */
 
 namespace Drupal\Core\Render;

Fixed on commit.

Great refactorings of D8 in the final stages. Awesome! :)

This indeed looks good, thanks all! Since this removed _theme(), we are left with quite a few stale references in docs so I created this follow-up: #2388247: Documentation refers to _theme() function, which has been removed