Use service closure for form builder in node module hook classes

Looks right to me!

So if I understand it correctly this means it will only be set up once it's used by the page_top. Another way to do that would be to move the page_top implementation to it's own class right?

Another way to do that would be to move the page_top implementation to it's own class right?

Assuming a Hook class is instantiated only at the time when any one of its methods invoked, then probably yes.

On standard install profile, this doesn't make a difference yet, because it's already initialized by \Drupal\search\Plugin\Block\SearchBlock.

Lets do a separate issue for that, because we do initialize plugins even when they are render cached.

Lets do a separate issue for that, because we do initialize plugins even when they are render cached.

In that issue, are we looking for a way to load services lazily in plugin classes? Or prevent block plugins from being instantiated if they're render cached? Or something else specific to the search block?

Valid question. We'd want to do the same as we do here. we can't not instantiate block plugins.

plugins self-inject, so we need to define our own closure, ugly, but not not rocket science. Based on some unit tests I had to update, this should work:

  public function __construct(array $configuration, $plugin_id, $plugin_definition, \Closure $form_builder, \Closure $search_page_repository) {
    parent::__construct($configuration, $plugin_id, $plugin_definition);
    $this->formBuilder = $form_builder;
    $this->searchPageRepository = $search_page_repository;
  }

  /**
   * {@inheritdoc}
   */
  public static function create(ContainerInterface $container, array $configuration, $plugin_id, $plugin_definition) {
    return new static($configuration, $plugin_id, $plugin_definition,
      fn() => $container->get('form_builder'),
      fn() => $container->get('search.search_page_repository')
    );
  }

So we just define the closure by hand.

Definitely a new issue, because for existing code, we have to think about BC. While working on 📌 Lazy inject database into session manager Active , I was thinking we could do something like \Drupal\Core\DependencyInjection\DeprecatedServicePropertyTrait, but for closures by following a standard naming pattern.

So we'd change protected FormBuilderInterface $formBuilder to protected \Closure $formBuilderClosure. And then the trait, if a subclasses access $this->formBuilder which no longer exists, just checks with a magic __get() if $this->{$name}Closure exists, is a closure and calls it. But we can discuss that further in that or a separate new issue.

So we'd change protected FormBuilderInterface $formBuilder to protected \Closure $formBuilderClosure. And then the trait, if a subclasses access $this->formBuilder which no longer exists, just checks with a magic __get() if $this->{$name}Closure exists, is a closure and calls it. But we can discuss that further in that or a separate new issue.

The downside to all these closures is we lose type safety. If we use Lazy Services instead we don't have that issue, but I suspect there's a reason we've already landed on service closures.

So if I understand it correctly this means it will only be set up once it's used by the page_top. Another way to do that would be to move the page_top implementation to it's own class right?

Moving it to its own class could be worthwhile. We're injecting RendererInterface and EntityTypeManagerInterface to hook_page_top even though we don't need them. In saying that, they're probably going to be loaded anyway.

See 📌 Deprecate Drupal ProxyBuilder in favor of Symfony lazy services Closed: won't fix on lazy services.

But yes, missing types is unfortunate. By allowing no docs on constructors. we also lose the ability to declare it for phpstan too with a docblock. We could say that closures can't be promoted properties but use a protected \Closure $fooClosure with an appropriate docblock? Or we add a protected getFormBuilder() method that returns the result of the closure?

Created 📌 Replace SearchBlock service properties with service closures Active per #8.

Also related: 📌 [meta] Replace lazy service proxy generation with service closures Active

By allowing no docs on constructors. we also lose the ability to declare it for phpstan too with a docblock.

It seems we can do this:

  public function __construct(
    protected readonly RouteMatchInterface $routeMatch,
    protected readonly RendererInterface $renderer,
    protected readonly EntityTypeManagerInterface $entityTypeManager,
    /**
     * @var \Closure(): \Drupal\Core\Form\FormBuilderInterface $formBuilderClosure
     */
    #[AutowireServiceClosure('form_builder')]
    protected readonly \Closure $formBuilderClosure,
  ) {

  }

PhpStorm resolves the type and phpcs doesn't complain. Presumably phpstan would understand this too. It's not very pretty though.

One more thing to consider, in Drupal 12 (PHP 8.4) we can do this:

class NodeThemeHooks {

  protected FormBuilderInterface $formBuilder {
    get => ($this->formBuilderClosure)();
  }

  public function __construct(
    protected readonly RouteMatchInterface $routeMatch,
    protected readonly RendererInterface $renderer,
    protected readonly EntityTypeManagerInterface $entityTypeManager,
    #[AutowireServiceClosure('form_builder')]
    protected readonly \Closure $formBuilderClosure,
  ) { }

}

And then just call the property directly rather than the closure.

I like #13. Maybe we should add a 'Major version only' follow up issue?

Yeah #13 looks really good, a new issue makes sense, maybe a meta for using property hooks with service closures or something like that?

The syntax in #13 is neat, FWIW, the following is almost identical in terms of complexity IMHO and we could use that now already:

protected function formBuilder(): FormBuilderInterface {
  return ($this->formBuilderClosure)();
}

Only thing is that it might need more docs than a property?

We have 📌 Allow omitting doxygen when type and variable name is enough Active which would mean no extraneous docs for the method in this case, but it's not closed yet.

What about using an assert() for now and then doing property hooks later?

Something like this:

iff --git a/core/modules/node/src/Hook/NodeThemeHooks.php b/core/modules/node/src/Hook/NodeThemeHooks.php
index 7dbacb044c6..d293fa66e48 100644
--- a/core/modules/node/src/Hook/NodeThemeHooks.php
+++ b/core/modules/node/src/Hook/NodeThemeHooks.php
@@ -5,6 +5,7 @@
 namespace Drupal\node\Hook;

 use Drupal\Core\Entity\EntityTypeManagerInterface;
+use Drupal\Core\Form\FormBuilderInterface;
 use Drupal\Core\Hook\Attribute\Hook;
 use Drupal\Core\Link;
 use Drupal\Core\Render\Element;
@@ -273,6 +274,8 @@ public function preprocessBlock(&$variables): void {
   public function pageTop(array &$page_top): void {
     // Add 'Back to content editing' link on preview page.
     if ($this->routeMatch->getRouteName() == 'entity.node.preview') {
+      $form_builder = ($this->formBuilderClosure)();
+      assert($form_builder instanceof FormBuilderInterface);
       $page_top['node_preview'] = [
         '#type' => 'container',
         '#attributes' => [
@@ -281,7 +284,7 @@ public function pageTop(array &$page_top): void {
             'container-inline',
           ],
         ],
-        'view_mode' => ($this->formBuilderClosure)()->getForm(NodePreviewForm::class, $this->routeMatch->getParameter('node_preview')),
+        'view_mode' => $form_builder->getForm(NodePreviewForm::class, $this->routeMatch->getParameter('node_preview')),
       ];
     }
   }

That works for me. It will be a bit annoying if the same closure is used in multiple different hook methods, but we can fix that in 12.x with property hooks anyway.

Stub meta issue for property hooks: 📌 [meta] Use property hooks to instantiate services from service closures Active

Sending back to NR for commit per #18.

Can't wait for those property hooks, this is looking good now

FYI we also have 📌 Get rid of node_page_top() Active which would make this moot.