Inject various dependencies into DisplayPluginBase

This looks crazy, but it does make sense!

+++ b/core/modules/views/lib/Drupal/views/Plugin/views/display/DisplayPluginBase.phpundefined
@@ -99,6 +106,165 @@
+  public function __construct(array $configuration, $plugin_id, array $plugin_definition, ViewsData $views_data, ViewsPluginManager $access_plugin_manager, ViewsPluginManager $argument_default_plugin_manager, ViewsPluginManager $argument_validator_plugin_manager, ViewsPluginManager $cache_plugin_manager, ViewsPluginManager $display_extender_plugin_manager, ViewsPluginManager $exposed_form_plugin_manager, ViewsPluginManager $query_plugin_manager, ViewsPluginManager $pager_plugin_manager, ViewsPluginManager $row_plugin_manager, ViewsPluginManager $style_plugin_manager, UrlGenerator $url_generator) {

I know we usually put the constructor on one line, but this seems like a special case.

+++ b/core/modules/views/lib/Drupal/views/Plugin/views/display/DisplayPluginBase.phpundefined
@@ -99,6 +106,165 @@
+      $container->get('url_generator'2015121)

Hmm, that might be the problem here :)

I've also created a draft CR for this change

Finally, ready for review.

P.S. I've deliberatly put the issue node instead of the draft CR link to the trigger_error(), per a conversation with @berdir at Slack.

Left a comment on the MR.

But think the MR and maybe title should be updated as not super clear why the Rest plugin is needed to be updated.

RestExport plugin is updated because it extends PathPluginBase, which itself extends DisplayPluginBase

Feedback appears to be addressed.

I think we should use constructor property promotion here.

Good call on the constructor promotion.

For the follow up issue of removing these deprecations would recommend tagging for novice. Would be excellent for new users.

As per @catch's comment on the MR we can remove the constructor docs everywhere and we can use property promotion on all the classes.

The issue summary has 'Add BC layer' deleted - this is a fairly common extension point.
We're not obligated to provide a BC layer, but should we do the right thing and try to avoid hard breaking people's things without warning?

Added the constructor property promotion.
About the constructor docs, I cannot remove them because PHPCS will complain and tests will fail. Do I need to add a phpcs bypass for this to work?

@larowlan BC layer exists, it's crossed out because it is already done

Edit: A fix of this #3459746: [meta] Method getMockForAbstractClass() of class PHPUnit\Framework\TestCase is deprecated in PHPUnit 10 has been implemented for \Drupal\Tests\views\Unit\Plugin\display\PathPluginBaseTest. Shall I separate it?

@dimitriskr sorry haven't had a chance to get to this sooner. I believe we have probably missed the 11.1x window mind updating those for 11.2.x please

I'd assign the issue to you if I could

Left comments and questions on the MR.

The Needs Review Queue Bot tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.

I was going to suggest that these constructors are good candidates for autowiring (though maybe in a follow up), since #3452852: Add create() factory method with autowired parameters to PluginBase is in. But there are container parameters involved here, which AutowireTrait/AutowireInstanceTrait don't support yet, so we'd need a an issue first for them to support autowiring container parameters.

Separately I wonder if it'd be useful for AutowireTrait/AutowireInstanceTrait::create*() methods to support the #Required attribute to do setter injection, which could make extending DisplayPluginBase easier.

Created #3558292: Support passing container parameters with the Autowire attribute in AutowireTrait and AutowiredInstanceTrait and #3558306: Support automatic setter injection using the #[Required] attribute in AutowireTrait/AutowiredInstanceTrait per #47.

I think there might be performance benefit from loading the services lazily into the plugin classes via service closures, but we'd need #3544994: Make service closures serializable in classes using DependencySerializationTrait in first. If we don't need to wait for that, then this lgtm.

It might be good to rebase to check that PHP 8.5 tests pass.

Running https://git.drupalcode.org/issue/drupal-2015121/-/pipelines/662891 for PHP 8.5

