Problem/Motivation

1. Create field of type "List (text)" with field add form, and fill the allowed values (they are not empty).
2. Export config of this field. It will be something like this (filename: 'field.storage.taxonomy_term.field_type.yml'):

langcode: en
status: true
dependencies:
  module:
    - options
    - taxonomy
id: taxonomy_term.field_type
name: field_type
entity_type: taxonomy_term
type: list_text
settings:
  allowed_values:
    -
      value: City
      label: City
    -
      value: 'Administrative area'
      label: 'Administrative area'
    -
      value: Continent
      label: Continent
    -
      value: Country
      label: Country
    -
      value: Region
      label: Region
  allowed_values_function: ''
module: options
locked: false
cardinality: 1
translatable: true
indexes: {  }

3. Try to import this config on some fresh-installed Drupal 8 site. You will get an error:

"The configuration property settings.allowed_values.0.label.0 doesn't exist."

If you put this config in module config/install directory, you will get following in your php error log:

[Sat Jul 26 00:47:02.830178 2014] [:error] [pid 4305] [client 127.0.0.1:36675] Uncaught PHP Exception InvalidArgumentException: "The configuration property settings.allowed_values.0.label.0 doesn't exist." at /var/www/drupal/drupal8-core/core/lib/Drupal/Core/Config/Schema/Mapping.php line 66, referer: http://d8.local/admin/modules/list/confirm

4. If allowed_values are empty in config ('allowed_values: { }'), the config imports normally.

Proposed resolution

Add new methods to the config entity storage to massage data structures. Use them when importing configuration.

Remaining tasks

No

User interface changes

No.

API changes

Two new methods on the config entity storage interface, only additions.

Files: 
CommentFileSizeAuthor
#33 2310093.33.patch23.78 KBalexpott
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 79,104 pass(es). View
#33 25-33-interdiff.txt9.03 KBalexpott
#32 2310093.32.patch30.43 KBhussainweb
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 79,090 pass(es). View
#32 interdiff-25-32.txt2.19 KBhussainweb
#25 2310093.25.patch30.61 KBalexpott
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 79,041 pass(es), 4 fail(s), and 1 exception(s). View
#25 23-25-interdiff.txt743 bytesalexpott
#23 2310093.23.patch30.61 KBalexpott
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 76,665 pass(es), 1 fail(s), and 0 exception(s). View
#23 21-23-interdiff.txt12.76 KBalexpott
#21 2310093.21.patch23.39 KBalexpott
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 76,594 pass(es), 12 fail(s), and 0 exception(s). View
#21 19-21-interdiff.txt3.37 KBalexpott
#19 2310093.19.patch21.49 KBalexpott
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 76,318 pass(es), 3 fail(s), and 0 exception(s). View
#19 17-19-interdiff.txt13.26 KBalexpott
#3 2310093.3.patch1.76 KBalexpott
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,412 pass(es), 1 fail(s), and 0 exception(s). View
#7 2310093.7.patch6.94 KBalexpott
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,315 pass(es), 1 fail(s), and 0 exception(s). View
#9 7-9-interdiff.txt1.17 KBalexpott
#9 2310093.9.patch8.12 KBalexpott
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 76,194 pass(es). View
#17 2310093.test-only.patch3.72 KBalexpott
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 76,345 pass(es), 2 fail(s), and 0 exception(s). View
#17 2310093.17.patch13.3 KBalexpott
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 76,359 pass(es). View

Comments

andyceo’s picture

Issue summary: View changes
andypost’s picture

Priority: Major » Critical
Issue tags: +Configurables
Related issues: +#2293773: Field allowed values use dots in key names - not allowed in config

This could lead to data loss, because no field is imported

alexpott’s picture

Status: Active » Needs review
FileSize
1.76 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,412 pass(es), 1 fail(s), and 0 exception(s). View

Hmmm... I guess we need to use mapFromStorageRecords in the import functions.

andypost’s picture

Issue tags: +Needs tests

Single config import is broken too but works differently

Status: Needs review » Needs work

The last submitted patch, 3: 2310093.3.patch, failed testing.

Berdir’s picture

Yeah, can confirm this.

I don't think this should be in the configuration system component, maybe field system or options.module.

The imported structure looks like this:

