#1763974: Convert entity type info into plugins will provide the first plugin type on behalf of an include.
Each plugin type has an $owner and a $type.
For modules, $owner is always the module name.

So in that issue, I chose "Core" as the $owner, and "Entity" as the $type.
But sun suggested "Entity" as the $owner and "Type as the $type, which also makes sense.

Should we standardize on the name of the subsystem as the $owner? If so, we'll need to rename that one.

Comments

tim.plunkett’s picture

Issue tags: +Plugin system

Also, it occurs to me that when contrib wants to provide an entity type, they'd still need to use the Core namespace, which is wonky.

EclipseGc’s picture

So, the annotation namespaces have the benefit of telling future devs where they can find the management classes for plugins of this type. a Core owner should immediately imply core\lib\Drupal\Core, and in the case of Core Entity it should imply the directory core\lib\Drupal\Core\Plugin\Core\Entity. As a developer, when I come across node module's lib\Drupal\node\Plugin\Core\Entity, I should be able to deduce the above directory structure immediately. This is why the plugin folks are kind of hard headed about this because whatever you pass there needs to be informative to future developers so that they can dig through the scaffolding for the system if they needed to get a better understanding of it.

I'm not opposed to Entity\Type as the owner\type here, as long as it resides in either the entity module, or if the entity subsystem ends up with its own PSR-0 namespace... and while were on the topic, that's exactly what the difference between Entity\Type (psr-0 namespace) and entity\Type (module namespace) should imply to a developer looking at this later.

In short, Yes, these constructor variables for the AnnotatedClassDiscovery are for discovery of plugins, and they COULD be arbitrary... but they REALLY REALLY shouldn't be because there's a whole set of "win" we could get in what I'll term "reverse discovery" with regard to DX.

Hopefully this rambling made sense.

Eclipse

sun’s picture

Thanks for the rambling, but unfortunately that does not answer the concrete question at hand. What is the answer?

sun’s picture

Re-reading #2 once more, the answer and solution that strictly follows those ramblings seems pretty clear, but it completely wrecks the architectural design of the plugin system:

new AnnotatedClassDiscovery('Core\Entity', 'Type')

Which obviously leads to: Drupal\node\Plugin\Core\Entity\Type\Node

Not to mention, yet another subdirectory level of doom.

In general, it looks and feels extremely wonky to have "Core" in there. Symfony and other frameworks also have "core services" for which bundles/extensions supply dependent implementations, but you never see a "Symfony" directory in them.

Also note that "Core" is really just our second trashbin for Components, which just happen to not be decoupled yet. The actual topic is:

What's the $owner for components?

PHP namespaces are case-insensitive, but PSR-0 is case-sensitive. So technically Drupal\node\Plugin\Entity\Type and Drupal\node\Plugin\entity\ViewMode could co-exist. However, only as long as $type does not overlap and the same extension (here: node) does not try to implement plugins for both owners — that is, because there are case-insensitive filesystems (e.g., on Windows), which are not able to differ between the subdirectory names.

If we want to avoid that situation at all costs, then we would have to go with Core\Entity\Type.

However, in reality, that only leads to the next best problem: "Core" is not really an owner, and if there is "Core", then there is also "Component" at some point. And alas, the plugin system was designed to be a generic component that can be adopted by other systems and components, so there is a chance that there might be a "Component" at some point that is not a Drupal component.

Thus, the ultimate bullet-proof answer can only be this:

new AnnotatedClassDiscovery('Drupal\Core\Entity', 'Type')
---
use Drupal\node\Plugin\Drupal\Core\Entity\Type\Node;
---
core/modules/node/lib/Drupal/node/Plugin/Drupal/Core/Entity/Type/Node.php
fago’s picture

The bullet-proof suggestion above is really verbose.. :-/

Howsoever, we also have TypedData plugin implementions below the TypedData and Entity component which right now do not use annotations, system.module register this classes. But we want to convert this to annotations and they probably won't stay the only plugins provided by components. Thus, we should make sure that core components are treated like modules for the plugin system, i.e. plugin managers and implementations below a core component should follow the same directory structure as used for a module.

sdboyer’s picture

#2 - i get what you're going for. but 'reverse discovery' is only an acceptable argument if it is readily discoverable for humans as well as machines, as far as i'm concerned. and human reverse discovery stops applying when we have umpteen patterns governing namespaces and directory locations. no one is going to be able to keep track of why anything is where it is. this is just magic string prefixing all over again, and that is one of the chief things that confused the living shit out of people with ctools plugins for years. and namespaces are *way* more confusing that string prefixing.

namespaces really aren't a system that scale well into multiple masters determining their structure. we need to get this madness under control. this is about the least fortunate way in which we could emulate Java.

attiks’s picture

FYI: Maybe not really related, but have a look at #1831154: Allow the AnnotatedClassDiscovery to use custom locations

sun’s picture

effulgentsia’s picture

