Inject renderer service into ThemeManager, disuse drupal_render()

The problem is this creates a circular service reference:

exception 'Symfony\Component\DependencyInjection\Exception\ServiceCircularReferenceException' with message 'Circular reference detected for service[error]
"theme.manager", path: "theme.manager -> renderer -> theme.manager".'

Did you tried using setter injection?

Setter injection results in a different circular reference:

exception 'Symfony\Component\DependencyInjection\Exception\ServiceCircularReferenceException' with message 'Circular reference detected for service[error]
"plugin.manager.element_info", path: "plugin.cache_clearer -> plugin.manager.element_info -> theme.manager -> renderer".'

We'll have to to with #4 without a major refactoring.

Here is an alternative idea ... as ThemeManagerInterface::render is already meant to be internal.

What about making $renderer default to NULL so we can have a BC change? Otherwise not sure we can justify this issue in the beta evaluation.

Well, for sure we could do that, but on the other hand we really pushed hard to make theme() => _theme() internal.

I think #10 is the way to go, especially in 8.1.x.

-1 on #11. Even if it weren't @internal, we should still inject all dependencies. We want to leave the seam so that the problem from #6 is visible and eventually gets fixed.

One thing though:

+++ b/core/modules/system/src/Tests/Theme/ThemeTest.php
@@ -63,13 +63,13 @@ function testThemeDataTypes() {
+      $output = \Drupal::theme()->render('theme_test_foo', array('foo' => $example), \Drupal::service('renderer'));
...
+    $output = \Drupal::theme()->render(array('suggestionnotimplemented'), array(), \Drupal::service('renderer'));

+++ b/core/modules/system/tests/modules/theme_test/src/ThemeTestController.php
@@ -50,7 +50,7 @@ public function testInfoStylesheets() {
+    return ['#markup' => \Drupal::theme()->render('theme_test_template_test', array(), \Drupal::service('renderer'))];

@@ -76,7 +76,7 @@ public function testInlineTemplate() {
+    return ['#markup' => \Drupal::theme()->render(array('theme_test__suggestion', 'theme_test'), array(), \Drupal::service('renderer'))];

+++ b/core/modules/system/tests/modules/twig_theme_test/src/TwigThemeTestController.php
@@ -18,7 +18,7 @@ class TwigThemeTestController {
+    return ['#markup' => \Drupal::theme()->render('twig_theme_test_php_variables', array(), \Drupal::service('renderer'))];

Change those to $this->container->service() instead of \Drupal.

Also need a followup to properly inject all the \Drupal dependencies in the rest of ThemeManager.

I agree with @plach in #11. To preserve BC we shouldn't change the public interface on ThemeManager. Maybe it leaves a bit of cruft in there but with a bit of comments it will be obvious to remove in D9.

Adding a parent issue.

See also: #2704871: Replace usages of deprecated method drupal_render()

After #2704871: Replace usages of deprecated method drupal_render(), ThemeManager does not have references to drupal_render() within it. However, it does use \Drupal::service('render')->render(), which doesn't count as injecting the service.

I haven't experimented, but it's likely still a circular reference between the two services.

I am running into some issues trying to render stuff for embedding in an email, and encounter this issue where things go pear-shaped. It looks like the render context is not being passed on. I've rerolled #10 and squashed some more uses of themeManager->render(). Hopefully it's all this impacts.

This fixes the two test failures (silly misplacement of brackets) and actually uses the passed renderer instance instead of asking for it from the Drupal global object, which is what originally led me to this issue.

Here's an interdiff, basically for 10 to 26 (24 was not only a reroll, it also contained fixes for some additional uses of themeManager::render()).

I'm not entirely sure this issue is still relevant. Its primary use was to get rid of drupal_render() usage, which has already been done.

Yes, it still uses \Drupal::service('renderer'), but that's a semi-by product of the circular dependency created by the Renderer service.

I'm also really not a huge fan of DI methods like the latest patches are doing.

I think something simliar to #6 can still be accomplished.

Instead of setting the renderer via the service definition, we could still create a ThemeManager::setRenderer() method that can be invoked when setting the $this->theme property in Renderer::__construct:

$this->theme = $theme->setRenderer($this);

An additional ThemeManager::getRenderer() method should be created and used so, if the renderer isn't set (for whatever reason), it can be set using \Drupal::service('renderer') for BC reasons; so the line in ThemeManager::render should read as:

$this->getRenderer()->render($preprocess_bubbleable);

This kind of approach avoids changing method signatures and essentially creating BC breaks with existing interfaces.

The only way to not break the signatures is to do what #11 suggested and use NULL. However, this is essentially no different from creating ThemeManager::getRenderer(), without the unnecessary extra parameter.

I really don't think there is any way to avoid \Drupal::service('renderer') in cases where ThemeManager::render is invoked before the Renderer service is constructed. While this use case isn't likely to be very common, it is still technically possible given the current architecture of Renderer and ThemeManager.

Well, I wish I had a better way of reproducing my problem, but it seems that somehow themeManager may end up with a different instance of the renderer than other parts of the system. I have some custom code that gets the renderer from the container and then calls renderPlain, because it needs to produce some HTML for use in an email. Somewhere down the render chain, the render context goes AWOL, resulting in "Render context is empty, because render() was called outside of a renderRoot() or renderPlain() call. Use renderPlain()/renderRoot() or #lazy_builder/#pre_render instead." With the patch from #26, I don't get this, presumably because it ensures the renderer is preserved through the render chain.

I'll take a shot at producing a patch using your proposed resolution, which does sound better from a BC point of view.

So, something like this. This too solves my problem, btw.

1. +++ b/core/lib/Drupal/Core/Render/Renderer.php
   @@ -111,6 +111,7 @@ class Renderer implements RendererInterface {
        $this->theme = $theme;
   +    $theme->setRenderer($this);
   
   +++ b/core/lib/Drupal/Core/Theme/ThemeManager.php
   @@ -458,4 +469,30 @@ public function alter($type, &$data, &$context1 = NULL, &$context2 = NULL) {
   +  public function setRenderer(RendererInterface $renderer) {
   +    $this->renderer = $renderer;
   +  }
   

   Set methods typically return $this to allow chaining.

   Would also allow this to be a one-liner in Renderer::__construct as I mentioned in #29:

   $this->theme = $theme->setRenderer($this);
   

2. I wish I had a better way of reproducing my problem, but it seems that somehow themeManager may end up with a different instance of the renderer than other parts of the system

   This means this needs a test to reliably reproduce this issue and semi-justify this change.

Agreed. It does feel like I am fighting symptoms instead of the actual cause. The render context going missing doesn't make a lot of sense, and there really should not be a functional difference between using \Drupal::service('renderer') or pseudo-injecting the renderer when it is constructed itself.

The drupal_render() is deprecated since 8.0 and already removed in 9.0 (only mentions in comments exist) so summary and title needs work

Meantime here's re-roll and fix for #32.1