Convert EntityManager to extend DefaultPluginManager

This one will be a bit more complicated, as we need to re-introduce support for cache tags.

Maybe we should wait to see the fate of #2005716: Promote EntityType to a domain object?

I started doing this in #2201879: Rename InfoHookDecorator to BuildHookDecorator.

Fixed EntityManagerTest (that thing is *so* complex) and addressed dawehner's review from over there.

> Given that the info hook decorator is cached I think we should just use it.
I don't understand this. Note that my patch does not use the info hook decorator at all anymore.

- Added the link to the related issue, didn't know that existed.

Re-roll, updated the mocked cache methods.

1. +++ b/core/lib/Drupal/Core/Entity/EntityManager.php
   @@ -160,6 +119,24 @@ public function clearCachedDefinitions() {
   +    foreach ($definitions as $plugin_id => &$definition) {
   +      $this->processDefinition($definition, $plugin_id);
   +    }
   +    foreach ($this->moduleHandler->getImplementations('entity_type_build') as $module) {
   +      $function = $module . '_' . 'entity_type_build';
   +      $function($definitions);
   +    }
   

   I think these two should be switched. That's how it is now, and we would still want to process anything being added...

2. +++ b/core/tests/Drupal/Tests/Core/Entity/EntityManagerTest.php
   @@ -111,20 +104,19 @@ protected function setUp() {
   -    $this->formBuilder = $this->getMock('Drupal\Core\Form\FormBuilderInterface');
   -
   
   @@ -149,7 +141,7 @@ protected function setUpEntityManager($definitions = array()) {
   -    $this->entityManager = new TestEntityManager(new \ArrayObject(), $this->container, $this->moduleHandler, $this->cache, $this->languageManager, $this->translationManager, $this->formBuilder);
   +    $this->entityManager = new TestEntityManager(new \ArrayObject(), $this->container, $this->moduleHandler, $this->cache, $this->languageManager, $this->translationManager);
   

   Heh, nice

The rest looks good.

1. Yes, true, not sure why I added that there. I also added the optional check thingy, another thing that is not compatible with objects btw, would have exploded if wouldn't have overridden the findDefinitions() method anyway...

Ha, nice, tricked myself out :)

Well, at least that part is then tested too ;)

Still applies, looking for reviews :)

#2168159: Plugin Derivatives don't work with Objects returned from Annotations landed, so the @todo can be removed, also added the same array casting to the base findDefinitions() implementation.

That means we now also have support for entity type derivatives ;)

