Do not reimport existing entities

Re-rolling patch using the right base to be added using composer.

Also removing call to deprecated \Drupal::entityManager()

Thanks

Not working for me. I'm using this module and exports in an installation profile. All of the nodes I exported are created by the admin (uid 1), so that user is exported as well. If I don't export that user, all nodes are given an anonymous author.

I'm getting:

Starting Drupal installation. This takes a while. Consider using the --notify global option.                                          [ok]
exception 'PDOException' with message 'SQLSTATE[23000]: Integrity constraint violation: 1062 Duplicate entry '1' for key 'PRIMARY'' in[error]
/var/www/docroot/core/lib/Drupal/Core/Database/Statement.php:59
Stack trace:
#0 /var/www/docroot/core/lib/Drupal/Core/Database/Statement.php(59): PDOStatement->execute(Array)
#1 /var/www/docroot/core/lib/Drupal/Core/Database/Connection.php(610): Drupal\Core\Database\Statement->execute(Array, Array)
#2 /var/www/docroot/core/lib/Drupal/Core/Database/Driver/mysql/Connection.php(81):
Drupal\Core\Database\Connection->query('INSERT INTO {us...', Array, Array)
#3 /var/www/docroot/core/lib/Drupal/Core/Database/Driver/mysql/Insert.php(32):
Drupal\Core\Database\Driver\mysql\Connection->query('INSERT INTO {us...', Array, Array)
#4 /var/www/docroot/core/lib/Drupal/Core/Entity/Sql/SqlContentEntityStorage.php(829):
Drupal\Core\Database\Driver\mysql\Insert->execute()
#5 /var/www/docroot/core/modules/user/src/UserStorage.php(27):
Drupal\Core\Entity\Sql\SqlContentEntityStorage->doSaveFieldItems(Object(Drupal\user\Entity\User), Array)
#6 /var/www/docroot/core/lib/Drupal/Core/Entity/ContentEntityStorageBase.php(263):
Drupal\user\UserStorage->doSaveFieldItems(Object(Drupal\user\Entity\User))
#7 /var/www/docroot/core/lib/Drupal/Core/Entity/EntityStorageBase.php(392):
Drupal\Core\Entity\ContentEntityStorageBase->doSave('1', Object(Drupal\user\Entity\User))
#8 /var/www/docroot/core/lib/Drupal/Core/Entity/Sql/SqlContentEntityStorage.php(747):
Drupal\Core\Entity\EntityStorageBase->save(Object(Drupal\user\Entity\User))
#9 /var/www/docroot/core/lib/Drupal/Core/Entity/Entity.php(358):
Drupal\Core\Entity\Sql\SqlContentEntityStorage->save(Object(Drupal\user\Entity\User))
#10 /var/www/docroot/modules/contrib/default_content/src/DefaultContentManager.php(230): Drupal\Core\Entity\Entity->save()
#11 /var/www/docroot/modules/contrib/default_content/default_content.module(15):
Drupal\default_content\DefaultContentManager->importContent('test')
#12 [internal function]: default_content_modules_installed(Array)
#13 /var/www/docroot/core/lib/Drupal/Core/Extension/ModuleHandler.php(402): call_user_func_array('default_content...',
Array)
#14 /var/www/docroot/core/lib/Drupal/Core/Extension/ModuleInstaller.php(305):
Drupal\Core\Extension\ModuleHandler->invokeAll('modules_install...', Array)
#15 /var/www/docroot/core/lib/Drupal/Core/ProxyClass/Extension/ModuleInstaller.php(83):
Drupal\Core\Extension\ModuleInstaller->install(Array, false)
#16 /var/www/docroot/core/includes/install.core.inc(1547): Drupal\Core\ProxyClass\Extension\ModuleInstaller->install(Array,
false)
#17 /var/www/docroot/core/includes/install.core.inc(652): install_install_profile(Array)
#18 /var/www/docroot/core/includes/install.core.inc(530): install_run_task(Array, Array)
#19 /var/www/docroot/core/includes/install.core.inc(115): install_run_tasks(Array)
#20 phar:///usr/local/bin/drush/includes/drush.inc(726): install_drupal(Object(Composer\Autoload\ClassLoader), Array)
#21 phar:///usr/local/bin/drush/includes/drush.inc(711): drush_call_user_func_array('install_drupal', Array)
#22 phar:///usr/local/bin/drush/commands/core/drupal/site_install.inc(80): drush_op('install_drupal',
Object(Composer\Autoload\ClassLoader), Array)
#23 phar:///usr/local/bin/drush/commands/core/site_install.drush.inc(247): drush_core_site_install_version('test', Array)
#24 [internal function]: drush_core_site_install('test')
#25 phar:///usr/local/bin/drush/includes/command.inc(366): call_user_func_array('drush_core_site...', Array)
#26 phar:///usr/local/bin/drush/includes/command.inc(217): _drush_invoke_hooks(Array, Array)
#27 [internal function]: drush_command('test')
#28 phar:///usr/local/bin/drush/includes/command.inc(185): call_user_func_array('drush_command', Array)
#29 phar:///usr/local/bin/drush/lib/Drush/Boot/BaseBoot.php(67): drush_dispatch(Array)
#30 phar:///usr/local/bin/drush/includes/preflight.inc(66): Drush\Boot\BaseBoot->bootstrap_and_dispatch()
#31 phar:///usr/local/bin/drush/includes/startup.inc(321): drush_main()
#32 phar:///usr/local/bin/drush/drush(114): drush_startup(Array)
#33 /usr/local/bin/drush(10): require('phar:///usr/loc...')
#34 {main}