I have a slight modification to #2: plugin manager classes should ideally always be named Drupal\[$owner]\Plugin\[$CamelizedType]Manager

If the entity type manager class is Drupal\Core\Entity\Plugin\EntityManager, that means that $owner should be Core\Entity and $type should be entity. However, I would suggest renaming the manager class to Drupal\Core\Entity\Plugin\EntityTypeManager or Drupal\Core\Entity\Plugin\TypeManager, which would leave $owner as Core\Entity, and make $type into entity_type or type.

#4 is correct that this convention does not allow for plugin types / managers to be provided by non-Drupal projects. Frankly, I think that's ok. Those projects can pick their own convention. At some point, we may want Drupal plugin types and non-Drupal plugin types to more cleanly co-exist, but let's cross that bridge when we have more concrete examples of non-Drupal plugin types in the wild.

sun’s picture

I think this issue mainly targets plugins though, not plugin managers. Although that might not matter that much.

What I rather want to stress on is that we should forget about "Core". Ideally, yesterday.

"Core" has to be replaced with "Component". The only reason "Core" exists is that the components within are not properly decoupled yet. As soon as they are, there is no "Core" anymore.

"Core" shares the same problem with "Component" though: It has literally no meaning at all. All that both are denoting is a trashbin for components. They are taxonomy terms. They do not represent an owner. Like, at all. Not in the way the plugin system understands owners. The components within are owners.

We not only need to forget about "Core", we also need to forget about "Component".

That leaves us with "Drupal" and "Entity". If I could make any sense of this, then the $owner would have to be Drupal\Entity. So as to uniquely differentiate it from Doctrine\Entity.

Once we arrive at that point, we only have to decide whether the plugin system is truly generic or not. If it is, then the $owner must be Drupal\Entity. If it is not, then the $owner could just be Entity.

This is crystal clear from my perspective. All we need is a decision on whether To Be Generic, Or Not To Be. That is the question.

In the case of just Entity, due to the way the plugin system works, that would inherently prevent co-existence of components and extensions that share the same, case-insensitive name. (e.g., a contributed entity module would not be able to participate in the plugin system)

All in all, Drupal\Entity as $owner appears to be the correct answer for me. That would leave us with:

new AnnotatedClassDiscovery('Drupal\Entity', 'Type')
---
use Drupal\node\Plugin\Drupal\Entity\Type\Node;
---
core/modules/node/lib/Drupal/node/Plugin/Drupal/Entity/Type/Node.php
effulgentsia’s picture

So we'd have $owner = Drupal\Entity, Drupal\Mail, etc. for Drupal's various non-module subsystems, and entity, mail, etc. for correspondingly named modules? I can accept that.

effulgentsia’s picture

Though, if we're breaking the mapping of plugin manager lives in Drupal\[$owner]\Plugin\[$CamelizedType]Manager, then why not $owner = Drupal, $type = entity_type?

tstoeckler’s picture

Why, instead of Drupal\Mail, Drupal\Entity, not simply Mail, Entity?

sun’s picture

#13:
I don't consider "Drupal" to be a formal subject/owner. "Drupal" only complements the actual component name, with the sole purpose to uniquely differentiate it from a non-Drupal Entity component.

That, on its own, circles back into the ultimate question of whether the plugin system should be super-generic or not. If we decide against, then there's little point in a "Drupal" prefix in the first place.

#12:
Having plugin namespaces of components in core "extra-vendorized", but not those of extensions, is actually weird from my perspective — but mayhaps acceptable. True consistency without any weirdness would circle back into the full-blown bullet-proof plugin namespaces of #4.

effulgentsia’s picture

The problem with owner of Entity is that if you also have an entity.module creating an owner of entity, then you cannot have modules that implement plugins for both owners, because on a case-insensitive file system, you can't simultaneously have modules/node/Plugin/Entity/ and modules/node/Plugin/entity/.

If the reason for a Drupal prefix is to avoid collision with non-Drupal code, then we need it for modules too: i.e., Drupal\aggregator, which means we can't use Drupal\Entity as that collides with Drupal\entity.

Unlike sun, I do consider "Drupal" to be a perfectly ok formal owner of everything that's not in a module, so #13 remains my current preference.

But, if we must allow for non-collision of Drupal module owners with non-Drupal owners (i.e., we want to allow owner of Assetic (the non-Drupal project) and assetic (a hypothetical Drupal module)), then we can do owner = full namespace (i.e., Drupal\Core\Entity and Drupal\aggregator), but I'm not convinced that we'll have enough use cases of needing this collision protection to justify the DX loss.

effulgentsia’s picture

If PSR-0 allows for namespace parts to start with a leading _, then we can use that for subsystems: e.g., allow the $owner of plugin types defined by Drupal\Core\Entity to be _entity.

But, I'm still wondering what the objection to Drupal, Core, or Component as $owner values is. All $owner needs to be is a space within which plugin type names can be unique. Each contrib module needs to be its own $owner, because there's no way to realistically coordinate unique plugin type names among thousands of contrib maintainers. But, we can certainly ensure unique plugin type names across the entirety of Drupal subsystems.

