Entity type names/IDs are not properly namespaced by owner (e.g., taxonomy.term vs. taxonomy_term)

I don't think I get what the problem is.

- The current entity type names are namespaced the way we namespace everything else : hooks, field type names, input filters... - pretty much anything that's pluggable. That's a "soft" namespacing, which is not enforced but recommended as a best practice. It doesn't *fully* prevent name clashes, but most of them.
(and yes, on this regard, contact categories currently are currently misnamed)

- "The lack of an owner requires entity info mapping in some places where it wouldn't be required if the owner was known"
What does this mean more specifically ? Do we expect to be able to get the entity info for this entity type alone without building the info for all entity types ? I'd love that, but that's far fetched IMO.

- We currently build hook names out of entity type names : hook_[entity_type]_load()
The dot would get transposed to an underscore ? Works I guess, but could also qualify as confusing magic.

- There is nothing specific about entity types here. If the reasons about name clashes apply, they apply equally to all plugin ids whatever the plugin type.
So do we want all plugins to specify an owner in their plugin id ? Meaning, every plugin must identify an owner module ?

Do we expect to be able to get the entity info for this entity type alone without building the info for all entity types ? I'd love that, but that's far fetched IMO.

Yes, we do.
It should be way easier than what we've got atm and would save a lot of "stress" for the bootstrap system

There is nothing specific about entity types here. If the reasons about name clashes apply, they apply equally to all plugin ids whatever the plugin type.

Right.

So do we want all plugins to specify an owner in their plugin id ? Meaning, every plugin must identify an owner module ?

Yes.

