Move main complexity of drupal_render() into Drupal\Core\Render\Render

Baby step: Just move all of the related functions into static class methods in a new Drupal\Core\Render\Render.

Does that mean that the next step could be potentially to have all that as a non-static class in some kind of follow up?

+++ b/core/includes/common.inc
@@ -3897,391 +3579,19 @@ function show(&$element) {
+  return Render::getCacheId($elements);

+++ b/core/lib/Drupal/Core/Render/Render.php
@@ -0,0 +1,733 @@
+  public static function getCacheId(array $elements) {

I wonder whether we should to distinct between just getting something and actually computing the result. Maybe generateCacheId?

Most of the complexity added by #2118703: Introduce #post_render_cache callback to allow for personalization without breaking the render cache is removed again by #2099131: Use #pre_render pattern for entity render caching.

that exponentially increasing recursion bug

Do you talk about the two following code snippets?

  if (isset($elements['#cache']) || !$is_recursive_call) {
    $post_render_cache = drupal_render_collect_post_render_cache($elements);
    if ($post_render_cache) {
      $elements['#post_render_cache'] = $post_render_cache;
    }
  }

  // ...

  if (!$is_recursive_call || isset($elements['#cache'])) {
    $elements['#cache']['tags'] = drupal_render_collect_cache_tags($elements);
  }

I guess, all we have to do is to separate the manual setting of tags and the collected data, so we can use the collected data, and no longer recursive for each element again. drupal_render_collect_cache_tags could store the collected tags in each tree vertex while iterating, so there would be no need for additional tree-walking in the future.

Oh, how I've wished that RenderTest was a PHPUnit test! This will make it possible :)

15: render.15.patch queued for re-testing.

+++ b/core/lib/Drupal/Core/Render/Render.php
@@ -445,8 +445,7 @@ protected static function getCachePlaceholder($callback, array $context, $token)
+  public static function generateCacheToken() {

generatePostRenderCacheToken() would be a better name, I think.

Adding a related issue link, but what's being discussed there should absolutely not hold up this issue :)

There's little to no benefit to a big static class for this functionality. If anything it's counter-productive. There is no reason this can't be a normal object just calling $this->someProtectedMethod() as needed. That would let it be injected into other objects, too, thus not polluting them.

#32: Let us please at least take baby steps. A big static class would still be a step forward.

After having written the above, I went back to the issue summary, and sun apparently thinks exactly the same:

Proposed solution

1. Baby step: Just move all of the related functions into static class methods in a new Drupal\Core\Render\Render class.

Is there a follow-up to make it a for-reals object? The added work to do should not be that big, but as this is an API-affecting patch we should try to get as close to the "final" public facing API as possible so we don't hit beta and get stuck in a sloppy intermediary state.

Baby steps are all well and good but as long as patches can take around here I worry that we will only get through one step; make it a good one.

#34: Let me again quote sun to indicate why this is a very worthwhile baby step:

FWIW, this change here also allows us to add proper PHPUnit tests for the Render class to verify that the recursion works correctly + the right methods are called at the right time — however, just moving/renaming the code is complex enough already (I hope the patch still applies), so we should do that in a follow-up issue.

On top of that, we'll finally be able to get rid of all the wrapper methods for drupal_render() that live in so many classes.

To me, that sounds like a solid single step.

Babystep nr1: #2346937: Implement a Renderer service; reduces drupal_render / _theme service container calls

.

@dawehner would it be possible for you to add a short comment stating on what this is postponed. Or will this not get done for 8.0.x-dev now and should be bumped to 8.1.x-dev?

The title of this issue got fixed by #36. Renderer::doRender() is still a 300+ LOC function, but is protected, so refactors can happen any time. I think it makes sense to mark this issue fixed and if someone wants to open new issues for ideas on refactoring protected code in Renderer, go for it.

Actually, maybe "duplicate" is the more correct status.