Problem/Motivation

There is still some configuration data in core that is not covered by configuration schemata. By throwing an exception during Config::save(), we can expose the core configuration data that is not covered in automated tests.

  50 config_test.noschema
  48 migrate.migration.d6_action_settings
  30 language.settings
  18 content_translation.settings
  10 form_test.object
  10 color.bartik
   8 language.config.de.locale_test.translation
   6 language.config.xx.system.site
   4 language.config.xx.system.date_format.fallback
   4 language.config.xx.image.style.medium
   4 language.config.xx.contact.category.feedback
   4 config_test.query.1
   4 config_events_test.test
   4 color_test_theme.settings
   4 breakpoint.breakpoint_group.atestset
   2 views_test_data.tests
   2 language.config.xx.system.maintenance
   2 language.config.nl.menu_test.menu_item
   2 language.config.it.tour.tour.tour-test
   2 language.config.fr.system.site
   2 language.config.de.block.block.stark_admin
   2 foo.bar
   2 config_test.query.entity1
   2 config_test.crud
   2 breakpoint.breakpoint.custom.user.

Note the trailing . on the last item for breakpoint.

Proposed resolution

See #1910624: [META] Introduce and complete configuration schemas in all of core for instructions on adding configuration schemata. While configuration schemata are not currently required, all core modules should provide them.

Remaining tasks

  • Fix all child issues needed.
  • Are there core exceptions that do not need or should not have schemata?
  • Can we provide developer tools or another means to identify missing schemata and to remind developers that they should create them, without making them required?

Sub tasks #

Here is a consolidated list of tasks that are pending on different sections below:

Component name Issue
Action #2259525: system.action.user_add_role_action.ROLE_ID does not match any config schema
Breakpoint #2245727: Add missing configuration schema in Breakpoint component
Color #2245729: Add missing configuration schema in Color component
Config Test #2245725: Add missing configuration schema in Config Test component
Entity #2246557: Missing schema for entity area plugin
Language #2245721: Add missing configuration schema in language component
Migrate #2183957: Provide configuration schema for Migration module
System #2245733: Add missing configuration schema in system component
Views #2245731: Add missing configuration schema in Views component

Identified later:

User interface changes

N/A

API changes

Schema additions only.

Original report by @effulgentsia

See patch and bot failures.

Files: 
CommentFileSizeAuthor
#23 interdiff.txt598 byteseffulgentsia
#23 config-schema-23.patch1.75 KBeffulgentsia
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 63,907 pass(es), 282 fail(s), and 150 exception(s). View
#16 config-schema-16.patch1.74 KBYesCT
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Drupal installation failed. View
config-schema.patch1.71 KBeffulgentsia
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 62,854 pass(es), 225 fail(s), and 86 exception(s). View

Comments

Status: Needs review » Needs work

The last submitted patch, config-schema.patch, failed testing.

tim.plunkett’s picture

Here are all the exceptions (just grepped the HTML of the qa results page)

  50 config_test.noschema
  48 migrate.migration.d6_action_settings
  30 language.settings
  18 content_translation.settings
  10 form_test.object
  10 color.bartik
   8 language.config.de.locale_test.translation
   6 language.config.xx.system.site
   4 language.config.xx.system.date_format.fallback
   4 language.config.xx.image.style.medium
   4 language.config.xx.contact.category.feedback
   4 config_test.query.1
   4 config_events_test.test
   4 color_test_theme.settings
   4 breakpoint.breakpoint_group.atestset
   2 views_test_data.tests
   2 language.config.xx.system.maintenance
   2 language.config.nl.menu_test.menu_item
   2 language.config.it.tour.tour.tour-test
   2 language.config.fr.system.site
   2 language.config.de.block.block.stark_admin
   2 foo.bar
   2 config_test.query.entity1
   2 config_test.crud
   2 breakpoint.breakpoint.custom.user.

Note the trailing . on that last breakpoint one

Here they are per namespace, maybe an issue per each?

  66 language
  58 config_test
  48 migrate
  18 content_translation
  10 form_test
  10 color
   6 breakpoint
   4 config_events_test
   4 color_test_theme
   2 views_test_data
   2 foo
xjm’s picture

Priority: Normal » Major
Issue tags: +beta target

Wowza. That's a lot of missing data model.

xjm’s picture

xjm’s picture

Title: Find out what config schemas are still missing » [meta] Find out what config schemas are still missing and add them
Priority: Major » Critical
Status: Needs work » Active
Issue tags: -beta target +beta blocker

Discussed with @alexpott. We really kinda need to make this beta-blocking. =/

xjm’s picture

Issue summary: View changes
xjm’s picture

vijaycs85’s picture