We have some serious issue atm with configuration workflow: you cannot figure out configuration dependencies (or on which modules a view depends on) without having such modules installed.
This is partially fixed (but just partially) with configuration manifest files. But still you need to have the module enabled to figure out the dependencies so you can create the 'manifest' file (Though I still don't know how it works for exporting/importing configuration)

Do we expect to be able to get the entity info for this entity type alone without building the info for all entity types ? I'd love that, but that's far fetched IMO.

That's technically possible, AFAICS
drupal_container()->get('plugin.manager.entity')->getDefinition('node');

So do we want all plugins to specify an owner in their plugin id ? Meaning, every plugin must identify an owner module

At least views and blocks plugins already do this, though mostly as a workaround for #1780396: Namespaces of disabled modules are registered during test runs

- So do we want all plugins to specify an owner in their plugin id ? Meaning, every plugin must identify an owner module ?
- Yes.

OK, but then we should retitle accordingly and reassign to the Plugin API system ?
(I'll let @sun chime in before doing so under his feet)

Sorry, I misread. No plugins specify an owner in their plugin_id, but in a separate 'module' key.

If you want to make Views stupid, go for it. There are 250+ plugins that would need to be converted, let alone all of the places they are referenced.
Then, plus the impending blocks patch.

I'm obviously -1 on this.

@tim.plunkett:

drupal_container()->get('plugin.manager.entity')->getDefinition('node');

WIth most discoveries (including cached ones), this still means collecting all definitions first, so there's no gain.

- So do we want all plugins to specify an owner in their plugin id ? Meaning, every plugin must identify an owner module
- At least views and blocks plugins already do this, though mostly as a workaround for #1780396: Namespaces of disabled modules are registered during test runs

You're talking about an explicit 'module' entry in the plugin info array, right ?
Yes, widgets and formatters do this as well (although they probably don't have a real case actually in D8).
But this issue is about baking the owner in the plugin id, which is different.

[edit: heh, serial crosspost :-)]

I think @sun #7 is a good summary of how badly we need this for the plugin system.

Also, at the point where views needs to add a explicit module name into its configuration, I think we can see how our 'plugin ids' are pretty useless if not properly namespaced.

So I think we should simply add a module name as prefix for all of them in a first step wich would at least give 'Views' (or any other module using plugins) the power to figure out it's own dependencies without adding explicit module names.

And we don't need to change anything else yet, as this one will be a big enough (but simple) patch that will get us two important advantages:
- Prevent clashes with plugin ids for the future (that will be a problem as soon as contrib starts building with plugins).
- Modules being able to figure out its own dependencies from plugin ids.

Then we'll see later whether this can be used for anything else or we better keep our current info hooks etc.. But in any case the need for proper names for plugin ids seems clear to me.

#7 reminds me of #1810480: Provide the plugin_id to support views metadata integration and #1825896: Add module owner to plugin data on handlers

You're talking about an explicit 'module' entry in the plugin info array, right ?
Yes, widgets and formatters do this as well (although they probably don't have a real case actually in D8).

Yes, I think having a (auto-filled) 'module' key in info-hook arrays was already good practice in d7 and very often needed. We really should bake this into plugin definitions by default.

A question is though whether the origin of a plugin must be a module?

Then, whether we use this "origin" information and force it to prefix plugin IDs is another good question. Entity types currently follow our current approach to namespacing by having the module name as prefix. Thus, this issue is questioning our current approach to namespacing in general and should be treated as such - i.e. if changed, change it everywhere.

A question is though whether the origin of a plugin must be a module?

Currently at least the base parts of the Plugin system live in \Drupal\Component, so technically it doesn't/shouldn't know whateverthehell a module is. On the other hand, I've seen many people reject plugins being provided by core subsystems in other issues and instead request to create modules that provide those.

That's #1821846: Consider better naming conventions for plugin types owned by includes.

I think I'm +1 to renaming all plugin ids of plugins provided by modules to $module.$foo. The ability for code to be able to directly know the module of a plugin from just its id will be very useful for a range of things, such as the ones in #7. To me, collision prevention is a nice bonus, but not the primary benefit.

We'll need to figure out what we want to do for plugins provided by core subsystems, but we don't have any yet, due to other problems we'll be solving in already open issues. I'm sure we can come up with some reasonable convention for those ids.

Seems like the main concerns with this that have been raised so far are:

We currently build hook names out of entity type names : hook_[entity_type]_load()
The dot would get transposed to an underscore ? Works I guess, but could also qualify as confusing magic.

Yep, anywhere plugin ids are used by something that doesn't allow a dot (function names, CSS class names, other places?), that code will need to run some kind of string replacement on it to convert it to another delimiter. It's a bit annoying, but I don't see it as a big problem.

If you want to make Views stupid, go for it. There are 250+ plugins that would need to be converted, let alone all of the places they are referenced. I'm obviously -1 on this.

@tim.plunkett: are you -1 because you know of reasons in Views that make this a bad idea, or just because it'll be tedious to roll the patch? If someone else volunteered to roll the patch, would that change your mind?

I've thought about this a lot lately. The separate 'module' key in the annotation is theoretically just a hack for #1780396: Namespaces of disabled modules are registered during test runs, and will go away, in which case we'd need to do this anyway.
This sounds like just an entity-specific version of the direction #1879496: Do we need to worry about plugin ID collisions? is heading.

Also, this issue title mentions only an entity type with an underscore in it already. Are we saying that "the entity type has to start with $module" or "all plugin IDs should be $module.$foo"?

Because when a module only provides one entity type, it will be redundant to have $node = entity_load('node.node', 1);. If we're just talking about ensuring that the entity type name begins with $module, I'm okay with that.

(However, if we do decide to confuse everyone forever by having node.node, user.user, etc., then we can eliminate the concept of a config_prefix altogether...)

the module/owner could be omitted in case it is identical to the plugin ID

So that solves all of the cases where a module only provides one entity type, except Views.

Views has the entity type 'view'. :)
Renaming Views already been discussed and won't fixed. Do we change everything to entity_load('views', 'frontpage') instead of entity_load('view', 'frontpage');? :(

It makes no sense for an entity type name to be plural. If we're stuck with views.module staying plural, then I think entity_load('views.view', 'frontpage') is our best option.

Here's a couple conversions just to see what sort of non-obvious things blow up.
The views.view seems rather natural, actually.

sooooo... yeah there's a lot of stuff here.

Just to get everyone on the same page it's worth addressing a few points that have been brought up here.

sun observers:

1) The plugin system invests tons of logic and structure in order to achieve a multidimensional extensions system, which separates the owner from types and types from implementations, using separate namespaces for discovery. That's why we have to deal with these immense amounts of subdirectories.

2) But then, all of that nice architectural design is dismissed and thrown away, since all of the plugin implementations are using custom plugin IDs, which do not contain anything of the previously set up structure anymore, and plugins are suddenly referenced by their plugin ID only.

