Early Bird Registration for DrupalCon Portland 2024 is open! Register by 23:59 PST on 31 March 2024, to get $100 off your ticket.
Problem/Motivation
This issue is to resolve all the remaining fails from #2183983: Find hidden configuration schema issues other than #2391021: Config schema issues in config tests themselves. These are hopefully small problems here and there that may be too much to open its own issues for...
Proposed resolution
1. Add strict schema checking to these tests.
2. Fix issues in schema and/or tests.
Remaining tasks
Test. Fixes. Review. Commit.
User interface changes
None.
API changes
Schema fixes as appropriate.
Comment | File | Size | Author |
---|---|---|---|
#22 | interdiff.txt | 1.05 KB | Gábor Hojtsy |
#22 | 2391245-resolve-remaining-config-schema-22.patch | 19.98 KB | Gábor Hojtsy |
#18 | 2391245-resolve-remaining-config-schema-18.patch | 21.57 KB | Wim Leers |
#15 | interdiff.txt | 896 bytes | Gábor Hojtsy |
#15 | 2391245-resolve-remaining-config-schema-14.patch | 16.07 KB | Gábor Hojtsy |
Comments
Comment #1
Gábor HojtsyComment #2
Gábor HojtsyComment #4
Gábor HojtsyBrought in fixes from @Berdir in #2183983-74: Find hidden configuration schema issues for some of these:
1. action_bulk_test.info.yml needs to depend on node, the shipped views are node based.
2. CKEditorAdminTest.php sets plugins the wrong way. @Berdir was not sure about this fix and neither am I. Will need a @WimLeers review.
Fixes figured out on my own:
3. block_test needs a block_test.schema.yml to describe block.settings.test_block_instantiation with its own setting.
4. Color module litters theme settings with its own settings. Haha. We need to set the color submit function early enough AND unset the color added elements. Ideally this form would be refactored because the quick-fix is not very nice, but that is definitely not in scope here.
5. Color test theme is missing its theme settings schema. See also #2382671: Automatically define schema for themes.
More to come.
Comment #6
Gábor HojtsyMore fixes:
1. CommentActionsTest: the schema for the comment action was wrong in not pluralizing keywords
2. ImageDimensionsTest: uses an image effect with no settings that has no schema; we could provide that schema or just provide an empty fallback like everywhere else, which is what I did; this will fail the same way if the effect has any settings but not supposed to
3. SearchTokenizerTest: set the minimum_word_size at the wrong place once (it set it right at least one other place)
4. StatisticsReportsTest: defines its own block type but had no schema for it. Found #2391403: Statistics block not properly migrated, schema incorrect on the way. Normal bug, not to be resolved now.
Comment #8
Gábor Hojtsy1. One more fix in SearchTokenizerTest: overlap_cjk was not set in the right settings place one also.
2. CKEditorAdminTest used a llama_contextual_and_button plugin with a custom setting but that did not have schema. Also it was testing the result as 1 instead of TRUE.
3. CKEditorLoadingTest put a button at the wrong structure in the config.
Comment #10
Gábor HojtsyThe tour fail was due to language overrides being tested for validity which is futile. Made the schema checker not test config in collections other than the default which should fix this one.
Comment #11
Wim LeersAttached is a patch that fixes the fail in
CKEditorLoadingTest
. Apparently callingWebTestBase::resetAll()
after installing a module — as recommended! — causes the wrong container to be used. I fear this might be a fairly widespread problem?Simply removing the
::resetAll()
call makes the test pass!I'm still digging into the
EditorAdminTest
, but it's already certain that it's a similar container/caching problem.Wow, that looked weird! I don't understand how this test ever worked to be honest.
A remnant from the days where CMI was casting booleans to ints somehow… and where we didn't have config schemas yet :)
At least this wasn't my fault.
Indeed, was also not updated correctly a long time ago. How did we (I) miss all this, and how did the tests still pass? :( :O
Comment #12
Gábor HojtsyRe #11/3: the answer to how did that test pass is if you don't add the button at the right place, the plugin is not added either. Later the same editor setting is used to compute the expected value, so if you add the button it compares values after that fact, if you add it at the wrong place, it compares values without that. Not sure how to make that more solid without hardwiring a WHOLE LOT of stuff, but not the scope of this issue :)
Comment #14
Wim Leers#12: I actually did the code review before debugging that test. I came the same conclusion as you did. But that hardwiring was explicitly not done, for this documented reason:
i.e. that other test already tests
CKEditor::getJSSettings()
.Comment #15
Gábor HojtsyThe vocabulary crud test was failing because even though it uninstalled the taxonomy module which uninstalled the taxonomy_crud module that provided a 3rd party setting, it kept the old (now invalid) config entity in memory. Then it only installed the taxonomy module again (not the crud module), so the crud 3rd party setting was invalid. To avoid this, the simplest was to remove the 3rd party setting for this test. We do not need it in this one. It is tested in other tests in this class.
Comment #18
Wim LeersSo apparently
EditorAdminTest
's only problem was that the form was generating settings that weren't reflected in the default settings NOR in the schema. By making all of that use a single setting, the problem is now solved.(The removal of the "default settings provided/altered by modules" concept in the other issue helped make this simplification possible.)
Comment #19
Gábor Hojtsy@Wim: your changes look good and all makes sense. Now to find a reviewer :)
Comment #21
bircherThis seems to not be wanted here.
Appart from that I didn't find anything wrong, but someone else with more knowledge on the topic still might want to review it.
Comment #22
Gábor HojtsyFixed that and one phpdoc error phpstorm alerted me to there :D I think given Wim reviewed all CK and Editor related changes and I reviewed his stuff, this should be good? :)
Comment #23
bircherComment #24
webchickThis one looks a bit above my pay grade. :)
Comment #25
alexpottThis issue addresses a major bug and is allowed per https://www.drupal.org/core/beta-changes. Committed 348a681 and pushed to 8.0.x. Thanks!
Comment #27
Wim LeersSomehow we lost this hunk along the way:
Hence one last exception at #2183983-166: Find hidden configuration schema issues.
Comment #28
Gábor HojtsyWim: let's resolve at #2183983: Find hidden configuration schema issues.