Problem/Motivation

The Export UI does this:

  $form['export']['#value'] = Yaml::encode($this->configStorage->read($name));

and hence does not respect ConfigEntityTypeInterface::getPropertiesToExport().

The cacheability metadata in the View config entity should not be exported, but the above bug causes it to be exported anyway.

Consequently, all configuration exported through the config export UI until this is fixed will contain data that, when its schema changes, forces people to re-export or manually fix their exported config, rather than that being something that is fixed transparently.

Proposed resolution

Fix it.

Remaining tasks

TBD

User interface changes

None.

API changes

None.

Data model changes

None.

Comments

Wim Leers’s picture

Relevant IRC discussion:

12:36:57 WimLeers: alexpott: RE: https://www.drupal.org/node/2464427 + default views
12:36:59 Druplicon: https://www.drupal.org/node/2464427 => Replace CacheablePluginInterface with CacheableDependencyInterface #2464427: Replace CacheablePluginInterface with CacheableDependencyInterface => 110 comments, 12 IRC mentions
12:37:34 WimLeers: alexpott: why does the export UI do this:

  $form['export']['#value'] = Yaml::encode($this->configStorage->read($name));

and not respect

  \Drupal\Core\Config\Entity\ConfigEntityTypeInterface::getPropertiesToExport()

?
12:37:50 WimLeers: alexpott: the cacheability metadata in the View config entity should not be exported
12:37:52 WimLeers: dawehner: ^^
12:38:09 dawehner: WimLeers: i know that getPropertiesToExport got added later, maybe this is why
12:38:25 WimLeers: ah hmmm
12:38:33 WimLeers: that seems like it's at least major, potentially critical?
12:38:45 WimLeers: Because it means that all exported config as of today could contain things it should not contain?
12:39:02 dawehner: WimLeers: why is the export UI critical?
12:39:20 WimLeers: dawehner: because the export UI is how people create default views for example
12:39:34 dawehner: WimLeers: so?
12:39:37 WimLeers: dawehner: and because it exports things it should not export, https://www.drupal.org/node/2464427 disrupts default views
12:39:38 Druplicon: https://www.drupal.org/node/2464427 => Replace CacheablePluginInterface with CacheableDependencyInterface #2464427: Replace CacheablePluginInterface with CacheableDependencyInterface => 110 comments, 13 IRC mentions
12:39:53 dawehner: well then we have >100 criticals in the major queue
12:39:55 WimLeers: dawehner: i.e. data model changes in NON-EXPORTED things should not require re-exporting
12:40:02 dawehner: there was major issues way worse, just telling you
12:40:16 WimLeers: dawehner: I'm asking because this will affect every piece of exported config in the long run
12:40:25 WimLeers: dawehner: not every major has such long-term consequences as this one
12:40:40 alexpott: WimLeers: I think this is just a major bug
12:40:44 dawehner: well, I consider the export UI as pure debugging tool
12:40:53 WimLeers: alexpott: ok
12:41:01 dawehner: and config_devel the way how you actually export default views
12:41:07 WimLeers: dawehner: aha!
12:41:10 WimLeers: dawehner: I didn't know that
tim.plunkett’s picture

Yeah, I wrote the config export UI in Prague, and it went in October 2013.
getPropertiesToExport() wasn't added til May 2015 :)

tim.plunkett’s picture

Status: Active » Needs review
Issue tags: +Needs tests
FileSize
1.65 KB
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 113,560 pass(es). View
tim.plunkett’s picture

