Problem/Motivation

#1043198: Convert view modes to ConfigEntity turns view modes into an entity provided by the entity system (Drupal\Core\Entity) itself. Since entity types are now plugins (yay!), the entity system needs to register that plugin. This leads to the following points, which aren't bugs/problems necessarily, but warrant some discussion:

  1. The Plugin system dictates that the class be in the following namespace:
    Drupal\$plugin_owner\Plugin\$plugin_type_owner\$plugin_type\$class_name

    2. which amounts to:
    Drupal\Core\Plugin\Core\Entity\EntityViewMode
    in the case mentioned above. This works, but it seems strange that this class is not contained inside the Drupal\Core\Entity namespace, because the owner is not Entity (which it conceptually is), but Core (which, on its own, is meaningless).

  2. Each entity type needs to declare the module it belongs to. I went with $module = 'Core' but that feels very unnatural.
  3. The previous point leads to the situation where we need to check an arbitrary module owner for whether it is core or not. For example, in the referenced issue, I've had to do
    if ($module != 'Core' && !module_exists($module)) { ... }
    which doesn't feel very clean either.

Proposed resolution

No idea. :-)
Please discuss.

Remaining tasks

Find out what we want to do, and do it. :-)

User interface changes

None.

API changes

Don't know yet.

CommentFileSizeAuthor
#45 wrong-namespace-1828616-45.patch729 bytesberdir
#38 component-plugins-1828616-38.patch8.78 KBberdir
#30 component-plugins-1828616-30.patch8.77 KBberdir
#30 component-plugins-1828616-30-interdiff.txt1.97 KBberdir
#27 component-plugins-1828616-27.patch8.49 KBberdir
#27 component-plugins-1828616-27-combined.patch58.12 KBberdir

Comments

sun’s picture

sun
Primary language German
Location Karlsruhe
commented
Title: Figure out DX for plugins provided by Drupal\Core and Drupal\Components » Plugin owner names "Core" or "Component" impose problems
Priority: Normal » Major

This issue borderline duplicates #1821846: Consider better naming conventions for plugin types owned by includes

However, that issue talks about "naming" only. The consequences outlined in this issue summary here are major, IMO.

tstoeckler’s picture

tstoeckler
he/him
Primary language German
Location Essen, Germany
commented

Right, while the two are actually distinct, as that is about plugin types and this one is about plugins, the resulting problems are similar/identical. I would suggest to leave this open for now, though.

berdir’s picture

berdir
Primary language German
Location Switzerland
commented

Note: the third point is currently, at least that example, only necessary because of #1780396: Namespaces of disabled modules are registered during test runs. Once we've fixed that issue then this shouldn't be necessary anymore.

tstoeckler’s picture

tstoeckler
he/him
Primary language German
Location Essen, Germany
commented

Re #3: Well in the specific case we had at hand in the mentioned issue, you are correct. But still wherever you are dealing with $plugin_definition['module'] you might have the weird 'Core' at your hands so any function call that would usually make sense (module_en/disable(), module_invoke(), ...) need that check. I'm not sure I can think of a real use-case for that, but it's weird nonetheless.

attiks’s picture

attiks commented

Related #1831154: Allow the AnnotatedClassDiscovery to use custom locations

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
commented

#4, the only reason that $plugin_definition['module'] exists is to work around that bug, we could delete all of those keys afterward if we desired to.

neclimdul’s picture

neclimdul
He/Him
commented

3-6 yeah that module bit is not part of the original design and works around a pretty bit bug in simpletest and should not be there. It doesn't have anything to do with plugins actually functioning and doesn't seem relevant to this issue.

Longer post on the issue to come.

neclimdul’s picture

neclimdul
He/Him
commented

I'm tempted to close this because there aren't actually any problems imposed by the owner names.

1) Core totally has meaning. Its something Core to Drupal and not provided by a module. Every namespace has meaning as is shown defined by your psuedocode. What is important to see here is Drupal is providing the implementation not the sub system. The entity subsystem just provides the code to make things work which puts Drupal core on equal footing with any module implementations and doesn't special case a subsystem into the mix.

2) 3) non-issues per discussions.

tstoeckler’s picture

tstoeckler
he/him
Primary language German
Location Essen, Germany
commented

I thought about this a lot in the past days and I would totally agree with #8 if the owner of the plugin type and/or plugin were Entity instead of Core. I.e. I think your argument is valid if you think in terms core subsystems but not in terms of Drupal core as a whole. Something in Drupal\Core\Plugin belonging to the Entity subsystem is just non-sensical, whichever you put it.