1. +++ b/core/lib/Drupal/Component/Plugin/Discovery/DerivativeDiscoveryDecorator.php
   @@ -155,7 +155,11 @@ protected function encodePluginId($base_plugin_id, $derivative_id) {
   +<<<<<<< HEAD
       * @param mixed $base_definition
   +=======
   +   * @param array|object $base_definition
   +>>>>>>> applied patch
   

   Heh

2. +++ b/core/lib/Drupal/Core/Entity/EntityManager.php
   @@ -160,6 +119,31 @@ public function clearCachedDefinitions() {
   +    // If this plugin was provided by a module that does not exist, remove the
   +    // plugin definition.
   +    foreach ($definitions as $plugin_id => $plugin_definition) {
   +      if (!in_array($plugin_definition->getProvider(), array('Core', 'Component')) && !$this->moduleHandler->moduleExists($plugin_definition->getProvider())) {
   +        unset($definitions[$plugin_id]);
   +      }
   +    }
   

   Why add this? I see its basically copied from DefaultPluginManager, but do we need this?

3. +++ b/core/lib/Drupal/Core/Plugin/DefaultPluginManager.php
   @@ -270,7 +270,7 @@ protected function findDefinitions() {
   -      if (isset($plugin_definition['provider']) && !in_array($plugin_definition['provider'], array('Core', 'Component')) && !$this->moduleHandler->moduleExists($plugin_definition['provider'])) {
   +      if ((is_array($plugin_definition) || ($base_definition = (array) $plugin_definition)) && isset($plugin_definition['provider']) && !in_array($plugin_definition['provider'], array('Core', 'Component')) && !$this->moduleHandler->moduleExists($plugin_definition['provider'])) {
   

   Hm, it's a shame it's too late to use AnnotationInterface::getProvider() here

1. Ups :)
2. Yeah, I guess providing an entity type for a different module doesn't make much sense, valid point ;) Removed.

Looks good, thanks

12: entity-default-plugin-manager-2039435-12.patch queued for re-testing.

Re-roll look good, thanks.

18: drupal_2039435_18.patch queued for re-testing.

RTBC, under the condition that the tests pass.

24: drupal_2039435_24.patch queued for re-testing.

Re-roll and fixing new unit tests...

1. +++ b/core/lib/Drupal/Core/Entity/EntityManager.php
   @@ -179,6 +137,24 @@ public function clearCachedDefinitions() {
   +    foreach ($this->moduleHandler->getImplementations('entity_type_build') as $module) {
   +      $function = $module . '_' . 'entity_type_build';
   +      $function($definitions);
   +    }
   

   Is there a reason we can't just do invokeAll?

2. +++ b/core/lib/Drupal/Core/Entity/EntityManager.php
   @@ -731,7 +707,7 @@ protected function getAllDisplayModesByEntityType($display_type) {
   diff --git a/core/lib/Drupal/Core/Plugin/DefaultPluginManager.php b/core/lib/Drupal/Core/Plugin/DefaultPluginManager.php
   
   diff --git a/core/lib/Drupal/Core/Plugin/DefaultPluginManager.php b/core/lib/Drupal/Core/Plugin/DefaultPluginManager.php
   index e9c4ce9..dfd0028 100644
   
   index e9c4ce9..dfd0028 100644
   --- a/core/lib/Drupal/Core/Plugin/DefaultPluginManager.php
   
   --- a/core/lib/Drupal/Core/Plugin/DefaultPluginManager.php
   +++ b/core/lib/Drupal/Core/Plugin/DefaultPluginManager.php
   
   +++ b/core/lib/Drupal/Core/Plugin/DefaultPluginManager.php
   +++ b/core/lib/Drupal/Core/Plugin/DefaultPluginManager.php
   @@ -270,7 +270,7 @@ protected function findDefinitions() {
   
   @@ -270,7 +270,7 @@ protected function findDefinitions() {
        // If this plugin was provided by a module that does not exist, remove the
        // plugin definition.
        foreach ($definitions as $plugin_id => $plugin_definition) {
   -      if (isset($plugin_definition['provider']) && !in_array($plugin_definition['provider'], array('Core', 'Component')) && !$this->moduleHandler->moduleExists($plugin_definition['provider'])) {
   +      if ((is_array($plugin_definition) || ($base_definition = (array) $plugin_definition)) && isset($plugin_definition['provider']) && !in_array($plugin_definition['provider'], array('Core', 'Component')) && !$this->moduleHandler->moduleExists($plugin_definition['provider'])) {
            unset($definitions[$plugin_id]);
          }
        }
   

   Is there a way to also extend the test coverage for DefaultPluginManager?

3. +++ b/core/tests/Drupal/Tests/Core/Entity/EntityManagerTest.php
   @@ -162,8 +154,9 @@ protected function setUpEntityManager($definitions = array()) {
   -    $this->discovery->expects($this->once())
   -      ->method('clearCachedDefinitions');
   +    $this->cache->expects($this->once())
   +      ->method('deleteTags')
   +      ->with(array('entity_types' => TRUE));
    
   

   Just to be sure: We switch to the default plugin manager, so we don't longer use the cache decorator?

4. +++ b/core/tests/Drupal/Tests/Core/Entity/EntityManagerTest.php
   @@ -524,9 +515,15 @@ public function testGetBaseFieldDefinitionsWithCaching() {
        $this->cache->expects($this->at(2))
   +      ->method('set');
   +    $this->cache->expects($this->at(3))
   +      ->method('set');
   
   @@ -555,12 +552,18 @@ public function testGetFieldDefinitionsWithCaching() {
        $this->cache->expects($this->at(3))
   +      ->method('set');
   +    $this->cache->expects($this->at(4))
   +      ->method('set');
   
   @@ -606,12 +611,18 @@ public function testGetFieldStorageDefinitionsWithCaching() {
   +    $this->cache->expects($this->at(4))
   +      ->method('set');
   +    $this->cache->expects($this->at(5))
   
   @@ -804,14 +815,25 @@ public function testGetAllBundleInfo() {
   +      ->method('set');
   +    $this->cache->expects($this->at(3))
   +      ->method('set');
   

   Can we also set the parameters for set, maybe just the name of the cid?

Thanks for the review.

1. I copied that from the existing implementation, the reason is that we can pass it to the hook by reference, it's build, so we should allow adding more of them, especially since derivatives won't work for entities (we can't have entity types that contain :).

2. Yeah there is, also proved that it wasn't actually working yet..

3. Yeah, due to that change, we have a lot more calls to the mocked cache backend that we need to care about, so that's causing all those test changes.

4. Did that.

I can only find nitpicks, but I'm no expert in this area.

1. +++ b/core/lib/Drupal/Core/Entity/EntityManager.php
   @@ -179,6 +137,24 @@ public function clearCachedDefinitions() {
   +      $function = $module . '_' . 'entity_type_build';
   +      $function($definitions);
   

   Why this when you could use ModuleHandler::invoke()?

2. +++ b/core/lib/Drupal/Core/Plugin/DefaultPluginManager.php
   @@ -270,7 +270,7 @@ protected function findDefinitions() {
   -      if (isset($plugin_definition['provider']) && !in_array($plugin_definition['provider'], array('Core', 'Component')) && !$this->moduleHandler->moduleExists($plugin_definition['provider'])) {
   +      if ((is_array($plugin_definition) || ($plugin_definition = (array) $plugin_definition)) && isset($plugin_definition['provider']) && !in_array($plugin_definition['provider'], array('Core', 'Component')) && !$this->moduleHandler->moduleExists($plugin_definition['provider'])) {
   

   Wowza. Could we maybe do the array check/cast first, do a continue if it fails, and then do the rest? That'd be a lot more legible.

Thanks for the review, for 1., I don't think that works properly with by-reference arguments, which is what we need here? If it would, we could also just use invokeAll() directly...

And yes, that condition there is crazy, I copied that as a pattern from a related issue that did something similar for derivatives, 1+ to splitting that it up, will do that asap.

This is easier to understand I think...

Yes, thanks :)

33: drupal_2039435_33.patch queued for re-testing.

Looks like the last test run did pass?

$function
This at least needs a comment as to why it's not using invokeAll().

Also wonder if we shouldn't add an invokeAllByRef() that's basically the same as alter() except without changing the hook name?

This enough?

This code is copied 1:1 from InfoHookDecorator, I'm worried that introducing such a method would result in discussions about naming and numbers of argument and stuff and I'd love to get this in as this is the last standard plugin manager not extending from the DefaultPluginManager.

Can we look at that in a follow-up issue?

Reviewed this again, looks great. Thanks @Berdir!

This looks like great clean-up, rock.

Committed and pushed to 8.x. Thanks!