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

Files: 
CommentFileSizeAuthor
#35 2065571-35.patch5.27 KBpwolanin
PASSED: [[SimpleTest]]: [MySQL] 58,516 pass(es).
[ View ]
#35 2065571-33-35.increment.txt1.62 KBpwolanin
#34 2065571-29-33.increment.txt1.13 KBpwolanin
#33 2065571-33.patch6.89 KBpwolanin
PASSED: [[SimpleTest]]: [MySQL] 58,538 pass(es).
[ View ]
#29 2065571-29.patch6.87 KBdamiankloip
PASSED: [[SimpleTest]]: [MySQL] 58,635 pass(es).
[ View ]
#29 interdiff-2065571-29.txt1.05 KBdamiankloip
#25 2065571-25.patch6.93 KBdamiankloip
PASSED: [[SimpleTest]]: [MySQL] 58,325 pass(es).
[ View ]
#25 interdiff-2065571-25.txt1.43 KBdamiankloip
#18 2065571-12real.patch6.97 KBpwolanin
PASSED: [[SimpleTest]]: [MySQL] 58,232 pass(es).
[ View ]
#12 2065571-12.patch3.26 KBpwolanin
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 2065571-12.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#12 2065571-10-12.increment.txt3.26 KBpwolanin
#10 2065571-10.patch6.9 KBdamiankloip
PASSED: [[SimpleTest]]: [MySQL] 58,238 pass(es).
[ View ]
#10 interdiff-2065571-10.txt743 bytesdamiankloip
#3 2065571-3.patch3.37 KBdamiankloip
PASSED: [[SimpleTest]]: [MySQL] 57,871 pass(es).
[ View ]
#3 interdiff-2065571-3.txt2.76 KBdamiankloip
#2 2065571-yaml_discovery-3.patch1.71 KBneclimdul
PASSED: [[SimpleTest]]: [MySQL] 58,112 pass(es).
[ View ]
#1 2065571-yaml_discovery.patch1.7 KBneclimdul
PASSED: [[SimpleTest]]: [MySQL] 58,097 pass(es).
[ View ]
yaml_plugin_discvery.patch3.4 KBneclimdul
FAILED: [[SimpleTest]]: [MySQL] 57,382 pass(es), 903 fail(s), and 531 exception(s).
[ View ]

Comments

Title:Add YAML Plugin parser.Add YAML Plugin discovery.
StatusFileSize
new1.7 KB
PASSED: [[SimpleTest]]: [MySQL] 58,097 pass(es).
[ View ]

Please don't judge me...

Attached patch has a more tested implementation.

StatusFileSize
new1.71 KB
PASSED: [[SimpleTest]]: [MySQL] 58,112 pass(es).
[ View ]

One more, forgot to set keys on definition return.

Status:Active» Needs review
StatusFileSize
new2.76 KB
new3.37 KB
PASSED: [[SimpleTest]]: [MySQL] 57,871 pass(es).
[ View ]

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.

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

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.

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?

Also, mixing would make for some confusing DX.

Issue summary:View changes

Provide a summary

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.

Assigned:Unassigned» damiankloip

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

Assigned:damiankloip» Unassigned
StatusFileSize
new743 bytes
new6.9 KB
PASSED: [[SimpleTest]]: [MySQL] 58,238 pass(es).
[ View ]

Here are some tests for the discovery class.

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

Status:Needs work» Needs review
StatusFileSize
new3.26 KB
new3.26 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 2065571-12.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

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.

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.

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

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

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

StatusFileSize
new6.97 KB
PASSED: [[SimpleTest]]: [MySQL] 58,232 pass(es).
[ View ]

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

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.

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 :)

FWIW, I prefer #10 as well.

+1 for #10

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

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.

Status:Reviewed & tested by the community» Needs work
StatusFileSize
new1.43 KB
new6.93 KB
PASSED: [[SimpleTest]]: [MySQL] 58,325 pass(es).
[ View ]

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.

Status:Needs work» Needs review

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

Last nitpick: Missing starting "\"

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

StatusFileSize
new1.05 KB
new6.87 KB
PASSED: [[SimpleTest]]: [MySQL] 58,635 pass(es).
[ View ]

Thanks both.

Status:Needs review» Reviewed & tested by the community

Thank you!

Issue summary:View changes

Clarify some questions that have come up.

Issue summary:View changes

Updated issue summary.

Issue summary:View changes

Updated issue summary.

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

Issue tags:+blocker

StatusFileSize
new6.89 KB
PASSED: [[SimpleTest]]: [MySQL] 58,538 pass(es).
[ View ]

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

StatusFileSize
new1.13 KB

Title:Add YAML Plugin discovery.Add YAML Plugin discovery
StatusFileSize
new1.62 KB
new5.27 KB
PASSED: [[SimpleTest]]: [MySQL] 58,516 pass(es).
[ View ]

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

Status:Reviewed & tested by the community» Fixed

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

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?

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.

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

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.

@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

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?

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

Issue summary:View changes

Updated issue summary.