tstoeckler’s picture

I don't think not being able to have a generic entity.module should be an argument against this approach. If you imagine a fully decoupled and component-ized Drupal, the differences between modules and components are not that large. Modules can have a bunch of things (bundle, schema, config, ...) that components can't, but those are really implementation details (i.e. we could easily support a Schema.php or whatever in each component if we wanted to). Conceptually, though, the two are not far apart. So having both an Entity component and an entity.module could be considered a WTF, actually. Regardless of what we do here, I would always suggest to name the module something more specific (i.e. entity_ui.module, entity_remote.module, or whatever the heck a theoretical entity.module might do) in order for it not to be confused with the component.

fago’s picture

The problem with owner of Entity is that if you also have an entity.module creating an owner of entity, then you cannot have modules that implement plugins for both owners, because on a case-insensitive file system, you can't simultaneously have modules/node/Plugin/Entity/ and modules/node/Plugin/entity/.

Yes, having component names and modules names in prefixes is a general namespacing problem - it's not special to plugin types - as pointed ou by #18 also. Therefore, I'd suggest we generally solve the namespacing problem and use Entity as owner. We could disallow modules being named like components? But then, is it possible to add non-core components without having a module?

effulgentsia’s picture

I don't think not being able to have a generic entity.module should be an argument against this approach.

While it's okay to tell people, "don't name your 8.x modules the same as any Drupal core component names that exist when 8.0 is shipped", what if we want to add a core component in 8.1-8.n? Not only would we need to check whether a d.o. contrib project already exists by that name (annoying, but possible), but additionally, we'd have no way of knowing if some sites out there are running a custom module with that name.

Yes, having component names and modules names in prefixes is a general namespacing problem - it's not special to plugin types

With function names in include files, yes, we've always had this problem, and would therefore not introduce new function name prefixes in minor releases. But a benefit of namespaced classes is that we no longer have this problem, except for plugin discovery.

If PSR-0 allows for namespace parts to start with a leading _, then we can use that for subsystems: e.g., allow the $owner of plugin types defined by Drupal\Core\Entity to be _entity.

https://github.com/php-fig/fig-standards/blob/master/accepted/PSR-0.md, confirms that "_ has no special meaning in a namespace." Given this, is there any objection to making $owner = '_' . strtolower($third_part_of_namespace) (i.e., "_entity")? It would also have the benefit of, in a folder listing of $moduledir/lib/Drupal/$modulename/Plugin, putting all core subsystem plugin type owners at the top.

EclipseGc’s picture

I think the tweaks we're trying to make to abstracting annotated class discovery up to a component should in effect remove this complaint as arbitrary directories would be able to be injected as namespaces, so search Drupal\Core\Entity for plugins local to it would become totally kosher. Is this a fair evaluation of what the actual complaint here is?

Eclipse

effulgentsia’s picture

I think the tweaks we're trying to make to abstracting annotated class discovery up to a component should in effect remove this complaint

#1849752: Abstract non-Drupal-specific parts of AnnotatedClassDiscovery into a Drupal\Component base class will help allow us to do whatever arbitrary thing we want, including pulling entity type classes out of Plugin entirely, so potentially we could decide that in the case of entity types, we want to abandon the Plugin/$owner/$type convention entirely and rename Drupal\node\Plugin\Core\Entity\Node to something more like Drupal\node\Entity\Node. I guess one option is we decide to do that not just for entity types, but for all other plugin types defined by core subsystems. However, if we decide we want to keep the Plugin/$owner/$type convention for at least some core subsystem plugin types, then we'll still need to set a convention for what $owner should be. Contrib can always break whatever conventions it wants, but we still need conventions for core.

EclipseGc’s picture

So the reason the convention is Plugin\$owner\$type is because I wanted plugin directories to be obviously plugins. Drupal\node\Entity\Node, while nice and clean, doesn't tell anyone it's a plugin (which is fine considering the fact that it's actually not right now, but my point stands). If we want to occasionally break that convention, I'm ok with providing methodologies for doing that as #1849752: Abstract non-Drupal-specific parts of AnnotatedClassDiscovery into a Drupal\Component base class does, but another thing that I thought this was complaining about was the fact that Drupal\Core\Entity can't contain plugins, which that issue also fixes. I'm just questioning if that removes the need for this issue to be anything but meta. And agreed upon convention is great, exceptions to that convention are acceptable, but if the referenced issue takes care of the technical sides of these concerns, are we down to just a convention discussion?

Eclipse

andypost’s picture

So issue about menus as config entities needs suggestion to place a required Menu entity, glad to hear any suggestions #1814916: Convert menus into entities

Chx recomends lib/Drupa/Core/Menu

effulgentsia’s picture

tim.plunkett’s picture

Issue summary: View changes
Status: Active » Closed (won't fix)