We aware few of them in the list and omitted schema with reason when getting #2167623: Add test for all default configuration to ensure schema exists and is correct in. Here is an update on each item:

We have Drupal\config\Tests\DefaultConfigTest to test incomplete schema.

  1. config_test.noschema - Has to be skipped as it tests TypedConfigManagerInterface::hasConfigSchema() method.
  2. migrate.* - #2183957: Provide configuration schema for Migration module
  3. language.config.*.*.* - #2168609: Move config translations (language.config.[langcode]) in to new location but it is closed as duplicate of #2224887: Language configuration overrides should have their own storage

Yet to fix:


Language:
  30 language.settings
  18 content_translation.settings

Config Test:
   4 config_test.query.1
   4 config_events_test.test
   2 config_test.query.entity1
   2 config_test.crud

Breakpoint:
   4 breakpoint.breakpoint_group.atestset
   2 breakpoint.breakpoint.custom.user.
Color:
  10 color.bartik
   4 color_test_theme.settings

Views:
   2 views_test_data.tests

System:
  10 form_test.object
   2 foo.bar
vijaycs85’s picture

Issue summary: View changes
vijaycs85’s picture

Issue summary: View changes
vijaycs85’s picture

Issue summary: View changes
xjm’s picture

dawehner’s picture

Gábor Hojtsy’s picture

Gábor Hojtsy’s picture

Issue summary: View changes
YesCT’s picture

Status: Active » Needs review
FileSize
1.74 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Drupal installation failed. View

I wanted re re-run the patch to see the current errors. but it didn't apply.

conflict in core/lib/Drupal/Core/Config/Config.php was:

    if ($this->typedConfigManager->hasConfigSchema($this->name)) {
      // Ensure that the schema wrapper has the latest data.
      $this->schemaWrapper = NULL;
      foreach ($this->data as $key => $value) {
        $this->data[$key] = $this->castValue($key, $value);
      }
    }
<<<<<<< HEAD
    else {
      foreach ($this->data as $key => $value) {
        $this->validateValue($key, $value);
      }
=======
    // @todo Why are these schema not recognized during installation?
    elseif (!in_array($this->name, array('system.module'))) {
      throw new ConfigException(format_string('Configuration file @name cannot be saved due to lack of schema.', array(
        '@name' => $this->name,
      )));
>>>>>>> errors on april1
    }

resolved by putting the elseif from the patch in before the else...

    if ($this->typedConfigManager->hasConfigSchema($this->name)) {
      // Ensure that the schema wrapper has the latest data.
      $this->schemaWrapper = NULL;
      foreach ($this->data as $key => $value) {
        $this->data[$key] = $this->castValue($key, $value);
      }
    }
    // @todo Why are these schema not recognized during installation?
    elseif (!in_array($this->name, array('system.module'))) {
      throw new ConfigException(format_string('Configuration file @name cannot be saved due to lack of schema.', array(
        '@name' => $this->name,
      )));
    }
    else {
      foreach ($this->data as $key => $value) {
        $this->validateValue($key, $value);
      }
    }

but I'm not sure. validate sounds like it might be trying to catch problems also?

--
needs review just for the testbot, can put it back to active.

Status: Needs review » Needs work

The last submitted patch, 16: config-schema-16.patch, failed testing.

xjm’s picture

Status: Needs work » Active
YesCT’s picture

viewing the testbot results error was:

WD php: Drupal\Core\Config\ConfigException: Configuration file [error]
update.settings cannot be saved due to lack of schema. in
Drupal\Core\Config\Config->save() (line 220 of
/var/lib/drupaltestbot/sites/default/files/checkout/core/lib/Drupal/Core/Config/Config.php).
Drupal\Core\Config\ConfigException: Configuration file update.settings cannot be saved due to lack of schema. in Drupal\Core\Config\Config->save() (line 220 of /var/lib/drupaltestbot/sites/default/files/checkout/core/lib/Drupal/Core/Config/Config.php).

xjm’s picture

To clarify #19, Drupal no longer installs with this test patch applied, so we want to look at a different way of finding them. :)

YesCT’s picture

we could run it, and patches on a git checkout from "April 2, 2014" ... not conclusive, but should provide some kind of automation of testing.

xjm’s picture

That information is right here, yep: https://qa.drupal.org/pifr/test/762278

effulgentsia’s picture

Status: Active » Needs review
FileSize
1.75 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 63,907 pass(es), 282 fail(s), and 150 exception(s). View
598 bytes

A way to move past the update.settings error to see what else there is.

Status: Needs review » Needs work

The last submitted patch, 23: config-schema-23.patch, failed testing.

alexpott’s picture

So given that schema were introduce to solve the issue of knowing what to translate (#1866610: Introduce Kwalify-inspired schema format for configuration, #1648930: Introduce configuration schema and use for translation) and are now also used to ensure that data types are consistent regardless of how the config is edited (#2130811: Use config schema in saving and validating configuration form to ensure data is consistent and correct ). Before we decide to do all the related issues we need to decide whether or not config schema are mandatory. Given the two use cases I don't think that we need schema for all test config. However I do think we need schema for all deployable config in core.

Gábor Hojtsy’s picture

So far we did not make schemas mandatory BUT we introduced them as best practice to educate contribs. Adding schemas to tests is not something we need contribs to do either, but I think even if we don't technically need those schemas for tests, for contribs copy-pasting example code from the tests, it may set an example. Not sure how realistic is that, but that may be the only reason left to write schemas for those too?

Gábor Hojtsy’s picture

Discussed again on the Drupal 8 Multilingual meeting with Alex Pott attending. We'll not do schemas for tests since they are not mandatory and they have very marginal benefits. Will close sub-issues that are test specific.

xjm’s picture

Issue tags: -beta blocker

Discussed with @alexpott and @Gábor Hojtsy. Not all of the child issues here are beta blocking as some are just related to tests, so I've explicitly tagged two beta blockers and am untagging the meta.

xjm’s picture

Status: Needs work » Active
Gábor Hojtsy’s picture

Issue summary: View changes
Status: Active » Closed (duplicate)

Added #2259525: system.action.user_add_role_action.ROLE_ID does not match any config schema as sub-issue. That is closed and only #2183957: Provide configuration schema for Migration module is left now, so we can close this down I believe. There is no point in having a META for one other issue, is there? Closing as duplicate of #2183957: Provide configuration schema for Migration module on principle then because this is not fixed per say.

dawehner’s picture

I am quite convinced that there is A LOT of views stuff which does not have yet proper schema.

Gábor Hojtsy’s picture

Status: Closed (duplicate) » Active

Hum, hum. How do we find them then? :)

vijaycs85’s picture

I am quite convinced that there is A LOT of views stuff which does not have yet proper schema.

/me worried to hear the word 'LOT' from @dawehner :)

