Problem/Motivation

After #2065571: Add YAML Plugin discovery it is now possible to use YamlDiscovery instead of AnnotatedClassDiscovery. All hell will break lose if some people begin to define plugin types using Yaml for metadata.

Proposed resolution

Remove YamlDiscovery from the Core and Component namespaces; move them under Drupa\Core\Menu and do something radical that stops it from being used in general. As the core YamlDiscovery is not used yet, it's only #2050919: Replace local task plugin discovery with YamlDiscovery that does use it. So, after that:

if (isset($definition['class'])) && array_search('LocalTaskInterface', class_implements($definition['class'])) === FALSE) {
  throw Exception('Nope');
}

as the class will be specified really rarely it is not a big deal that this loads the actual class. To make this usable with LocalActionInterface as well, we might need to tweak the implements check -- but I also observe the three methods of LocalActionInterface appears in LocalTaskInteface so we can certainly do some interface hierarchy magic here to make it happen.

Remaining tasks

Agree, code.

User interface changes

None.

API changes

The parent issue is two days old...

#2065571: Add YAML Plugin discovery
#2074407: Write tests for hook-based plugin discovery
#2050919: Replace local task plugin discovery with YamlDiscovery

Comments

dawehner’s picture

Even I kind of think that drupal should rather provide choice of openness rather than beging a restricted system I agree, the yaml discovery method will not really fit for most
plugin systems out there, at least how plugins are used at the moment.

Local task and local action plugins are different because you don't override the actual method most of the time, in contrast to every other system in core.

chx’s picture

Issue summary: View changes

Updated issue summary.

chx’s picture

Openness is good but we are not going to allow modules to ship with phptemplates to avoid a nightmare where contrib maintainers need to keep two sets of templates. Similarly, we must not allow a situation where some plugin type metadata is annotations and some is in YAML. I am well aware that people could write a YAML discovery themselves but I do think there's a world of a difference between having core support for it.

chx’s picture

Issue summary: View changes

Updated issue summary.

chx’s picture

Issue summary: View changes

Updated issue summary.

chx’s picture

Issue summary: View changes

Updated issue summary.

chx’s picture

Issue summary: View changes

Updated issue summary.

chx’s picture

Issue summary: View changes

Updated issue summary.

chx’s picture

Issue summary: View changes

Updated issue summary.

chx’s picture

Issue summary: View changes

Updated issue summary.

pwolanin’s picture

We are also possibly talking about contextual links as plugins - not sure if that would fit yaml, but seems likely.

however, having a consistent use of annotation as the base mechanism and have something like yaml or hook discovery for derivates when it makes sense is fine with me.

however, I expect this sort of thing will be available in contrib day 1, so it may be worth deciding if there is some variant we like best and supporting it in core.

pwolanin’s picture

Title: Make YamlDiscovery local task/action only » Revert YamlDiscovery if it doesn't make sense as first-class discovery

Instead of doing this I think we should just roll back YAML discovery and code it as derivates in the few cases it's needed.

tim.plunkett’s picture

So this issue and #2074407: Write tests for hook-based plugin discovery are directly contradictory.
I think we should leave all three and stop pretending we know what's best for contrib.

pwolanin’s picture

Changed the title on the other issue: #2074407: Write tests for hook-based plugin discovery

In any case I think it's a bit academic whether we remove them or not since new and easily abused discovery mechanisms will tend to be invented by contrib right out of the gate I expect.

twistor’s picture

All hell will break lose if some people begin to define plugin types using Yaml for metadata.

Really? Really?

I don't appreciate the (contrib is a bunch of monkeys) approach to this issue. I grant that there are nuanced reasons why core would want to use Yaml versus annotations. Why would contrib developers not be able to make the distinction? This sounds like a cop-out so that you don't have to explain the difference. But, actually the reasoning is not so nuanced. It's very simple. The definitions are mostly metadata. The implementation classes are hardly, if at all, different. Why is it that you think contrib won't have similar use cases? There's four and counting in core alone. Nobody's asking you to support it if it's not the intended use case. If I decided to wear Drupal as a hat, would you support it? I'm guessing this issue is is a knee-jerk reaction to people not using annotations. Anybody who gets far enough to have to decide whether or not to use annotations deserves our support and guidance, and will probably choose annotations simply on the basis of that's how it's done. I'm also sure that there will be some projects who defy all logic and do things incorrectly. Is that any different than today? At least give the incompetent masses something that will work. On the scale of what you have to deal with, versus, how much people will benefit, I think the answer is to the later.

tim.plunkett’s picture

Status: Active » Closed (won't fix)

#7 is a perfect explanation of why this issue is a no go.

EclipseGc’s picture

I've been back and forth on a bunch of this discovery stuff through out the 8.x cycle. The main problem here is just that it's hard to know what is best. On the one hand, having options is of course awesome. On the other hand, having options is confusing. pwolanin points this out in his issue about removing hook discovery, but in the same breath we're introducing yaml discovery. Understand, I have 0 problem with there being discovery options, but I do have a vested interest in our Annotation discovery because I feel it's best for Drupal long term, and best for Plugins as well. That being said, the argument for yaml discovery in the menu task/action/context links is convincing because as annotations we're needlessly subclassing other classes, or building a derivative decorator against yaml, which is a kind of hilarious abstraction when we don't really need it. Derivatives are best for BC layers or UI exposure to plugins, that's when they really really shine. Using the derivative as the basic discovery should give us pause, and when neclimdul pointed this out to me, I had to concede the point. If (virtually) all plugins of a type are running through the same class, then Annotation discovery actually don't make a lot of sense for the reasons I already outlined.

By the same token, we want to remove hook discovery because core's not using it? Ok, I can buy that too, but more convincing to me was the fact that most hooks don't really transition to plugins w/o a lot of work. There is of course the classed nature of plugins and this generally replaces a bunch of what I'll term "sub-hooks" (any hook fired as a result of information garnered in a related info hook). While I feel like HookDiscovery COULD do a lot of good, it could do the same amount of good in a module like Devel or some other development helper module.

All in all, I think the argument for the yaml discovery made sense at the time. I think Annotations on empty subclasses should give us pause, and rerouting through derivatives to avoid empty subclasses should also probably give us pause. @chx, if you can specifically respond this last sentence, I think we can discuss this further. If you don't agree with me, I'm completely open to discussion here, I just wanted to make sure the basics of the conversation were captured here in what I hope is a fair representation, because "technically" I think there's merit to the use case. I do agree that the fit would hit the shan if people just started embracing YamlDiscovery over Annotations all the time, but I also believe that yaml (or json, or really anything along these lines) has a time when it's the better solution. Feel free to convince me otherwise, I'm super open to this discussion.

Eclipse

EclipseGc’s picture

@chx, actually nvm, I see all the action is over on #2074407: Write tests for hook-based plugin discovery, let's keep the conversation there.

Eclipse

chx’s picture

Issue tags: +sad chx

tagging.

chx’s picture

Using YAML for local tasks? Fine. But don't make it a plugin discovery: uniform wins.

> I don't appreciate the (contrib is a bunch of monkeys) approach to this issue.

Contrib is not a bunch of monkeys but I see only the confusing side of the arguments here as valid. Drupal 8 has a lot of concepts. Increasing that count is not a win.

Gábor Hojtsy’s picture

YAML discovery also 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

neclimdul’s picture

Thanks Gabor. Lets discuss that there, this has no bearing and is closed.

neclimdul’s picture

Issue summary: View changes

Updated issue summary.