Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.Right now, core condition implementations miss the metadata that's required for Rules to be able to make use of the conditions also. Rules requires metadata about the configuration values what right now should live in the condition plugin definition below the 'configuration' key:
From ExecutableInterface
* - configuration: An array of configuration option definitions, keyed by
* option name. Each option definition is a typed data definition describing
* the configuration option. Check the typed data definition docs for details.Let's make sure that's implemented in all core conditions such that Rules can build upon them.
Related issue: #1920816: Factor getConfig(), setConfig(), getConfigDefinition(), and getConfigDefinitions() out of ExecutablePluginBase
Next steps
See if patch can be rerolled for current d8 head. https://drupal.org/contributor-tasks/reroll
| Comment | File | Size | Author |
|---|---|---|---|
| #12 | d8_conditions_metadata_12.patch | 4.42 KB | Sam Hermans |
Comments
Comment #1
fagoI forgot to explain why the initial conditions patch went in without providing metadata: At this point we had no general TypedData list class, so providing metadata for the NodeType configuration let it fail validation. Meanwhile we've that general TypedData list class, so there is no reason to not add the metadata any more.
ok, here is a patch which adds in the missing metadata for all conditions existing right now. Unfortunately,
the language condition was relying on a FAPI data structure as returned from the FAPI array, for which it would be (unnecessary) hard to provide the metadata. Let's better don't design around FAPI structures, but make FAPI follow our API. Patch attached.
Comment #2
benjifisher#1: d8_conditions_metadata.patch queued for re-testing.
Comment #3
dcam CreditAttribution: dcam commentedhttp://drupal.org/node/1427826 contains instructions for updating the issue summary with the summary template.
Comment #5
claar CreditAttribution: claar commentedRerolled to cleanly apply against latest 8.x
Comment #7
fago#5: d8_conditions_metadata_2.patch queued for re-testing.
Comment #8
Berdir#5: d8_conditions_metadata_2.patch queued for re-testing.
Comment #9.0
(not verified) CreditAttribution: commentedimproved language
Comment #10
YesCT CreditAttribution: YesCT commentedComment #11
Sam Hermans CreditAttribution: Sam Hermans commentedI'll check this out
Comment #12
Sam Hermans CreditAttribution: Sam Hermans commentedRe-rolled to current d8 head .. the patch changes the Language plugin array from key value to list and updates the test.
Tested locally and fails with the following message:
Argument 1 passed to Drupal\Core\TypedData\TypedDataManager::create() must be an instance of Drupal\Core\TypedData\DataDefinitionInterface, array given, called in /core/lib/Drupal/Core/Executable/ExecutablePluginBase.php on line 98 and definedSetting status to 'needs review' so we can get the results from the bot.
Comment #14
fagoComment #15
tim.plunkettDo we have any way to go from this to something like
For config schema?
Also we need to do this (or something like it) for all of configurable plugins.
Comment #16
fagoNot right now, but true - config schema is really related and we could come up with code to generate it from the annotation. But the config schema has to be written to a file, right? :/
Comment #17
tim.plunkettComment #18
fagoComment #19
fagoComment #20
yched CreditAttribution: yched commentedThe problem is that localize.d.o forbids an approach where you have to execute code in order to generate config schemas. See @gabor for more details.
Comment #21
fagoSo for #d8rules we currently do not separate between context and configuration (which is just context which has by default a form input), but use context for everything. We considered going with two keys for defining context + configuration, but it turned out this just makes things unnecessarily more complex when it's required to determine a ordering across all parameters (context+configuration).
Still an open question is how exactly we'll handle the UI challenges of generating a form that has context mapping and makes use of the provided configuration form (or not).
Comment #22
XanoIs this in any way related to the configuration plugins get through the factory or
ConfigurablePluginInterface?Comment #23
XanoBump?
Comment #35
smustgrave CreditAttribution: smustgrave at Mobomo commentedThe code in question seems to have changes from 8 years ago
Can anyone confirm if they are still having this issue?
Comment #37
smustgrave CreditAttribution: smustgrave at Mobomo commentedSince there hasn't been a follow up in almost a year going to close out for now
If still a bug please reopen
Thanks everyone!