In #2029509: Add a generic entity argument validation plugin. it became clear that dealing with derivative plugin ids causes all kind of problems, as a ':' is problematic when it comes to FAPI/JS

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

tim.plunkett’s picture

Xano’s picture

Issue tags: -Plugin system

I posted #2091327: Do not use a colon as a separator for derivative plugin IDs before finding this issue. My problem:

I have a plugin type Foo that has a derivative for every instance of field type Bar. The derivative IDs consist of the base plugin ID, a colon, and the field instance ID.
For every derivative, entity type Baz of which the bundle == Foo plugin ID gets a field instance of field type Qux. As Qux is attached to an entity type with a bundle name that contains a colon (the derivative plugin ID), Qux's primary identifier will also contain that colon.
When saving Qux, which is a field instance config entity, its identifier (which contains the colon), will be used as the name of the file by ConfigStorageController, which causes an exception in \Drupal\Core\Config\Config::validateName().

Xano’s picture

Issue tags: +Plugin system
Xano’s picture

Status: Active » Needs review
FileSize
1.18 KB

Let's see how much this will blow up.

Status: Needs review » Needs work

The last submitted patch, drupal_2035345_4.patch, failed testing.

Xano’s picture

The reason I tried the space was that most file systems support it and it is not allowed in machine names. I did not check if any code in core actually supports it.

A more realistic solution @dawehner and I discussed on IRC was using two underscores or two dashes. The characters themselves are supported by file systems and browsers for use in HTML and CSS. They are also allowed as part of machine names, but we can prohibit the occurrence of multiple of these characters.

tim.plunkett’s picture

The code will likely support it just fine. But you have to actually make the change in the usages, not just the code that checks it.

Xano’s picture

what usages are there? Shouldn't they all in one way or another be routed through the encodePluginId() and decodePluginId() methods of the deriver?

tim.plunkett’s picture

Status: Needs work » Needs review
FileSize
13.97 KB

We currently don't use those methods anywhere but within that class because they are protected, non-static, and buried on a *decorator*.

In order to make this truly flexible, we have to use this method *everywhere*

This is only a small chunk of the changes needed to do that.
And it doesn't even attempt to change the separator yet.

The reason it doesn't install is files like core/profiles/minimal/config/block.block.stark.admin.yml:
plugin: 'system_menu_block:admin'

tim.plunkett’s picture

After further discussion, I think we should go the opposite route and make encode/decode private, and say "the separator is not swappable".

That is partially unrelated to the task of actually changing the separator.

neclimdul’s picture

Could we update the summary with what "it became clear that dealing with derivative plugin ids causes all kind of problems" means? Examples would be good so its clear we're fixing the right problems without having to digest all the patches on the other issue.

Issue tags: +Plugin system

The last submitted patch, plugin-2035345-9.patch, failed testing.

Status: Needs review » Needs work
Issue tags: -Plugin system

The last submitted patch, plugin-2035345-9.patch, failed testing.

Xano’s picture

Chx just suggested using a short, but very unlikely string, which is similar to using __, but as two underscores are likely to be used due to the machine_name element's transliteration is a lot less likely to cause trouble.

EclipseGc’s picture

I'm still in favor of a plugin manager dictating what separator it will use. I always have been.

Eclipse

EclipseGc’s picture

Status: Needs work » Needs review
FileSize
2.13 KB

Maybe something along these lines? I'm totally spitballing.

Eclipse

dawehner’s picture

The problem is that the plugin base would also use this parameter to provide some helper methods

Xano’s picture

tim.plunkett’s picture

It's the opposite of that, so yes it overlaps some, but should not be held up on it.

Xano’s picture

So if we shift the responsibility for encoding and decoding plugin IDs, is this issue still relevant?

tim.plunkett’s picture

Oh hm, I completely forgot what we were doing in that issue.

We could still press forward with this issue, and do the same thing to PluginBase (splitting the out separator to a property).
Someone wanting to change it for their plugin type would need to override it in their base class.

Xano’s picture

What if we move this to Drupal\Component\Plugin\Derivative\DerivativeInterface? The actual code for generating derivatives would then also be responsible for identifying them.

// Edit: this is not as simple as I thought last night. For the system to know which derivative 'handler' to talk to, it first needs the base plugin ID.

EclipseGc’s picture

right, I really did attempt to do all this on the first pass of plugins. This was, in fact, one of the things that slowed us down from getting it committed earlier. It's not an easy problem.

Eclipse

Xano’s picture

Any more ideas on this? It would be great to fully allow plugin IDs as entity bundles.

ianthomas_uk’s picture

The patch on #16 makes this character configurable rather than changing it, but I don't think that good idea? We've found that a colon is too-special a character for some places that these IDs are getting used, if we make it configurable then theoretically we should support any character. Why would people want to change that character on a per-plugin basis?

Instead we should choose a character that is not special in javascript or yaml, but cannot be used in machine names, or a fairly unusual string if no such character exists.

Xano’s picture

I told @dawehner the other day that as a solution we can make managers store a map of all plugin IDs and their base plugin IDs when getting definitions, so when the manager needs to get the base ID for an arbitrary plugin ID, it can just use the map rather than trying to decompose the ID it was given. This eliminates the need for a separator.

dawehner’s picture

I told @dawehner the other day that as a solution we can make managers store a map of all plugin IDs and their base plugin IDs when getting definitions, so when the manager needs to get the base ID for an arbitrary plugin ID, it can just use the map rather than trying to decompose the ID it was given. This eliminates the need for a separator.

Well, sadly this adds back the problem of namespaces. I can easily imagine to have mulitple implementation of derivative plugins, one for each entity.

Xano’s picture

Can you illustrate how that may cause problems with the solution I proposed in #26?

