Allow #pre_render, #post_render and #post_render_cache callbacks to be service methods

Why are we using @ sign here?

Yeah for being able to use services all over the place!

Let's not reinvent the wheel here and just use the controller resolver.

      if (strpos($callback, '::') == FALSE) {

This sadly is needed at the moment because there are callbables like 'Drupal\comment\CommentViewBuilder::attachNewCommentsLinkMetadata' which sadly don't implement ContainerFactoryInterface
but EntityControllerInterface

If we do this for #post_render_cache callbacks, then we should also do it for all other types of callbacks (#pre_render, #process, #post_render …). catch said that also — see #2118703-45: Introduce #post_render_cache callback to allow for personalization without breaking the render cache.

Also: what do we really win by supporting this? Let's explain the rationale in the issue summary.

Why are static methods insufficient? My best guess: because static methods can only access things in the container via \Drupal::getContainer(), thus breaking/circumventing dependency injection?

#2266275: Don't use the #post_render_cache callback as an array key was closed, but was related to this.

I'm now effectively seeing use cases for #pre_render callbacks being service methods in core also.

Updating title.

@Wim Leers
One point is consistency, we do support the same pattern all over in core now, the other one is what you said before: you want to potentially have nice unit-testable code.

#18: Yep, I understand it now — it just wasn't clear initially :)

Straight reroll. This was broken completely by several patches that went in lately.

And now with support for #pre_render and #post_render.

#22's impressive mass test failure was caused by a stupid typo.

I've now added a use case for #pre_render: I've moved editor_pre_render_format() to element.editor:preRenderTextFormat (i.e. a new element.editor service; for providing functionality related to Drupal render elements, which is the case here). I also know that editor module has very strict tests in this area, so if it passes, we can rest assured.

There are zero uses for #post_render in Drupal core, hence I can't convert anything there.

This blocks #1805054: Cache localized, access filtered, URL resolved, and rendered menu trees. Bumping to critical.

(Not having this would require expanding the public API of MenuTreeInterface more than necessary.)

1. +++ b/core/includes/common.inc
   @@ -3305,8 +3305,16 @@ function drupal_render(&$elements, $is_recursive_call = FALSE) {
   +      else {
   +        $callable = $callable;
   +      }
   
   @@ -3407,6 +3415,12 @@ function drupal_render(&$elements, $is_recursive_call = FALSE) {
   +      else {
   +        $callable = $callable;
   +      }
   
   @@ -3703,10 +3723,19 @@ function drupal_render_cache_generate_placeholder($callback, array &$context) {
   +      else {
   +        $callable = $callback;
   +      }
   

   Please remove

2. +++ b/core/modules/comment/comment.services.yml
   @@ -18,3 +18,7 @@ services:
   +  comment.post_render_cache:
   +    class: Drupal\comment\CommentPostRenderCache
   +    arguments: ['@entity.manager', '@entity.form_builder']
   
   +++ b/core/modules/editor/editor.services.yml
   @@ -2,3 +2,6 @@ services:
   +  element.editor:
   +    class: Drupal\editor\Element
   +    arguments: ['@plugin.manager.editor']
   

   is it fine to have services for render?

3. +++ b/core/modules/comment/src/CommentPostRenderCache.php
   @@ -0,0 +1,80 @@
   +class CommentPostRenderCache {
   

   This looks great, in light of #2188287: Split CommentManager in two that name could be reused to render "forbidden message" as well

#26

1. Eh… huh? Why? catch said he wanted this for things other than #post_render_cache as well, see #2118703-45: Introduce #post_render_cache callback to allow for personalization without breaking the render cache. It wouldn't make sense to only support this for one callback category.
2. Why not? EntityViewBuilder is also a rendering service, for example. That being said, I don't think CommentPostRenderCache makes a whole lot of sense as a separate service. Maybe larowlan just wanted to use that as an example? I hope larowlan can chime in on that.
3. Heh…

Nice. RTBC for me . We need a change record or update an existing one.

+++ b/core/includes/common.inc
@@ -3305,8 +3305,16 @@ function drupal_render(&$elements, $is_recursive_call = FALSE) {
+      if (is_string($callable) && strpos($callable, '::') === FALSE) {
+        $callable = $controller_resolver->getControllerFromDefinition($callable);
+      }
+      else {
+        $callable = $callable;
+      }

Here and below: Why the strpos() check?

The controller resolver handles $class::$method strings just fine. Or if it doesn't, then that's a bug.

Or is this just for performance? Then we should add a comment, I think.

#30: Please see dawehner's comment in #12:

Let's not reinvent the wheel here and just use the controller resolver.

      if (strpos($callback, '::') == FALSE) {

This sadly is needed at the moment because there are callbables like Drupal\comment\CommentViewBuilder::attachNewCommentsLinkMetadata which sadly don't implement ContainerFactoryInterface but EntityControllerInterface

Let's add an inline comment for the strpos check, I can see someone 'fixing' that if we don't.

Would this work if my callback is inside a plugin, not a "service"?

@Dave you can use a static method on a plugin now and that should work.

The render system has no way to instantiate your plugin itself, so it can only handle static methods or services.

It is probably feasible to have a class that is both a plugin and also registered as a service - but those would be two separate class instances (and it confused me just typing that).

Looks like this will need a conflict merge after #2217877: Text filters should be able to add #attached, #post_render_cache, and cache tags landed.

Straight reroll. No changes were necessary for #2217877 actually, it was #2275491: CKEditor does not support the "readonly" attribute that broke this.

Trying this out in contrib with https://github.com/drupal-media/entity_embed/pull/38 and confirmed this seems to be working well.

Back to RTBC per #29 and #37.

if (is_string($callable) && strpos($callable, '::') === FALSE) {

element.editor:preRenderTextFormat

*confused* the first is two : the second is : How does this work?

#39: Ugh, you're right. This was introduced in #11, and only noticed now :(

It doesn't cause any fails because it's a bug that only exists in the if-test to throw an exception… which has no test coverage.

Will fix, unless somebody beats me to it.

It would be nice to enhance the test coverage for the various types of callbacks that could be used: function, static method on a class, and service method.

#41: I agree. But that's only really feasible when we can write unit tests. And that will only be the case once #2239457: Move main complexity of drupal_render() into Drupal\Core\Render\Render lands. So please help review that :)

Instead of having a special version of the same logic that is used elsewhere, let's just use the exact same logic again:

 function drupal_render_cache_generate_placeholder($callback, array &$context) {
+  if (is_string($callback) && strpos($callback, '::') === FALSE) {
+    /** @var \Drupal\Core\Controller\ControllerResolverInterface $controller_resolver */
+    $controller_resolver = \Drupal::service('controller_resolver');
+    $callable = \Drupal::service('controller_resolver')->getControllerFromDefinition($callback);
+  }
+  else {
+   $callable = $callback;
+  }
+
+  if (!is_callable($callable)) {
+    throw new Exception(t('$callable must be a callable function or of the form service_id:method.'));
+  }

Much simpler.

No interdiff, because #2228763: Create a comment-type config entity and use that as comment bundles, require selection in field settings form broke this.

Back to RTBC per #28, #29 and #37.

I will leave this as RTBC but that's really convoluted and repeated code, you know? Why can't we do $callable = \Drupal::service('controller_resolver')->getControllerFromDefinition($callback); ? If we can't , follow up with that pleaaaaaase.

#46: Yes, I'd love that too, but — again — see #31/#12. Let's do that in a follow-up.

Yeah agreed on that as a follow-up.

Committed/pushed to 8.x, thanks!

Finally filed that follow-up: #2323301: Deprecate the "two double colons" check in Renderer::doCallback().