Problem/Motivation

We're using a akward hack as a pattern through out core where to provide YAML plugin definitions we use a deriver to stash plugins into the definition list so we can rely on all the "Magic" in the DefaultPluginManager. This is not only wrong and ends up with all our definitions looking something like "random_string:actual_id" with not actual value. There's also a heavy technical dept we're imposing on ourselves by hacking the logic of these plugins into a manager optimizing itself for Annotations that's going to increase as we try to tune these Managers to their use cases.

Proposed resolution

Implement a YAML Discovery method and move managers hacking yaml discovery to this.

Remaining tasks

Review, manager conversion(follow ups).

User interface changes

No interface changes. Just implementation fixes.

API changes

Additional implementation of an existing API and cleanup of unfinished API's as follow ups.

#2050919: Replace local task plugin discovery with YamlDiscovery
#2074407: Write tests for hook-based plugin discovery
#2065481: Make the interaction of the Annotation, Reflection & Plugin Discovery make sense

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

neclimdul’s picture

Title: Add YAML Plugin parser. » Add YAML Plugin discovery.
FileSize
1.7 KB

Please don't judge me...

Attached patch has a more tested implementation.

neclimdul’s picture

One more, forgot to set keys on definition return.

damiankloip’s picture

Status: Active » Needs review
FileSize
2.76 KB
3.37 KB

I haven't given it much time, but I was briefly talking to berdir on IRC, and he suggested to make a discovery decorator too, which is a great idea. I should have thought of that first off tbh when doing the patch for #2050227: Add local action plugin deriver to use YAML discovery for static definitions. I was more worried about not having any api changes.

Anyway, here is a quick start, build on neclimdul's patch.

damiankloip’s picture

Sorry, didn't really want/mean to change that to needs review.

neclimdul’s picture

No problem. There's no new tests so we'll just steal a few minutes from testbot. I'm not sure what the decorator really gives us... but its also straight forward.

damiankloip’s picture

The decorator would allow us to use the existing annotations, as well as YAML files. But I guess this could get tricky with what plugin class they actually load?

neclimdul’s picture

Also, mixing would make for some confusing DX.

neclimdul’s picture

Issue summary: View changes

Provide a summary

neclimdul’s picture

Status: Needs review » Reviewed & tested by the community

Just to bump this and make it clear, I basically see this as RTBC at this point. It solves the problem.

damiankloip’s picture

Assigned: Unassigned » damiankloip

I'm writing some tests for this now. Let's add these too.

damiankloip’s picture

Assigned: damiankloip » Unassigned
FileSize
743 bytes
6.9 KB

Here are some tests for the discovery class.

pwolanin’s picture

Status: Reviewed & tested by the community » Needs work

This is very confusing to have 2 Drupal classes both named YamlDiscovery that do different things. Also tthis adds a test class with the same name as an existing one. I'll re-roll to differentiate them

pwolanin’s picture

Status: Needs work » Needs review
FileSize
3.26 KB
3.26 KB

renames the discovery class and its test since there were existing classes of exactly the same name.

Status: Needs review » Needs work

The last submitted patch, 2065571-12.patch, failed testing.

neclimdul’s picture

Status: Needs work » Needs review

I prefer the shorter names in #10. I think it follows our class naming standards more clearly and uses namespaces better but I don't feel its worthy of holding the issue up. Which ever looks better to commiters works for me. Will let the test come back and we can bump it back up.

damiankloip’s picture

Peter, this is what we have namespaces for. Also, its good to discuss these things before wading in and rerolling patches IMO.

neclimdul’s picture

peter actually talked to me on IRC and after voicing my *megh* he went ahead and rolled a patch.

pwolanin’s picture

@damiankloip - I discussed for a while w/ neclimdul and EclipseGC in IRC.

pwolanin’s picture

FileSize
6.97 KB

oops - posted the increment as the patch - here's the real one.

neclimdul’s picture

Status: Needs review » Reviewed & tested by the community

Bumping it up into commiter queue. 10 and 18 are the 2 commitable approaches with https://drupal.org/files/2065571-10-12.increment.txt containing the naming difference.

damiankloip’s picture

Status: Needs work » Reviewed & tested by the community

Well, I'm sticking to my view on this.

The patch in #10 is the one. We have namespaces for a good reason, it's already in the Plugin namespace, we don't need plugin in the class name too. Plus NO other discovery classes have Plugin in their name? So this will just introduce another inconsistency in our class naming.

I don't feel its worthy of holding the issue up.

The issue was not held up, it was 'hijacked', let's say :)

tim.plunkett’s picture

FWIW, I prefer #10 as well.

dawehner’s picture

+1 for #10

jibran’s picture

I am also +1 on #10 becuse namespace says it all Drupal\Core\Plugin\Discovery\YamlDiscovery.

olli’s picture