dawehner’s picture

Issue summary: View changes

I guess that I feared that two plugins might have the same base plugin ID, but seriously this is BS because it would be broken already.

jhedstrom’s picture

Status: Needs review » Needs work
Issue tags: +Needs reroll

Patch will need a reroll if this is still being pursued.

AkshayKalose’s picture

Rerolled #16

AkshayKalose’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 31: derivative-plugin-separator-2035345-31.patch, failed testing.

Status: Needs work » Needs review

Xano’s picture

Status: Needs review » Needs work
+++ b/core/lib/Drupal/Component/Plugin/Discovery/DerivativeDiscoveryDecorator.php
@@ -35,14 +35,25 @@ class DerivativeDiscoveryDecorator implements DiscoveryInterface {
+  protected $separator;

\Drupal\Component\Plugin\PluginBase::DERIVATIVE_SEPARATOR also already specified separators. We shouldn't specify them in two different places.

Xano’s picture

Two problems with \Drupal\Component\Plugin\PluginBase::DERIVATIVE_SEPARATOR:

  1. It's a constant and as such cannot be overriden by child classes.
  2. It's not on any interface, so code from outside the class cannot rely on it.
Xano’s picture

It seems we can't centralize this, as both plugins themselves (through \Drupal\Component\Plugin\DerivativeInspectionInterface) and derivative discovery methods (such as \Drupal\Component\Plugin\Discovery\DerivativeDiscoveryDecorator) need to be able to split up and build plugin IDs using a separator. Plugins do not know about their discovery, and the discovery needs to perform these tasks before plugin definitions/classes are known.

Version: 8.0.x-dev » 8.1.x-dev

Drupal 8.0.6 was released on April 6 and is the final bugfix release for the Drupal 8.0.x series. Drupal 8.0.x will not receive any further development aside from security fixes. Drupal 8.1.0-rc1 is now available and sites should prepare to update to 8.1.0.

Bug reports should be targeted against the 8.1.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.2.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.1.x-dev » 8.2.x-dev

Drupal 8.1.9 was released on September 7 and is the final bugfix release for the Drupal 8.1.x series. Drupal 8.1.x will not receive any further development aside from security fixes. Drupal 8.2.0-rc1 is now available and sites should prepare to upgrade to 8.2.0.

Bug reports should be targeted against the 8.2.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.3.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.2.x-dev » 8.3.x-dev

Drupal 8.2.6 was released on February 1, 2017 and is the final full bugfix release for the Drupal 8.2.x series. Drupal 8.2.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.3.0 on April 5, 2017. (Drupal 8.3.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.3.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.3.x-dev » 8.4.x-dev

Drupal 8.3.6 was released on August 2, 2017 and is the final full bugfix release for the Drupal 8.3.x series. Drupal 8.3.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.4.0 on October 4, 2017. (Drupal 8.4.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.4.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.5.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.4.x-dev » 8.5.x-dev

Drupal 8.4.4 was released on January 3, 2018 and is the final full bugfix release for the Drupal 8.4.x series. Drupal 8.4.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.5.0 on March 7, 2018. (Drupal 8.5.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.5.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.6.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.5.x-dev » 8.6.x-dev

Drupal 8.5.6 was released on August 1, 2018 and is the final bugfix release for the Drupal 8.5.x series. Drupal 8.5.x will not receive any further development aside from security fixes. Sites should prepare to update to 8.6.0 on September 5, 2018. (Drupal 8.6.0-rc1 is available for testing.)

Bug reports should be targeted against the 8.6.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.7.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.6.x-dev » 8.8.x-dev

Drupal 8.6.x will not receive any further development aside from security fixes. Bug reports should be targeted against the 8.8.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.9.x-dev branch. For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

Version: 8.8.x-dev » 8.9.x-dev

Drupal 8.8.7 was released on June 3, 2020 and is the final full bugfix release for the Drupal 8.8.x series. Drupal 8.8.x will not receive any further development aside from security fixes. Sites should prepare to update to Drupal 8.9.0 or Drupal 9.0.0 for ongoing support.

Bug reports should be targeted against the 8.9.x-dev branch from now on, and new development or disruptive changes should be targeted against the 9.1.x-dev branch. For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

Version: 8.9.x-dev » 9.2.x-dev

Drupal 8 is end-of-life as of November 17, 2021. There will not be further changes made to Drupal 8. Bugfixes are now made to the 9.3.x and higher branches only. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.2.x-dev » 9.3.x-dev

Version: 9.3.x-dev » 9.4.x-dev

Drupal 9.3.15 was released on June 1st, 2022 and is the final full bugfix release for the Drupal 9.3.x series. Drupal 9.3.x will not receive any further development aside from security fixes. Drupal 9 bug reports should be targeted for the 9.4.x-dev branch from now on, and new development or disruptive changes should be targeted for the 9.5.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Wim Leers’s picture

This came up again at #3313473-33: CKEditor 5 plugin definitions should be derivable.

Note this prevents creating derived-plugin-specific config schemas, because : is a reserved character in YAML.

Version: 9.4.x-dev » 9.5.x-dev

Drupal 9.4.9 was released on December 7, 2022 and is the final full bugfix release for the Drupal 9.4.x series. Drupal 9.4.x will not receive any further development aside from security fixes. Drupal 9 bug reports should be targeted for the 9.5.x-dev branch from now on, and new development or disruptive changes should be targeted for the 10.1.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.5.x-dev » 11.x-dev

Drupal core is moving towards using a “main” branch. As an interim step, a new 11.x branch has been opened, as Drupal.org infrastructure cannot currently fully support a branch named main. New developments and disruptive changes should now be targeted for the 11.x branch. For more information, see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.