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.
Problem/Motivation
The eventual plan of #2183983: Find hidden configuration schema issues is to force all tests to check for config schema issues.
We need to be prepared for that change.
Proposed resolution
In the interim, mark tests for strict schema checking.
Remaining tasks
Fix any bugs that crop up.
User interface changes
API changes
Comment | File | Size | Author |
---|---|---|---|
#18 | 2391925-19.patch | 1.25 KB | Sam152 |
#17 | 2391925-18.patch | 768 bytes | benjy |
#14 | config_schema_change-2391925-14.patch | 4.87 KB | giancarlosotelo |
#8 | page_manager-2391925-diff-1-8.txt | 431 bytes | vijaycs85 |
#8 | page_manager-2391925-config-schema-test-8.patch | 1.87 KB | vijaycs85 |
Comments
Comment #1
tim.plunkettComment #3
tim.plunkettWe have the page_manager config entity, which wraps a display variant plugin, which has a sequence of blocks. These are like the core block plugins, but with additional keys.
So we have
When placing a block like system_menu_block, I can confirm that
page_manager.block_plugin.*
is expanded topage_manager.block_plugin.system_menu_block:navigation
.However, the
block.settings.[id]
which *contains*id
, is not expanded properly. It needs to matchblock.settings.system_menu_block:navigation
, but only matchesblock.settings.*
.Therefore it doesn't get the custom mappings.
Looking at other examples, it seems like [id] refers to the next level up from what I want. How do I get it to be
system_menu_block:navigation
?Comment #4
Gábor HojtsyCan you provide an example config snippet that has this page_manager.block_plugin.* in it?
Comment #5
tim.plunkettComment #6
tim.plunkettAnd here is the config schema:
The two parts that are invalid are the menu block's depth/level, and the views block's views_label/items_per_page, since those are the custom settings from block.settings.system_menu_block:* and block.settings.views_block:*
Comment #8
vijaycs85Very interesting, just removing this property removed fatal and made PageManagerAdminTest green locally.just noticed I got $strictConfigSchema = FALSE. Still investigating locally.Comment #9
Gábor HojtsyWithout running the code here is my hyphotesis:
1. Mapping::getPropertyDefinitions() finds
page_manager.block_plugin.[id]
, so it calls itsbuildDataDefinition
which is delegated to TypedDataManager::buildDataDefinition().2. In TypedDataManager::buildDataDefinition() if that definition has a dynamic component, like in this case
page_manager.block_plugin.[id]
, it is resolved to the value, such aspage_manager.block_plugin.system_menu_block:main
.3. Then still from TypedDataManager::buildDataDefinition() TypedDataManager::getDefinition() is called on that again to resolve to any wildcard matches if there was no direct match for this one (and there is no direct match for that one), so it resolves to
page_manager.block_plugin.*
I believe. We are still inside getDefinition() right after getFallbackName() has its chance.4. Then this code kicks in:
So now we have $type as
page_manager.block_plugin.*
which itself has a type ofblock.settings.[id]
, so and this code throws that value back into getDefinition(). That only allows for wildcard fallback, no value replacement, so it goes withblock.settings.*
here.There is not a chance that the id value is replaced again because once a value was replaced there is only getDefinition() called merging their results upwards. So this way
block.settings.*
goes one more level down toblock_settings
which goes tomapping
which does not have a type anymore, so that is bubbled up, adding up the following as the definition:- the definition of mapping
- the definition of
block_settings
- the definition of
block.settings.*
- the definition of
page_manager.block_plugin.*
And that's it. The merge of these upwards ends up being the definition of
page_manager.block_plugin.system_menu_block:main
.Comment #10
Gábor HojtsyTo resolve this I think we should fix this in core with tests and somehow let control back to buildDataDefinition() where we still have the data if the resolved type is still dynamic. Because getDefinition() is too late, that is not data dependent anymore. (And cannot be since this is how typed data defines the interfaces I believe). It seems like multi-level dynamic types are not really supported ATM. (Unless of course you put a concrete type inbetween like embed the further layer of dynamic types in a mapping which are the kind of things we encountered in core so far). But it does not seem like a stretch to be able to resolve this one in core.
Comment #11
tim.plunkettI opened #2392057: Config schema fails to expand dynamic top-level types
Comment #12
Gábor HojtsyQuoting a revelation I had this morning. Sorry I did not have it last night :/
I mean how can we be sure that no block plugin that page_manager would work with will have settings keys like 'region', 'weight' or 'uuid'? (Or for that matter any other key that you may want to introduce on this level in your config later). Imagine a block that connects to a third party service that returns news for your region. It may have a region setting to set the machine name of the geographic area to take the news for. Same for weight, etc. I don't think its possible to ensure an arbitrary dynamic array does not contain the keys you want to add on to it?
If I were you I would use a structure like block.block.* where the settings are boxed well into a settings child-key (edited snippet from block.schema.yml):
This allows you to add any keys that may exist in a random block's setting and also allows random blocks to add settings that may exist in plugin maanager. They don't clash.
Comment #13
Gábor HojtsySo instead of
You would have:
And the schema would be (just relevant snippets):
Comment #14
giancarlosotelo CreditAttribution: giancarlosotelo at MD Systems GmbH commentedI have been working in this issue, probably a lot must be changed.
I am providing an initial patch where I applied recommendations #12 and #13 with more necessary fields.
Functions are overridden because of the change in the schema (this is tested and works well in
PageConfigSchemTest
). For the next step some structures in the form have to change to achieve what we want to do here.Maybe is not the best way but a feedback is needed to continue this work.
Comment #16
benjy CreditAttribution: benjy at PreviousNext commentedI think #2392057: Config schema fails to expand dynamic top-level types will make the tests green here, it worked for an installation profile I was working on.
However, I ran into an additional schema problem to do with views_block, this seems to be an issue with how derivatives match schema in TypedConfigManager::getFallbackName().
EDIT: I think the root of the issue is how we tie the custom settings from views_block into the custom settings for page manager blocks?
Comment #17
benjy CreditAttribution: benjy at PreviousNext commentedJust leaving the patch here now for reference, will investigate core a little further.
Comment #18
Sam152 CreditAttribution: Sam152 commentedI have the same problem for system menu blocks.
Comment #19
tim.plunkettNote that we also have
So what we really need is a way for
page_manager.block_plugin.[whatever]
to auto becometype: block.settings.[whatever]
, and add this mapping.We're not going to be able to copy/paste this every time.
Comment #20
Gábor Hojtsy@tim.plunkett: So long as the value of [whatever] appears in the data, you can use dynamic config schema typing to refer to that piece of data and automate it.
Comment #21
benjy CreditAttribution: benjy at PreviousNext commentedI actually think we should break out the config schema derivative issues into a new issue and add a dedicated test for that. Then keep this issue for the config expansion here: #2392057: Config schema fails to expand dynamic top-level types ?
@Gabor, I briefly looked at this and the problem was that [whatever] looked like "type.some_thing:big_long_id" and it needs to get the definition of "type.some_thing:*" but it was actually trying to get the definition of simply "type.*"
Comment #22
tim.plunkettSo this all completely changed when we split variants into their own entities, and now #2392057: Config schema fails to expand dynamic top-level types is in. No changes were required to our schema to become completely valid, so closing.
Comment #23
BerdirNot so fast :)
Great that is in. Variants didn't really change anything though, we just moved it.
We still have strict config schema check disabled in the tests. Lets enable that to see if it really works.. there might be other problems too that were hidden so far.
Comment #24
tim.plunkettSorry, I meant to point out that I just removed that
http://cgit.drupalcode.org/page_manager/commit/?id=8fa43f7
Comment #25
BerdirOh, sorry. All fine then :)
Comment #26
Gábor HojtsyYay, finally :) I am glad config schema stood the test of this complex case (with a bugfix :). Thanks for your contribution! :)
Comment #27
BerdirPage manager definitely needs more test coverage. See #2663410: TypedConfigManager::_getDefinitionWithReplacements() incorrectly replaces generic definition, the follow-up that I created.
Apparently our tests only create blocks of a single type at the moment. We need a test with at least two blocks of different types.
Not sure if we want to do this here or in a new issue.