Allow plugin derivative classes to use services from the container [#2028511]

Similar to how plugins can now get services from the container, I think we may need to do a similar thing for plugin derivatives. We can then remove procedural calls in these derivative fetcher classes. Otherwise these classes are not actually very unit testable when they could be.

I have added an example implementation in ViewsBlock, and made the change in DefaultPluginManager, which is probably not where it wants to live.

I would like some validation on the approach etc... before I continue any further and add tests etc...

Comment	File	Size	Author
#11	2028511-11.patch	11.63 KB	damiankloip
#11
#11	interdiff-2028511-11.txt	2.34 KB	damiankloip
#9	plugin-2028511-9.patch	11.44 KB	dawehner
#9
#3	2028511-3.patch	5.03 KB	damiankloip
#3
#3	interdiff-2028511-3.txt	6.68 KB	damiankloip
	d8.ContainerDerivativeInterface.patch	6.5 KB	damiankloip

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Comment #1

dawehner

German

CreditAttribution: dawehner commented 26 June 2013 at 11:14

+++ b/core/modules/views/lib/Drupal/views/Plugin/Derivative/ViewsBlock.phpundefined
@@ -24,6 +26,43 @@ class ViewsBlock implements DerivativeInterface {
   /**
+   * The base plugin ID.
+   *
+   * @var string
+   */
+  protected $base_plugin_id;
...
+    $this->basePluginId = $base_plugin_id;

This doesn't seem to be needed but yeah I don't care.

+++ b/core/modules/views/lib/Drupal/views/Plugin/Derivative/ViewsBlock.phpundefined
@@ -24,6 +26,43 @@ class ViewsBlock implements DerivativeInterface {
+   * @param \Drupal\Core\Entity\EntityStorageControllerInterface $view_storage_controller
+   *   The entity storage controller to load views.
...
+  public function __construct($base_plugin_id, EntityStorageControllerInterface $view_storage_controller) {

I guess we should use the ViewStorageControllerInterface?

+++ b/core/modules/views/lib/Drupal/views/Plugin/Derivative/ViewsBlock.phpundefined
@@ -39,7 +78,7 @@ public function getDerivativeDefinition($derivative_id, array $base_plugin_defin
-    foreach (views_get_all_views() as $view) {
+    foreach ($this->viewStorageController->load() as $view) {
       // Do not return results for disabled views.

What a big amount of work for just this single change

Comment #2

damiankloip CreditAttribution: damiankloip commented 26 June 2013 at 13:36

I guess we should use the ViewStorageControllerInterface?

We don't have one of those do we? We have ViewStorageInterface, but not for controllers iirc.

It looks like alot for this small change, but almost all Derivative classes need to get something injected (and are using procedural functions) at the moment. I don't mind doing that here or in a follow up though. Whichever we think is easier. If we get this in the the others are just conversions I guess.

Comment #3

damiankloip CreditAttribution: damiankloip commented 26 June 2013 at 15:02

File	Size
interdiff-2028511-3.txt	6.68 KB
2028511-3.patch	5.03 KB

ok, I spoke to Eclipse on IRC, and he likes the other approach here; Allow the derivative key to be a service name. So let's try that.

Comment #4

EclipseGc CreditAttribution: EclipseGc commented 26 June 2013 at 15:08

Other than the fact that we're injecting the EntityManager when we actually need

+++ b/core/modules/views/lib/Drupal/views/Plugin/Derivative/ViewsBlock.phpundefined
@@ -24,6 +25,25 @@ class ViewsBlock implements DerivativeInterface {
+  public function __construct(EntityManager $entity_manager) {
+    $this->viewStorageController = $entity_manager->getStorageController('view');

Seems odd that we're getting the entity manager when what we actually need is the view storage controller, but perhaps this is just how it is right now. Otherwise this looks great to me.

Comment #5

damiankloip CreditAttribution: damiankloip commented 26 June 2013 at 15:29

Yep, I'm afraid that has to go through the entity manager.

Comment #6

EclipseGc CreditAttribution: EclipseGc commented 26 June 2013 at 19:32

Status:

Needs review

» Needs work

This needs tests, and then I'd be happy to rtbc it.

Eclipse

Comment #7

neclimdul

He/Him

CreditAttribution: neclimdul commented 26 June 2013 at 19:35

Status:

Needs work

» Needs review

I was pretty certain when I first skimmed the patches but now I'm a little torn. I think I much prefer the former patch over the later. It feels like it would provide a little more flexibility and I'm dubious of the utility of providing the fetcher as a service. It seem like an entirely internal object that is neither useful to other implementations or particularly swapable.

The downside though is it makes the fetcher slightly less testable because the actual used services are embedded in the container. So we're not really doing the best form of injection here.

Comment #8

EclipseGc CreditAttribution: EclipseGc commented 26 June 2013 at 19:56

Status:

Needs review

» Needs work

needs work either way.

Eclipse

Comment #9

dawehner

German

CreditAttribution: dawehner commented 5 July 2013 at 05:02

Status:	Needs work	» Needs review
Issue tags:	-Needs tests

File	Size
plugin-2028511-9.patch	11.44 KB

My review on #1 was odd/

Added some tests

Comment #10

5 July 2013 at 06:10

Status:

Needs review

» Needs work

The last submitted patch, plugin-2028511-9.patch, failed testing.

Comment #11

damiankloip CreditAttribution: damiankloip commented 5 July 2013 at 17:31

Status:

Needs work

» Needs review

File	Size
interdiff-2028511-11.txt	2.34 KB
2028511-11.patch	11.63 KB

Great work, thanks for the tests!

We now need to use loadMultiple() instead of load() to get all entities, that's the only reason for those failures I think. I also added a little bit more to the unit test, to also mock the container and test that get is called for our example service. Atleast it lets us know that hte container is passed into our fetcher ok.

Comment #12

dawehner

German

CreditAttribution: dawehner commented 6 July 2013 at 17:07

Status:

Needs review

» Reviewed & tested by the community

Nice addition.

Comment #13

EclipseGc CreditAttribution: EclipseGc commented 16 July 2013 at 15:39

I'm generally ++ to this and am also RTBC. I might caution that we probably shouldn't re-invent the ContainerInterface stuff over and over and over again for different use cases, considering it's the same thing all the time. But that seems like a general followup for all of core, not Derivatives specific.

Seconding the RTBC

Eclipse