Early Bird Registration for DrupalCon Portland 2024 is open! Register by 23:59 PST on 31 March 2024, to get $100 off your ticket.
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
Comment | File | Size | Author |
---|---|---|---|
#31 | derivative-plugin-separator-2035345-31.patch | 1.97 KB | AkshayKalose |
#16 | 2035345-16.patch | 2.13 KB | EclipseGc |
#9 | plugin-2035345-9.patch | 13.97 KB | tim.plunkett |
#4 | drupal_2035345_4.patch | 1.18 KB | Xano |
Comments
Comment #1
tim.plunkettMarked #2091327: Do not use a colon as a separator for derivative plugin IDs as a dupe
Comment #2
XanoI posted #2091327: Do not use a colon as a separator for derivative plugin IDs before finding this issue. My problem:
Comment #3
XanoComment #4
XanoLet's see how much this will blow up.
Comment #6
XanoThe 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.
Comment #7
tim.plunkettThe 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.
Comment #8
Xanowhat usages are there? Shouldn't they all in one way or another be routed through the encodePluginId() and decodePluginId() methods of the deriver?
Comment #9
tim.plunkettWe 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'
Comment #10
tim.plunkettAfter 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.
Comment #11
neclimdulCould 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.
Comment #14
XanoChx 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.
Comment #15
EclipseGc CreditAttribution: EclipseGc commentedI'm still in favor of a plugin manager dictating what separator it will use. I always have been.
Eclipse
Comment #16
EclipseGc CreditAttribution: EclipseGc commentedMaybe something along these lines? I'm totally spitballing.
Eclipse
Comment #17
dawehnerThe problem is that the plugin base would also use this parameter to provide some helper methods
Comment #18
XanoDoes this overlap with #1874498: Provide and use API methods for retrieving base plugin and derivative names from block plugin IDs?
Comment #19
tim.plunkettIt's the opposite of that, so yes it overlaps some, but should not be held up on it.
Comment #20
XanoSo if we shift the responsibility for encoding and decoding plugin IDs, is this issue still relevant?
Comment #21
tim.plunkettOh 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.
Comment #22
XanoWhat 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.
Comment #23
EclipseGc CreditAttribution: EclipseGc commentedright, 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
Comment #24
XanoAny more ideas on this? It would be great to fully allow plugin IDs as entity bundles.
Comment #25
ianthomas_ukThe 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.
Comment #26
XanoI 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.
Comment #27
dawehnerWell, sadly this adds back the problem of namespaces. I can easily imagine to have mulitple implementation of derivative plugins, one for each entity.
Comment #28
XanoCan you illustrate how that may cause problems with the solution I proposed in #26?
Comment #29
dawehnerI 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.
Comment #30
jhedstromPatch will need a reroll if this is still being pursued.
Comment #31
AkshayKalose CreditAttribution: AkshayKalose commentedRerolled #16
Comment #32
AkshayKalose CreditAttribution: AkshayKalose commentedComment #37
Xano\Drupal\Component\Plugin\PluginBase::DERIVATIVE_SEPARATOR
also already specified separators. We shouldn't specify them in two different places.Comment #38
XanoTwo problems with
\Drupal\Component\Plugin\PluginBase::DERIVATIVE_SEPARATOR
:Comment #39
XanoIt 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.Comment #51
Wim LeersThis 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.