Next exception 'Drupal\Core\Database\IntegrityConstraintViolationException' with message 'SQLSTATE[23000]: Integrity constraint
violation: 1062 Duplicate entry '1' for key 'PRIMARY': INSERT INTO {users} (uid, uuid, langcode) VALUES (:db_insert_placeholder_0,
:db_insert_placeholder_1, :db_insert_placeholder_2); Array
(
    [:db_insert_placeholder_0] => 1
    [:db_insert_placeholder_1] => ad3e8dda-8345-41de-8522-5805a82ce248
    [:db_insert_placeholder_2] => en
)
' in /var/www/docroot/core/lib/Drupal/Core/Database/Connection.php:668
Stack trace:
#0 /var/www/docroot/core/lib/Drupal/Core/Database/Connection.php(635):
Drupal\Core\Database\Connection->handleQueryException(Object(PDOException), 'INSERT INTO {us...', Array, Array)
#1 /var/www/docroot/core/lib/Drupal/Core/Database/Driver/mysql/Connection.php(81):
Drupal\Core\Database\Connection->query('INSERT INTO {us...', Array, Array)
#2 /var/www/docroot/core/lib/Drupal/Core/Database/Driver/mysql/Insert.php(32):
Drupal\Core\Database\Driver\mysql\Connection->query('INSERT INTO {us...', Array, Array)
#3 /var/www/docroot/core/lib/Drupal/Core/Entity/Sql/SqlContentEntityStorage.php(829):
Drupal\Core\Database\Driver\mysql\Insert->execute()
#4 /var/www/docroot/core/modules/user/src/UserStorage.php(27):
Drupal\Core\Entity\Sql\SqlContentEntityStorage->doSaveFieldItems(Object(Drupal\user\Entity\User), Array)
#5 /var/www/docroot/core/lib/Drupal/Core/Entity/ContentEntityStorageBase.php(263):
Drupal\user\UserStorage->doSaveFieldItems(Object(Drupal\user\Entity\User))
#6 /var/www/docroot/core/lib/Drupal/Core/Entity/EntityStorageBase.php(392):
Drupal\Core\Entity\ContentEntityStorageBase->doSave('1', Object(Drupal\user\Entity\User))
#7 /var/www/docroot/core/lib/Drupal/Core/Entity/Sql/SqlContentEntityStorage.php(747):
Drupal\Core\Entity\EntityStorageBase->save(Object(Drupal\user\Entity\User))
#8 /var/www/docroot/core/lib/Drupal/Core/Entity/Entity.php(358):
Drupal\Core\Entity\Sql\SqlContentEntityStorage->save(Object(Drupal\user\Entity\User))
#9 /var/www/docroot/modules/contrib/default_content/src/DefaultContentManager.php(230): Drupal\Core\Entity\Entity->save()
#10 /var/www/docroot/modules/contrib/default_content/default_content.module(15):
Drupal\default_content\DefaultContentManager->importContent('test')
#11 [internal function]: default_content_modules_installed(Array)
#12 /var/www/docroot/core/lib/Drupal/Core/Extension/ModuleHandler.php(402): call_user_func_array('default_content...',
Array)
#13 /var/www/docroot/core/lib/Drupal/Core/Extension/ModuleInstaller.php(305):
Drupal\Core\Extension\ModuleHandler->invokeAll('modules_install...', Array)
#14 /var/www/docroot/core/lib/Drupal/Core/ProxyClass/Extension/ModuleInstaller.php(83):
Drupal\Core\Extension\ModuleInstaller->install(Array, false)
#15 /var/www/docroot/core/includes/install.core.inc(1547): Drupal\Core\ProxyClass\Extension\ModuleInstaller->install(Array,
false)
#16 /var/www/docroot/core/includes/install.core.inc(652): install_install_profile(Array)
#17 /var/www/docroot/core/includes/install.core.inc(530): install_run_task(Array, Array)
#18 /var/www/docroot/core/includes/install.core.inc(115): install_run_tasks(Array)
#19 phar:///usr/local/bin/drush/includes/drush.inc(726): install_drupal(Object(Composer\Autoload\ClassLoader), Array)
#20 phar:///usr/local/bin/drush/includes/drush.inc(711): drush_call_user_func_array('install_drupal', Array)
#21 phar:///usr/local/bin/drush/commands/core/drupal/site_install.inc(80): drush_op('install_drupal',
Object(Composer\Autoload\ClassLoader), Array)
#22 phar:///usr/local/bin/drush/commands/core/site_install.drush.inc(247): drush_core_site_install_version('test', Array)
#23 [internal function]: drush_core_site_install('test')
#24 phar:///usr/local/bin/drush/includes/command.inc(366): call_user_func_array('drush_core_site...', Array)
#25 phar:///usr/local/bin/drush/includes/command.inc(217): _drush_invoke_hooks(Array, Array)
#26 [internal function]: drush_command('test')
#27 phar:///usr/local/bin/drush/includes/command.inc(185): call_user_func_array('drush_command', Array)
#28 phar:///usr/local/bin/drush/lib/Drush/Boot/BaseBoot.php(67): drush_dispatch(Array)
#29 phar:///usr/local/bin/drush/includes/preflight.inc(66): Drush\Boot\BaseBoot->bootstrap_and_dispatch()
#30 phar:///usr/local/bin/drush/includes/startup.inc(321): drush_main()
#31 phar:///usr/local/bin/drush/drush(114): drush_startup(Array)
#32 /usr/local/bin/drush(10): require('phar:///usr/loc...')
#33 {main}

Next exception 'Drupal\Core\Entity\EntityStorageException' with message 'SQLSTATE[23000]: Integrity constraint violation: 1062
Duplicate entry '1' for key 'PRIMARY': INSERT INTO {users} (uid, uuid, langcode) VALUES (:db_insert_placeholder_0,
:db_insert_placeholder_1, :db_insert_placeholder_2); Array
(
    [:db_insert_placeholder_0] => 1
    [:db_insert_placeholder_1] => ad3e8dda-8345-41de-8522-5805a82ce248
    [:db_insert_placeholder_2] => en
)
' in /var/www/docroot/core/lib/Drupal/Core/Entity/Sql/SqlContentEntityStorage.php:756
Stack trace:
#0 /var/www/docroot/core/lib/Drupal/Core/Entity/Entity.php(358):
Drupal\Core\Entity\Sql\SqlContentEntityStorage->save(Object(Drupal\user\Entity\User))
#1 /var/www/docroot/modules/contrib/default_content/src/DefaultContentManager.php(230): Drupal\Core\Entity\Entity->save()
#2 /var/www/docroot/modules/contrib/default_content/default_content.module(15):
Drupal\default_content\DefaultContentManager->importContent('test')
#3 [internal function]: default_content_modules_installed(Array)
#4 /var/www/docroot/core/lib/Drupal/Core/Extension/ModuleHandler.php(402): call_user_func_array('default_content...', Array)
#5 /var/www/docroot/core/lib/Drupal/Core/Extension/ModuleInstaller.php(305):
Drupal\Core\Extension\ModuleHandler->invokeAll('modules_install...', Array)
#6 /var/www/docroot/core/lib/Drupal/Core/ProxyClass/Extension/ModuleInstaller.php(83):
Drupal\Core\Extension\ModuleInstaller->install(Array, false)
#7 /var/www/docroot/core/includes/install.core.inc(1547): Drupal\Core\ProxyClass\Extension\ModuleInstaller->install(Array,
false)
#8 /var/www/docroot/core/includes/install.core.inc(652): install_install_profile(Array)
#9 /var/www/docroot/core/includes/install.core.inc(530): install_run_task(Array, Array)
#10 /var/www/docroot/core/includes/install.core.inc(115): install_run_tasks(Array)
#11 phar:///usr/local/bin/drush/includes/drush.inc(726): install_drupal(Object(Composer\Autoload\ClassLoader), Array)
#12 phar:///usr/local/bin/drush/includes/drush.inc(711): drush_call_user_func_array('install_drupal', Array)
#13 phar:///usr/local/bin/drush/commands/core/drupal/site_install.inc(80): drush_op('install_drupal',
Object(Composer\Autoload\ClassLoader), Array)
#14 phar:///usr/local/bin/drush/commands/core/site_install.drush.inc(247): drush_core_site_install_version('test', Array)
#15 [internal function]: drush_core_site_install('test')
#16 phar:///usr/local/bin/drush/includes/command.inc(366): call_user_func_array('drush_core_site...', Array)
#17 phar:///usr/local/bin/drush/includes/command.inc(217): _drush_invoke_hooks(Array, Array)
#18 [internal function]: drush_command('test')
#19 phar:///usr/local/bin/drush/includes/command.inc(185): call_user_func_array('drush_command', Array)
#20 phar:///usr/local/bin/drush/lib/Drush/Boot/BaseBoot.php(67): drush_dispatch(Array)
#21 phar:///usr/local/bin/drush/includes/preflight.inc(66): Drush\Boot\BaseBoot->bootstrap_and_dispatch()
#22 phar:///usr/local/bin/drush/includes/startup.inc(321): drush_main()
#23 phar:///usr/local/bin/drush/drush(114): drush_startup(Array)
#24 /usr/local/bin/drush(10): require('phar:///usr/loc...')
#25 {main}

Kinda had the same issue : The patch helped but I had a duplicate on the revision table (block_content_revision).

This patch add a single line that fixes this particular problem, but i'm not sure this is how we want to do it ?

Putting this to needs review but the comment above kinda says need work.

I think morsok was right about adding a single file to solve the duplicate on the revision table but some entities does not support revision.
This patch add "try - catch" to get the LogicException in that cases and scape this line when the entity does not support revisions (user or file entity type as example).

Not sure if I'm in the right way, I hope this helps.
This is my first patch, exciting to help drupal community ;-)

It seems it works well. I can import my content multiple times now, which is cool, specially to test content recently exported !

deleted because that was no useful input ...

Patch works fine for me. Thanks

I guess this is RTBC then..

Can we have a test to go with this please - thanks.

#6 worked for me as well.

While the patch allows to reimport the same content, it doesn't prevent the following error when trying to reimport users:

