Support FQCN as plugin id

Created on 11 April 2024, 10 months ago
Updated 27 August 2024, 5 months ago

Problem/Motivation

The plugin id in Drupal is currently a magic string that needs to be unique. In many cases, using the fully qualified class name (FQCN) of the plugin class could provide a more reasonable choice for ensuring uniqueness and eliminating magic.

Example:

<?php
// in FooConstraint.php

/**
 * @Constraint(
 *   id = \Drupal\foo\Plugin\Validation\Constraint\FooConstraint.php::class,
 *   label = @Translation("Validates foo", context = "Validation"),
 *   type = "string"
 * )
 */
class FooConstraint extends Constraint {}

// in FooDataTypeDefinition.php

    $properties['foo'] = DataDefinition::create('string')
      ->setLabel(new TranslatableMarkup('Foo'))
      ->setRequired(TRUE)
      ->addConstraint(FooConstraint::class);

Considerations:

  • Some developers have resorted to adding "PLUGIN_ID" constants to plugin classes to reduce magic, but this does not guarantee uniqueness. In fact, this approach has led to the consideration of using FQCN as the plugin id.

Blockers

Steps to reproduce

Proposed resolution

Remaining tasks

User interface changes

API changes

Data model changes

Release notes snippet

โœจ Feature request
Status

Active

Version

11.0 ๐Ÿ”ฅ

Component
Pluginย  โ†’

Last updated 1 day ago

Created by

๐Ÿ‡ญ๐Ÿ‡บHungary mxr576 Hungary

Live updates comments and jobs are added and updated live.
  • Needs issue summary update

    Issue summaries save everyone time if they are kept up-to-date. See Update issue summary task instructions.

Sign in to follow issues

Comments & Activities

  • Issue created by @mxr576
  • ๐Ÿ‡ญ๐Ÿ‡บHungary mxr576 Hungary
  • ๐Ÿ‡ญ๐Ÿ‡บHungary mxr576 Hungary
  • ๐Ÿ‡ญ๐Ÿ‡บHungary mxr576 Hungary
  • ๐Ÿ‡ญ๐Ÿ‡บHungary mxr576 Hungary
  • ๐Ÿ‡ญ๐Ÿ‡บHungary mxr576 Hungary
  • ๐Ÿ‡ฌ๐Ÿ‡ง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.

  • ๐Ÿ‡ฆ๐Ÿ‡บAustralia mstrelan

    Re: attributes solving this. This helps in that we that we can pass the class constant as the plugin id, but ideally we could drop the id property and just use getClass, or make them one and the same. See MR !8693 for a POC.

  • ๐Ÿ‡ฎ๐Ÿ‡ฉ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 introduce AliasPluginDefinition 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:

    1. 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.
    2. The class name becomes the default for the plugin id which becomes optional.
    3. 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.
    4. 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.
    5. 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.
    6. 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.
    7. 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?

Production build 0.71.5 2024