This is a lot of dependencies to inject - the number of constructor arguments is kinda scary - and as #49 points out they are not always used.

Maybe all the views plugin managers should be put into a single service locator and they can be instantiated on demand from there?

The Needs Review Queue Bot tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.

Pushed an alternative approach using a service locator but because there are a lot of subclasses this is going to be super painful to implement.

I think setter injection with #[Required] is the way to go.

@longwave, if you push an example with the #[Required] or link to another issue, I could work on this new approach

See #48: #3558306: Support automatic setter injection using the #[Required] attribute in AutowireTrait/AutowiredInstanceTrait

Thinking about this a bit more, if the primary concern is to be able to use mocks in unit test, I think setters can be added here without being blocked on #3558306: Support automatic setter injection using the #[Required] attribute in AutowireTrait/AutowiredInstanceTrait (and/or #3558292: Support passing container parameters with the Autowire attribute in AutowireTrait and AutowiredInstanceTrait to be able to autowire container parameters in plugins, etc.). A getter can be used as well to replace to replace all the Views::somethingManager() calls.

It'd be something like:

public function setAccessManager(ViewsPluginManager $accessManager): static {
  $this->accessManager = $accessManager;
  return $this;
}

...

protected function getManager(string $type): ViewsPluginManager {
  $property = $type . 'Manager';
  if (!isset($this->{$property})) {
    $this->{$property} = Views::pluginManager($type);
  }
  return $this->{$property};
}

Then replace the uses of Views::pluginManager($type) with getManager(string $type).

In unit tests, you can then inject mocks into the display with the setters.

Once the #[Required] issue is in, we can add a follow up to put the attribute on all the setters in DisplayPluginBase, then there's the performance issue mentioned in #49 where all the service dependencies are being instantiated, possibly unnecessarily, when the plugin class is instantiated.

I think the service locator approach from MR!14258 is somewhat nicer, and solves the instantiation problem as well. Maybe we could split this up and add the service locator and change to Views.php in another issue, then we can start to deprecate ::pluginManager() and ::handlerManager() as there is a new service that replaces those methods?

I think the service locator approach from MR!14258 is somewhat nicer

Oh, do you mean using a setter to inject the locator? If so, yes, that is a great idea and much cleaner.

Yes, if we add something like this to DisplayPluginBase:

protected ContainerInterface $pluginManagers;

#[Required]
public function setPluginManagers(#[Autowire(service: 'views.plugin_managers') ContainerInterface $pluginManagers): void {
  $this->pluginManagers = $pluginManagers;
}

then this avoids the deprecation dance in every Views display plugin that would otherwise need to add this itself.

Created the issue to introduce the service locator per #59: #3566424: Deprecate Views::pluginManager() and Views::handlerManager() and replace with service locator

So the next steps are to first do #3566424: Deprecate Views::pluginManager() and Views::handlerManager() and replace with service locator based on MR!14258 by @longwave? And then, when #3558306: Support automatic setter injection using the #[Required] attribute in AutowireTrait/AutowiredInstanceTrait lands, we change it again?

#3558306: Support automatic setter injection using the #[Required] attribute in AutowireTrait/AutowiredInstanceTrait has landed.

Wondering if we can use property hooks, or asymmetric visibility on the property to skip the setter. Looks like it would be possible -
https://github.com/symfony/symfony/blob/8.1/src/Symfony/Component/Depend...

I think we're still waiting on #3558292: Support passing container parameters with the Autowire attribute in AutowireTrait and AutowiredInstanceTrait here, but it's close.

Property hooks are somewhat blocked right now because of PHPStan (well, mglaman/phpstan-drupal) and PHPCS:
https://github.com/mglaman/phpstan-drupal/pull/935
https://github.com/PHPCSStandards/PHP_CodeSniffer/issues/734

The first likely will have resolution soon, but PHPCS will probably be a while. There've been discussions on slack on whether PHPCS should be a blocker for property hooks.

Looking at the documentation for asymmetric properties: https://www.php.net/manual/en/language.oop5.visibility.php#language.oop5...

