- Issue created by @mxr576
- ๐ฌ๐งUnited Kingdom joachim
This suffers from the same problem as the proposal to use class names to identify services: it breaks the idea that the class is separate from the concept.
Plugin classes are swappable - you can implement the alter info hook for any plugin type, and change the class that a plugin uses. If we use classnames, then it gets messy when you do that.
Furthermore, currently plugin classes are NOT part of public API in our BC policy. Only the plugin ID and the plugin type's interface are API. If the class name is the identifier, then the class name becomes part of the public API.
- ๐ฆ๐บAustralia larowlan ๐ฆ๐บ๐.au GMT+10
Also we're nearly finished moving to attributes instead of annotations so from 10.3 I suspect if you want to do this with custom code, you can if you use attributes
- ๐ญ๐บHungary mxr576 Hungary
@joachim, Thanks for your feedback; you raised some valid points.
First and foremost, the FQCN could also refer to an interface, not just a class... I think that would address/justify the need I raised and the concerns you raised about "the class is separate from the concept," which I also agree with as a generic rule of thumb.
Plugin classes are swappable.
I think swappability is definitely one of the strengths of Drupal, but sometimes I also consider it a weakness; since everything becomes swappable when you use Drupal APIs (like the Plugin API), important domain logic and requirements could get lost, and preventing domain invariants becomes impossible.
Plugin classes are swappable.... if we use classnames, then it gets messy when you do that.
Furthermore, currently plugin classes are NOT part of the public API in our BC policy. Only the plugin ID and the plugin type's interface are API.I understand your concerns, and I also do not consider the plugin class (as a standalone code unit) part of the public API. However, there won't be a chicken and egg problem, because a plugin id only exists if there is a class that defines it in annotation. So, as a middle ground solution, I could accept that the implementation in the plugin class DOES NOT become part of the public API still, but the class name does.
Also, in reality, there are potential cases when you would like to replace a plugin implementation - like using the service decorator pattern for (tagged) services - but for hotfixing problems, patching is still a viable and potentially more frequently used approach by developers.
- ๐ญ๐บHungary mxr576 Hungary
Also, we're nearly finished moving to attributes instead of annotations, so from 10.3, I suspect if you want to do this with custom code, you can if you use attributes.
@larowlan, yes, this is also on my list: checking whether attributes will address this problem - and if they do, for consistency reasons, as long as annotations are supported, it could be a good idea to keep feature parity.
- ๐ญ๐บHungary mxr576 Hungary
It just came to my mind that Plugin derivatรญves provides an interesting aspect, but they are still not a blocker. The ticket is about supporting not making this a de-facto, only practice.
- ๐ฆ๐บAustralia mstrelan
Marking โจ Use class names instead of plugin IDs Active as a dupe of this, although it has a POC with one plugin.
- ๐ฎ๐ฉIndonesia el7cosmos ๐ฎ๐ฉ GMT+7
I propose a different approach, instead of using the class name as plugin ID (which will be a problem for derivative definitions), we can introduce plugin alias, just like service container can have service alias.
For example, container element can have definition something like this
$definitions = [ 'container' => [ 'id' => 'container' 'class' => 'Drupal\Core\Render\Element\Container' 'provider' => 'core' ], 'Drupal\Core\Render\Element\Container' => [ 'alias' => 'container', ], ];
And for definition with
\Drupal\Component\Plugin\Definition\PluginDefinition
instance, we can introduceAliasPluginDefinition
and have something like this:$definitions = [ // instance of \Drupal\Core\Entity\ContentEntityType 'node' => '',, // instance of \Drupal\Component\Plugin\Definition\AliasPluginDefinition that have `alias` property with the value `node` 'Drupal\node\Entity\Node' => '', ];
Derivative definitions won't have an alias for them, but can still be accessed with their ID.
A plugin discovery should be responsible for building the alias definitions and returning a correct definition if an alias plugin is requested.
The drawback is the plugin definitions will grow in size, but shouldn't be too much. If this is a problem, there should be a way for plugin manager to opt out of this behaviour (eg. Block plugin manager can decide to not have an alias for all their definitions).
In the end, we can get a plugin definition and create its instance either by ID or its alias if it has one.
- ๐จ๐ฆCanada Charlie ChX Negyesi ๐Canada
Here's how I imagine this could work:
- The FQCN is just a string. It happens to be the FQCN of the original base class but it's just a random string serving as the plugin id. "Drupal\text\Plugin\Field\FieldWidget\TextareaWidget" carries no more meaning than "text_textarea". The default is what's expected but it's a default, not a requirement. Almost always the plugin will be in the TextareaWidget class but this is not required.
- The class name becomes the default for the plugin id which becomes optional.
- When asking the plugin manager for an instance of a plugin with an ID that looks like an FQCN -- I guess it starts with Drupal\ -- it could attempt to read the attributes of this class and run alter as if this plugin was the only implementation then cache the results. If it fails then it could fall back to the normal full "find all definitions" routine.
- If a module decides to change the class of a plugin, it's not a problem, just put the old class name into the plugin id.
- When asking for an instance of an FQCN the results might be a different class either because the plugin changed classes since the id was created or because of alter. This shouldn't matter because they all implement the same plugin interface. If the specific plugin has additional functionality -- is this a thing? -- it should put that also in an interface which the overriding class can implement.
- Derivatives are still colon + random string. So you have
Drupal\foo\Plugin\whatever\Bar:derivativeid
as the full name for the derivative. Almost always the actual code will live in the Bar class. - For UI purposes it's still necessary to scan for all plugins of a type but if after a cache rebuild the full cache for a plugin type doesn't exist yet , it might not need to exist indeed.
- ๐ฌ๐งUnited Kingdom joachim
If we do this, then because of this:
> If the class name is the identifier, then the class name becomes part of the public API.
-- we need to have a system for deprecating and renaming plugins. We don't currently -- ๐ Create a way to declare a plugin as deprecated Needs work , and it's a pain because code is tied to a plugin ID. If we do this, when we lock the class name AS WELL and that's making a bad situation worse.
Also, the issue needs some further scoping -- is this just plugins that use class-based discovery, and not YAML plugins?
- ๐ฌ๐งUnited Kingdom catch
In slack one of the main motivations here was to avoid having to parse all classes in order to discover plugins, when in many cases we only need the class for one plugin (or a handful of plugins) that is configured for the thing being rendered. Think blocks or fields etc. where there can be hundreds on a site but a few on a page. I can see how using the class or interface, + an alter tied to that class interface, would help here because it should be possible to reliably get information about the one thing without needing information about all the things.
Except, I don't see how the above could work for derivatives, which is often just as big or usually much worse a problem for memory usage as annotation/attribute parsing. If a plugin is requested, and it can't be located via the class name, then don't we end up having to get all derivatives anyway?
- ๐ฌ๐งUnited Kingdom joachim
On Slack, one of the reasons given was 'moving away from magic strings'.
But FQCNs are also magic strings! They are not guaranteed to be universally unique. They are unique *within a codebase* because of PHP. And they are sort-of-guaranteed unique *in d.org hosted code* because we enforce unique machine names for projects AND we have the convention of using the project name as the module name.
But:
- some projects on d.org are called foo_bar and contain a module foobar
- many projects have submodules, and in those, the FQCNs have no connection to the unique d.org project name.Remember the mess we have when there was a file_entity d.org project, and also a file_entity module inside the file d.org project? That sort of thing could happen again and give us repeated FQCNs.
If we want plugin IDs to be unique, we could have the plugin manager class add the provider module name as a prefix to all plugins. That doesn't guarantee universal uniqueness, but two modules with the same name won't get as far as getting installed at least.
- ๐ญ๐บHungary mxr576 Hungary
I really loved to see the fruitful conversation that had happened on this issue and on Slack threads! :)
If we want plugin IDs to be unique, we could have the plugin manager class add the provider module name as a prefix to all plugins.
Am I missing something or that would not solve the "file_entity" in core and contrib problem either?
I would rather say that uniqueness is hard to guarantee, independently from whether we have a random string or a FQCN is in use.
I also liked the aliasing idea from #14... maybe we do not need to kill the random ids completely, just let ppl to use FQCN-s also.
- ๐ฎ๐ฉIndonesia el7cosmos ๐ฎ๐ฉ GMT+7
I make an attempt for aliases in ๐ฌ Allow plugin definition to be aliased Active
That at least works for something like this:
\Drupal::entityTypeManager()->getStorage(\Drupal\node\Entity\Node::class); \Drupal::entityTypeManager()->getDefinition(\Drupal\node\Entity\Node::class);
- ๐ฌ๐งUnited Kingdom longwave UK
I still wonder why we can't do this for entity storage:
Drupal\node\NodeStorageInterface: factory: ['@entity_type.manager', 'getStorage'] arguments: [node]
and then just inject it directly as a service?