Avoid storing numeric target_id's for default values in field instance CMI files

@larowlan : that's nothing that gets exported in CMI files ?

Thanks @plopesc! That's totally the idea.
Review below.

1. +++ b/core/modules/entity_reference/lib/Drupal/entity_reference/Plugin/field/field_type/ConfigurableEntityReferenceField.php
   @@ -0,0 +1,64 @@
   +
   +
   

   one empty line too many

2. +++ b/core/modules/entity_reference/lib/Drupal/entity_reference/Plugin/field/field_type/ConfigurableEntityReferenceField.php
   @@ -0,0 +1,64 @@
   +class ConfigurableEntityReferenceField extends ConfigField {
   

   Missing a phpdoc

3. +++ b/core/modules/entity_reference/lib/Drupal/entity_reference/Plugin/field/field_type/ConfigurableEntityReferenceField.php
   @@ -0,0 +1,64 @@
   +    $stored_value = $this->getFieldDefinition()->getFieldDefaultValue($this->getEntity());
   +    // If retrieved value is NULL, exit.
   +    if (!isset($stored_value)) {
   +      return $stored_value;
   +    }
   +
   +    // Process values, converting uuids on their corresponding target_id.
   +    $default_value = array();
   

   I think we should start with $default_value = parent::getDefaultValue(), then iterate and replace uuids if we find some.

   Also note: core standardizes on capital letters in comments : UUID, UUIDs (comments only, not property names or variable names)

4. +++ b/core/modules/entity_reference/lib/Drupal/entity_reference/Plugin/field/field_type/ConfigurableEntityReferenceField.php
   @@ -0,0 +1,64 @@
   +      if (isset($item['uuid'])) {
   

   since the original property is 'target_id', I'd vote for 'target_uuid' ?

5. +++ b/core/modules/entity_reference/lib/Drupal/entity_reference/Plugin/field/field_type/ConfigurableEntityReferenceField.php
   @@ -0,0 +1,64 @@
   +        $entity = entity_load_by_uuid($target_type, $item['uuid']);
   

   is there a way to do a multiple-load ? it *seems* StorageController::loadByProperties(array('uuid' => array(several uuids)); will load all the entities in one single multiple-load.

6. +++ b/core/modules/entity_reference/lib/Drupal/entity_reference/Plugin/field/field_type/ConfigurableEntityReferenceField.php
   @@ -0,0 +1,64 @@
   +        $default_value[] = array('target_id' => $entity->id(), 'revision_id' => $entity->getRevisionId());
   

   'revision_id' is more complex, we cannot just ditch it on submit and use the "current revision" of the entity associated to the uuid on load.

   However, revisions don't have UUIDs, so I'm not sure how we can support referencing a specific revision in entity_reference field...

7. +++ b/core/modules/entity_reference/lib/Drupal/entity_reference/Plugin/field/field_type/ConfigurableEntityReferenceField.php
   @@ -0,0 +1,64 @@
   +    $default_value = array();
   +    $target_type = $this->getFieldDefinition()->getFieldSetting('target_type');
   +    // Extract the submitted value.
   +    $widget = $this->defaultValueWidget($form_state);
   +    $widget->extractFormValues($this, $element, $form_state);
   +    $value = $this->getValue();
   

   Similarly, this should be parent::defaultValuesFormSubmit()

8. +++ b/core/modules/entity_reference/lib/Drupal/entity_reference/Plugin/field/field_type/ConfigurableEntityReferenceField.php
   @@ -0,0 +1,64 @@
   +    foreach ($value as $index => $item) {
   +      $entity = entity_load($target_type, $item['target_id']);
   

   This should use entity_load_multiple(), though - and preferably, \Drupal->entityManager()->getStorageController($target_type)->loadMultiple();
   First, iterate on values to collect ids, then load the entities, then iterate again to assign uuids from the loaded entities.

9. +++ b/core/modules/entity_reference/lib/Drupal/entity_reference/Plugin/field/field_type/ConfigurableEntityReferenceField.php
   @@ -0,0 +1,64 @@
   +  }
   +}
   

   Minor: missing empty line after the last method of a class

10. +++ b/core/modules/entity_reference/lib/Drupal/entity_reference/Plugin/field/field_type/ConfigurableEntityReferenceField.php
    @@ -0,0 +1,64 @@
    \ No newline at end of file
    

    should not happen :-)

Thanks for being so quick ;-)

