Come together with the global Drupal community in Rotterdam, 28 Sept – 1 Oct 2026. Sessions, contribution, connection, and Early Bird savings until 8 June.Problem/Motivation
The new method added by #2392057: Config schema fails to expand dynamic top-level types has a small but nasty bug.
Instead of adding a definition for the computed type, it replaces the original definition. That means the second and following elements for the same wildcard/generic type use the definition for the first and fail.
Proposed resolution
Use the correct key to set it.
Part of the problem seems to be the confusing naming between $type and $base_plugin_id. $base_plugin_id isn't "base", it's the real, full, replaced type. We could switch the variables to make the code easier to understand.
Remaining tasks
User interface changes
API changes
Data model changes
| Comment | File | Size | Author |
|---|---|---|---|
| #14 | 2663410-8.patch | 4.7 KB | berdir |
| #8 | 2663410-8.patch | 4.7 KB | alexpott |
| #8 | 6-8-interdiff.txt | 3.48 KB | alexpott |
| #6 | 2663410-6.patch | 3.27 KB | alexpott |
| #6 | 5-6-interdiff.txt | 1.38 KB | alexpott |
Comments
Comment #2
berdirComment #4
alexpottHere's a test that (i think) shows we have a problem as describe by @Berdir.
Comment #5
alexpottThe patch attached just doesn't dynamically cache if we have a sub type
Comment #6
alexpottHmmm actually
$sub_typeis problematic we're actually still working out the type.Comment #8
alexpott@Berdir that the type was not correct - in order for it to be meaningful we need to combine both $type and $sub_type. I've used
||as the separator because it is not used in schema yet.The test in #4 failed as expected.
Comment #9
berdirCan confirm that the latest patch works for my page_manager page variant example. Since nothing is left of my initial patch I think I can RTBC this, although someone else confirming wouldn't hurt.
Comment #11
catchThis looks good to me as well. Committed/pushed to 8.1.x, thanks!
Comment #13
berdirShould we also add this to 8.0.x? The issue that this fixes also went into 8.0.x, so this is currently still broken there.
Comment #14
berdirReuploading the patch.
Comment #15
yanniboi commentedThis isn't quite finished, because the $sub_type isn't being built fully...
Comment #16
yanniboi commentedDoes this make sense?
Comment #17
yanniboi commentedComment #19
yanniboi commentedDam I uploaded the last patch against 8.0.x not 8.1.x. Sorry....
Comment #20
berdirThis works fine for me, if you still see a problem then that might be better as a separate issue, with steps to reproduce.
Comment #24
yanniboi commentedsorry @Berdir I shouldn't have just jumped on this issue like that, I was half way through posting a separate issue when @timplunkett pointed me at this issue. I have created a follow up issue to explain what is not working for me:
From issue summary:
With patch from #8 the second definition is no longer replacing the first, however the second definition isn't fully built. The second definition does not inherit any mappings from its parent definitions.
The way I came across this was working with page_manager and ctools submodule ctools_views:
block.settings.views_block:* is of type parent definition type views_block which provides mappings for 'views_label' and 'items_per_page'. When adding a views block to a page_manager page, the resulting config object has a definition of ctools.block_plugin.*||block.settings.views_block:* and was inheriting all of the correct mappings from ctools.block_plugin.* (eg. 'region', 'weight', etc.) but none of the mappings for block.settings.views_block:*.
This is because when the definitions are merged, it currently only looks at block.settings.views_block:* and not its parents:
I have a patch to fix this here: #2693081: Merging data types leads to incomplete definition
Sorry again for disrupting this issue.
Comment #25
alexpott@yanniboi did you mean to re-close this issue in #24?
Comment #26
berdirNo, I reopened this in #13/14 to also get this into 8.0.x since we only committed this to 8.1.x without a good reason as far as I see (the referenced issue did get into 8.0.x..).
Soon no longer worth the trouble, not sure anymore at this point.
Comment #35
jhedstromDoes this still need to be open? Looks like it's been committed to 8.4 and 8.3.
Comment #36
berdirI guess we can close this, yes.
Comment #38
wim leersComment #39
wim leersThis introduced a regression: #3400181: Follow-up for #2663410: calling TypedConfigManager::getDefinition() causes cache pollution.