Status: Reviewed & tested by the community » Needs work
  1. +++ b/core/lib/Drupal/Core/Plugin/Discovery/YamlDiscoveryDecorator.php
    @@ -0,0 +1,50 @@
    +   * Constructs a \Drupal\Component\Plugin\Discovery\StaticDiscoveryDecorator object.
    

    \Drupal\Core\Plugin\Discovery\YamlDiscoveryDecorator?

  2. +++ b/core/lib/Drupal/Core/Plugin/Discovery/YamlDiscoveryDecorator.php
    @@ -0,0 +1,50 @@
    +  public function getDefinitions() {
    +    return $this->getDefinitions() + $this->decorated->getDefinitions();
    +  }
    

    return parent::getDefinitions() + ..?

  3. +++ b/core/lib/Drupal/Core/Plugin/Discovery/YamlDiscoveryDecorator.php
    @@ -0,0 +1,50 @@
    +   * Passes through all unknown calls onto the decorated object
    

    Missing a dot.

damiankloip’s picture

Status: Reviewed & tested by the community » Needs work
FileSize
1.43 KB
6.93 KB

Great, thanks olli. Good points.

It's nice to have an actual code review rather than a naming bikeshed ;)

Here is a reroll of #10, as that seems to be the consensus here.

damiankloip’s picture

Status: Needs work » Needs review
dawehner’s picture

+++ b/core/lib/Drupal/Core/Plugin/Discovery/YamlDiscovery.php
@@ -0,0 +1,65 @@
+ * Contains Drupal\Core\Plugin\Discovery\YamlDiscovery.

Last nitpick: Missing starting "\"

olli’s picture

+++ b/core/tests/Drupal/Tests/Core/Plugin/Discovery/YamlDiscoveryTest.php
@@ -0,0 +1,94 @@
+    // @todo Stolen from Component YamlDiscoveryTest.

Is "@todo" needed here? It's not clear to me what the thing left to do is.

damiankloip’s picture

Thanks both.

dawehner’s picture

Status: Needs review » Reviewed & tested by the community

Thank you!

dawehner’s picture

Issue summary: View changes

Clarify some questions that have come up.

pwolanin’s picture

Issue summary: View changes

Updated issue summary.

pwolanin’s picture

Issue summary: View changes

Updated issue summary.

pwolanin’s picture

#29: 2065571-29.patch queued for re-testing.

Gábor Hojtsy’s picture

Issue tags: +blocker
pwolanin’s picture

FileSize
6.89 KB

Per discussion w/ alexpott in IRC, renaming the class alias.

pwolanin’s picture

FileSize
1.13 KB
pwolanin’s picture

Title: Add YAML Plugin discovery. » Add YAML Plugin discovery
FileSize
1.62 KB
5.27 KB

And after further discussion in IRC with alexpott, neclimdul, and timplunkett - removing the discovery decorator in the absence of a clear use case.

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Committed 182ba50 and pushed to 8.x. Thanks!

tstoeckler’s picture

I wasn't part of that IRC discussion so I'm interested to know: In how far is #2050227: Add local action plugin deriver to use YAML discovery for static definitions no longer a use-case of the decorator?

damiankloip’s picture

Yes, I'm also not sure why this was just removed. This is also a good candidate for #2050919: Replace local task plugin discovery with YamlDiscovery IMO, as the current patch in that issue is a complete API change.

pwolanin’s picture

for both local actions and local tasks, I think a clean API change would be preferred to a more complex system.

chx’s picture

Xano’s picture

How does this work for plugins that have translatable strings, such as labels? POTX can be hardcoded to look for those in files defined by core's APIs, but this is not possible for contrib.

Gábor Hojtsy’s picture

@Xano: Yes, YAML discovery poses issues for translations. Currently YML files that have translatable strings (names, labels, etc) are either translatable via config schemas (where the schema defines item types and which ones are translatable) or have hardcoded parsing logic (such as for routing.yml where we get the title from or info.yml where we get descriptions or tabs yml where we'll get tab labels). Once we let the yml structures loose outside of the config system, if we don't have a set structure for them (and here we don't) then we cannot extract translatable strings from them for the community. We may be able to apply config schemas to YAML files outside of the config directory, but that would be an exception, and then we still need to define how we figure out which are those files, so we don't run into other YML files :/ Sounds like it gets confusing quickly as to what would config schema apply to :/

#2088371: YAML discovery incompatible with translations

pwolanin’s picture

Well, we do have a set structure for each plugin, just not as a general feature of this type of discovery.

In another issue we are talking about improving plugin documentation by documentation a defaultConfiguration() method that would list the possible keys. Is that a place we could e.g. have a doc annotation and maybe something that tells you what yaml file pattern to look for?

Gábor Hojtsy’s picture

fgm’s picture

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

Anonymous’s picture

Issue summary: View changes

Updated issue summary.