What I would be fine with, and which goes in the direction you describe, would be the following:
We could register each subsystem (I mean both of those in Core and Component) as its own namespace below Drupal, e.g. Drupal\Plugin, Drupal\Entity, etc. and then we would have the following situation:
A: node.module registers the Node entity at:
Drupal\node\Plugin\Entity\Type\Node
B: the entity subsystem (see above) would register its EntityViewMode entity at:
Drupal\Entity\Plugin\Entity\Type\EntityViewMode

In that case the plugins *provided* by the entity system would actually lie inside of entity system itself.

neclimdul’s picture

neclimdul
He/Him
commented

I think Core is the correct location still. What you are suggesting is the special casing I was saying we don't want.

tstoeckler’s picture

tstoeckler
he/him
Primary language German
Location Essen, Germany
commented

Well, that's arguable. I 100% disagree with your following statement in #8:

What is important to see here is Drupal is providing the implementation not the sub system.

The modules in core/modules are part of Drupal core as well. Modules are just another sub-system. There's currently a specific difference in that modules can have a schema, etc. and pure sub-systems can't, but in principal there's little conceptual difference in core/modules/node and core/lib/Drupal/Core/Entity. So, following your suggestion, we might as well replace Drupal\node with Drupal\Core...

neclimdul’s picture

neclimdul
He/Him
commented

Yeah, thought we disagreed on that. Since we fundamentally disagree we should probably look for an outside opinion. Maybe @effulgentsia has time for a quick review? Wishing tagging someone actually notified them...

I will say the difference at least with nodes being modules interact within module space. Core plugins as I see them are plugin implementations provided by Drupal core but not working in a module space. If they did they'd be in system or the appropriate module. You're not enabling or disabling(in the module sense), integrating with hooks, etc with these implementations though. Is see the Core namespace and Core/Plugins as the logical place for these sort of things. And if node's code wasn't interacting at that level I would totally agree with their move into the Drupal/Core namespace.

sun’s picture

sun
Primary language German
Location Karlsruhe
commented

I think we're making a big mistake in talking about "Core" in the first place here. IMO, forget about that term, now.

The actual topic is "Component". As /core/lib/Drupal/Core/README.txt clarifies, "Core" is just a custom construct to denote that the Components contained in the Core namespace are not fully decoupled from Drupal yet. That's the only difference to Drupal\Component.

The moment you think about components, the owner question becomes crystal clear: The Entity component is the owner.

But I'm only repeating myself here. #1821846-4: Consider better naming conventions for plugin types owned by includes contains an in-depth analysis of the owner problem space with regard to the plugin system already, which still needs to be addressed.

neclimdul’s picture

neclimdul
He/Him
commented

Well, I still disagree. Drupal something own's the implementation, not the Entity whatever

fago’s picture

fago
Primary language German
Location Vienna
commented

1) Core totally has meaning. Its something Core to Drupal and not provided by a module. Every namespace has meaning as is shown defined by your psuedocode. What is important to see here is Drupal is providing the implementation not the sub system. The entity subsystem just provides the code to make things work which puts Drupal core on equal footing with any module implementations and doesn't special case a subsystem into the mix.

I disagree. It's the components that are relevant. Else, we can go back and squeeze everything in a single system/core module now. E.g. the Entity subsystem provides some typed data plugin implementations. Having all core plugins in a single place, would untie their relationship to the Entity system. That's a nogo.

From /core/lib/Drupal/Core/README.txt

Each Subsystem should be in its own namespace, and should be as self-contained as possible.

sun’s picture

sun
Primary language German
Location Karlsruhe
commented

@fago nailed it.

effulgentsia’s picture

effulgentsia commented

E.g. the Entity subsystem provides some typed data plugin implementations.

Suppose the Drupal\Core\Entity subsystem implements a plugin for a plugin type defined by the Drupal\Core\TypedData subsystem. I agree that that plugin should live in Drupal\Core\Entity\Plugin\OWNER\TYPE\. However, I do not believe that necessitates or implies that OWNER needs to be TypedData, but that's a discussion for #1821846: Consider better naming conventions for plugin types owned by includes.

attiks’s picture

attiks commented

#17: Drupal\Core\TypedData ships with his own plugin implementations, so this is more about where it should put those, not where other modules implementing the TypedData plugins will put theirs, because they can put them inside their own modules Plugin directory. The same is true for the Validation we're trying to add, it ships with some default plugins.

#15++

effulgentsia’s picture

