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

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

damiankloip’s picture

Status: Active » Needs review
FileSize
3 KB
4.67 KB

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.

dawehner’s picture

+++ 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?

dawehner’s picture

Status: Needs review » Reviewed & tested by the community

I got convinced

damiankloip’s picture

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.

dawehner’s picture

Status: Needs review » Reviewed & tested by the community

I disagree damian, this was RTBC.

pwolanin’s picture

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.

msonnabaum’s picture

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

pwolanin’s picture

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.

damiankloip’s picture

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.

dawehner’s picture

FileSize
615 bytes
4.83 KB

There we go.

pwolanin’s picture

Status: Needs review » Reviewed & tested by the community
FileSize
762 bytes
4.82 KB

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.

damiankloip’s picture

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.

alexpott’s picture

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.

Anonymous’s picture

Issue summary: View changes

Updated issue summary.