Specifically, the set visibility may be specified separately, provided it is not more permissive than the default visibility.

From there, it doesn't seem like it would work? The property needs to be public set for #[Required].

Makes sense. I think it could be public readonly but not sure how people feel about that.

I'm not sure readonly would work, because the value would be set outside the constructor. We also can't use readonly on the properties set by a setter.

I was mistaken again. Oh well, I guess the only way this would work is if the #[Required] magic worked from inside the class, e.g. via a trait, but it doesn't. Let's stick with setter methods then.

I'm not personally in favor of having service properties be public, but if people do like the DX of using the Required attribute directly on properties, I don't feel that strongly against it. That said, the setter is already in, and we'd have to open an issue for Required properties. I do vaguely wonder about how much the Autowire* traits for these non-service objects will eventually need to expand to re-create what Symfony's container compiler passes do, but that's neither here nor there for now.

Updated to use setter injection and #[Required] as in #61, but there is a problem: the Page plugin has a create() method that overrides the autowire create() so the setter injection never gets called for Page. We could remove create() here but it doesn't solve it for contrib display plugins that might already have that method.

Perhaps the only solution is a getPluginManagers() fallback that calls \Drupal::service() that we can eventually deprecate?

@longwave, I think you should change the target branch on !14258 to `main`

Also, in !5928 I injected the ViewsData (found in core/modules/views/src/Plugin/views/display/DisplayPluginBase.php#L812). Are we going to inject it here too? if not, should the title change to represent the plugin mangers only?

@dimitriskr let's figure out a way forward here with the plugin managers before we decide about injecting anything else.

Added support for #[Required] to ContainerFactory, not yet sure if this is a good idea though.

Re: #71

Maybe we can make sure the #[Required] methods run from calls to
ClassResolver::getInstanceFromDefinition(), relevant implementations of FactoryInterface::createInstance() like ContainerFactory and ConstraintFactory and anywhere else that I'm missing.

For example in ContainerFactory:

public function createInstance($plugin_id, array $configuration = []) {
    $plugin_definition = $this->discovery->getDefinition($plugin_id);
    $plugin_class = static::getPluginClass($plugin_id, $plugin_definition, $this->interface);

    // If the plugin provides a factory method, pass the container to it.
    if (is_subclass_of($plugin_class, 'Drupal\Core\Plugin\ContainerFactoryPluginInterface')) {
      $instance = $plugin_class::create(\Drupal::getContainer(), $configuration, $plugin_id, $plugin_definition);
      foreach ($reflection->getMethods(\ReflectionMethod::IS_PUBLIC) as $method) {
        if (!empty($method->getAttributes(Required::class))) {
          // Autowire the arguments and invoke the method.
        }
      }
      return $instance;
    }

    // Otherwise, create the plugin directly.
    return new $plugin_class($configuration, $plugin_id, $plugin_definition);
  }

I'm groaning because it makes the work we did in the trait seem redundant though.

@godotislate well I'm glad we had the same idea here, but we can reuse the trait if we break it up more! Still not sure this is the right way to go though, this might be a slippery slope - are there too many ways that plugins can be instantiated?

Oh, and I guess the setter is getting called twice now where create() isn't overridden...

I think the performance thing is equivalent to if all the plugin classes were using the trait. Adding this funcitonality to the factory now means reflection is running to do the same thing twice, which should be something to follow up with.

Yes, I fear the slippery slope as well, but I have no ideas at the moment.

The getPluginManagers() fallback might otherwise be a good solution, but I think we're still blocked by #3558292: Support passing container parameters with the Autowire attribute in AutowireTrait and AutowiredInstanceTrait?

I think the performance thing is equivalent to if all the plugin classes were using the trait.

So even if we removed the #[Required] stuff from the trait to the factories, reflection would still be running twice, but the setters would not be. So there would be some performance impact, though I _think_ it's not too bad.

Added a check that should prevent the setters being called twice, but it requires reflection to do so!