Gábor Hojtsy’s picture

Issue summary: View changes

I wanted to extend config inspector to help debug missing schemas (see #2266427: Add more info on schema validity to configuration inspector for debugging for current state) and found on the way that the core schema tester is not complete, it misses several cases where it should fail. Also if it could be reusable for testing config with schemas, I would not need to copy the code in the config inspector issue. Yeah.

So I opened #2266501: Array level schema errors are not reported, fix wrong schemas identified which found several legit missing schemas for empty container elements that the tests did not find before. They are not THAT many so I think we can fix them there, so adding that as a child issue of this one.

Gábor Hojtsy’s picture

Issue summary: View changes
Gábor Hojtsy’s picture

Issue summary: View changes
dawehner’s picture

We do have a test which creates views for all available fields https://api.drupal.org/api/drupal/core!modules!views!lib!Drupal!views!Te...

We would just have to combine this with the way how schema test validates its available schemas.

Gábor Hojtsy’s picture

@dawehner: my current idea is to start throwing exceptions form castValue() once we have #2266501: Array level schema errors are not reported, fix wrong schemas identified, #2264179: Clarify use of property/undefined and add an ignore type in configuration schema and #2268975: Fix named schema elements missing when installing Drupal which will uncover those things in ALL the tests. That will likely be a long list, so we'll need ways to partition those fails as well. Or its a short list because we fix all the things ahead... We'll see.

Thankfully one of those 3 is RTBC, one is I think at RTBC but needs confirmation/review form @vijaycs85 and the third one actually @vijaycs85 is working on :)

tim.plunkett’s picture

Gábor Hojtsy’s picture

Also @vijaycs85 is finding and fixing lots of config and test issues on migrations in #2293419: Add config schema test to all configuration test in migration, fix bugs all found by schema :)

Gábor Hojtsy’s picture

Of the ones from the prior comment #2283977: Create a new ConfigEntity type for storing bundle-specific customizations of base fields and #2224761: Add a generic way to add third party configuration on configuration entities and implement for field configuration are definitely in the works still. Now that the standard profile fix is in, I could continue with #2183983: Find hidden configuration schema issues, which uncovered more problems. So will need to open more issues for those. The first one is for a whole lot of the fails: #2309247: Views do not depend on modules providing their displays.

Berdir’s picture

So, plenty got done :)

The only issue left is #2183983: Find hidden configuration schema issues. Should we close this as there is nothing actionable left to do here except that issue, which is a major task as well?

Gábor Hojtsy’s picture

I think that is fine if we elevate that to critical? :)

Gábor Hojtsy’s picture

Status: Active » Closed (duplicate)
jibran’s picture