Array
(
    [0] => Array
        (
            [value] => 0
            [label] => Array
                (
                    [0] => Array
                        (
                            [value] => value
                            [label] => 3600
                        )

                    [1] => Array
                        (
                            [value] => label
                            [label] => 1h
                        )

                )

        )
alexpott’s picture

Status: Needs work » Needs review
FileSize
6.94 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,315 pass(es), 1 fail(s), and 0 exception(s). View

This fixes import, install and the single import - and adds a test for single import updating an entity. Still need to write tests for the importer and installer. Also we need to clean up the code and agree the approach - updateFromStorageRecord and createFromStorageRecord are quite ugly - but I'm not sure how we can avoid this.

Status: Needs review » Needs work

The last submitted patch, 7: 2310093.7.patch, failed testing.

alexpott’s picture

Status: Needs work » Needs review
FileSize
1.17 KB
8.12 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 76,194 pass(es). View

Fixed the test.

Gábor Hojtsy’s picture

Title: "The configuration property settings.allowed_values.0.label.0 doesn't exist" on field storage config import with list_text field » Config install and import should map from storage record not set properties directly

Hopefully cleaner title for this issue.

Gábor Hojtsy’s picture

Looking at the patch, it is definitely the new / current reality following #2293773: Field allowed values use dots in key names - not allowed in config that config entities should be created with mapping from storage record and not by setting each property one by one. Is it not possible to swap out storage and load from the install / staging storage relying on the same code that would otherwise map from storage? Is that a pipe dream? :) Looks like needing to repeat that code several times may be painful.

alexpott’s picture

#2314347: Remove config.storage dependency from ConfigEntityStorage will make it easier to explore @Gábor Hojtsy's pipe dream.

andyceo queued 9: 2310093.9.patch for re-testing.

Berdir queued 9: 2310093.9.patch for re-testing.

alexpott’s picture

I'm not sure that swapping out the config storage on the config entity storage is the right thing to do - the mapping takes place on the config entity storage level. And we can not swap the config entity storage out since it is overridden by several classes. Is the current approach in #9 that bad?

Berdir’s picture

Status: Needs review » Needs work

Yes, I don't have a better idea, so lets move on with this.

I guess we still need tests for list field types?

  1. +++ b/core/lib/Drupal/Core/Config/Entity/ConfigEntityStorage.php
    @@ -345,15 +336,49 @@ public function importDelete($name, Config $new_config, Config $old_config) {
    +  /**
    +   * @param array $values
    +   * @return ConfigEntityInterface
    +   */
    +  public function createFromStorageRecord(array $values) {
    ...
    +  /**
    +   * @param ConfigEntityInterface $entity
    +   * @param array $values
    +   * @return ConfigEntityInterface
    +   */
    +  public function updateFromStorageRecord(ConfigEntityInterface $entity, array $values) {
    

    Needs proper documentation and we probably have to add them to ImportableEntityStorageInterface?

  2. +++ b/core/lib/Drupal/Core/Config/Entity/ConfigEntityStorage.php
    @@ -345,15 +336,49 @@ public function importDelete($name, Config $new_config, Config $old_config) {
    +    // No preCreate is run? Ho hum.
    +    // No UUID is created. Ho hum.
    +    $data = $this->mapFromStorageRecords(array($values));
    +    $entity = current($data);
    

    Not sure what the comments are trying to say :)

  3. +++ b/core/modules/field/src/Tests/FieldImportDeleteUninstallUiTest.php
    @@ -102,6 +102,16 @@ public function testImportDeleteUninstall() {
    +    // Delete all the text fields in staging, entity_test_install() adds quite
    +    // a few.
    +    foreach (\Drupal::entityManager()->getFieldMap() as $entity_type => $fields) {
    

    Yeah, we have tons of entity types now there, wondering if we should solve that differently and only add the field when it is needed by tests. Not here, obviously.

alexpott’s picture

Status: Needs work » Needs review
FileSize
13.3 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 76,359 pass(es). View
3.72 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 76,345 pass(es), 2 fail(s), and 0 exception(s). View

Added a test for importing list_float fields.

re: #16

  1. Moved to ConfigEntityStorageInterface since ConfigInstaller is where these are externally relied upon - hmmm maybe we need a module to test creating a list_float field using default config too.
  2. Notes whilst writing the patch :)
  3. Yep

The last submitted patch, 17: 2310093.test-only.patch, failed testing.

alexpott’s picture

Issue tags: -Needs tests
FileSize
13.26 KB
21.49 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 76,318 pass(es), 3 fail(s), and 0 exception(s). View

Improved the test to cover the config installer too.

Status: Needs review » Needs work

The last submitted patch, 19: 2310093.19.patch, failed testing.

alexpott’s picture

Status: Needs work » Needs review
FileSize
3.37 KB
23.39 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 76,594 pass(es), 12 fail(s), and 0 exception(s). View

Interesting fail - floats can be integers - we need to update SchemaCheckTrait!

The other fails are because the config was generate before #2313757: Remove text_processing option from text fields, expose existing string field types as plain text in UI

Status: Needs review » Needs work

The last submitted patch, 21: 2310093.21.patch, failed testing.

alexpott’s picture

Status: Needs work » Needs review
Issue tags: +Needs issue summary update
FileSize
12.76 KB
30.61 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 76,665 pass(es), 1 fail(s), and 0 exception(s). View

So... we have a problem comparing configuration that contains floats which have integer values. Patch contains a solution to do cast the source config data using schema to ensure they don't match.

Status: Needs review » Needs work

The last submitted patch, 23: 2310093.23.patch, failed testing.

alexpott’s picture

Status: Needs work » Needs review
FileSize
743 bytes
30.61 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 79,041 pass(es), 4 fail(s), and 1 exception(s). View

Fixed tests.

Status: Needs review » Needs work

The last submitted patch, 25: 2310093.25.patch, failed testing.

Status: Needs work » Needs review

alexpott queued 25: 2310093.25.patch for re-testing.

alexpott’s picture

Here is a better upstream fix for the storage comparer issue - https://github.com/symfony/symfony/pull/11976

The issue we have to best illustrated by the following code:

$yaml = new \Symfony\Component\Yaml\Yaml();

$expected = array('float' => (float) 1);
$test = $yaml->parse($yaml->dump($expected));

if ($expected === $test) {
  print "match!\n";
}
else {
  var_dump($test);
}

which outputs

array(1) {
  'float' =>
  int(1)
}

Our float has become an integer!

catch’s picture

Issue tags: +beta target, +D8 upgrade path

h3rj4n queued 25: 2310093.25.patch for re-testing.

Status: Needs review » Needs work

The last submitted patch, 25: 2310093.25.patch, failed testing.

hussainweb’s picture

Status: Needs work » Needs review
FileSize
2.19 KB
30.43 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 79,090 pass(es). View

I am trying to fix the tests. The problem is that the property names have been changed:

  • default_value_function -> default_value_callback
  • name -> field_name

The interdiff shows these changes.

alexpott’s picture

FileSize
9.03 KB
23.78 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 79,104 pass(es). View

We have to remove all the changes to ConfigImporter and StorageComparer now that floats are properly persisted by the Yaml Component- now that #2350917: Update Symfony YAML library to support whole number floats landed.

pseudo-Interdiff back to #25 since I had to revert stuff from my local branch

Berdir’s picture

Issue summary: View changes
Status: Needs review » Reviewed & tested by the community
+++ b/core/lib/Drupal/Core/Config/Entity/ConfigEntityStorage.php
@@ -392,15 +383,44 @@ public function importDelete($name, Config $new_config, Config $old_config) {
+    $entity->postCreate($this);
+
+    // Modules might need to add or change the data initially held by the new
+    // entity object, for instance to fill-in default values.
+    $this->invokeHook('create', $entity);

Wondering why the postCreate() is not below the comment as well, but I assume that this was copied from somewhere?

This looks good to me, I think a change record is not needed, maybe we can explain the new methods somewhere in an existing one about config entities (which are probably all hopelessly outdated ;))

catch’s picture

Status: Reviewed & tested by the community » Fixed

Committed/pushed to 8.0.x.

Assuming the comment placement is just referring to the hook, we can follow-up to move it if we want.

  • catch committed ac9054b on 8.0.x
    Issue #2310093 by alexpott, hussainweb : Fixed Config install and import...
Gábor Hojtsy’s picture

Yayayayay. Can finally ship option fields in modules/profiles. This has been an issue with the D8 multilingual demo profile too :) Thanks folks!

Status: Fixed » Closed (fixed)

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

BR0kEN’s picture

Version: 8.0.x-dev » 8.3.x-dev
Status: Closed (fixed) » Active

Now this problem appears in 8.2.0-rc1 and this is really annoying. Could someone confirm this?

alexpott’s picture

Version: 8.3.x-dev » 8.0.x-dev
Status: Active » Closed (fixed)

@BR0kEN rather than opening an issue that was fixed 2 years ago can you please open a new issue that describes the exact steps to reproduce what you are seeing?

BR0kEN’s picture

Did, @alexpott. Waiting for your opinion here: https://www.drupal.org/node/2802379