Use uuid as GUID for Drupal entities

You are right. A target for the UUID of an entity is missing. Do you want to implement one?

I've tried in the past to make a generic FeedsTarget plugin to make all fields available but at the time I ran into the issue that you couldn't override that generic plugin with a specific one. So currently for every field type there needs to be a specific FeedsTarget plugin.

Some FeedsTarget plugins do more than just setting a value. They also try to "correct" the data or provide configuration for how to interpret the incoming data. For example: the number target trims the incoming data. The date target converts the data to a DrupalDateTime object and then to the storage format being used by the date field. So not for every field just only passing a value is sufficient.

I tried to set pointers in the "Remaining tasks" section of the issue summary. Does that help or do you need more information?

I already have a proof of concept working. Was late in a day yesterday to start on issue on the topic, so thanks Jonathan!

Will finilise that plugin and look at test now.

Added UUID target. Manually tested on nodes and terms, but I don't see any reason it not working on other entity types as it is field type specific, not entity type.

Implemented only unit tests , but I believe they cover most important bit - validation of the incoming value. Setting the value on destination entity is out of scope for added plugin and so is testing of this functionality.

Leaving "Remaining tasks" unchanged for now. Could be updated later, if proposed changes are acceptable.

Thanks for working on this, @RoSk0

1. +++ b/src/Feeds/Target/Uuid.php
   @@ -0,0 +1,63 @@
   +  /**
   +   * {@inheritdoc}
   +   */
   +  public function isMutable() {
   +    // Overriding parent to be able to populate the property,
   +    // otherwise, UUID is generated for new entity and will not be overridden.
   +    // @see \Drupal\feeds\Feeds\Processor\EntityProcessorBase::map().
   +    return TRUE;
   +  }
   

   I see that Drupal Core has explicitly set the UUID field to be read only in ContentEntityBase, so I assume that means that Drupal doesn't allow the UUID value to ever change. But I must admit that I don't understand the meaning of a field being declared as "read only" when its value can still be mutated. When searching for ->isReadOnly() in core's code base I don't see it actively being used.

2. +++ b/src/Feeds/Target/Uuid.php
   @@ -0,0 +1,63 @@
   +    $values['value'] = trim($values['value']);
   +    if (empty($values['value'])) {
   +      unset($values['value']);
   +      throw new TargetValidationException($this->t('UUID value cannot be empty'));
   +    }
   

   I wonder if we should allow empty UUID values. That if empty, the system would fallback on generating the UUID instead.

   But maybe you would want to be informed if an UUID value is missing if you meant to set it. Hm...
   Well, we could always make it configurable on the target: we could add a setting "Require source to set a UUID value".

3. Setting the value on destination entity is out of scope for added plugin and so is testing of this functionality.

   I do think we need test coverage for setting the value on the entity. Especially since we now allow the target to change a read only field: we want to ensure that updating a read only field doesn't result into errors.

Ah, I see the issue with the UUID field now. A value gets set immediately upon creation in ContentEntityStorageBase:

  /**
   * {@inheritdoc}
   */
  public function create(array $values = []) {
    $bundle = $this->getBundleFromValues($values);
    $entity_class = $this->getEntityClass($bundle);
    // @todo Decide what to do if preCreate() tries to change the bundle.
    // @see https://www.drupal.org/project/drupal/issues/3230792
    $entity_class::preCreate($this, $values);

    // Assign a new UUID if there is none yet.
    if ($this->uuidKey && $this->uuidService && !isset($values[$this->uuidKey])) {
      $values[$this->uuidKey] = $this->uuidService->generate();
    }

    $entity = $this->doCreate($values);
    $entity->enforceIsNew();

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

    return $entity;
  }

And EntityProcessorBase skips it if a value has already been set:

      // Skip immutable targets for which the entity already has a value.
      if (!$plugin->isMutable() && !$plugin->isEmpty($feed, $entity, $mapping['target'])) {
        continue;
      }

Maybe the isMutable() override should instead check if the entity is new and in that case allow the UUID to be set?

isMutable() doesn't have access to the $entity in the way isEmpty() does. We can of course abuse isEmpty() to check that the $entity is new and allow to set the value only in this case.

we want to ensure that updating a read only field doesn't result into errors.

The whole purpose of been able to use UUID is to be a unique identifier as it is. Unique identifiers are not expected to change - another identifier is another item.

I manually tested what would happen if I try to change UUID as it stands. The behaviour is based purely on the feed type configuration. Specifically , if both NID and UUID are present in mapping and both are marked as unique, than the first one on a list wins. So, if the first one in the mapping would be NID than it wins and it's possible to update UUID. It is actually still possible to update UUID if the UUID is first on the mapping list, because new UUID would not be matched buy DB lookup and NID will, but it feels like allowing to change UUID is increasing the surface of user error and works against core indication of UUID field being read only.

Would you be OK to abuse isEmpty() as suggested above to avoid UUID updates?

Suggested in #11 abuse implemented.

Please review and provide feedback.

It really looks like more higher level testing is required - the change in #12 actually broke plugin functionality and now one will see "UUID (uuid): UUID: this field cannot hold more than 1 values" trying import content with UUID set.

Will look into fixing it and adding some functional testing.

Code is fixed. Added additional unit test for the case.

Kernel tests are still in progress, will add when finished.

Kernel test added.

Thanks for the update! I'm going holiday today, so it could take a few weeks before I can properly review this.

Additional testing by business indicated there is a bug in the implementation.

However it looks like we all got confused about what is expected and what is not. And that is reflected in the implemented kernel test. Bug report says that where CSV item doesn't have UUID filled in it should be skipped as non-conformant , but in reality this is not true, latest implementation revision in #12 made that possible. UUID could be used as unique identifier and still be optional because it is generated in the new entities.

I'm working on a kernel test update to reflect this.

However, I believe that the biggest problem is possible usage of multiple fields as unique identifiers.
During debugging it I uncovered that if we configure feed type to:

1. use UUID as unique identifier AND
2. map UUID into GUID AND and use GUID as unique identifier AND have it first on the mapping page AND
3. there will be two or more items with UUID left blank

first item with the missing UUID will be stored with empty GUID and next one with the empty UUID will be treated as an update to the previous one, if the updates are allowed.

It feels like mapping page should recommend to have only one definition marked unique. @MegaChriz thoughts?

• Updated tests
• Improved UX by not showing 'UUID value cannot be empty' message, entities could be imported without UUID
• Removed GUID from test mapping based on the findings in #17 and #11

Tagging for BADCamp. It would be good to manual test various cases for importing UUID's:

• Try to import an empty value for UUID.
• Try to import a valid UUID value.
• Try to import an invalid UUID value.
• Try to update an existing node where UUID is the unique target.
• Try to update an existing node where title is the unique target.

Getting php 8.1+ deprecation warning with the above patch

Deprecated function: trim(): Passing null to parameter #1 ($string) of type string is deprecated in Drupal\feeds\Feeds\Target\Uuid->prepareValue() (line 57 of /app/web/modules/contrib/feeds/src/Feeds/Target/Uuid.php)

Fixing php 8.1 deprecation warnings

Added an MR of the patch in #21 to make this easier to review and keep up to date.

I rewrote some of the comments for clarity, but the code looks good to me. All tests are passing, and I've been using this to import entities while specifying the UUIDs on a site for about a year now.

Although I created the MR, I did not write the original code (the MR was derived from patch #21), so I am setting this to RTBC based on my review and testing.

I merged the changes! Thanks all for contributing!