Drupal\Core\Database\IntegrityConstraintViolationException: [error]
SQLSTATE[23000]: Integrity constraint violation: 1062 Duplicate entry
'0' for key 'PRIMARY': INSERT INTO {users} (uid, uuid, langcode)
VALUES (:db_insert_placeholder_0, :db_insert_placeholder_1,
:db_insert_placeholder_2); Array
(
[:db_insert_placeholder_0] => 0
[:db_insert_placeholder_1] =>
82472cdf-55bb-4514-9fbd-0338bed71547
[:db_insert_placeholder_2] => en
)
in Drupal\Core\Database\Connection->handleQueryException() (line 668
of /core/lib/Drupal/Core/Database/Connection.php).
PDOException: SQLSTATE[23000]: Integrity constraint violation: 1062 [error]
Duplicate entry '0' for key 'PRIMARY' in core/lib/Drupal/Core/Database/Statement.php:59

Hi, after applying #6, I got the following error on re-enabling the module, I guess it is because 'id' field is not exist for block_content entity.

Okay, id not exist is caused by json files left in the folder without uuid setup in the yml file, I think #6 is working for me now.

Patch #6 works for me. tested taxonomy_terms only.

Probably duplicate of https://www.drupal.org/node/2640734, #7 patch has "update" feature too

Hi,

I am seeing the same issue as mstef from comment #4, either with or without patch from #6 applied.

I too have content with admin as author.

I am exporting the content in this way:

if echo $ENABLED_MODULES | grep -q default_content;
then
  for nid in $(drush sql-query 'SELECT nid FROM node');
  do
    drush default-content-export-references --folder="$PWD/$PROFILE_MACHINE_NAME/content" node $nid
  done
fi

So the admin user is also exported in content/user, as it's referenced in the content.

When I install the profile which provides the previously exported content I get the error, apparently the code tries to re-add the admin user?

It would be great to sort this out, providing default content written by the admin user may be convenient when setting up a demo site.

I also tried the patch from #2640734-22: Allow manual imports which does something similar, but it does not solve the problem either.

Thanks,
Antonio