effulgentsia commented
Title: Plugin owner names "Core" or "Component" impose problems » AnnotatedClassDiscovery is not finding plugins in Drupal\(Core|Component)\COMPONENT_NAME\Plugin directory
Category: task » bug

Ok, my confusion then was around the term "plugin owner". That term doesn't make sense to me. $owner is always about who owns the plugin type. Plugins don't have owners (in my mind), they just exist in a namespace somewhere. Retitling and reclassifying as a bug. Correct me if I'm misunderstanding the issue though.

tstoeckler’s picture

tstoeckler
he/him
Primary language German
Location Essen, Germany
commented

The re-titling is correct. I don't really know what the adequate term for "he/she who provides/declares an actual plugin" is, so I went with "plugin owner" vs. "plugin type owner". You're right that that is confusing, though, because $owner generally signifies the latter.

fago’s picture

fago
Primary language German
Location Vienna
commented

Created #1867856: Use annotation discovery for data type plugins and postponed it on this issue.

eclipsegc’s picture

eclipsegc commented

I'm pretty sure this is resolved. I am working against an unpatched HEAD currently with conditions in Drupal\Core\Plugin\Core\Condition and it totally picks them up. Can someone else confirm and close this if so?

Eclipse

fago’s picture

fago
Primary language German
Location Vienna
commented

Drupal\Core\Plugin\Core\Condition is not the right directory, see #15. It should be Drupal\Core\Condition\Plugin\*something*

berdir’s picture

berdir
Primary language German
Location Switzerland
commented

Yes, we were just discussing that in IRC.

See #1836008: Remove drupal_classloader() use in Drupal\Core\AnnotatedClassDiscovery

The easiest way to do that after the above issue is in, without any architectural changes, will be that the condition manager simply adds the directory/namespace to the array he gets. He (It?) can even decide if he wants to use $plugin_namespaces, where he has full control over the namespace (and could just use \Drupal\Core\Condition\ or \Drupal\Core\Condition\Plugin, or $root_namespaces, which would enforce Drupal\Core\Condition\Plugin\Core\Condition\.

That won't solve the problem of Component A providing plugins for component B, that would require something like a bundle per component which then could add itself as a possible plugin location, as @sun suggested in the issue above.

eclipsegc’s picture

eclipsegc commented

once #1836008: Remove drupal_classloader() use in Drupal\Core\AnnotatedClassDiscovery is committed, I'm pretty sure it'll solve that.

sun’s picture

sun
Primary language German
Location Karlsruhe
commented

Note that I'm not sure whether the issue title change in #19 was appropriate, and whether this issue here originally encompassed further/other topics.

The new issue title duplicates the following issues to some extent:
#1821846: Consider better naming conventions for plugin types owned by includes
#1847002-26: Move entity type classes from Drupal\$provider\Plugin\Core\Entity to Drupal\$provider\Entity

But then again, this new issue title is very focused, and apparently #1867856: Use annotation discovery for data type plugins is postponed on this issue here.

berdir’s picture

berdir
Primary language German
Location Switzerland
commented
Status: Active » Needs review
StatusFileSize
new component-plugins-1828616-27-combined.patch58.12 KB
new component-plugins-1828616-27.patch8.49 KB

Simple patch that does what I suggested in #24 for the Constraint plugins.

berdir’s picture

berdir
Primary language German
Location Switzerland
commented

Note that we could now also remove the hardcoded Drupal\Core and Drupal\Component from the plugin namespace list as having plugin under that namespace makes no sense and was only done because it currently worked and this did not.

sun’s picture

sun
Primary language German
Location Karlsruhe
commented

I'm not happy with one-off registrations of component namespaces in individual plugin managers.

As mentioned in #28, I think we just simply want to replace the two Drupal\Core and Drupal\Component entries in DrupalKernel with all components that may provide plugin types or plugin implementations.

Note that this is also fine in terms of performance. The plugin discovery slowness is caused by Annotation parsing only. If there is no .\Plugin\$owner\$type directory in the first place, then performance is not negatively affected. Thus, registering the individual components will not have a performance impact.

berdir’s picture

berdir
Primary language German
Location Switzerland
commented
StatusFileSize
new component-plugins-1828616-30-interdiff.txt1.97 KB
new component-plugins-1828616-30.patch8.77 KB

Yep. What about this?

Added an isDir() check to only care about components that have a Plugin directory. That's not going to change, so I think it's fine to dump that information.

I think the current location of plugins and how it's moved with this patch shows that this makes a lot of sense. There is zero reason that the default Constraint plugins of the Validation component should be in the Plugin component, that is just wrong. Whether or not Core is the owner or not will have to be resolved elsewhere, e.g. in the issues that @sun referenced in #26.

fago’s picture

fago
Primary language German
Location Vienna
commented

#30: component-plugins-1828616-30.patch queued for re-testing.

Status: Needs review » Needs work

The last submitted patch, component-plugins-1828616-30.patch, failed testing.

fago’s picture

fago
Primary language German
Location Vienna
commented
Status: Needs work » Needs review

#1930020: Inject a class into the Core annotated discovery implementation so that namespaces can be updated got committed - not sure if it has an impact on this issue?

ad#30:
Wouldn't this let the classloader unnecessarily look into all component directories? Maybe, we should differentiate between namespaces for classloading and namespace for discovering plugins?

eclipsegc’s picture

eclipsegc commented

I specifically chose NOT to make such a distinction since "Plugins" is a component, and should be utilizable outside of Drupal. Differentiating between those two types of namespaces would be a clear Drupalism, and one I'd prefer to avoid.

Eclipse

berdir’s picture

berdir
Primary language German
Location Switzerland
commented

what distinction/drupalism are you talking about?

All my patch does is adding the components to the list of plugin namespace. Having components *and* modules is obviously a Drupalism, this patch implements that Drupalism within DrupalKernel, the Plugin component doesn't care or know if those namespaces are modules or components.

Right now, plugin implementations of component X are below Drupal\Core\Plugin\X instead of Drupal\Core\X, that makes no sense at all.

berdir’s picture

berdir
Primary language German
Location Switzerland
commented

Ok, missed the edit.

@fago: We don't register namespaces to the classloader. We register plugin locations (which happen to be in the form of namespaces. Two separate things.

eclipsegc’s picture

eclipsegc commented

@Berdir

I was responding to fago, not your patch.

Eclipse

berdir’s picture

berdir
Primary language German
Location Switzerland
commented
StatusFileSize
new component-plugins-1828616-38.patch8.78 KB

Git rebase powered re-roll.

fago’s picture

fago
Primary language German
Location Vienna
commented

@fago: We don't register namespaces to the classloader. We register plugin locations (which happen to be in the form of namespaces. Two separate things.

That is what I was referring to ;-) Great to hear it's already that way.

