#2005716: Promote EntityType to a domain object introduced the ability for Annotation classes to return a class object to the annotation instead of an array. This is great, but derivatives is hardcoded to expect arrays, so we need to fix that.

Eclipse

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

dawehner’s picture

Status: Active » Needs review
FileSize
34.99 KB

That was a bit more tricky than expected.

EclipseGc’s picture

if ((is_array($base_definition) || ($base_definition = (array) $base_definition)) && (isset($base_definition['derivative']) && $class = $base_definition['derivative'])) {

This is my only qualm with any of this, I really think we want a method for derivative retrieval if it's a class based definition. Casting the annotation class to an array just doesn't jive for me after going to this much trouble to make it a class, so let's get a real method for this.

Eclipse

dawehner’s picture

Rerole.

dawehner’s picture

Status: Needs review » Needs work

The last submitted patch, 3: plugin_derivative-2168159-3.patch, failed testing.

The last submitted patch, 3: plugin_derivative-2168159-3.patch, failed testing.

damiankloip’s picture

+++ b/core/tests/Drupal/Tests/Core/Plugin/Discovery/DerivativeDiscoveryDecoratorTest.php
@@ -52,6 +52,28 @@ public function testGetDerivativeFetcher() {
+    $this->assertEquals(2, count($definitions));

Could we test the data in the definitions too? Just to make super sure.

dawehner’s picture

Status: Needs work » Needs review
FileSize
37.18 KB
2.82 KB

Sure.

damiankloip’s picture

  1. +++ b/core/tests/Drupal/Tests/Core/Plugin/Discovery/DerivativeDiscoveryDecoratorTest.php
    @@ -48,7 +48,14 @@ public function testGetDerivativeFetcher() {
    +    print_r($definitions);
    

    Oops.

  2. +++ b/core/tests/Drupal/Tests/Core/Plugin/Discovery/DerivativeDiscoveryDecoratorTest.php
    @@ -48,7 +48,14 @@ public function testGetDerivativeFetcher() {
    +    $this->assertEquals(array('id' => 'non_container_aware_discovery', 'derivative' => '\Drupal\Tests\Core\Plugin\Discovery\TestDerivativeDiscovery'),
    

    Array items on separate lines I think. Makes it much more readable.

Otherwise, this looks good to go.

Berdir’s picture

The new optional provider check in DefaultPluginManager is affected by this too, see #2039435: Convert EntityManager to extend DefaultPluginManager. Do we want to fix this here as well, or somewhere else?

dawehner’s picture

Can't we fix that in the other issue?

tim.plunkett’s picture

Status: Needs review » Reviewed & tested by the community

Oh yay! Glad to see this done.

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Committed 48025f9 and pushed to 8.x. Thanks!

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.