Looks like #2640734: Allow manual imports has all the changes we have here. We can either
a) Add tests for this change and commit this issue and make #2640734: Allow manual imports blocked until this issue get in.
b) Close this issue (and give credit people worked here on #2640734: Allow manual imports ) as duplicate
either way, we need tests...

This issue is about to allow imports again so it have nothing common with #2640734: Allow manual imports

Taken all the work and comments from #2640734: Allow manual imports and rolled them into this patch.

I will leave someone else to decide if tests for import functionality should be part of this ticket or another.

Based on #6, but made sure the entity id is set to NULL. This is basically what Entity::createDuplicate() does , but we want the uuid to stay. https://api.drupal.org/api/drupal/core%21lib%21Drupal%21Core%21Entity%21...

Patch #22 failed because of Dependency to rest module

This patch takes into account the issues with manual import as described in https://www.drupal.org/node/2640734#comment-11862933 && https://www.drupal.org/node/2640734#comment-11900803 just setting this back to the default patch to work from once the rest dependency issue is fixed in tests.

I commited #2848425: Dependency to rest module so tests no longer break, but this still needs a basic test

Seems sensible.

Added additional functional tests, as per the current import tests, to check re-importing of content.

Looks good to me. Let's wait for the tests to finish...

I have tested the latest patch with our thunder demo module and it works as expected

Also reviewed and tested. Works as designed.

+1 RTBC but we'd like to get #2614644: Split DefaultContentManager into Exporter and Importer in first

Remove diff in tests/modules/default_content_test/default_content_test.info.yml because causes errors when using make file.

Still had problems with duplicate id's, so made sure the entity id and revision id are set to NULL when it's a new entity.

Interdiffs plzzz!!! Make it easier... Anyhow, we gotta wait for #2614644: Split DefaultContentManager into DefaultContentExporter and DefaultContentImporter

+++ b/src/DefaultContentManagerInterface.php
@@ -20,11 +20,13 @@ interface DefaultContentManagerInterface {
-  public function importContent($module);
+  public function importContent($module, $update_existing = FALSE);

Needs update because #2614644: Split DefaultContentManager into Exporter and Importer

about the problem with uid 1, what you need to do is calling ->accessCheck(FALSE) on the entity query.

Ah cool awesome @berdir. That's useful if writing own entity query. So now to decide whether to write own entity query or to use account switcher??

Re-rolled.

Incorporating #37 makes #2689069: Do not import entity IDs (included in exports from Drupal 8.1.x) a duplicate of this ticket.

@berdir would you suggest writing a custom query for the uuid lookup?

I'd like to get @Berdir eyes on the rest

While we enable updates then event should allow subscribers to separate updated and created entities
So I see 2 ways
- 2 events (updated & created)
- one event and a way to separate updated/created

Also we can store result of save into event

Mostly nits

1. +++ b/src/Importer.php
   @@ -186,9 +211,39 @@ class Importer implements ImporterInterface {
   +          // This must be run a user 1 to bypass access permissions.
   +          $this->accountSwitcher->switchTo(new UserSession(['uid' => 1]));
   +          $old_entity = $this->entityRepository->loadEntityByUuid($entity_type_id, $entity->uuid());
   +          // Restore the user account.
   +          $this->accountSwitcher->switchBack();
   ...
   +            $entity->save();
   

   Save also a question, because some properties receive default values (node author)

2. +++ b/src/Importer.php
   @@ -255,4 +310,17 @@ class Importer implements ImporterInterface {
   +  public function isRevisionableEntity(EntityInterface $entity) {
   +    return $entity instanceof RevisionableInterface && $entity->getEntityType()->isRevisionable();
   +  }
   

   I'd better keep this protected

+++ b/src/Importer.php
@@ -186,9 +211,39 @@ class Importer implements ImporterInterface {
+          // Allow existing entities to be updated.
+          // This must be run a user 1 to bypass access permissions.
+          $this->accountSwitcher->switchTo(new UserSession(['uid' => 1]));
+          $old_entity = $this->entityRepository->loadEntityByUuid($entity_type_id, $entity->uuid());
+          // Restore the user account.
+          $this->accountSwitcher->switchBack();

Is there an existing core issue about it / does anyone mind open up a core issue about that? This API functions should NOT respect access checking, IMHO

Sure, so as @berdir mentioned it would be perfectly possible to look up uuid with a custom query that does NOT respect access checking (which may be better) but using the existing entity repository service it looks like the entity uuid lookup does not have a skip access permission flag.

That's sounds like needs core issue

btw it makes sense to coordinate in #2691085: Schedule alpha 9 release

Reroll of the patch in #45 against the latest dev version which already has nitpick 1 from #47 in it, and I fixed nitpick 2.

Also I created an extra event as also suggested in #45. This way the existing event stays the same (only imports), so existing integrations will keep working exactly the same.

Here's a bit of clean-up + todo
I'm sure we need to resolve that here somehow

FYI Solving import of existing contend is blocker for all import issues like #2640734: Allow manual imports

a bit more clean-up

btw What if entity with parsed ID exist? the whole import will fail but should not, imo.

@andypost #48 was addressing the issue of entity with parsed ID existing. Patch #52 removed this ability to lookup the existing entity correctly without adding an alternative. Suggest we either add #48 back in, add a core patch or add our own entity lookup which bypasses access checks.

I think lookupEntity() better to isolate in ptotected method at least, I needs to dig it deeper
And I also sure that we need core issue about loading with skip entity access
As hotfix we can use entity query to skip access check and if entity found load it

Possibly as temporary solution.
I removed, from patch #55, the assignment of NULL to id and revision of the entity, because for some entities import with ID is a necessary . Import and update work is correctly for me.

Cool @andypost.

@nabiyllin I don't think this is correct, IDs and revisions should not be imported, that's what the uuid is for. Can you describe your use case?

We still need IDs because there still parts that reference by them, see #2304849: Stop excluding local ID and revision fields from HAL output

+ that required to detect uid 0 & 1 to assign them properly to local ones

Also I think about collision with #2640734: Allow manual imports

Ok, but that is an exception rather than the rule I feel. Trouble is we now need figure out which is the actual reference id. As in should we lookup whether the entity id exists and update that, or should we lookup whether the entity uuid exists and update that, or both, but what if the uuid exists for a different id?

Let's say we're importing nodes:

Into an existing site:

My feeling is that for certain drupalisms, like uid 0 & 1 which have other meanings then we have 'exceptions' and can lookup the id instead of the uuid and/or provide an event for this.

that's main pita here, mostly everywhere reference made by UUID except few places

+++ b/src/Importer.php
@@ -178,19 +183,52 @@ class Importer implements ImporterInterface {
+          // @todo Search entity by parsed ID to prevent exceptions.
+          $old_entity = $this->entityTypeManager
+            ->getStorage($entity_type_id)
+            ->loadByProperties([$entity_type->getKey('uuid') => $entity->uuid()]);
+          $old_entity = reset($old_entity);

That's exactly a lookup that I'd prefer to delegate to other class method

Cool, yea definitely worth delegating. Will have a play later.

Introducing an entityLookup method. This abstracts just enough and also takes care of the access check issues, although I notice in the master branch the whole import is running as root user anyway??

@nabiyllin @andypost Considering the root user anomaly I still believe we would never import a new ID. We may update an entity and use that ID, but the IDs that come through from the json input should not be used as they are unreliable across sites. Take the following example:

Json Import

New Site

In this example, when we import we would want:
User 1 in the json to inherit the uuid of User 1 in the new site, this avoids any collisions with User 2 in the new site which has the same uuid (we may even want to automatically detect these collisions and remove User 2 in the new site??
User 2 in the json to inherit the uuid from User 3 in the new site (but we would not want to remove user 2 in the new site).
User 3 in the json to be created in the new site as User 4.

We do not want:
User 3 in the json to become User 3 in the new site.

When you import new taxonomies with parents, you should, as least, import parent taxonomy with ID from json, because in daughter taxonomy in links and embedded parts filled parent id which is used to build a hierarchy.

Are you using this for default content or content staging? We're interested in a modules/profiles providing default content at install time, not in perpetuity.

I have no interest in making this module aware of all the nuances required for content staging. Use another module for that.

If you're using it for default content - in what instance would you get uuid collisions/issues other than for user 1 and 0?

I don't understand the use-case that can lead to the issues you're listing.

@larowlan my main case here is predictable error handling & basic colision detection for #2640734: Allow manual imports

If module has default content there no way to install it again. #2842749: A single failure during module install prevents any further content processing

Secondly #2803005: Add helper function to allow individual content import (useful in update hooks)

And finally #2849128: Add handling of ConfigEvent::IMPORT_MISSING_CONTENT

Btw that's looks like a part of content staging because it's) We stage predefined content for install time) and it leads us to related issue again

And predictable collision detection when enabling the module on a site with existing content.

IMO in all cases, except user 1, this can be achieved by looking up the uuid and ignoring internal IDs.

In regards to the taxonomy terms case I think content items maintaing references to each other by internal site IDs is a problem with the normalisation of the content item.

Internal site IDs are only relevant on the site on which they were generated.

Ok, gotcha - thanks for the explanation

@dawehner opened up #2878483: loadEntityByUuid() should skip access checks in relation to #48.

@nabiyllin With taxonomy terms I can see what you mean by the _links still referring to internal IDs:

"_embedded": {
        "http:\/\/drupal.org\/rest\/relation\/taxonomy_term\/tags\/parent": [
            {
                "_links": {
                    "self": {
                        "href": "http:\/\/default\/taxonomy\/term\/1?_format=hal_json"
                    },
                    "type": {
                        "href": "http:\/\/drupal.org\/rest\/type\/taxonomy_term\/tags"
                    }
                },
                "uuid": [
                    {
                        "value": "550f86ad-aa11-4047-953f-636d42889f85"
                    }
                ]
            }
        ]
    },

However the actual reference is still done by uuid, I'd be interested to hear what the self link is used for in this module. I've done a test importing a parent and child term into a site which already has taxonomy terms IDs 1 & 2 registered and it's maintained the correct parent-child relationship but inserted as new so I don't think this is an issue.

I think that's everything listed in the comments, this seems to avoid all the collisions I've detected in my testing.

If the main use case of default_content is to import content assuming that no different/unrelated content is already in the database, it may be worth improving the delete_all module (see #2857363: Add the --reset option to the D8 branch) and propose it as a companion module.

After applying patch: https://www.drupal.org/files/issues/default_content_do_not_reimport-2698...
error appears
Listing here: ttps://www.drupal.org/files/issues/default_content_do_not_reimport-2698425-64.txt
Occurs, during enabled/disabled "my_custom_default_content" module
Before applying patch I had:
Uncaught PHP Exception Drupal\\Core\\Entity\\EntityStorageException: "SQLSTATE[23000]: Integrity constraint violation: 1062 Duplicate entry '9' for key 'PRIMARY': INSERT INTO {block_content} (id, revision_id, type, uuid, langcode) ....

Minor adjustment: the Importer has a default value for the $update_existing parameter (FALSE) in the importContent-function. The interface does not. The patch included fixes this.

The interface is a contract, it shouldn't define values for these properties IMO.

Are you happy with the rest of this patch @LammensJ ?? Would be good to focus on getting this reviewed and tested.

I tend to agree with LammensJ. The interface is a contract, yes, but default values for parameters have a direct bearing on how the method is called and whether a value can be left out when calling the method or not. If the default value is left to the implementation, the caller will need to care about what implementation is used in order to know whether they can leave out the parameter, which is contrary to the point of having an interface in the first place.

I would have liked to do a complete review, but unfortunately that'll have to wait until next week, maybe. Unless somebody else beats me to the punch, of course :)

Is there a reason this patch does not actually enable the use of the $update_existing flag? (Maybe it's been mentioned and I missed it).

Apart from that, just some nits:

1.            $entity_type_id = $file->entity_type_id;
   -          $class = $this->entityTypeManager->getDefinition($entity_type_id)->getClass();
   +          /* @var $entity \Drupal\Core\Entity\EntityTypeInterface */
   +          $entity_type = $definitions[$entity_type_id];
              $contents = $this->parseFile($file);
   

   Variable name in the docblock is incorrect, should be $entity_type.

2.        $this->eventDispatcher->dispatch(DefaultContentEvents::IMPORT, new ImportEvent($created, $module));
   +      $this->eventDispatcher->dispatch(DefaultContentEvents::UPDATE, new ImportEvent($updated, $module));
       

   Shouldn't we be checking if there is anything in the $created and $updated arrays before sending the event? Or is there any value in sending an event notifying that there was nothing updated or deleted?

3.    /**
   +   * Lookup whether an entity already exists.
   +   *
   +   * For most typical entities this is done by uuid.
   +   * For core user 1 this is done by id.
   +   *
   +   * @param EntityInterface $entity
   +   *   The entity that will be imported.
   +   * @param EntityTypeInterface $entity_type
   +   *   The entity type for this entity.
   +   *
   +   * @return EntityInterface|NULL
   +   */
   +  public function lookupEntity($entity, $entity_type) {
       

   The return value needs a description and the arguments need type hints (according to php_cs).

1, 2 & 3 all fair points.

The reason for not including $update_existing flag is that at the time #2640734: Allow manual imports hadn't been completed. It can be added now.

We're currently facing an issue with Multiversion module plus Default Content module. Multiversion has an entity type Workspace, and all content is linked to a workspace via an entity reference. Default content tries to export/import these workspace entities, but Multiversion also creates the default one on install.

I am hitting issues similar to #4. The latest patch fixes the issue, but then install (via drush si) never completes.

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.

Fixing nits in #76.

Sorry, that was completely the wrong patches in #80.

Has anyone got #82 to work? I tried installed through composer-patches but it failed.

Works like a charm. Let's get this in...

This is generally looking good. Here's a review noting a few outstanding issues.

1. +++ b/src/Importer.php
   @@ -178,19 +182,58 @@ class Importer implements ImporterInterface {
   +            // Never import site level IDs.
   

   The comment says "Never", but the code will not be executed in the case that $update_existing is TRUE and there is no $old_entity.

2. +++ b/src/Importer.php
   @@ -178,19 +182,58 @@ class Importer implements ImporterInterface {
   +            $entity->{$entity_type->getKey('revision')} = NULL;
   

   Since not all content entity types are revisionable, we should test (using the ::isRevisionableEntity() method introduced in this patch) before setting this property.

3. +++ b/src/Importer.php
   @@ -178,19 +182,58 @@ class Importer implements ImporterInterface {
   +          !$is_new && $old_entity ? $entity->setOriginalId($old_entity->id()) : $entity->enforceIsNew($is_new);
   

   This line is hard to follow and could use a code comment.

4. +++ b/src/Importer.php
   @@ -178,19 +182,58 @@ class Importer implements ImporterInterface {
   +        $this->eventDispatcher->dispatch(DefaultContentEvents::IMPORT, new ImportEvent($created, $module));
   

   The naming here is inconsistent. The method is ::importContent(). In the update case, we test for a variable $updated and, appropriately, dispatch an UPDATE event. But in the create case, we test for $created but then dispatch an IMPORT event. The expected event constant would be CREATE.

5. +++ b/src/Importer.php
   @@ -200,6 +243,41 @@ class Importer implements ImporterInterface {
   +   * Lookup whether an entity already exists.
   

   This description (and the method name) implies a boolean return value, but what we're actually doing is loading or getting the entity (if it exists).

6. +++ b/src/Importer.php
   @@ -200,6 +243,41 @@ class Importer implements ImporterInterface {
   +   *   The old entity, or NULL if no entity.
   

   The fallback return value is an empty array.

7. +++ b/tests/src/Functional/DefaultContentTest.php
   @@ -102,4 +102,34 @@ class DefaultContentTest extends BrowserTestBase {
   +      // Enable the module and import the content.
   

   Indentation issue.

8. +++ b/tests/src/Functional/DefaultContentTest.php
   @@ -102,4 +102,34 @@ class DefaultContentTest extends BrowserTestBase {
   +      // Re-import the content and check there are no changes.
   

   Indentation issue.

+++ b/src/Importer.php
@@ -178,19 +182,58 @@ class Importer implements ImporterInterface {
+          elseif (!$old_entity) {
+            // Never import site level IDs.
+            $entity->{$entity_type->getKey('id')} = NULL;
+            $entity->{$entity_type->getKey('revision')} = NULL;
+          }

This is breaking the import of new entities with references to each other, since the exported entity id is being used for the reference, and this part triggers a new entity id to be generated. At least this is as far as we've been able to investigate.

I'm sure revision support needs review

I see 2 cases here
1) embed - mostly users, 0 & 1 both needs mapping, others need own conflict resolution
2) relations - block_content, menu items, terms, files all needs selector at revision uuid change & they may have other revisionable dependencies

Ah... and how to deal with translation revisions?

+++ b/src/Importer.php
@@ -178,19 +182,58 @@ class Importer implements ImporterInterface {
+          elseif (!$old_entity) {
+            // Never import site level IDs.
+            $entity->{$entity_type->getKey('id')} = NULL;
+            $entity->{$entity_type->getKey('revision')} = NULL;
+          }

This lines break import entiies where entity_key "id" is not serial but is string. For example entity_subqueue:
entity_keys = {

 *     "id" = "name",
 *     "bundle" = "queue",
 *     "label" = "title",
 *     "langcode" = "langcode",
 *     "uuid" = "uuid",
 *     "uid" = "uid"
 *   },

Will add fix soon

Thanks @andreyks - I like where #91 is going. Looks to be addressing #87 too. Setting it to needs review for the tests to run.

+++ b/tests/src/Functional/DefaultContentTest.php
@@ -102,4 +102,34 @@ class DefaultContentTest extends BrowserTestBase {
+      // Enable the module and import the content.
...
+      // Re-import the content and check there are no changes.

Nits: indentation is off.

coding standards fixes only

Just a comments review

+++ b/src/Importer.php
@@ -203,9 +203,11 @@
+            // Don't import site level IDs if them are used.

them -> they

+++ b/src/Importer.php
@@ -279,6 +281,25 @@
+   * Check if imported entity id already exists.

Check if imported -> Check if an imported

+++ b/src/Importer.php
@@ -279,6 +281,25 @@
+   *   The entity type for this entity.

The entity type of the entity that will be imported?
If so, let's say so instead :)

TODO: handle error if current item already exists and $entity_type->getKey('id') is required and is not auto incremented

I edited the title. I had this issue: Taxonomy terms causes duplicate entity errors - possible solution integrate with taxonmy_machine_name

This patch solved it.

See the other issue for the error.

I don't know if you disable the module with default terms and then re-enable it. Will the terms be re-imported again?

+++ b/src/Importer.php
@@ -178,19 +182,60 @@ class Importer implements ImporterInterface {
+            $entity->{$entity_type->getKey('revision')} = NULL;
+          }
+
+          !$is_new && $old_entity ? $entity->setOriginalId($old_entity->id()) : $entity->enforceIsNew($is_new);
+          if ($this->isRevisionableEntity($entity)) {
+            $entity->setNewRevision($is_new);
+          }

Resetting the revision id or enforcing a new revision seems to break EntityReferenceRevisionsItems. This reference type references a specific revision of an entity. The serialized data maintains the revision ids.
So if we reset the revision id to avoid collisions we'd need to update all field contents with EntityReferenceRevisionsItems as well.

I've tried to tackle the issue I've reported above but the solution doesn't feel very clean.
If someone has a an idea how to solve that in a nicer way please let me know :)

@SocialNicheGuru: Please excuse my bluntness in renaming the issue back, but I feel "renumbering" entries is an implementation detail and has no business in the issue title; new content should just get the next numeric ID, whether it is the result of an import or a regular, manual content entry. In general, I would say this module should not be concerned with local IDs; what uniquely identifies a content item (and should determine what is "existing" in terms of this issue) is the UUID. The actual numeric database ID is an implementation detail (and since we are exporting it, and using the exported value when creating the content (?), maybe we should indeed get rid of that completely, as suggested in #79).

Or, maybe I am missing something, in which case, please explain in more detail why you feel the title change is warranted.

@das-peter: Do you have an interdiff (i.e. a "patch" between your version and the last contributed patch)? That makes reviewing your changes easier.
@#104: Huh.. Not sure how I missed it. Sorry about that.

@eelkeblok: Interdiff is already in #102 - see the file "interdiff-2698425-99-102-do-not-test.diff"

Right.. Sorry about that, not sure how I missed it. One quick remark. A way to maybe unfold the "rat's nest" of ifs and foreaches might be to reverse some of the ifs and do a continue to move onto the next item. This allows the "happy flow" to move a level up.

Two nitpicks

1. +++ b/src/Importer.php
   @@ -201,6 +246,60 @@ class Importer implements ImporterInterface {
   +  public function lookupEntity($entity, $entity_type) {
   

   Can type hint these two parameters

2. +++ b/src/Importer.php
   @@ -201,6 +246,60 @@ class Importer implements ImporterInterface {
   +  public function existEntityId($entity, $entity_type) {
   

   Can type hint these two parameters.

I don't know whether this has been mentioned (so many comments) but `existEntityId` isn't working as expected. See below:

$entity_storage = \Drupal::entityTypeManager()->getStorage('entity_type');
$entity_query = $entity_storage->getQuery()->accessCheck(FALSE);
$entity_query->condition('id', (array) 999, 'IN');

$count1 = $entity_query->count()->execute();
$count2 = count(\Drupal\module\Entity\EntityType::loadMultiple([999]));

The first count comes out as 0 whereas the second comes out as 1. Whether this is a product of some kinda weirdness in my use case I cannot tell yet but there seems to be an issue with the matching query.

EDIT: Scrub that it is a bit of database corruption :)

+++ b/src/Importer.php
@@ -114,15 +117,18 @@ class Importer implements ImporterInterface {
+    $revision_links = [];

@@ -178,19 +184,89 @@ class Importer implements ImporterInterface {
+                      if (isset($revision_links[$target_entity->getTargetDefinition()->getEntityTypeId()][$target_entity->getTargetIdentifier()])) {
+                        $target_entity->setValue(array(
+                          'target_id' => $target_entity->getTargetIdentifier(),
+                          'target_revision_id' => $revision_links[$target_entity->getTargetDefinition()->getEntityTypeId()][$target_entity->getTargetIdentifier()],
+                        ));
+                      }
...
+              $revision_links[$entity->getEntityTypeId()][$entity->id()] = $entity->{$entity_type->getKey('revision')}->value;
...
+              $revision_links[$entity->getEntityTypeId()][$entity->id()] = $entity->{$entity_type->getKey('revision')}->value;

I'm not sure how this is connected and it is going to be super brittle in all those cases were content does already exists. The array will often not be populated because no content is being created or updated.

I think if we fixed the fact revisions can go haywire during import maybe that's less an issue though. This issue is specifically about dealing with entity reference revisions and could easily be merged in I wager.
https://www.drupal.org/project/default_content/issues/2989887#comment-12...

Added missing type hints and removed unused variables.
Fixed existEntityId() not to fail for empty entity id. Related to #2969631: NID / VID / TID conflicts in branches though I see that the tests failed with the same error.

The patch just allowed a clean import for me. Nice work.

1. +++ b/default_content-dont-reimport-existing-entities-2698425-102.patch
   @@ -0,0 +1,379 @@
   + ¶
   ...
   + ¶
   ...
   + ¶
   ...
   + ¶
   

   whitespace errors here

2. +++ b/default_content-dont-reimport-existing-entities-2698425-102.patch
   @@ -0,0 +1,379 @@
   ++use Drupal\Core\Entity\EntityInterface;
   ...
   ++use Drupal\Core\Entity\RevisionableInterface;
   ...
   +-  public function importContent($module) {
   ++  public function importContent($module, $update_existing = FALSE) {
   

   this looks like the patch contains a patch file too?

   Confirmed locally:

   new file:   default_content-dont-reimport-existing-entities-2698425-102.patch
   	modified:   src/Event/DefaultContentEvents.php
   	new file:   src/Event/UpdateEvent.php
   	modified:   src/Importer.php
   	modified:   src/ImporterInterface.php
   	modified:   tests/src/Functional/DefaultContentTest.php

3. +++ b/src/Event/DefaultContentEvents.php
   @@ -25,6 +26,21 @@ final class DefaultContentEvents {
   +   * \Drupal\default_content\Event\ImportEvent instance.
   

   does it? Or is it an UpdateEvent?

4. +++ b/src/Importer.php
   @@ -178,19 +185,89 @@ class Importer implements ImporterInterface {
   +                if ($data instanceof EntityReferenceRevisionsFieldItemList) {
   

   Sorry, I don't want to introduce this. Instanceof is a code smell, and it is very much out of scope here.

   This is about reimporting existing entities, not adding support for ERR. A separate issue for that is where it should go, and with tests. Combining the two means the patch is far less commit-able.

5. +++ b/src/Importer.php
   @@ -178,19 +185,89 @@ class Importer implements ImporterInterface {
   +          if ($old_entity && $update_existing) {
   +            // All unique keys need to match the old entity.
   +            $entity->{$entity_type->getKey('uuid')} = $old_entity->uuid();
   +            $entity->{$entity_type->getKey('id')} = $old_entity->id();
   +            $is_new = FALSE;
   +            if ($this->isRevisionableEntity($entity)) {
   +              $entity->{$entity_type->getKey('revision')} = $old_entity->getRevisionId();
   +            }
   ...
   +              $entity->{$entity_type->getKey('id')} = NULL;
   ...
   +            $entity->{$entity_type->getKey('revision')} = NULL;
   

   I don't like this knowledge being handled here, I'd much prefer a conflict resolution system like entity pilot module has, so it is pluggable. However, perhaps it would look nicer without the out of scope ERR changes.

6. +++ b/src/Importer.php
   @@ -178,19 +185,89 @@ class Importer implements ImporterInterface {
   +            if ($this->isRevisionableEntity($entity)) {
   +              $revision_links[$entity->getEntityTypeId()][$entity->id()] = $entity->{$entity_type->getKey('revision')}->value;
   +            }
   ...
   +            if ($this->isRevisionableEntity($entity)) {
   +              $revision_links[$entity->getEntityTypeId()][$entity->id()] = $entity->{$entity_type->getKey('revision')}->value;
   +            }
   

   we're repeating this logic, it feels like this hunk could be refactored

7. +++ b/src/Importer.php
   @@ -200,6 +277,62 @@ class Importer implements ImporterInterface {
   +   * @return \Drupal\Core\Entity\EntityInterface|null
   +   *   The old entity, or NULL if no entity.
   ...
   +    $old_entity = $result ? $entity_storage->load(current($result)) : [];
   

   we're returning an empty array, not null?

8. +++ b/src/Importer.php
   @@ -200,6 +277,62 @@ class Importer implements ImporterInterface {
   +    // Alter the lookup properties for known core irregularities.
   +    if ($entity_type->id() === 'user' && $entity->id() == 1) {
   +      $lookup_properties = [$entity_type->getKey('id') => $entity->id()];
   +    }
   

   I don't want to special case this. Again, a pluggable system like entity pilot has would be more elegant

9. +++ b/src/Importer.php
   @@ -200,6 +277,62 @@ class Importer implements ImporterInterface {
   +  public function existEntityId(EntityInterface $entity, EntityTypeInterface $entity_type) {
   ...
   +      $entity_storage = $this->entityTypeManager->getStorage($entity_type->id());
   

   do we need to pass the entity type if we have the entity?

10. +++ b/src/Importer.php
    @@ -200,6 +277,62 @@ class Importer implements ImporterInterface {
    +      $entity_query->condition($entity_type->getKey('id'), (array) $entity->id(), 'IN');
    

    when would this ever be more than one value?

+++ b/src/Importer.php
@@ -178,19 +185,89 @@ class Importer implements ImporterInterface {
+              foreach ($entity as $data) {
+                // If this is a field that requires the revision id ensure it
+                // has the one assigned during the import and not the one stored
+                // in the export.
+                if ($data instanceof EntityReferenceRevisionsFieldItemList) {
+                  foreach ($data as $item) {

Agreed that this is not nice, however I assume the reason for having it here would break existing default content that right now does not require anything special for ERR.

hi folks, for sake of completeness ... this issue #109 patch is corrected (not to wrongly include the default_content-dont-reimport-existing-entities-2698425-102.patch file) and embedded (and extended) into this related one: Allow updating existing content (and don't reimport existing), that stably applies to the 8.x-1.0-alpha7 branch

Looks like the patch #109 still has some issues. I created a module and when the site has no content worked prefect but when I had some content I got this at the moment to enabled it:
Error: Maximum function nesting level of '512' reached, aborting! in Drupal\Core\Extension\ModuleHandler->getImplementationInfo() (line 577 of /app/webroot/core/lib/Drupal/Core/Extension/ModuleHandler.php).

I didn't change the functionality of this, but I addressed most (all?) of the feedback in #111. I also changed a bit the patch structure to improve (hopefully) readability a bit.

I put the ERR in a private method so it can be removed easily if needed after @larowlan addresses @Berdir's comment on #112.

Using either #109 or #155 patches, I have a scenario which still raises an exception when trying to import existing Users:

How to reproduce

1. Fresh install of Drupal 8.7.x (currently 8.7.2) with the minimal profile (drush si minimal)
2. Create a new user (drush user-create --mail=yourmail@gmail.com)
3. Export users into a custom module (drush dcer node --folder=modules/custom/<module>/content -y)
4. Reinstall website (drush si config_installer -y)
5. Try to import default content

=> Exception:

SQLSTATE[23000]: Integrity constraint violation: 1062 Duplicate entry '0' for key 'PRIMARY'

Workaround

Either

• Do not export Users
• Delete the JSON file of the first user (UID: 0)

1. +++ b/src/Importer.php
   @@ -114,85 +123,153 @@ class Importer implements ImporterInterface {
   +        $updated[$entity->uuid()] = $entity;
   

   Why do we pollute RAM by collecting $updated?

2. +++ b/src/Importer.php
   @@ -200,6 +277,57 @@ class Importer implements ImporterInterface {
        return $created;
   

   Why return a list of created content is needed? I'd stop collecting $created and make the method return nothing.

3. +++ b/src/Importer.php
   @@ -255,4 +383,58 @@ class Importer implements ImporterInterface {
   +   * @param $revision_links
   

   The array type hint is missing.

4. +++ b/src/Importer.php
   @@ -255,4 +383,58 @@ class Importer implements ImporterInterface {
   +  private function fixErrTargets(FieldableEntityInterface $entity, $revision_links) {
   

   - private makes it impossible to even reuse the method. I prefer to inherit from the original service and this makes impossible to use its functionalities fully. I'd make it final instead.

   - The array type is missing for $revision_links param.

Validate ERR exists before fixing targets.

Avoid unnecessarily asking for ERR on every iteration.

Rerolling previous patches (118 & 119) as there was an issue applying them.

+++ b/src/Importer.php
@@ -255,4 +385,58 @@ class Importer implements ImporterInterface {
+      if (!$field instanceof Drupal\entity_reference_revisions\EntityReferenceRevisionsFieldItemList) {

If I'm not mistaken this won't ever resolve to true.
Either we use the full namespace \Drupal\entity_reference_revisions\EntityReferenceRevisionsFieldItemList or we change it to use Drupal\entity_reference_revisions\EntityReferenceRevisionsFieldItemList and then check with if (!$field instanceof EntityReferenceRevisionsFieldItemList) {

I think the use approach is out of question because this relies on a module namespace which might not be available.
So I changed the patch to use the full namespace path in the comparison.

It implements in the https://www.drupal.org/project/default_content_deploy module

@das-peter are we sure about that? IIRC use statements are besically just aliasing the full name space. It would only cause problems if the code tried to actually instantiate an object for the class that does not actually exist.

Hi all, I used the --122.patch applied to 1.0-alpha8 to install a new module that contains default content that is already in place from another module.
This happens due to topology changes and distributing content in more modules.
Importing configuration with that module enabled produces the

[ERROR] An error occurred while trying to write the config file: "Type
         http://drupal.org/rest/type/file/file does not correspond to an entity
         on this site."

error.

I do not have the file_entity module enabled for any instance @larowlan, as I saw in other issues you mention it can be related.

Any piece of advice is more than welcome, thank you

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.

This specific issue isn't resolved yet, but this also seems to do a lot of changes and seemingly unrelated changes like ERR related improvements. 2.0.x supports ERR composite entities like paragraphs out of the box in the same file/normalization structure, so possibly this can be simplified a lot.

The patch is quite hard to review as it is changing huge parts of code. an updated issue summary that explains what it does exactly would be usefu.

Tested with patch #122 and 2.0.x-dev

Result : imports fails because it tries to re-create some content with some unique attributes already existing (uuid, id)

A patch on this closed, duplicate issue was made for the 2.x branch. It successfully applies to my build and fixes the error I was seeing on import.

https://www.drupal.org/files/issues/2020-08-03/default_content-integrity...
https://www.drupal.org/project/default_content/issues/3162987

Here's the patch from issue 3162987 (not my work!) attached to make it easier to test and so we can update the status to 'needs review'.

The patch was contributed by dcraig91.

Removed the .txt appended to the patch filename.

Ugh sorry, I think Chrome added the .txt when I downloaded the patch file from the closed issue. Thanks for spotting that @scotthorn!

Patch in comment #130 worked for me great.

Any plans to backport this to 1.x?

Adding a patch for the 8.x-1.x since this is still an issue in 1.x.

Thank you.

I ran into this issue as well. I tested the patch in #130 and it worked great. Thank you

1. +++ b/src/Normalizer/ContentEntityNormalizer.php
   @@ -178,8 +178,15 @@ class ContentEntityNormalizer implements ContentEntityNormalizerInterface {
   +    // Check exits.
   

   It would be nice to have a better comment here. Like "Load the entity by UUID and check if it exists."

2. +++ b/src/Normalizer/ContentEntityNormalizer.php
   @@ -518,3 +525,4 @@ class ContentEntityNormalizer implements ContentEntityNormalizerInterface {
   +
   

   Extra white line.

Added drush command to reimport entities.

I tested the patch from #137 and it did not apply to the latest dev. #136 applies fine.

I've tested #137 in a Drupal 9.1.4 project with default content for taxonomy terms, nodes, and menu links, and was able to import new content (nodes and menu links) successfully. Much appreciated.

I used an update hook to run the re-import automatically:

/**
 * Update the default content.
 */
function foo_default_content_update_9001() {
  // @see default_content_modules_installed().
  \Drupal::service('default_content.importer')->importContent('foo_default_content');
}

For future content additions, just incrementing the update hook number should be sufficient.

I have apply #137 in 2.0.0-alpha1 without problem.

Thanks @Eduardo Morales Alberti

#137 also worked for me. The command to import module's contents is also a needed addition.
Thanks!

Will be changing it to rtbc if noone has any objections.

Confirming that patch in #136 applied against Drupal version 9.2-dev worked for me when using the update routine in #140 in a hook_deploy_NAME() function (content updates were dependent on configuration executing in a hook_deploy_NAME() function was necessary for content to import properly).

Patch in #137 did not apply; have not tried MR patch from #138.

+1 for RTBC.

For entities with translation, #137 doesn't work for me.

An error occurred when saving the entity, when importing a yml with translations like the following for a multilingual site like Umami demo.

_meta:
  version: '1.0'
  entity_type: node
  uuid: 1505c11c-e62f-477f-8825-d001771762c2
  bundle: page
  default_langcode: en
default:
  revision_uid:
    -
      target_id: 1
  status:
    -
      value: true
  uid:
    -
      target_id: 1
  title:
    -
      value: 'Englsih page'
  created:
    -
      value: 1623223364
  promote:
    -
      value: false
  sticky:
    -
      value: false
  moderation_state:
    -
      value: published
  path:
    -
      alias: ''
      langcode: en
  content_translation_source:
    -
      value: und
  content_translation_outdated:
    -
      value: false
  body:
    -
      value: "<p>Englsih page body</p>\r\n"
      format: basic_html
      summary: ''
translations:
  es:
    status:
      -
        value: true
    uid:
      -
        target_id: 1
    title:
      -
        value: 'Español page'
    created:
      -
        value: 1623223403
    sticky:
      -
        value: false
    revision_translation_affected:
      -
        value: true
    moderation_state:
      -
        value: published
    path:
      -
        alias: ''
        langcode: es
    content_translation_source:
      -
        value: en
    content_translation_outdated:
      -
        value: false
    body:
      -
        value: "<p><em>Español</em> page body</p>\r\n"
        format: basic_html
        summary: ''

In ContentEntityBase.php line 955:

  Invalid translation language (es) specified.

For entities without translations, #137 works for me.

I have had no issue importing multilingual content. Can you ensure that Spanish is enabled and that the field you are trying to translate are indeed translatable?

@Marios Anagnostopoulos

Thanks for the comment.
I'm trying it out with the page conten type of the Umami demo.

In ContentEntityNormalizer::denormalize(), when $data['translations'] exists

I think I'm getting an error similar to the following

$node = \Drupal::service('entity_type.manager')->getStorage('node')->load(19);

$node->hasTranslation('es');
=> true

$translation_node = $node->addTranslation('es', $node->toArray());

=> InvalidArgumentException with message 'Invalid translation language (es) specified.

I see that addTranslation purposefully throws InvalidArgumentException if a translation for the specified language already exists:

  public function addTranslation($langcode, array $values = []) {
    // Make sure we do not attempt to create a translation if an invalid
    // language is specified or the entity cannot be translated.
    $this->getLanguages();
    if (!isset($this->languages[$langcode]) || $this->hasTranslation($langcode) || $this->languages[$langcode]->isLocked()) {
      throw new \InvalidArgumentException("Invalid translation language ($langcode) specified.");
    }
    ...

Maybe you can check this issue here: https://www.drupal.org/project/default_content/issues/3176839.

Let us know if it worked well.

@jweowu
Thanks for checking the code. I understand.

@Marios Anagnostopoulos
Thanks for letting me know #3176839.
It works as expected. The #137 patch is no problem for me.
I appreciate your help.

I'm changing the title of the issue because the old title is deceitful.
With the given patch that is RTBC, existing entities will be updated when importing. So, "Do not reimport existing entities" is incorrect.

Changing this back to Needs Work.

After applying the patch (merge diff) from 145, as well as the patch from 150, the default_content module is not handling file entities (from the file_managed table). All file entities are getting imported as new content, even though an entry in the file_managed table with the appropriate UUID is present, and there has been no changes to the image file or the yml content for the exported file entities.

Because of this, when you export changed entities, you get all of the file entities exported with updated image file names, where the original image name was numerically incremented to prevent a file name collision in Drupal when the images were reimported.

Update: There is another issue in the issue queue in RBTC status, #3200212: Import doesn't overwrite files, that is addressing the issue with overwriting vs. updating files on import. That patch may unblock this issue from "Needs Work" to "RBTC".

Update: patch from #145 above was applied on an existing site when originally tested.

Adding patch to new site under development, and then attempting to enable a module results in a stack dump:

ArgumentCountError: Too few arguments to function Drupal\default_content\Commands\DefaultContentCommands::__construct(), 1 passed in /var/www/html/web/core/lib/Drupal/Component/DependencyInjection/Container.php on line 262 and exactly 2 expected in /var/www/html/web/modules/contrib/default_content/src/Commands/DefaultContentCommands.php on line 38 #0 /var/www/html/web/core/lib/Drupal/Component/DependencyInjection/Container.php(262): Drupal\default_content\Commands\DefaultContentCommands->__construct(Object(Drupal\default_content\Exporter))
#1 /var/www/html/web/core/lib/Drupal/Component/DependencyInjection/Container.php(176): Drupal\Component\DependencyInjection\Container->createService(Array, 'default_content...')
#2 /var/www/html/web/core/lib/Drupal/Component/DependencyInjection/Container.php(437): Drupal\Component\DependencyInjection\Container->get('default_content...', 1)
#3 /var/www/html/web/core/lib/Drupal/Component/DependencyInjection/Container.php(276): Drupal\Component\DependencyInjection\Container->resolveServicesAndParameters(Array)
#4 /var/www/html/web/core/lib/Drupal/Component/DependencyInjection/Container.php(176): Drupal\Component\DependencyInjection\Container->createService(Array, 'drush.command.s...')
#5 /var/www/html/vendor/drush/drush/src/Boot/DrupalBoot8.php(280): Drupal\Component\DependencyInjection\Container->get('drush.command.s...')
#6 /var/www/html/vendor/drush/drush/src/Boot/DrupalBoot8.php(246): Drush\Boot\DrupalBoot8->addDrupalModuleDrushCommands(Object(Drush\Boot\BootstrapManager))
#7 /var/www/html/vendor/drush/drush/src/Boot/BootstrapManager.php(293): Drush\Boot\DrupalBoot8->bootstrapDrupalFull(Object(Drush\Boot\BootstrapManager), NULL)
#8 /var/www/html/vendor/drush/drush/src/Boot/BootstrapManager.php(493): Drush\Boot\BootstrapManager->doBootstrap(5, 6, NULL)
#9 /var/www/html/vendor/drush/drush/src/Application.php(229): Drush\Boot\BootstrapManager->bootstrapMax()
#10 /var/www/html/vendor/drush/drush/src/Application.php(196): Drush\Application->bootstrapAndFind('pm:enable')
#11 /var/www/html/vendor/symfony/console/Application.php(237): Drush\Application->find('pm:enable')
#12 /var/www/html/vendor/symfony/console/Application.php(149): Symfony\Component\Console\Application->doRun(Object(Drush\Symfony\DrushArgvInput), Object(Symfony\Component\Console\Output\ConsoleOutput))
#13 /var/www/html/vendor/drush/drush/src/Runtime/Runtime.php(118): Symfony\Component\Console\Application->run(Object(Drush\Symfony\DrushArgvInput), Object(Symfony\Component\Console\Output\ConsoleOutput))
#14 /var/www/html/vendor/drush/drush/src/Runtime/Runtime.php(48): Drush\Runtime\Runtime->doRun(Array, Object(Symfony\Component\Console\Output\ConsoleOutput))
#15 /var/www/html/vendor/drush/drush/drush.php(72): Drush\Runtime\Runtime->run(Array)
#16 /var/www/html/vendor/drush/drush/drush(4): require('/var/www/html/v...')
#17 {main}
ArgumentCountError: Too few arguments to function Drupal\default_content\Commands\DefaultContentCommands::__construct(), 1 passed in /var/www/html/web/core/lib/Drupal/Component/DependencyInjection/Container.php on line 262 and exactly 2 expected in Drupal\default_content\Commands\DefaultContentCommands->__construct() (line 38 of /var/www/html/web/modules/contrib/default_content/src/Commands/DefaultContentCommands.php).

There are both patches and a merge request for this issue, I think it makes sense to only use the merge request going forward (note that it is perfectly possible to create alternative merge requests for the same issue). I've hidden the attached files (including a screenshot, but I think that is only relevant to one particular comment - where it is actually embedded - and the file is not gone, it just doesn't appear in the list of attachments for the issue).

Maybe I'm misremembering, but I think the patch behaved differently in the past, and would simply ignore existing content. I personally feel that is much safer, so I wonder whether this is actually something that needs to be controllable with an option, somehow (i.e. default behaviour is non-destructive, if overwrite is desired, an option needs to be passed). For the default installation action, I think it is not a great idea to have content overwritten by default. Disabling and re-enabling a module would become a potentially destructive action. Maybe a warning is appropriate, such as "the module contains content X, but that already exists. If you want to overwrite it, use drush command Y with the Z option".

RTBC +1. The patched code from this issue branch worked great in updating existing content.

+1 from the open merge request https://git.drupalcode.org/project/default_content/-/merge_requests/5

I agree with eelkeblok that the module default content should only import content once and ignore them if they were changed. An option to override would also be nice.

The MR doesn't work for me currently. Just did the D9 upgrade. Will debug tomorrow further.

I have my default content serialized as json. For some reason the Drupal\default_content\Normalizer\ContentEntityNormalizer doesn't run, but the Drupal\hal\Normalizer\ContentEntityNormalizer does. Any idea why this is? I tried to add the normalizer tag to the service with a priority of 11, but the hal service was still prefered.
Obviously if the default_content normalizer is not used, we have no impact on checking if the entity exists.
I wonder if the normalizer is the right place to check the existence?

Also I saw if the entity exists further processing is skipped, which is great. (regarding prior comment.)

default_content 8.x-1.x only supports hal_json, it won't use anything else.

default_content 8.x-1.x is also basically unsupported and 2.0.x uses a custom serialization format that does not use the serialization API at all and I strongly suggest to update to that.

Hi

Long issue, I have to admit to not reading all the comments but the last few.
I agree that the safer option is to not override existing entities.
I created a new MR because the 2.0.x branch of the other one was confusing to work with. It does, however, contain the commit.

I just realized that there are other issues about the drush command, (#2640734: Allow manual imports) So I am removing that in order for the patches to conflict less.

Thanks @bircher! The code looks good. I tested the patch on a project with paragraphs. The PDOException is gone. Paragraphs are imported now without any issues 🎉

I'd vote for merging the MR and creating a release 🙏