berdir’s picture

berdir
Primary language German
Location Switzerland
commented

Great, sounds like we're all on the same page then ;)

Now... who wants to RTBC? :)

fago’s picture

fago
Primary language German
Location Vienna
commented
Status: Needs review » Reviewed & tested by the community

Yes, let's do this.

fago’s picture

fago
Primary language German
Location Vienna
commented

#38: component-plugins-1828616-38.patch queued for re-testing.

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
commented
Category: bug » task
Priority: Major » Normal
Status: Reviewed & tested by the community » Fixed

This seems like good clean-up but not sure at all why this is major, nor a bug.

I tried to reach neclimdul to get an updated opinion on this issue given recent comments but wasn't successful. However, Eclipse said it looked good to him, so...

Committed and pushed to 8.x. Thanks!

tstoeckler’s picture

tstoeckler
he/him
Primary language German
Location Essen, Germany
commented
Status: Fixed » Needs work 
+++ b/core/lib/Drupal/Core/DrupalKernel.php
@@ -408,8 +408,16 @@ protected function buildContainer() {
+    foreach (array('Core', 'Component') as $parent_directory) {
...
+          $namespaces['Drupal\Core\\' . $component->getFilename()] = DRUPAL_ROOT . '/core/lib';
...
+    }

I might be missing something, but I'm pretty sure that this does not allow things in \Drupal\Component to provide plugins. It should be 'Drupal\\' . $parent_directory . '\\' . $component->getFilename().

berdir’s picture

berdir
Primary language German
Location Switzerland
commented
Status: Needs work » Needs review
StatusFileSize
new wrong-namespace-1828616-45.patch729 bytes

Well, yeah, that was stupid.

effulgentsia’s picture

effulgentsia commented
Status: Needs review » Reviewed & tested by the community

I'm a little torn on whether this needs a test or not. If we end up having plugins in Drupal\Component, that'll be the test. If we don't, then whether it works or not doesn't really matter. Therefore, RTBC, unless someone provides a good reason for needing separate test coverage.

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
commented
Status: Reviewed & tested by the community » Fixed

Wah-wah. :\ Yeah, I think I agree with that rationale on test coverage.

Committed and pushed to 8.x. Thanks!

Status: Fixed » Closed (fixed)

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