Do not import entity IDs (included in exports from Drupal 8.1.x)

Another case here when entity has no canonical URL...

#2... as in #2700723: Paragraphs and Default Content

Just got bit by this myself.

Patch should be pretty simple.
Instead of relying on "enforceIsNew", we can simply unset the id and revision keys, as applicable, immediately before saving entities during ::importContent

If we're actually importing duplicate content, we'll get a UUID collision, so intended behavior remains intact.

In this patch:
- Updated to fix failing tests (missing getDefinition() call on entityManager)
- New test coverage for conflicting ids.

- and forgot the ->save() call

Entity::create is fluent.
Entity::save is not.
Simpletest is all but impossible to work with locally.

+++ b/src/DefaultContentManager.php
@@ -219,7 +219,14 @@ class DefaultContentManager implements DefaultContentManagerInterface {
+          unset($entity->{$keys['id']});
...
+            unset($entity->{$keys['revision']});
...
           $entity->save();

btw But what about to use if ($entity->validate()) save(); else solve(); instead of save?

1. +++ b/src/DefaultContentManager.php
   @@ -219,7 +219,14 @@ class DefaultContentManager implements DefaultContentManagerInterface {
   +          // Unset entity keys to avoid collisions. UUID will conflict will
   

   English needs some work here - will conflict will?

2. +++ b/tests/src/Functional/DefaultContentTest.php
   @@ -51,4 +53,35 @@ class DefaultContentTest extends BrowserTestBase {
   +    $this->drupalLogin($this->drupalCreateUser(array_keys(\Drupal::moduleHandler()->invokeAll(('permission')))));
   

   drupalCreateUser has a third argument 'admin user' which you can use instead here

3. +++ b/tests/src/Functional/DefaultContentTest.php
   @@ -51,4 +53,35 @@ class DefaultContentTest extends BrowserTestBase {
   +    $conflict_node = $this->createNode([
   

   Should this get saved?

4. +++ b/tests/src/Functional/DefaultContentTest.php
   @@ -51,4 +53,35 @@ class DefaultContentTest extends BrowserTestBase {
   +    $conflict_term = Term::create([
   

   I'd like to see a similar test with a user, where the username or email already exists. Because there are unique constraints on those fields, we need to prevent them from being created/prevent the SQL error. Which would align with @andypost's comment about calling ::validate()

createNode() (via NodeCreationTrait) calls save(). There's an analogous TermCreationTrait in ctools, but I figured better not to add a dependency.

if ($entity->validate()) save(); else solve();

Are you saying, in other words, only unset the entity keys if we fail validation?

I'd like to see a similar test with a user, where the username or email already exists. Because there are unique constraints on those fields, we need to prevent them from being created/prevent the SQL error. Which would align with @andypost's comment about calling ::validate()

I agree with this sentiment, but do we just try/catch around entity save()?
Feels like it's out of scope to try and resolve email or username collision.

in this patch:
- new "user" directory in default_content_test
- user2.json and user2fail.json:
- moved admin account creation and login into DefaultContentTest::setUp
- add test that user2.json uid != 2 (even though it's specified in the json file)
- add test that user2fail.json doesn't get imported (because of conflicting email address)
- add validate check prior to saving entity during DefaultContentManager::importContent
- add skip entities which don't validate
- add exception handling around entity::save() during ::importContent

open questions:
- do we actually want to catch entity save exceptions during import?
-- if so, do we need a new ImportException event or similar?
- do we need similar accommodation for validate failure?

Patch has some test-fails, but works fine in my manual test.

I'm not sure about the approach taken though. I'd be cautious with changing all entity-key values - let's better hard-code the changing to IDs and revision-IDs only. Other entity keys should not be changed.

hard-code the changing to IDs and revision-IDs only.

Sorry I didn't make it clear in the description, but the only entity keys getting ignored are id and revision:

+            unset($entity->{$keys['id']});
+            if (isset($keys['revision'])) {
+              unset($entity->{$keys['revision']});
+            }

I'm trying to get the tests to pass, but simpletest is killing me.
I'll keep plugging, but anyone else feel free to take a stab

This is kinda now covered by #2698425-42: Do not reimport existing entities. Suggest moving credits and closing as duplicate...?

IMO anyway.

Working on a related patch in Better Normalizers: #2913001: Add option to exclude local ID and revision ID values. Rather than ignoring on import, this would remove local ID values on export.

I am not sure why this is closed but I recently ran into the same issue and " #2698425-42: Do not reimport existing entities. " does not address this issue when you already have nodes with the same IDs exported from a different env. For example, export nodes locally, and one node would have an ID = 123. If this id already exist on a different en then the import fails.

Below is a patch a re-relled based on patch #14:

As suggested in #20, and in the workaround mentioned on #2969631: NID / VID / TID conflicts in branches, would it make more sense to prevent the entity IDs being included in the export?

I've recently created the 2.0.x branch, see the project page on all the improvements in the 2.0.x branch. Testing that and providing feedback would be very welcome. The 1.x branch isn't actively maintained and won't receive new features anymore, so I'm closing this and other issues as won't fix.

2.0.x uses a completely different normalization implementation that does not depend on hal.module anymore and skips all ID fields.