Yeah, so this is legitimate at first blush but falls apart in the long term. If you KNEW the class you wanted, then the factory to instantiate it is sort of a formality. Beyond that, things like AlterDecorators give us the ability to actually replace live plugins with a completely different class, a thing we'd have not been able to do had we simply referenced the class name directly. The plugin system is about discovery more than it is about instantiation (though plugins of a type should be universally instantiate-able in the same way). Asking for $manager->createInstance('node); is a lot nicer than $manager->createInstance('Drupal\node\Plugin\Core\Entity\Node') and that's especially true if someone were to replace the node class whole hog.

fago asks:

A question is though whether the origin of a plugin must be a module?

To which I offer a resounding "No". Plugins do not have to come from a module. Fago has already answered this question for himself in TypedData validators where he's offered Symfony classes as legitimate plugins to the plugin system (the rest of the plugins for this use case simply extend the Symfony provided interface and are found in typical plugin ways while the Symfony classes are manually registered). Furthermore, an awful lot of work has go into the AnnotatedClassDiscovery class in order to make it generic and even more capable of providing for finding plugins in non-namespace-specific environments.

On this same topic I think it was fago who observed that module name in the definitions returned by info hooks was quite common, and I do indeed have an objective of providing this automatically where possible once the improvements that need to be made to the AnnotatedClassDiscovery are in.

Most of this brings us back to the basic question of providing module prefixes to plugin ids, and the general question of plugin id conflicts. This is perplexing to me to a certain extent. I'm not sure in what scenario we are imagining that someone would create ANOTHER node entity type, because this is the only possible conflict. I could make a "node" plugin for blocks and it would be irrelevant to any other plugin type. Yes, plugin ids have to be unique within their plugin type... I'm not sure how that's news, or a technical difficulty. It's certainly not a difficulty introduced by plugins because info hooks have this same problem.

With all of that said, I'll end by addressing this:

yched asks, and jose responds:

Do we expect to be able to get the entity info for this entity type alone without building the info for all entity types ? I'd love that, but that's far fetched IMO.

Yes, we do.
It should be way easier than what we've got atm and would save a lot of "stress" for the bootstrap system

"No, we do not." You can't do this with hooks, you can't do it with annotations, there may be some esoteric mechanism by which we can do it, but we don't currently have access to such a thing if so. You have to retrieve (and hopefully cache) all the definitions returned by the annotations or hook invocation of a plugin type before you can retrieve a single plugin definition. That's the nature of the beast.

In short, I'm pretty -- to prefixing all plugin ids with their providing modules. Plugins are not necessarily always provided by a module. I understand this could make some sort of introspectable system by which we can determine what modules we're missing in order to create a working view, or page, or whatever, but even that's not actually true since those things can be altered, so it's reliability is questionable (as is the extent to which this class replacement methodology I'm so in love with will be used by contrib). In short, this seems like the sort of thing that could be documented in config entities through live introspection on a working system before being synced to another site, and I still don't understand why plugin ids have become a hot button topic. This isn't different from what we have in the info hook system for basically any version of Drupal ever.

Eclipse

After further discussion with tim on IRC it's apparent I should make my stance more clear:

I'm not opposed to this change, I'm opposed to code that imposes the change. If you want to specify that plugins, as a rule, must prefix their plugin_id with the module that provides them, fine whatever, I don't really care. But let's not automatically append such a prefix, and let's be smart about our exceptions (of which I think entity is likely a good example of an example of an exception we should make, but I digress). So this sounds more like a meta to me with a potential additional gateway change.

Eclipse

Ok, these are reasonable questions, so let's walk through them:

How do I look up the plugin IDs of 13,000+ contributed and custom modules?

How do you do that today? You don't, info hooks have this exact same problem. Core has a history of squatting namespaces, that's its right as core, and if we want to change that, again I'm completely ok with it, but I'd prefer we did so manually, not enforced some sort of automatic solution to do so.

if, in your example, text_plain.module is intending to fix core's text_plain plugin, it should do thus (again code guaranteed to be wrong because I'm totally cowboy psuedocoding, but you should basically get it)