+++ b/core/modules/config/src/Form/ConfigSingleExportForm.php
@@ -142,17 +142,24 @@ public function updateConfigurationType($form, FormStateInterface $form_state) {
+      $value2 = $this->configStorage->read($name);

lol, left that bit of debugging in

Wim Leers’s picture

Assigned: Unassigned » alexpott
Issue tags: +rc target, +Needs committer feedback

Seems to me this is important to land during the RC phase, because otherwise lots of people will end up exporting config incorrectly…

Per https://groups.drupal.org/node/484788, I cannot actually add that tag, but I did anyway for clarity. So, also added the "Needs committer feedback" tag and assigning to Alex Pott, who's both a committer and the authority on the configuration system, so he can decide whether to keep or remove that tag.

alexpott’s picture

So this is part of a bigger concern I have about the single import export UI - it is not doing an validation that is done during a full config import... I have an issue for that somewhere.

alexpott’s picture

Issue tags: -rc target, -Needs committer feedback +rc target triage
xjm’s picture

Title: Config export UI exports entity properties that should not be exported » Single config export UI exports entity properties that should not be exported
xjm’s picture

Status: Needs review » Needs work
Issue tags: -rc target triage +rc target

@alexpott and I agreed with this going in during RC because of the implications for data integrity, developer experience, etc.

NW for tests.

xjm’s picture

Issue tags: +Triaged D8 major
alexpott’s picture

Assigned: alexpott » Unassigned
tim.plunkett’s picture

Status: Needs work » Needs review
FileSize
1.47 KB

Rerolled. Still needs tests.

alexpott’s picture

I think we also have a problem with mapFromStorageRecords / mapToStorageRecords - this was introduced for #2293773: Field allowed values use dots in key names - not allowed in config

alexpott’s picture

So yes we do have a problem because of #13... I tried importing a field storage for a float options field...

uuid: 257de02d-3501-49be-bcf2-91431d163824
langcode: en
status: true
dependencies:
  module:
    - node
    - options
id: node.field_test
field_name: field_test
entity_type: node
type: list_float
settings:
  allowed_values:
    '0.1': '0.1'
    '0.2': '0.2'
  allowed_values_function: ''
module: options
locked: false
cardinality: 1
translatable: true
indexes: {  }
persist_with_no_fields: false
custom_storage: false

And this resulted in the following error:

Unexpected error during import with operation create for field.storage.node.field_test: 0.1 key contains a dot which is not supported.

alexpott’s picture

Here's a patch that fixes the issue described in #14 and adds a test for it.

alexpott’s picture

Title: Single config export UI exports entity properties that should not be exported » Single config export UI exports the wrong entity properties and sometimes in the wrong format
xjm’s picture

Issue tags: -rc target

This issue was an rc target for 8.0.0. It could also be an RC target for 8.1.0 but we can discuss that if needed.

Shouldn't the test-only patch have failed?

alexpott’s picture

So thinking about this some more I'm not sure that we have a bug here at all. We get the configuration to export directly from the configuration storage. The only way a configuration entity should have got there is through a config entity save which will obviously export its properties correctly. The only way the config storage can have properties which no longer exist is if there are module updates that have changed the exported properties and they haven't written a hook_update_N() or update.php has not been run yet. Neither of those situations should be handled by this form.

I think the test added in #15 is a good addition to core because field creation is always worth testing.

I'll reach out to @tim.plunkett to talk about re-scoping this issue to add the test and make it a task.

Version: 8.0.x-dev » 8.1.x-dev

Drupal 8.0.6 was released on April 6 and is the final bugfix release for the Drupal 8.0.x series. Drupal 8.0.x will not receive any further development aside from security fixes. Drupal 8.1.0-rc1 is now available and sites should prepare to update to 8.1.0.

Bug reports should be targeted against the 8.1.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.2.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

alexpott’s picture

Status: Needs review » Closed (won't fix)

After discussing on IRC with @tim.plunkett we agreed to close this issue because

if there is stuff that is in the config storage that does not comply with schema / the config entity class definition - then something has changed and we've forgotten to write an update function

I think eventually we'll have schema validation during config import which will catch the scenario when you export on site with a different module version and try an import on a site with another

I'm going to open another issue to add the float field test coverage because:

that is a super complex config entity because of dots in array keys

Wim Leers’s picture

Status: Closed (won't fix) » Active

I'm fine with #20's course of action. But I'd like an answer to the original reason for reporting this issue:

The cacheability metadata in the View config entity should not be exported, but the above bug causes it to be exported anyway.

Consequently, all configuration exported through the config export UI until this is fixed will contain data that, when its schema changes, forces people to re-export or manually fix their exported config, rather than that being something that is fixed transparently.

I just checked, and View config entities' cache_metadata is still being exported at /admin/config/development/configuration/single/export. So, what I reported is still happening.

But then I started wondering… what is actually wrong with that? ConfigEntityType::getPropertiesToExport() looks at the config_export key in a config entity type's annotation. For the View config entity, that is:

 *   config_export = {
 *     "id",
 *     "label",
 *     "module",
 *     "description",
 *     "tag",
 *     "base_table",
 *     "base_field",
 *     "core",
 *     "display",
 *   }

… and since cache_metadata lives several levels under the display key, this is actually totally correct.

So, actually …

  1. my bug report didn't make sense in the first place?
  2. I wonder why I even expected cache_metadata not to be exported? I now wonder if I expected that because the patch in #2318377: Determine whether a view is cacheable and its required contexts, store this i/t config entity didn't update the exported View config entity YAML files? Which was something we did quite frequently back then, which is why we've re-exported all built-in configuration in one go at a later time: #2567183: Re-export all built-in configuration in core.

It's fine to re-close this with the simple statement that I was indeed completely wrong. But then… why didn't anybody call me out on that? :P

alexpott’s picture

Status: Active » Closed (won't fix)

I totally expect cache_metadata to be exported - it is in all of our default views. If we don't want to export then we need to unset on save. Nothing is doing that.

I remember once we change where we stored the key and everything had to change. I think this is what you are referring to in #21.2