Problem/Motivation
When a data type definition is built by combing two other definition (a data type definition with a sub definition) in \Drupal\Core\Config\TypedConfigManager::getDefinitionWithReplacement() the sub definition doesn't get fully built by looping through its parent definitions, so the resulting merged definition is incomplete.
I noticed this when the resulting merged definition did not inherit any of the mappings provided by the sub definition.
Proposed resolution
Loop through the parent definition types of the sub definition with \Drupal\Core\Config\TypedConfigManager::getDefinition() before merging.
Remaining tasks
Test to show failingPatch to fix
Non blocking: #2755825: Add tests for 3 dynamic merges to verify dynamic data types dependencies lead to complete definitions
User interface changes
NA
API changes
NA
Data model changes
NA (there is but now it will be what we thought it should be)
Comment | File | Size | Author |
---|---|---|---|
#21 | merging_data_types-2693081-15.patch | 3.88 KB | YesCT |
#18 | merging_data_types-2693081-15-testsonly.patch | 2.58 KB | YesCT |
#15 | merging_data_types-2693081-15.patch | 3.88 KB | yanniboi |
#15 | interdiff.txt | 950 bytes | yanniboi |
#3 | merging_data_types-2693081-3.patch | 2.33 KB | yanniboi |
Comments
Comment #2
yanniboi CreditAttribution: yanniboi at FreelyGive commentedThis patch is the start of a test to show the problem, although I have not got any tests failing yet.
This can easily be added to an existing test module rather than creating a new one, but I wasn't sure which so I am open to suggestions.
Comment #3
yanniboi CreditAttribution: yanniboi at FreelyGive commentedOk, so I have noticed that there is already a good test to build on rather than writing my own...
Disregard the first patch.
I have added a test to show that there is a problem in core.
Basically, I am saving a config object that should build its data definition from 2 definitions ('wrapping.test.other_double_brackets.*' and 'test.double_brackets.cat:*.*') and showing that there is a problem when there is missing schema for a mapping on 'test.double_brackets.cat:*.*' (ie. 'breed').
This is to illustrate that when 2 data definitions are merged as part of TypedConfigManager::getDefinitionWithReplacement() the second definition (referenced in code as $sub_type) isn't a fully built data definition because none of its information is inherited from parent definitions.
Comment #4
yanniboi CreditAttribution: yanniboi at FreelyGive commentedHere is my solution, to fully build the data definition of the $sub_type definition before merging it.
Comment #5
yanniboi CreditAttribution: yanniboi at FreelyGive commentedFor clarity, looking at the rest of TypedConfigManager::getDefinitionWithReplacements() helps a lot because the pattern that I have implemented in my patch is used quite a lot:
Comment #6
yanniboi CreditAttribution: yanniboi at FreelyGive commentedComment #8
yanniboi CreditAttribution: yanniboi at FreelyGive commentedI just realised that the sub definition may not have type set and may not need looping over in some cases.
Patch to fix and interdiff...
Comment #9
yanniboi CreditAttribution: yanniboi at FreelyGive commentedLooks like core/modules/config/src/Tests/ConfigSchemaTest.php has moved or been deleted... I will have a look at that and rebase.
Comment #10
yanniboi CreditAttribution: yanniboi at FreelyGive commentedOk, I've (sorta) rebased my patch. ConfigSchemaTest moved to being a phpunit Kernel test.
Comment #11
yanniboi CreditAttribution: yanniboi at FreelyGive commentedMoving to 8.2.x
Comment #12
YesCT CreditAttribution: YesCT commentedwhy the colon in the id? is that what indicates an inheritance? ... or is the colon just part of an arbitrary string, and not syntactically significant (maybe it is a convention used in panels id strings and so used here in a similar use case example to match something practical that people might use).
webflow says: I guess it is because we didn't fully resolve the parent dynamic type, so the parent property was missing?
(in the current test, without the fix, the config gets either 1 or 3 properties.. I wonder if we could count the properties in the assert, nah, probably better to assert the expected ones are there with the values than to just count them)
maybe that means what is missing is the parent stuff, in this case the breed.
wait...
test.double_brackets.breeed "extends" test.double_brackets
so like php classes and extending, breed is a property from a "subclass". so... id, foo, and bar are parent properties, and breed is a child property?
??? (trying to use the vocabulary to understand what the words in the issue mean, and how we can document what this is doing)
maybe re-reading http://hojtsy.hu/blog/2014-dec-12/drupal-8-configuration-schema-cheat-sheet would help.
the test fails nicely on the ->save() because it can't save data for a property (?) that is missing from the bad merge.
it would be extra good to also assert the properties are there, and have the values expected in an actual assert.
Can answer this two ways probably, looking at the fix in context (to see where it is in loops), and a test that has 3 dynamic types would prove it would work for any number.
I was wondering why the test did this in the code instead of a config yml file. I looked; the rest of ConfigSchemaTest follows that pattern. ok. :)
Comment #13
YesCT CreditAttribution: YesCT commentedComment #14
yanniboi CreditAttribution: yanniboi at FreelyGive commentedThe precise error that is thrown when this test runs without the merge fix is the following:
So this confirms that it is breed that is missing from the bad merge.
I like the idea of checking more that 2 dynamic types. Will have a look at that.
I have no idea how to simplify the language, but I am open to suggestions :)
Comment #15
yanniboi CreditAttribution: yanniboi at FreelyGive commentedI've had another look at the test in the context of other schema definitions and tests in the config_schema_test and come to the following conclusions.
I was tempted to also test the config entity storage of foo and bar, etc. but I decided that because foo and bar are used in the evaluation of the config definition, there would be other errors if there was a problem with setting them to cat/dog as part of the merge.
Comment #16
YesCT CreditAttribution: YesCT commented@yanniboi Thanks, that additional assert makes me feel like the test is more complete.
I triggered a test run on the patch on 8.1.x so we can see how it does there.
I think it would be good to get a test only patch (and have tests run on it against 8.1.x and 8.2.x).
I would be comfortable with making another issue to look into the 3 dynamic test case, cause I think thinking exactly what that would look like will be challenging and take time, and end up blocking this fix.
Comment #17
YesCT CreditAttribution: YesCT commentedI'm going to make that test only patch now.
Comment #18
YesCT CreditAttribution: YesCT commentedthe tests only patch
Comment #21
YesCT CreditAttribution: YesCT commentedThose fails look appropriate to me. (combined with the review, discussion and recent changes to the test: rtbc)
Re-uploading the whole patch from #15 so that when the automatic retest runs, it will run on a passing patch so that the status doesn't change to needs work
Comment #22
YesCT CreditAttribution: YesCT commentedopened #2755825: Add tests for 3 dynamic merges to verify dynamic data types dependencies lead to complete definitions (not urgent) for a place to think about the test case of 3 levels
Comment #25
Leksat CreditAttribution: Leksat at Amazee Labs commentedIt seems like the last test fail was caused by some CI issue. I retested the last patch and tests are okay.
I also tested this patch manually and I can confirm it works great! For example, it solves page_manager blocks translation issue.
Reverting status to RTBC.
Comment #26
andrewbelcher CreditAttribution: andrewbelcher at FreelyGive commentedThis issue causes problems for contributed modules that run schema checks on of their config. Marking as major ("Block contributed projects with no workaround."). It also causes fatal errors via the UI if the config checker service is enabled (i.e. dev workflows).
Comment #28
alexpottCommitted and pushed 1fbdcc7 to 8.4.x and 9298caf to 8.3.x. Thanks!
Comment #32
Wim Leers