function text_plain_field_formatter_alter(&$plugin_definitions) {
  if (!empty($plugin_definitions['text_plain']['class'])) {
    $plugin_definitions['text_plain']['class'] = 'Drupal\\text_plain\\TextPlainFieldFormatter';
  }
}

Where Drupal\\text_plain\\TextPlainFieldFormatter extends and fixes Drupal\\text\\Plugin\\Field\\Formatter\\TextPlainFieldFormatter.

If, on the other hand text_plain.module is providing a new plugin, then it should name it uniquely. The contrib author in charge of this plugin would be wise to probably prefix it with his module name (as you're suggesting), but they are not compelled to so long as they have no conflicts within this plugin type for their currently installed set of modules. Which all comes back to the fact that I agree with you, so long as we are enforcing this through manual means, and not automagic functionality. That is really my only caveat I have in this issue. You want node.node? great do it, that's going to cause some weird issues in css classes and the like, but if that's what you want, fine. Core benefits from being core and being able to squat namespaces and have the potential for less weirdness by not having to prefix various things with the providing module, and we are more and more moving away from a completely module centric approach anyway, so we might be best advised to find other methods, but again, I am not opposed to this standard, I'm just opposed to code that enforces this standard.

Eclipse

I'm just opposed to code that enforces this standard.

The part of this I agree with is that core's discovery implementations should not change the 'id' value set by the plugin. The plugin should be responsible for setting it to match the conventions we want. However, I wouldn't be opposed to plugins leaving 'id' blank, and setting some other definition key, like 'local_id', and letting the discovery implementation construct a default 'id' by combining the module and the 'local_id'.

One other consideration about using "." as a delimiter though: how will that affect our ability for plugin ids to be used as keys within CMI files? I don't know if we do this anywhere yet, or if it's ever a good idea to do so, but would we be shooting ourselves in the foot by closing that possibility off or making it require some cumbersome escaping?

It could conceivably break the new config entity query, for sure. That's another reason to have views__view, taxonomy__term. Still no prettier.

this issue was brought up in #1938062: Convert the recent_comments block to a view as something to address a problem noticed there (but not part of that issue).

Added to #2120003: [META] Create sensible limits for the maximum length of configuration object filename components. Raised to Critical and beta bocker status.

This issue would make the config_prefix property of configuration entities redundant -- or, at least, a concept that belongs to the parent entity class, rather than being special for config entities. I think this issue should not only rename a bunch of entity type names in core, but actually enforce that there are two separate concepts, the module and the entity-type-identifier-thing-that-is-config-prefix-for-config-entities ("taxonomy" and "term", respectively). The entity ID would then always be automatically composed of these two parts.

@berdir pointed out that #33 this would result in node.node, like we already have for block.block. Options:

1. Just be rigid about it, and deal with the redundancy.
2. Just enforce that the first part before any dot is the module name, and make the second part after the dot optional. In other words, the second part is needed when the entity type ID isn't simply identical to the module name. This has the advantage of not being redundant, but as @berdir pointed out, the disadvantage of having an inconsistent number of parts in config object names, e.g., field.instance.taxonomy.term.tags.field_something.yml and field.instance.node.article.field_something.yml. On the other hand, we should be using API to compose and decompose the names, always, so at first glance, I don't think that it would actually be a problem.

Let's try changing taxonomy_term to taxonomy.term and see what breaks as a first step (suggested during a CMI call discussion).

Hum, dots are the separator char used between "parts" of cmi filenames and ids (e.g entity_type.bundle). It seems a really bad idea if those parts can contain dot-separated subparts themselves - it becomes difficult to know how to parse a given aaa.bbb.ccc string... Or we'd then need to add parenthesis to convey the intended precedencies ?

Each level needs its own separator - typically in current core:
. and _ in cmi filenames,
__ and _ in field storage db table names,
-- and - in template names...

@yched: Yeah I'm with you. I did raise concerns about this too and after trying it out, am now -1 on this.

I suggested to try it out to know what kind of problems we would encounter.

Here's a patch that renames everything that I could find. Here are the things that I noticed to be seriously broken:

* Route placeholders aren't replaced, instead, you get something like this: taxonomy/term/%7Btaxonomy.term%7D?taxonomy.term=2
* Manually accessing also don't work, looks like upcasting is broken too.
* _entity_access is broken as it does explode('.', 'taxonomy.term.update'). (by bypassing that, you can see that upcasting actually accepts ?taxonomy.term=2 as a valid argument.
** _entity_create_access works because we're use : and not . (I did that, don't remember why)
* However, when you bypass entity access, this happens: "RuntimeException: Controller "Drupal\taxonomy\Controller\TaxonomyController::termPage()" requires that you provide a value for the "$taxonomy_term" argument (because there is no default value or because there is a non optional argument after this one).". => Route arguments need to match variable names and we can't have $taxonomy.term
* _entity_form is also broken.
* Creating fields does not work: "CREATE TABLE {taxonomy.term__field_test}"

View modes and templates do seem to be working though, when hacking around all those routing issues.

Based on that, my suggestion for this issue is won't fix or postpone to 9.x and live with the current situation. And as mentioned before, allowing 50+32 entity_types makes our config files longer, not shorter nor easier. I'd rather increase the limit of entity_type name length to 50 as well, so that extensions with 32+ characters can have a correctly namespaced entity_type. But seriously, \Drupal::entityManager()->getDefinition('i_am_sorry_blame_my_developer_for_so_much_typing')? ;)