1. +++ b/core/modules/entity_reference/lib/Drupal/entity_reference/Plugin/field/field_type/ConfigurableEntityReferenceField.php
   @@ -0,0 +1,75 @@
   +    foreach ($default_value as $index => $item) {
   

   Use $delta rather than $index, for consistency with the rest of core.
   $item might be misleading, since, *unlike* everywhere else in core, this is an array and not a Field (list of FieldItem's) object.
   Maybe $properties ?

2. +++ b/core/modules/entity_reference/lib/Drupal/entity_reference/Plugin/field/field_type/ConfigurableEntityReferenceField.php
   @@ -0,0 +1,75 @@
   +        $uuids[] = $item['target_uuid'];
   +        $indexes[] = $index;
   

   If you key $uuids by delta, you shouldn't need the separate $indexes array.

3. +++ b/core/modules/entity_reference/lib/Drupal/entity_reference/Plugin/field/field_type/ConfigurableEntityReferenceField.php
   @@ -0,0 +1,75 @@
   +    if (empty($uuids)) {
   +      return $default_value;
   

   Matter of taste, but I'd rather wrap the rest of the code in if ($uuids) { rather than exit early. Exit at the end, less code paths to figure out mentally.

4. +++ b/core/modules/entity_reference/lib/Drupal/entity_reference/Plugin/field/field_type/ConfigurableEntityReferenceField.php
   @@ -0,0 +1,75 @@
   +    foreach ($indexes as $index => $item) {
   +      $entity = current($entities);
   

   I don't think we can rely on the order of the $entities array returned by loadByProperties(), the current implementation might preserve the order, but it's not documented anywhere and thus could change at any given time.

   So IMO we need to iterate on entities first, to build an $ids array of (uuid => id). Then,

   foreach ($ûuids as $delta => $uuid) {
    $default_value[$delta] = array('target_id' => $ids[$uuid], ...)
   }
   

5. +++ b/core/modules/entity_reference/lib/Drupal/entity_reference/Plugin/field/field_type/ConfigurableEntityReferenceField.php
   @@ -0,0 +1,75 @@
   +    foreach ($default_value as $index => $item) {
   

   same as above, $delta => $properties

6. +++ b/core/modules/entity_reference/lib/Drupal/entity_reference/Plugin/field/field_type/ConfigurableEntityReferenceField.php
   @@ -0,0 +1,75 @@
   +      $items[] = $item['target_id'];
   

   misleading var name - $ids instead of $items

7. +++ b/core/modules/entity_reference/lib/Drupal/entity_reference/Plugin/field/field_type/ConfigurableEntityReferenceField.php
   @@ -0,0 +1,75 @@
   +      $entity = $entities[$item['target_id']];
   +      $uuid = $entity->uuid();
   

   Minor, but the intermediate $entity variable could go away, $uuid = $entities[$item['target_id']]->uuid() is fine.
   Also, the if(isset($uuid)) could use a comment about some entities not having UUIDs (and why it's not an issue - if that's not an issue ?)

Multiple load in getDefaultValue() makes code a bit weird IMHO

We can streamline it a bit (see remarks above) - and multiple loading is fairly important for performance, not doing it when we can could qualify as a bug.

I think we could split the code in getDefaultValue() and defaultValuesFormSubmit() to try to keep it cleaner, what do you think?

Let's see how the final code looks like, but I don't think that should be needed. Let's avoid cluttering the class with a separate helper method unless we need to.

1. +++ b/core/modules/entity_reference/lib/Drupal/entity_reference/Plugin/field/field_type/ConfigurableEntityReferenceField.php
   @@ -0,0 +1,76 @@
   +    // If retrieved default_value is not NULL, we can massage it.
   +    if ($default_value) {
   +      // Process values, converting UUIDs on their corresponding target_id.
   

   First comment is not really needed, The second one could be moved above the if(). Also, "process" is vague &nd doesn't add much. Just "Convert UUIDs to numeric IDs." ?

   Leave one empty line after the parent call, like you do in the other method ? (consistency ++)

2. +++ b/core/modules/entity_reference/lib/Drupal/entity_reference/Plugin/field/field_type/ConfigurableEntityReferenceField.php
   @@ -0,0 +1,76 @@
   +    // Process values and store depending on UUID existence.
   

   "Convert numeric IDs to UUIDs to ensure config deployability" ?

3. +++ b/core/modules/entity_reference/lib/Drupal/entity_reference/Plugin/field/field_type/ConfigurableEntityReferenceField.php
   @@ -0,0 +1,76 @@
   +      // Some entities does not have uuid. It's not an issue?
   +      // If it's not an issue, why?
   

   This should be left as a @todo then :-).
   And we need to solve that. What were those cases of entities not having a UUID ?
   + language : s/does/do

4. +++ b/core/modules/entity_reference/lib/Drupal/entity_reference/Plugin/field/field_type/ConfigurableEntityReferenceField.php
   @@ -0,0 +1,76 @@
   +        $default_value[$delta] = array('target_uuid' => $uuid);
   

   Actually I don't think we should overwrite the value completely, just set 'target_uuid' and unset 'target_id', leaving the rest untouched, whatever that may be now or in the future - and same in getDefaultValue(). The job here is UUID <-> ID, nothing more.

Regarding revision_id, I think the current UI doesn't let you select a specific revision anyway, so the revision_id entry will be empty. Let's just not care about that. How e_r can reference a specific revision is not fully clear to me, and this can be adressed in a followup.

Regarding namespace / folder for the Field class, I do think it's generally preferable to keep Field classes next to the FieldItem classes, both are really related. Putting non plugin classes in plugin discovery folders is fine, that's what we do with base classes for widgets and formatters. Sensible code organization beats minor overhead on plugin cache miss IMO.

So, regarding users 0 and 1:
I opened #2092603: Users 0 and 1 have no UUID about the missing UUIDs. Meanwhile, we could special case here and use 'anonymous' resp. 'administrator' as 'target_uuid' strings for user 0 resp. 1 ? (only if target_type is user of course) - with a @todo to clean this up after that other issue is fixed. Will make the methods a bit more convoluted, but that should be temporary...

I'm not actually sure entity_ref allows referencing the anonymous user, BTW. @amateescu ?

Also, we should add tests:
- Create an entity_ref field / instance through Field UI, enter a default value in the form (necessarily with a numeric id), check that the CMI record contains a UUID
- field_info_cache_clear()
- Create a fresh entity (with entity_create()), and check that it correctly receives the default value (field has the expected numeric target_id)

@amateescu: ok - it's just that HEAD gives me an "Integrity constraint violation: 1062 Duplicate entry '' for key 'name': INSERT INTO {users}" fatal when I try to create a node referencing user 0 (using a select widget) :-). Is it a known bug or should I open an issue for that ?

entity_create() should absolutely initialize fields with their default values - and according to a quick test, this does work in today's HEAD.

Well, on current HEAD:
- in Field UI, edit 'body' field instance on article nodes, and add a default value there
- then entity_create('node', array('type' => 'article'))->get('body')->getValue(); does return the default values I defined, meaning they get correctly assigned on a fresh entity.

No need for an upgrade path for entity_reference fields, since ER is a D8 addition.
But taxonomy_term_reference fields will need an upgrade function, yes.

At this point in the D8 cycle, we don't provide upgrades for older D8 versions, only for D7

I don't think I see how #2015687: Convert field type to FieldType plugin for taxonomy module is related, or why it should postpone this issue ? Our case here is only about updating the content of field instance definitions that come from D7, and #2015687: Convert field type to FieldType plugin for taxonomy module will not change anything in this area.

taxonomy needs to provide a new update function, and make sure that it runs after field_update_8003() (using hook_update_dependencies() - look at how file_update_dependencies() does it)

The code should be something like this:
(@todo in the middle: write an SQL query to fetch the UUID of a given term_id)

foreach (config_get_storage_names_with_prefix('field.field.') as $config_name) {
  $field_config = \Drupal::config($config_name);
  if ($field_config->get('type') == 'taxonomy_term_reference') {
    foreach (config_get_storage_names_with_prefix('field.instance.' . $field_config->get('entity_type')) as $instance_config_name) {
      $parts = explode('.', $config_name);
      $field_name = end($parts);
      if ($field_name == $field_config->get('name')) {
        $instance_config = \Drupal::config($instance_config_name);
        $default_value = $instance_config->get('default_value');
        foreach ($default_value as $delta => &$value) {
          if (isset($value['tid']) {
            // @todo find the corresponding $uuid
            $value['uuid'] = $uuid
            unset($value['tid'];
          }
        }
        $instance_config->set('default_value', $default_value);
      }
    }
  }
}

[edit: simplified the code a bit]

(Note that taxonomy.module did not provide an update to rename 'tid' to target_id' in default values in $instance config so far, so the D7 upgrade was broken on that regard so far)

Going afk for the next two weeks, I'll let you guys drive this home :-)

Yay, the tests are awesome !
Minor remarks below.

1. +++ b/core/lib/Drupal/Core/Field/LegacyFieldTypeDiscoveryDecorator.php
   @@ -63,10 +63,13 @@ public function getDefinitions() {
   -          $definition['id'] = $plugin_id;
   -          $definition['provider'] = $module;
   -          $definition['configurable'] = TRUE;
   -          $definition['list_class'] = '\Drupal\Core\Field\Plugin\Field\FieldType\LegacyConfigFieldItemList';
   +          $plugin_defaults = array(
   +            'id' => $plugin_id,
   +            'provider' => $module,
   +            'configurable' => TRUE,
   +            'list_class' => '\Drupal\Core\Field\Plugin\Field\FieldType\LegacyConfigFieldItemList',
   +          );
   +          $definition += $plugin_defaults;
   

   Agreed with the workaround for 'list_class', but we should keep the other properties forced - just $definition += array('list_class' => ...);

2. +++ b/core/modules/entity_reference/lib/Drupal/entity_reference/Plugin/Field/FieldType/ConfigurableEntityReferenceFieldItemList.php
   @@ -0,0 +1,87 @@
   +      // @todo Some entities do not have uuid. IE: Anonymous and admin user.
   +      //   Remove this exception code once #2050843 has been fixed.
   

   Minor: we usually put full issues URLs rather that #1234 markers in comments. Also, this is not an exception (PHP sense) - "Remove this special case ..." ?

3. +++ b/core/modules/entity_reference/lib/Drupal/entity_reference/Tests/EntityReferenceFieldDefaultValueTest.php
   @@ -0,0 +1,92 @@
   +  function testEntityReferenceDefaultValue() {
   

   Missies a phpdoc

4. +++ b/core/modules/entity_reference/lib/Drupal/entity_reference/Tests/EntityReferenceFieldDefaultValueTest.php
   @@ -0,0 +1,92 @@
   +    // Create some content to be referenced.
   ...
   +    // Set created content as default_value.
   

   Minor: in code comments, refer to a node as "node" rather than as "content" (when it's a node...) - clearer :-)

Also:

+++ b/core/modules/entity_reference/lib/Drupal/entity_reference/Plugin/Field/FieldType/ConfigurableEntityReferenceFieldItemList.php
@@ -0,0 +1,87 @@
+        foreach ($uuids as $delta => $uuid) {
+          $default_value[$delta]['target_id'] = $entity_ids[$uuid];
+          unset($default_value[$delta]['target_uuid']);
+        }

We cannot exclude the fact that we might receive 'target_uuids' that do not exist on the site - $entity_ids[$uuid] won't be set.
In that case, we should unset($default_value[$delta]) - and re-number $default_value at the end to make sure we leave no wholes in deltas (just return array_values($default_value))

1. +++ b/core/modules/entity_reference/lib/Drupal/entity_reference/Plugin/Field/FieldType/ConfigurableEntityReferenceFieldItemList.php
   @@ -0,0 +1,100 @@
   +      // Converting UUIDs to numeric IDs.
   

   Minor, phrasing: "Convert" rather than "Converting" ?

2. +++ b/core/modules/entity_reference/lib/Drupal/entity_reference/Plugin/Field/FieldType/ConfigurableEntityReferenceFieldItemList.php
   @@ -0,0 +1,100 @@
   +      if (!empty($default_value)) {
   +        $default_value = array_values($default_value);
   +      }
   +      else {
   +        $default_value = NULL;
   +      }
   

   Do we really need the empty / !empty cases ?
   Just something like
   // Ensure we return sequential deltas, in case we had to remove unknown UUIDs.
   $default_values = array_values($default_value);
   should work ?

3. +++ b/core/modules/entity_reference/lib/Drupal/entity_reference/Tests/EntityReferenceFieldDefaultValueTest.php
   @@ -0,0 +1,96 @@
   +   * Tests that entity_reference fields default values are stored in the
   +   * config entity.
   

   We try to keep those < 80 chars when possible.
   Since we are in a class that tests entity_reference, maybe we can avoid repeating that here ? Something like: "Tests that default values are correctly translated to UUIDs in config." ?

After that, we should really be there ;)

Yes, returning an empty array is just fine, that's what happens for regular field types on a field instance where no default value is configured (the CMI file contains "default_value: empty array").

I'd just vote to add the comment suggested in #51 above the array_values() line, because it's not frankly intuitive otherwise.
(// Ensure we return sequential deltas, in case we had to remove unknown UUIDs.)

Minor, other than that, RTBC; Thanks !

AFAIK, storing UUIDs instead of numeric local IDs for entity ref field values has been discussed long ago in the entity_reference contrib queue, and dismissed as impacting perf (JOINs if joining on strings rather than ints ?)

Storing / accepting both on input: dunno is this has been discussed, but not sure it would really be simpler.

JOINs are mostly leveraged by views relationships. But WHEREs would be impacted too, and they appear a lot with filters / contextual filters.

But yes, I think storing UUIDs is a whole discussion in itself. If/when this happens, the code added here will be easy to adapt/revert. Meanwhile, let's do this, coz config deployability is screwed without it.

Opened #2129583: Consider storing UUIDs instead of / in addition to local ids for entity_reference fields ?