#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

CommentFileSizeAuthor
#11 interdiff.txt1.67 KBdawehner
#11 plugin_derivative_object-2168159-11.patch37.24 KBdawehner
#8 interdiff.txt2.82 KBdawehner
#8 plugin_derivative_object-2168159-8.patch37.18 KBdawehner
#3 plugin_derivative-2168159-3.patch35.09 KBdawehner
#1 plugin_derivative-2168159.patch34.99 KBdawehner

Comments

dawehner’s picture

dawehner
Primary language German
commented
Status: Active » Needs review
StatusFileSize
new plugin_derivative-2168159.patch34.99 KB

That was a bit more tricky than expected.

eclipsegc’s picture

eclipsegc commented

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

dawehner
Primary language German
commented
StatusFileSize
new plugin_derivative-2168159-3.patch35.09 KB

Rerole.

dawehner’s picture

dawehner
Primary language German
commented

3: plugin_derivative-2168159-3.patch queued for re-testing.

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

damiankloip commented 
+++ 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

dawehner
Primary language German
commented
Status: Needs work » Needs review
StatusFileSize
new plugin_derivative_object-2168159-8.patch37.18 KB
new interdiff.txt2.82 KB

Sure.

damiankloip’s picture

damiankloip commented 
  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

berdir
Primary language German
Location Switzerland
commented

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

dawehner
Primary language German
commented
StatusFileSize
new plugin_derivative_object-2168159-11.patch37.24 KB
new interdiff.txt1.67 KB

Can't we fix that in the other issue?

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
commented
Status: Needs review » Reviewed & tested by the community

Oh yay! Glad to see this done.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
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.