@yched, the idea was that the entity type actually would become, canonically, two "parts" instead of one.

#37 is enlightening though. If that's the amount of work to create an utterly broken patch for only one of core's entity types, we need to seriously ask ourselves what the tradeoffs are, and as @Berdir points out, probably live with the inconsistency/redundancy/risk of namespace collisions/silliness of config_prefix/etc. until D9. (The BC break would be too major to introduce in a minor version IMHO.) After all, most of the same problems exist in Drupal 7 and they're typically edgecases.

So, I think we should:

• Downgrade this to major task and postpone to 9.x.
• Throw an exception and/or otherwise break installation for a module that does include a dot in the entity type ID, given all the assumptions @Berdir found in #37. (Not beta-blocking.)
• Proceed with #1709960: declare a maximum length for entity and bundle machine names (splitting bundles off into a separate issue) and reopen #2220749: Limit $extension.$(config_prefix/entity_id) combo value to 83 characters (82 plus a '.') as part of a 250 character limit on config entity file names.
• Fix the naming and documentation around getConfigPrefix and config_prefix.

But seriously, \Drupal::entityManager()->getDefinition('i_am_sorry_blame_my_developer_for_so_much_typing')?

LOL.

the idea was that the entity type actually would become, canonically, two "parts" instead of one

Would mean :

-explaining "field instance config filename is field.instance.[entity_type_id].[bundle].[field_name].yml"
becomes "... is field.instance.[entity_type_provider].[entity_type_id].[bundle].[field_name].yml"
ew :)

- parameters explosion:
All methods that receive an $entity_type_id param now receive [$entity_type_provider + $entity_type_id] (used to be just $entity_type in D7, net DX loss)
Similarly, identifying a bundle requires [$entity_type_provider + $entity_type_id + $bundle_name] instead of just the last two currently.
Similarly, identifying a field instance requires [$entity_type_provider + $entity_type_id + $bundle_name + $field_name] instead of just the last three currently.
ew as well :)

So yeah, happy that' we're not doing it :-)

Discussed with @alexpott, @catch, @berdir, and @jessebeach. Based on the scope of this and the cost/benefit for everything that would be required to solve everything in #37, we are postponing this issue to Drupal 9.

Looking at #40, I think this should be won't fix rather than 9.x. If there are strong reasons to do it still, it needs an issue summary update.