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.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Gábor Hojtsy’s picture

Gábor Hojtsy’s picture

Issue tags: +Ghent DA sprint

Status: Needs review » Needs work

The last submitted patch, 1: 2391245-resolve-remaining-config-schema.patch, failed testing.

Gábor Hojtsy’s picture

Status: Needs work » Needs review
FileSize
10.35 KB
4.87 KB

Brought 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.

Status: Needs review » Needs work

The last submitted patch, 4: 2391245-resolve-remaining-config-schema-4.patch, failed testing.

Gábor Hojtsy’s picture

Status: Needs work » Needs review
FileSize
12.56 KB
2.47 KB

More 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.

Status: Needs review » Needs work

The last submitted patch, 6: 2391245-resolve-remaining-config-schema-6.patch, failed testing.

Gábor Hojtsy’s picture

Status: Needs work » Needs review
FileSize
2.85 KB
14.35 KB

1. 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.

Status: Needs review » Needs work

The last submitted patch, 8: 2391245-resolve-remaining-config-schema-8.patch, failed testing.

Gábor Hojtsy’s picture

Status: Needs work » Needs review
FileSize
15.66 KB
1.31 KB

The 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.

Wim Leers’s picture

Attached is a patch that fixes the fail in CKEditorLoadingTest. Apparently calling WebTestBase::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.

  1. +++ b/core/modules/ckeditor/src/Tests/CKEditorAdminTest.php
    @@ -154,8 +159,7 @@ function testExistingFormat() {
    -      'items' => array('Undo', '|', 'Redo'),
    -      array('JustifyCenter')
    +      'items' => array('Undo', '|', 'Redo', 'JustifyCenter'),
    

    Wow, that looked weird! I don't understand how this test ever worked to be honest.

  2. +++ b/core/modules/ckeditor/src/Tests/CKEditorAdminTest.php
    @@ -186,7 +190,7 @@ function testExistingFormat() {
    -    $expected_settings['plugins']['llama_contextual_and_button']['ultra_llama_mode'] = 1;
    +    $expected_settings['plugins']['llama_contextual_and_button']['ultra_llama_mode'] = TRUE;
    
    …
    
    diff --git a/core/modules/ckeditor/tests/modules/config/schema/ckeditor_test.schema.yml b/core/modules/ckeditor/tests/modules/config/schema/ckeditor_test.schema.yml
    new file mode 100644
    index 0000000..e6734a7
    --- /dev/null
    +++ b/core/modules/ckeditor/tests/modules/config/schema/ckeditor_test.schema.yml
    @@ -0,0 +1,7 @@
    +ckeditor.plugin.llama_contextual_and_button:
    +  type: mapping
    +  label: 'Contextual Llama With Button'
    +  mapping:
    +    ultra_llama_mode:
    +      type: boolean
    +      label: 'Ultra llama mode'
    

    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.

  3. +++ b/core/modules/ckeditor/src/Tests/CKEditorLoadingTest.php
    @@ -130,7 +135,7 @@ function testLoading() {
    -    $editor_settings['toolbar']['buttons'][0][] = 'Llama';
    +    $editor_settings['toolbar']['rows'][0][0]['items'][] = 'Llama';
    

    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

Gábor Hojtsy’s picture

Re #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 :)

The last submitted patch, 10: 2391245-resolve-remaining-config-schema-10.patch, failed testing.

Wim Leers’s picture

#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:

    // NOTE: the tests in CKEditorTest already ensure that changing the
    // configuration also results in modified CKEditor configuration, so we
    // don't test that here.

i.e. that other test already tests CKEditor::getJSSettings().

Gábor Hojtsy’s picture

The 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.

The last submitted patch, 11: 2391245-resolve-remaining-config-schema-11.patch, failed testing.

Wim Leers’s picture

So 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.)

Gábor Hojtsy’s picture

@Wim: your changes look good and all makes sense. Now to find a reviewer :)

The last submitted patch, 15: 2391245-resolve-remaining-config-schema-14.patch, failed testing.

bircher’s picture

+++ b/core/modules/editor/src/Tests/EditorAdminTest.php
@@ -146,15 +149,15 @@ protected function selectUnicornEditor() {
+    debug($settings);

This 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.

Gábor Hojtsy’s picture

Fixed 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? :)

bircher’s picture

Status: Needs review » Reviewed & tested by the community
webchick’s picture

Assigned: Gábor Hojtsy » alexpott

This one looks a bit above my pay grade. :)

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

This 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!

  • alexpott committed 348a681 on 8.0.x
    Issue #2391245 by Gábor Hojtsy, Wim Leers: Resolve remaining misc issues...
Wim Leers’s picture

Somehow we lost this hunk along the way:

@@ -130,7 +135,7 @@ function testLoading() {
     $this->resetAll();
     $this->container->get('plugin.manager.ckeditor.plugin')->clearCachedDefinitions();
     $editor_settings = $editor->getSettings();
-    $editor_settings['toolbar']['buttons'][0][] = 'Llama';
+    $editor_settings['toolbar']['rows'][0][0]['items'][] = 'Llama';
     $editor->setSettings($editor_settings);
     $editor->save();
     $this->drupalGet('node/add/article');

Hence one last exception at #2183983-166: Find hidden configuration schema issues.

Gábor Hojtsy’s picture

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.