Problem/Motivation

Provide a generic implementation for using YAML as a form of discovery alongside other discovery mechanisms.

Proposed resolution

Add a YAML Discovery decorator, so this can be used with other discovery mechanisms. For example, 'static' plugins can be declared in yaml but custom implementations can still be declared via annotations.

Remaining tasks

Commit the patch

User interface changes

None

API changes

Just an addition of this decorator.

#2065571: Add YAML Plugin discovery

Original report by @damiankloip

Files: 
CommentFileSizeAuthor
#11 yaml-2076681-11.patch4.82 KBpwolanin
PASSED: [[SimpleTest]]: [MySQL] 58,508 pass(es).
[ View ]
#11 2076681-10-11.increment.txt762 bytespwolanin
#10 yaml-2076681-10.patch4.83 KBdawehner
PASSED: [[SimpleTest]]: [MySQL] 58,416 pass(es).
[ View ]
#10 interdiff.txt615 bytesdawehner
#1 2076681-1.patch4.67 KBdamiankloip
PASSED: [[SimpleTest]]: [MySQL] 58,473 pass(es).
[ View ]
#1 interdiff-2076681-1.txt3 KBdamiankloip
yaml-dd.patch1.62 KBdamiankloip
PASSED: [[SimpleTest]]: [MySQL] 58,443 pass(es).
[ View ]

Comments

Status:Active» Needs review
StatusFileSize
new3 KB
new4.67 KB
PASSED: [[SimpleTest]]: [MySQL] 58,473 pass(es).
[ View ]

So it looks like the idea from to use a decorator #2050919: Replace local task plugin discovery with YamlDiscovery that got unnecessarily removed in #2065571: Add YAML Plugin discovery has come back round full circle :)

Here is a unit test for the decorator.

+++ b/core/tests/Drupal/Tests/Core/Plugin/Discovery/YamlDiscoveryDecoratorTest.php
@@ -0,0 +1,100 @@
+    foreach ($this->expectedKeys as $expected_key) {
+      $this->assertArrayHasKey($expected_key, $definitions);
+    }
...
+    foreach ($definitions as $id => $definition) {
+      foreach (array('name', 'id', 'provider') as $key) {
+        $this->assertArrayHasKey($key, $definition);
+      }
+      $this->assertEquals($id, $definition['id']);
+      $this->assertEquals(array_search($id, $this->expectedKeys), $definition['provider']);
+    }
+  }

A foreach nearly all times shout to a data provider, as here?

Status:Needs review» Reviewed & tested by the community

I got convinced

Status:Reviewed & tested by the community» Needs review

:), the general summary was that using a data provider here would just be for the expected keys, and that would make the test way more confusing to read.

Status:Needs review» Reviewed & tested by the community

I disagree damian, this was RTBC.

Status:Reviewed & tested by the community» Needs review

i'm not sure about this now.

If the overall goal is to encourage only trivial variants to be discovered this way, I think this should unset at least 'class' and 'derivatives' key from anything that's discovered.

In the case of the local action/task use case, that would mean you'd need to directly annotate any custom class - which I think is a good idea.

#6 sounds insane to me. If implementations need to customize this generic behavior, they can.

Given a lack of enforcing a class or validating keys, I'm not sure this minimal/generic implementation is useful.

I don't care if it goes in or not, but I don't think it's something you'd want to use alone in most cases.

It makes alot of sense to have a generic base implementation that works, and is unit tested. If you want to then extend that, you can.

StatusFileSize
new615 bytes
new4.83 KB
PASSED: [[SimpleTest]]: [MySQL] 58,416 pass(es).
[ View ]

There we go.

Status:Needs review» Reviewed & tested by the community
StatusFileSize
new762 bytes
new4.82 KB
PASSED: [[SimpleTest]]: [MySQL] 58,508 pass(es).
[ View ]

Based on long-winded discussion in IRC: This class and the underlying YAML plugin discovery are very generic and in most cases need to be extended to provide better checks on the classes or data found in the YAML files.

Revising dawehner's suggested code comment since he's busy on another issue and setting back to rtbc.

You make it sounds like everybody agreed on that, really it was just you... I said this was a slightly pointless idea, as it sounds a bit presumptuous to me.

Whatever though, looks fine. Things like this sap any drive I have to work on core.

Status:Reviewed & tested by the community» Fixed

Committed 36a898e and pushed to 8.x. Thanks!

Status:Fixed» Closed (fixed)

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

Issue summary:View changes

Updated issue summary.