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.
Related Issues
#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
Comment | File | Size | Author |
---|---|---|---|
#35 | 2065571-35.patch | 5.27 KB | pwolanin |
#35 | 2065571-33-35.increment.txt | 1.62 KB | pwolanin |
#34 | 2065571-29-33.increment.txt | 1.13 KB | pwolanin |
#33 | 2065571-33.patch | 6.89 KB | pwolanin |
#29 | 2065571-29.patch | 6.87 KB | damiankloip |
Comments
Comment #1
neclimdulPlease don't judge me...
Attached patch has a more tested implementation.
Comment #2
neclimdulOne more, forgot to set keys on definition return.
Comment #3
damiankloip CreditAttribution: damiankloip commentedI 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.
Comment #4
damiankloip CreditAttribution: damiankloip commentedSorry, didn't really want/mean to change that to needs review.
Comment #5
neclimdulNo 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.
Comment #6
damiankloip CreditAttribution: damiankloip commentedThe 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?
Comment #7
neclimdulAlso, mixing would make for some confusing DX.
Comment #7.0
neclimdulProvide a summary
Comment #8
neclimdulJust to bump this and make it clear, I basically see this as RTBC at this point. It solves the problem.
Comment #9
damiankloip CreditAttribution: damiankloip commentedI'm writing some tests for this now. Let's add these too.
Comment #10
damiankloip CreditAttribution: damiankloip commentedHere are some tests for the discovery class.
Comment #11
pwolanin CreditAttribution: pwolanin commentedThis 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
Comment #12
pwolanin CreditAttribution: pwolanin commentedrenames the discovery class and its test since there were existing classes of exactly the same name.
Comment #14
neclimdulI 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.
Comment #15
damiankloip CreditAttribution: damiankloip commentedPeter, this is what we have namespaces for. Also, its good to discuss these things before wading in and rerolling patches IMO.
Comment #16
neclimdulpeter actually talked to me on IRC and after voicing my *megh* he went ahead and rolled a patch.
Comment #17
pwolanin CreditAttribution: pwolanin commented@damiankloip - I discussed for a while w/ neclimdul and EclipseGC in IRC.
Comment #18
pwolanin CreditAttribution: pwolanin commentedoops - posted the increment as the patch - here's the real one.
Comment #19
neclimdulBumping 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.
Comment #20
damiankloip CreditAttribution: damiankloip commentedWell, 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.
The issue was not held up, it was 'hijacked', let's say :)
Comment #21
tim.plunkettFWIW, I prefer #10 as well.
Comment #22
dawehner+1 for #10
Comment #23
jibranI am also +1 on #10 becuse namespace says it all
Drupal\Core\Plugin\Discovery\YamlDiscovery
.Comment #24
olli CreditAttribution: olli commented\Drupal\Core\Plugin\Discovery\YamlDiscoveryDecorator?
return parent::getDefinitions() + ..?
Missing a dot.
Comment #25
damiankloip CreditAttribution: damiankloip commentedGreat, 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.
Comment #26
damiankloip CreditAttribution: damiankloip commentedComment #27
dawehnerLast nitpick: Missing starting "\"
Comment #28
olli CreditAttribution: olli commentedIs "@todo" needed here? It's not clear to me what the thing left to do is.
Comment #29
damiankloip CreditAttribution: damiankloip commentedThanks both.
Comment #30
dawehnerThank you!
Comment #30.0
dawehnerClarify some questions that have come up.
Comment #30.1
pwolanin CreditAttribution: pwolanin commentedUpdated issue summary.
Comment #30.2
pwolanin CreditAttribution: pwolanin commentedUpdated issue summary.
Comment #31
pwolanin CreditAttribution: pwolanin commented#29: 2065571-29.patch queued for re-testing.
Comment #32
Gábor HojtsyThis would be needed for #2070023: Convert hook_config_translation_group_info to plugin as well.
Comment #33
pwolanin CreditAttribution: pwolanin commentedPer discussion w/ alexpott in IRC, renaming the class alias.
Comment #34
pwolanin CreditAttribution: pwolanin commentedComment #35
pwolanin CreditAttribution: pwolanin commentedAnd after further discussion in IRC with alexpott, neclimdul, and timplunkett - removing the discovery decorator in the absence of a clear use case.
Comment #36
alexpottCommitted 182ba50 and pushed to 8.x. Thanks!
Comment #37
tstoecklerI 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?
Comment #38
damiankloip CreditAttribution: damiankloip commentedYes, 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.
Comment #39
pwolanin CreditAttribution: pwolanin commentedfor both local actions and local tasks, I think a clean API change would be preferred to a more complex system.
Comment #41
chx CreditAttribution: chx commentedFollowup #2078405: Revert YamlDiscovery if it doesn't make sense as first-class discovery
Comment #42
XanoHow 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.
Comment #43
Gábor Hojtsy@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
Comment #44
pwolanin CreditAttribution: pwolanin commentedWell, 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?
Comment #45
Gábor Hojtsy#2088371-1: YAML discovery incompatible with translations
Comment #46
fgmRelated or affected issue: #2095113: DX improvement: reduce the number of yaml files.
Comment #47.0
(not verified) CreditAttribution: commentedUpdated issue summary.