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?

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.

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 #3541705: Lazy inject database into varous services, 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 #3396928: Deprecate Drupal ProxyBuilder in favor of Symfony lazy services 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?

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.

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 #3376518: Allow omitting @var, @param and @return tags or their descriptions if the variable name and type is enough which would mean no extraneous docs for the method in this case, but it's not closed yet.

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.

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

FYI we also have #3339905: Add page_top and page_bottom to #attached to get rid of NodeThemeHooks::pageTop() which would make this moot.

Committed/pushed to 11.x, thanks!

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

Why did we switch from a normal typed property to a local variable? As it stands, if we move to a virtual property we would have to move back from local variable to property.

If we had kept the typed $formBuilder property then:
- the assert() would not be needed, right?
- we would not have to bring back the property on PHP 8.4. (Even if not readonly anymore.)
- the rest of the code would not have had to switch to the local variable, right? (And back later.)
(EDITED)

So maybe the reason is that PHPStan complains when a readonly property is assigned outside of the constructor. One solution would be to just remove the readonly already, even if it looks a bit odd. Makes it even easier to transition to a virtual property.