Config install and import should map from storage record not set properties directly

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

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

Single config import is broken too but works differently

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
                        )

                )

        )

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.

Fixed the test.

Hopefully cleaner title for this issue.

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.

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

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?

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.

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

Improved the test to cover the config installer too.

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

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.

Fixed tests.

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!

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.

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

+++ 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 ;))

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.

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

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

@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?

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