[PP-1] Allow manual imports

1. +++ b/drush/default_content.drush.inc
   @@ -97,3 +109,42 @@ function drush_default_content_export_module($module_name) {
   +  if (!\Drupal::isConfigSyncing()) {
   

   Perhaps this check should be moved into the importContent if it's important? Currently it's checked twice in the drush file.

2. +++ b/drush/default_content.drush.inc
   @@ -97,3 +109,42 @@ function drush_default_content_export_module($module_name) {
   +function drush_default_content_import_module_validate($module) {
   

   Could this same validation be used by the other drush commands that take a module as an argument?

3. +++ b/drush/default_content.drush.inc
   @@ -97,3 +109,42 @@ function drush_default_content_export_module($module_name) {
   +    return drush_set_error('INEXISTENT_MODULE', dt("Module or profile '@module' doesn't exist or is uninstalled.", ['@module' => $module]));
   

   Is INEXISTENT_MODULE a drush thing? Is non-existent meant here?

4. +++ b/drush/default_content.drush.inc
   @@ -97,3 +109,42 @@ function drush_default_content_export_module($module_name) {
   + * Imports or updates the content provided by all enabled modules and default
   + * profile.
   

   Nit: short-descriptions should be one line.

5. +++ b/src/DefaultContentManager.php
   @@ -213,6 +213,11 @@ class DefaultContentManager implements DefaultContentManagerInterface {
   +          // Allow existing entities overwrite. Delete first an existing entity
   +          // with the same uuid.
   +          if ($existing_entity = $this->entityManager->loadEntityByUuid($entity_type_id, $entity->uuid())) {
   +            $existing_entity->delete();
   +          }
   

   Would be good to get some more eyes over this.

+++ b/src/DefaultContentManager.php
@@ -213,8 +213,16 @@ class DefaultContentManager implements DefaultContentManagerInterface {
+          $existing_entity = $this->entityManager->loadEntityByUuid($entity_type_id, $entity->uuid());

Shouldn't this be $this->entityRepository->loadEntityByUuid(...) ?

Worked for me. Thanks!

Created a fixed patch for those who need a link to file.

#7 confirmed as working, great!
I doubt the "$existing_entity->delete();" part though. For standalone entities this is OK, but what about references?
Case:
Import terms. Use these for tagging nodes (adding references from nodes to terms). Than "update" these terms by delete followed by save. All references would be gone right? This is not updating, this is re-adding, which can be OK in some cases.

This patch has a more intelligent "update" handling of existing entities: https://www.drupal.org/node/2698425#comment-11377803

I combined the patches

• https://www.drupal.org/node/2640734#comment-11774345
• https://www.drupal.org/node/2698425#comment-11377803

Tested #11 and it seems good. It fixes several issues we've had.

- Can install a content module on a site where the node already existed without throwing an error.
- Can install new content once the module has already be run by executing the drush command.

After applying patch #11 i was able to run the import once, but on the second time i was unable to access /admin/content and got the same error listed here: http://drupal.stackexchange.com/questions/200492/can-not-access-contents...

Recoverable fatal error: Argument 1 passed to Drupal\views\Plugin\views\field\EntityOperations::getEntityTranslation() must implement interface Drupal\Core\Entity\EntityInterface, null given, called in /mnt/www/html/secgovstg/docroot/core/modules/views/src/Plugin/views/field/EntityOperations.php on line 111 and defined in Drupal\views\Plugin\views\field\EntityOperations->getEntityTranslation() (line 69 of /mnt/www/html/secgovstg/docroot/core/modules/views/src/Entity/Render/EntityTranslationRenderTrait.php).

Interesting, van you add steps to reproduce please?

Like this?

Check, thanks.

Tested #11, complex entity (nested entity references), works smoothly

Very nice indeed @keesje, works like a dream. I have a complex setup as well with paragraphs and file entities.

I have a few small updates to this patch because I'm a pedant:

• Have removed the phpdoc @return declaration from the drush command functions because returning boolean and void is not particularly pleasant :)
• Have moved the !empty()check from the _drush_default_content_valid_module() function and made $module a required param on the basis that it seems a bit frivolous to check whether a non-existent module is a valid module :)
• Have renamed the new import commands (and added an export all command) on the basis that the naming was inconsistent: 'default-content-export' exports just a single entity and 'default-content-import' imports all entities, confused me a little.
• Added the new export all command to make triggering of single/multiple module imports/exports consistent, and because it would be a useful addition... contentious.

#22 Manually import working great, thanks!

It needs work, for translations and revisions

1. +++ b/drush/default_content.drush.inc
   @@ -93,15 +117,112 @@ function drush_default_content_export_references($entity_type_id, $entity_id = N
   - * Exports all of the content for a given module.
   + * Exports all of the content for a given module and default profile.
   

   I see no profile involved...

2. +++ b/drush/default_content.drush.inc
   @@ -93,15 +117,112 @@ function drush_default_content_export_references($entity_type_id, $entity_id = N
   +  if (empty($modules) || !is_array($modules)) {
   +    $modules = array_keys(\Drupal::config('core.extension')->get('module'));
   

   I'd better export nothing if no modules passed, so where is a profile?

3. +++ b/drush/default_content.drush.inc
   @@ -93,15 +117,112 @@ function drush_default_content_export_references($entity_type_id, $entity_id = N
   +  $t = \Drupal::translation();
   ...
   +      drush_log($t->formatPlural($count, '@module: 1 entry exported', '@module: @count entries exported', ['@module' => $module]), LogLevel::OK);
   ...
   +    drush_log($t->formatPlural($total, 'Total: 1 entry exported', 'Total: @count entries exported'), LogLevel::OK);
   

   No need, just use new PluralTranslatableMarkup

4. +++ b/src/DefaultContentManager.php
   @@ -220,8 +220,25 @@ class DefaultContentManager implements DefaultContentManagerInterface {
   -          $created[$entity->uuid()] = $entity;
   +          ¶
   +          // Allow existing entities overwrite.
   

   nit, trailing writespace

5. +++ b/src/DefaultContentManager.php
   @@ -220,8 +220,25 @@ class DefaultContentManager implements DefaultContentManagerInterface {
   +              try {
   +                $entity->setNewRevision(FALSE);
   +              } catch (\LogicException $e) {}
   

   better to check that entity supports this method, check core's RevisionableInterface

also better split that patch because it does not what subject said

Hi,

it is worth noting that importing existing users (in particular the admin user) works only on the same site installation.

For instance, this works:

$ drush default-content-export-references --folder=profiles/my_profile/content node
$ drush default-content-import-modules --update
my_profile: 4 entries imported                                                                                             [ok]
Total: 4 entries imported

However importing some previously exported content into a new site does not work, at least not when importing the admin user if any node references it.

To reproduce, install using a profile without content, and then try to import some exported nodes authored by admin:

$ cp ../previously_exported_content profiles/my_profile/content
$ drush default-content-import-modules --update
Drupal\Core\Database\IntegrityConstraintViolationException: SQLSTATE[23000]: Integrity constraint violation: 1062 Duplicate entry '1' for key       [error]
...

This is not specific to manual import tho, it happens also at install time if the profile provides the content/ dir, I am just mentioning the issue here to make people aware; see #2698425-4: Do not re-import existing entities and #2698425-19: Do not re-import existing entities for more details.

Thanks,
Antonio

BTW, the cp command in the example above also suggest that some import command which accepts a --folder option could make sense, for symmetry with default-content-export-references.

Feels like we're moving beyond the realm of default content here, into the deploy realm.

I'm not keen to introduce additional complexity to support use-cases that are beyond the scope of the module.

Can you convince the maintainers that this is in fact for default content and not for deploying?

@larowlan valid point!

Regarding my own comments, my point was just that default_content should be able to re-import (either manually or at install time) whatever has been previously exported.

If it exports the admin user because it was referenced by some node, I'd expect it to be able to re-import it.

My use case would be to share a baseline site (an installation profile with config and some default content) to make it easier to demonstrate bugs, or to create themes.

Personally I like to see issue with uid (0|1) fixed in separate in
#2698425: Do not re-import existing entities or #2815051: Skip exporting core users by default

Ok, thanks - seems reasonable @claudiu.cristea

Agree with @andypost that the existing issues cover the pain points around admin user.

In case anyone else is having errors related to importing entity reference revisions, see #2848029: Prevent duplicate revisions by checking whether a host entity is already saved

Just tested that on project, and discovered that domain_access module blocking loading nodes so this command require UID=1 switch and tests

1. +++ b/src/DefaultContentManager.php
   @@ -220,8 +220,25 @@ class DefaultContentManager implements DefaultContentManagerInterface {
              $entity->enforceIsNew(TRUE);
   ...
   +          else {
   +            $entity->enforceIsNew(TRUE);
   +          }
   

   called twice

2. +++ b/src/DefaultContentManager.php
   @@ -220,8 +220,25 @@ class DefaultContentManager implements DefaultContentManagerInterface {
   -          $created[$entity->uuid()] = $entity;
   +          ¶
   +          // Allow existing entities overwrite.
   

   trailing spaces

3. +++ b/src/DefaultContentManager.php
   @@ -220,8 +220,25 @@ class DefaultContentManager implements DefaultContentManagerInterface {
   +              $entity->setOriginalId($old_entity->id());
   +              $entity->enforceIsNew(FALSE);
   

   setOriginalId() already calls next line internally

4. +++ b/src/DefaultContentManager.php
   @@ -220,8 +220,25 @@ class DefaultContentManager implements DefaultContentManagerInterface {
   +              try {
   +                $entity->setNewRevision(FALSE);
   +              } catch (\LogicException $e) {}
   

   there should be check for revisionable interface and somehow bc for core 8.3.x

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

   I'd prefer that as separate patch #2698425: Do not re-import existing entities

Thanks Andy, could we clear up point 5 first. This ticket has a lot to do with #2698425: Do not reimport existing entities because the work for that ticket was originally done on this ticket. So as @vijaycs85 points out we either have to remove it from this ticket or combine the two together.

If we remove it, we have to block this ticket until that one is merged, so that we can correctly add the flag to the drush command. Also if we remove it I think it would be good to put your points 1-4 on that ticket and not this.

Happy either way, just trying to avoid blocking one ticket for too long.

Either way it would be good to have tests that cover the import. I guess this should really have been done previously so could either be covered by another ticket or by #2698425: Do not reimport existing entities.

Oh yea and in regards to tests, it might be quite good to convert them to PHPUnit tests before we get too far.Or at least write the new tests in PHPUnit.

As has been suggested by a few people, here is a patch that separates the other import issues. Of course this only provides drush commands now, so all comments on import bugs can be raised separately or on existing tickets https://www.drupal.org/node/2698425 and https://www.drupal.org/node/2815051

The patch does not apply cleanly anymore after commit 11fa54825b611dc280dd0621b090c2b53dea0f29

I rebased the patch on the latest code.

A normal interdiff between the two patches didn't work, but I am attaching a diff between the them, the changes since #38 are:

• the addition of the run-as option
• the consequent addition of the _drush_default_content_setup(); and _drush_default_content_teardown(); calls in drush_default_content_export_modules and drush_default_content_import_modules
• patch context updates

All looks super good. Reviewed and tested.

One thing, we should make this depend on #2698425: Do not reimport existing entities getting in. And then add the 'update' flag to all drush import commands.

I also successfully reviewed and tested this together with #2698425.

I'd like to commit more complicated issue first #2614644: Split DefaultContentManager into Exporter and Importer

In the meantime I am hiding the patch from #41 (because of #2857589: Remove the run-as option in drush commands) and showing again the one from #38, which applies again on the latest 8.x-1.x branch.

Yep, let's get the split in first, please help with rerolls and reviews over there

I generated a new patch for those like me who want a drush command to update the default content right now.
This patch adjusts the work in #38 to the changes in https://www.drupal.org/node/2698425#comment-12001085
and allows to update content using the option update_existing in "drush default-content-import-module" and "drush default-content-import-modules"

#2614644: Split DefaultContentManager into Exporter and Importer

Note: https://github.com/larowlan/default_content/pull/60 does something similar but instead moves most of the logic into the Importer class, so as such makes is much easier to test.

Cool, I think it would be good to try to get this one in first as there are quite a few tickets that could use or extend this. As a result I've stripped out all the functionality not directly related to adding a manual import command. Will raise any separate issues as needed.

@piggito I'm with you on wanting the update flag in right now, but in the interests of keeping issues separate I haven't added to the re-roll as per #36.

@dawehner agreed, getting as much logic into the importer and exporter classes will allow other UIs to use in the future. Updated with the re-roll.

No interdiff on that last patch because it's a complete re-roll. Everything's moved.

@kalpaitch
Do you want to pull in my test coverage as well? I strongly believe this would be valueable here :)

@dawehner I think your dead right, but really these are just drush commands. If it's alright I've added this to #2868710: Add tests for Importer to improve the test coverage of this new Importservice...

It is a bit hard to see why we don't pull them from https://github.com/dawehner/default_content/blob/import-command/tests/sr... directly :)

1. +++ b/drush/default_content.drush.inc
   @@ -107,3 +128,57 @@ function drush_default_content_export_module($module_name) {
   + *
   + * @param string $module_name
   ...
   +function drush_default_content_import_module($module) {
   

   Documentation no longer matches the signature.

2. +++ b/src/Importer.php
   @@ -201,6 +212,17 @@ class Importer implements ImporterInterface {
   +    $profile_name = drupal_get_profile();
   

   We could inject the installation profile container parameter.

3. +++ b/src/Importer.php
   @@ -201,6 +212,17 @@ class Importer implements ImporterInterface {
   +    return array_reduce(array_unique($module_names + [$profile_name]), function (array $entities, $module_name) {
   +      return array_merge($entities, $this->importContent($module_name));
   +    }, []);
   

   I added some documentation on top of it, to make it clear what this method tries to do. Yeah for using array_reduce().

4. +++ b/src/ImporterInterface.php
   @@ -18,4 +18,12 @@ interface ImporterInterface {
   +   * @return array[\Drupal\Core\Entity\EntityInterface]
   

   That is a weird syntax for a list of entities. I used the other one which is already used in the same file.

+++ b/tests/src/Kernel/ImporterIntegrationTest.php
@@ -0,0 +1,108 @@
+  } ¶

nit, whitespace issue

Otherwise, looking good - can be fixed on commit

#58 could be fixed on commit, it conflicts with another patch #2698425: Do not re-import existing entities
Guess to commit that later

+++ b/src/Importer.php
@@ -201,6 +222,17 @@ class Importer implements ImporterInterface {
+  public function importAllContent() {
...
+    return array_reduce(array_unique($module_names + [$this->installProfile]), function (array $entities, $module_name) {
+      return array_merge($entities, $this->importContent($module_name));

That may need another argument to pass after #2698425: Do not re-import existing entities

Reroll-ed

It seems like the ImportIntegrationTest wasn't considering 2 nodes in default_conten_test and caused patch #61 to fail. I worked over patch #61 and fixed the test.

Looks good to me, imo we should get this in before #2698425: Do not re-import existing entities

+++ b/tests/src/Kernel/ImporterIntegrationTest.php
@@ -0,0 +1,118 @@
+    $this->assertInstanceOf(TermInterface::class, $entities['550f86ad-aa11-4047-953f-636d42889f85']);
+    $this->assertInstanceOf(NodeInterface::class, $entities['65c412a3-b83f-4efb-8a05-5a6ecea10ad4']);
+    $this->assertInstanceOf(NodeInterface::class, $entities['78c412a3-b83f-4efb-8a05-5a6ecea10ad4']);
+    $this->assertInstanceOf(NodeInterface::class, $entities['78c412a3-b83f-4efb-8a05-5a6ecea10aee']);
+
+    $this->assertCount(4, $entities);
+    // Ensure the content can be actually loaded.
+    $this->assertEquals('A tag', Term::load($entities['550f86ad-aa11-4047-953f-636d42889f85']->id())->label());
+    $this->assertEquals('Imported node', Node::load($entities['65c412a3-b83f-4efb-8a05-5a6ecea10ad4']->id())->label());
+    $this->assertEquals('Imported node with owned by user 1', Node::load($entities['78c412a3-b83f-4efb-8a05-5a6ecea10ad4']->id())->label());
+    $this->assertEquals('Imported node with owned by user that does not exist', Node::load($entities['78c412a3-b83f-4efb-8a05-5a6ecea10aee']->id())->label());
...
+    $this->assertInstanceOf(TermInterface::class, $entities['550f86ad-aa11-4047-953f-636d42889f85']);
+    $this->assertInstanceOf(NodeInterface::class, $entities['65c412a3-b83f-4efb-8a05-5a6ecea10ad4']);
+    $this->assertInstanceOf(NodeInterface::class, $entities['78c412a3-b83f-4efb-8a05-5a6ecea10ad4']);
+    $this->assertInstanceOf(NodeInterface::class, $entities['78c412a3-b83f-4efb-8a05-5a6ecea10aee']);
+
+    $this->assertCount(4, $entities);
+    // Ensure the content can be actually loaded.
+    $this->assertEquals('A tag', Term::load($entities['550f86ad-aa11-4047-953f-636d42889f85']->id())->label());
+    $this->assertEquals('Imported node', Node::load($entities['65c412a3-b83f-4efb-8a05-5a6ecea10ad4']->id())->label());
+    $this->assertEquals('Imported node with owned by user 1', Node::load($entities['78c412a3-b83f-4efb-8a05-5a6ecea10ad4']->id())->label());
+    $this->assertEquals('Imported node with owned by user that does not exist', Node::load($entities['78c412a3-b83f-4efb-8a05-5a6ecea10aee']->id())->label());

these lines are duplicated, please move them to a method such as doImportTests and call it in both places

Patch was updated: Duplicated code in test was moved to the separated method.

+++ b/tests/src/Kernel/ImporterIntegrationTest.php
@@ -72,14 +72,12 @@
+   *
+   * @param array $entities
+   *   Entities list to test.

Nitpick: You could use TermInterface[] here, but this is a super minor change to be honest.

#67 works for me! When the patch could be applied?

Awesome, looks good.

Yep, this can go in before the next release

Added Drush 9 support from https://www.drupal.org/project/default_content/issues/2912723

Forgot add 'extra' section to composer.json

Hi, everyone.
Does this patch supports re-import of entities ?
I have following error on `drush dcia`

Integrity constraint violation: 1062 Duplicate entry '1' for key 'PRIMARY': INSERT INTO {block_content} (id, revision_id, type,            
uuid, langcode) VALUES (:db_insert_placeholder_0, :db_insert_placeholder_1, :db_insert_placeholder_2, :db_insert_placeholder_3, :db_insert_placeholder_4);

That would be #2698425: Do not re-import existing entities (I do believe it also supports updating existing entities, despite the title).

Would it make sense to commit this first as #67 and add the Drush 9 stuff in a separate issue? Sorry, disregard. Drush 9 support has been added to the existing drush commands, so it only makes sense to have newly added drush commands to support it too.

Patch from #74 doesn't apply to -dev anymore. Anyone have time / knowledge to re-roll?

After a quick look I realized the changes to composer.json were previously committed in https://cgit.drupalcode.org/default_content/commit/?id=a3bc209 for issue https://www.drupal.org/node/2826455. Just that bit needed to be removed for the patch to apply cleanly.

I've made the patch from #72 the most current and hidden #74 as the only difference between the two was the extras section in composer.json.

When running the drush dcia command provided by this patch, if the default content user directory doesn't exist (perhaps because you are using the patch from https://www.drupal.org/project/default_content/issues/2815051), the following error occurs.

 [error]  RecursiveDirectoryIterator::__construct(modules/custom/mfgusa_deploy/content/user): failed to open dir: No such file or directory 

If the directory exists and is empty, no such error occurs.

no new patch?

Sorry, I'm unable to contribute a patch to help resolve the error I mentioned. I also realize that the functionality from the other issue (Skip exporting core users) doesn't seem to be working anyway as it keeps exporting the admin user.

Re-rolled.

+1, this would be extremely helpful for us who want to deploy content and manage them through updates.

#83 working great.

Needs work for the @module arg to PluralTranslatableMarkup

1. +++ b/drush/default_content.drush.inc
   @@ -107,3 +128,57 @@ function drush_default_content_export_module($module_name) {
   +    $message = new PluralTranslatableMarkup($count, 'Total: 1 entity imported.', '@module: @count entries imported.');
   

   Should we have the arguments here for @module? Should @module be in the singular string too?

2. +++ b/drush/default_content.drush.inc
   @@ -107,3 +128,57 @@ function drush_default_content_export_module($module_name) {
   +      drush_set_error('INVALID_MODULE', dt("Module or profile '@module' doesn't exist or is uninstalled.", ['@module' => $module]));
   

   neat trick

3. +++ b/src/Commands/DefaultContentCommands.php
   @@ -89,11 +112,71 @@ class DefaultContentCommands extends DrushCommands {
   -      ->getModule($module)
   -      ->getPath() . '/content';
   +        ->getModule($module)
   +        ->getPath() . '/content';
   

   unneeded?

4. +++ b/src/Commands/DefaultContentCommands.php
   @@ -89,11 +112,71 @@ class DefaultContentCommands extends DrushCommands {
   +      $message = new PluralTranslatableMarkup($count, 'Total: 1 entity imported.', '@module: @count entries imported.');
   

   same issue here with the @module

5. +++ b/src/Commands/DefaultContentCommands.php
   @@ -89,11 +112,71 @@ class DefaultContentCommands extends DrushCommands {
   +   * @param string|null $module
   +   *   An installed module or the default profile. If not passed, the validation
   +   *   passes.
   

   why do we need to support the null case?

6. +++ b/src/Importer.php
   @@ -200,6 +221,17 @@ class Importer implements ImporterInterface {
   +    return array_reduce(array_unique($module_names + [$this->installProfile]), function (array $entities, $module_name) {
   +      return array_merge($entities, $this->importContent($module_name));
   +    }, []);
   

   nice

7. +++ b/src/ImporterInterface.php
   @@ -18,4 +18,12 @@ interface ImporterInterface {
   +   * Imports all default content across the site.
   

   should we qualify this with 'from enabled modules and the active profile'?

1 and 4 addressed. I mostly rerolled because I'd been annoyed by this for a while and was working on something related. Total is clearly copied from the other command so replaced with @module as well.
2. agreed
3. diff removed.
5. That very much looks like PhpStorm guessing something. I don't see a reason this method is different from the procedural version or a reason explicitly handling null would be needed. I just remove null from the doc.
7. probably. Changed to "Imports default content from all enabled modules and active profile."

Bah, I really messed that up. Description is right but I did my work on the wrong branch and got another patch mixed in. You'll notice the interdiff is mostly the same though.

I have successfully tested #88 and it looks like we now need options for Drush to set $update_existing to true, because there doesn't exist any code yet which changed that variable from FALSE to TRUE. Is that correct? If so, I'd be happy to provide a modified patch for that.

so #2698425: Do not re-import existing entities and this are kinda mucking about in the same place and conflict. That's were I messed up in #87 because I had been messing with merging the two patches and what that would look like. Which ever one goes in first will need to be updated to support the other and I have a patch locally that adds the drush options for update. Its pretty trivial but it doesn't make sense until one of the patches lands.

Just nitpicking, sometimes the word "entity" is used instead of "entry":

diff --git a/drush/default_content.drush.inc b/drush/default_content.drush.inc
index fc01d00..e2aaf29 100644
--- a/drush/default_content.drush.inc
+++ b/drush/default_content.drush.inc
@@ -143,7 +143,7 @@ function drush_default_content_import_module($module) {
   /** @var \Drupal\default_content\ImporterInterface $importer */
   $importer = \Drupal::service('default_content.importer');
   if ($count = count($importer->importContent($module))) {
-    $message = new PluralTranslatableMarkup($count, '@module: 1 entity imported.', '@module: @count entries imported.', [
+    $message = new PluralTranslatableMarkup($count, '@module: 1 entry imported.', '@module: @count entries imported.', [
       '@module' => $module,
     ]);
     drush_log($message, LogLevel::OK);
diff --git a/src/Commands/DefaultContentCommands.php b/src/Commands/DefaultContentCommands.php
index ee0c4d3..d8a757e 100644
--- a/src/Commands/DefaultContentCommands.php
+++ b/src/Commands/DefaultContentCommands.php
@@ -138,7 +138,7 @@ class DefaultContentCommands extends DrushCommands {
     }

     if ($count = count($this->defaultContentImporter->importContent($module))) {
-      $message = new PluralTranslatableMarkup($count, '@module: 1 entity imported.', '@module: @count entries imported.', [
+      $message = new PluralTranslatableMarkup($count, '@module: 1 entry imported.', '@module: @count entries imported.', [
         '@module' => $module,
       ]);
       $this->logger()->log(LogLevel::OK, $message);

I tested #88 and it works well for me when importing content the first time, so I'd say le't merge it and move forward.

The change is very useful even though it does not support all use cases yet.

I have added to the patch from #88 the option update-existing to the Drush 9 command default-content:import-module

Hi,
the patch in #93 wasn't applying anymore

line 11 in default_content.services.yml

arguments: ['@serializer', '@entity_type.manager', '@entity.repository', '@hal.link_manager', '@event_dispatcher', '@module_handler', '@info_parser', '%default_content.link_domain%']

is now injecting the account switcher services

arguments: ['@serializer', '@entity_type.manager', '@entity.repository', '@hal.link_manager', '@event_dispatcher', '@module_handler', '@info_parser', '%default_content.link_domain%', '@account_switcher']

so I've updated the patch to fix that.

Extra : Little summary about patches provided here
By the way, this patch (and previousely #93) only make sense if you also plan to use the patch of https://www.drupal.org/project/default_content/issues/2698425

this 2 issues (and their patches) are required if you want to be able to import content without having to uninstall / install a module and also if you want to avoid conflict when importing an already existing entity.

My patch and #93 allow to pass a flag (--update-existing) that is only used by the code provided in issues 2698425.
For the record you will have to choose which patch to apply automatically, and which one to apply manually, because the are incompatible.

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

The command reports that nothing has been importing when something has.

drush default-content:import-module re_report --update-existing
[warning] No content has been imported.

I think that importContent() should return $created + $updated;

Hi,
the patch in #94 wasn't applying anymore

line 11 in default_content.services.yml

arguments: arguments: ['@serializer', '@entity_type.manager', '@entity.repository', '@hal.link_manager', '@event_dispatcher', '@module_handler', '@info_parser', '%default_content.link_domain%', '@account_switcher']
is now injecting the file system services

arguments: arguments: ['@serializer', '@entity_type.manager', '@entity.repository', '@hal.link_manager', '@event_dispatcher', '@module_handler', '@info_parser', '%default_content.link_domain%', '@account_switcher', '@file_system']
so I've updated the patch to fix that.

And another re-roll of 2640734-94.default_content.Allow-manual-imports.patch if you have applied "Do not reimport existing entities": "https://www.drupal.org/files/issues/2020-02-05/2698425--accept-content-u..." already.

Since, as I understood from another issue, the 1.x version is not going to get any new features (great fan of the yml-approach of 2.x, btw), I suppose this will need retargeting to 2.x as well.

Here's a patch from the branch in the MR against 2.0.x, in case anyone wants to test without having to checkout the branch.

@bgihome You can have the patch when someone is doing a merge request:
https://git.drupalcode.org/project/default_content/-/merge_requests/1.patch

I can apply patch #102 on default_content 2.0.x-dev.
But if I execute the command on my exported nodes, like this:
drush default-content:import-module my_custom_module
Then I get this error:

In ContentEntityBase.php line 955:
Invalid translation language (fr) specified.

For me all import problems are fixed by using these patches:

• #102 from this issue
• #136 from https://www.drupal.org/project/default_content/issues/2698425
• #2 from https://www.drupal.org/project/default_content/issues/3176839

Reroll. Can someone check if this patch still does what it was intended to? There were apparently a lot of changes lately. Thanks.

drush dcim custom_module still overwrites existing content.
It has the same result as drush dcim custom_module --update-existing

The importContent() (Importer.php:152) does not allow the options argument given in contentImportModule() (DefaultContentCommands.php:140).
Therefore the flag 'update-existing' doesn't do anything. Even when documentation states otherwise.

Unfortunately the content is currently always being overwritten.

This adds support for the '--update-existing' flag to be used with the 'import all' and 'import module' commands. It will now force update all existing entities with exported values. If you don't use that flag it will only import new entities.

I noticed some code for exporting/importing json and wasn't sure if we care about that now. I'm not sure if that should be removed or not.

Using filter_var for better user experience while flag handling. I.e. you can pass '--update-existing=false' and expect it not to update existing entities. In the previous version only something like omitting or '--update-existing=0' would work.

Hi @rowrowrow. Can I check if it was intentional to introduce changes from #2698425: Do not re-import existing entities to this MR in #108? This patch no longer applies for us as a result.

FYI (you may be aware of this), you can combine patches in cweagans/composer-patches, eg:

    "drupal/default_content": {
      "#2698425: Do not reimport existing entities": "https://git.drupalcode.org/project/default_content/-/merge_requests/14.diff",
      "#2640734: Allow manual imports": "https://git.drupalcode.org/project/default_content/-/merge_requests/1.diff"
    },

However, the above no longer applies, and a user referring to MR1 would now have some or all of the changes from #2698425: Do not re-import existing entities / MR14 also. It does become complicated to handle multiple patches to a project. Currently MR1 here is unclear to me, and I'd propose that we either clarify that or revert those changes and rework so they can apply cleanly (one MR per issue, one issue per MR).

That said, I'm not the maintainer, just a fellow contributor - so what I say isn't the rules ;)

(Caveat: It's not recommended practice to refer to MR URLs for composer patches, as they are subject to breakage via changes. That's a separate discussion!)

Hey @xurizaemon, yes it was intentional. I found that I wasn't able to apply the patch from https://www.drupal.org/project/default_content/issues/2698425 cleanly after making some necessary changes and some of the logic from that patch was required to get the update-existing flags working. I was also using both this patch and the other but am able to solely use this one for both purposes.

To me there is clearly an overlap between the two issues as being able to stop the update of existing entities is a prerequisite for enabling/disabling them using something like an update-existing flag. Hopefully that makes sense and I hope I'm not messing things up here too much. If we need to undo stuff let me know and I'm happy to help.

Also, @here I think we should add a flag for updating entities only if their data differs, i.e. to avoid the consequences of saving an unchanged entity like caching etc... I had some thoughts about how to do it but I think that should be a separate ticket.

OK. Thanks for the quick reply @rowrowrowrow!

If it makes sense for the issues need to be combined then no objection from me, with the request that this be communicated / coordinated between the affected issues. The impact for us was that an automated dev build tracking both of those MR URLs started to fail because when 83fe76a5 appears in both patches, the patch process fails, breaking the build. Fixed now, no hard feelings, just wanted to clarify the plan here :)

Looking back - I don't recall what the purpose of the MR opened in #107 was - maybe a reroll of current patches, I'm not sure the MR added anything at all. It's likely the intent there was to generate a patch URL which applied to current 2.0.x, or something like that.

Anyway - attached is a version of the patch which I believe corresponds to !1 before that latest set of changes - this is just in case someone else finds themself in the same boat and winds up here looking for a patch to use.

Please note: the attached patch does not include any changes beyond #107 and is not a proposed update to the WIP in the MR.

Cheers

Hi everyone !

Thanks for this great patch, hope it'll be merged soon in the module ! (and thanks of course for this great module ;-) ).

I rerolled the patch against 2.0.0-alpha1 version just to fix some deprecations and an error displayed when calling "drush dcim":

In Logger.php line 161:
                                      
  The log level "ok" does not exist.  
                                      

I wanted to roll the patch against dev version of the module but it doesn't work so I kept with the 2.0.0-alpha1 release.

Thanks again !

Best regards,
Bastien

Using the patch in #114 I had an error while running drush dcia --update-existing:
[warning] Undefined variable $update_existing Importer.php:292

The reason is that the closure in importAllContent() does not inherit $update_existing from the parent scope.

Fix in line 291:

return array_reduce(array_unique($module_names + [$this->installProfile]), function (array $entities, $module_name) use ($update_existing) {

Attached the fixed patch.

Thanks, patch from #115 works for me

Updated MR 20 to resolve a conflict with the latest 2.x.
Does what it says though I did not look for edge cases. +1 RTBC.

Attaching a copy of the MR20 patch as at e164a354 (comment 124). No changes introduced here and not directing effort back to .patch uploads, just making a static patch available for builds which isn't a mutable MR patch URL.

(apologies for the update noise, .diff this time)

Updating to 'needs review'.

This still needs work, most of the remarks from @Berdir in the MR still need to be resolved.

I am working on removing the changes that were out of scope of this issue and in scope of #2698425: Do not re-import existing entities. However that breaks the tests here. We are getting integrity constraint violations on existing content (which is the exact problem that is being solved in #2698425: Do not re-import existing entities).

So this indicates that that #2698425: Do not re-import existing entities is a hard blocker for this issue.

I propose to do the following:

1. Leave the existing branch untouched to not needlessly disrupt existing users of this patch.
2. Create a new branch that will combine this issue with #2698425: Do not re-import existing entities. We can continue the work in that branch, but since it will contain the code from both issues it will be harder to review. Ideally we should finish the other issue first.
3. Mark this as postponed on #2698425: Do not re-import existing entities.

Postponed on #2698425: Do not re-import existing entities.

So this indicates that that #2698425: Do not reimport existing entities is a hard blocker for this issue.

Yeah it always kinda has been. LOL I'd been maintaining a combined patch for a project for that reason.

Because its a bit hard to test #2698425: Do not re-import existing entities without this here's a patch rerolling !24 over the merge request from the other issue.

The interface doesn't document it (which is its own bug of sorts) but Importer::importContent can throw exceptions. This triggers confusing fatal errors from Drush so it would be best to handle them in the command.

Also "No, I think we should have this logic inside the Drush command. " but the importAllContent command was removed entirely. I haven't added it here but seems like it was probably a useful command.

Also also, unrelated but the decode calls in importContent can throw exceptions which should be caught and re-thrown as something useful. To see this in action try putting your yaml export in a .json file and see the parse error crash things. Not that anyone would ever do that by accident...

I'm thinking to be on the safe side we should default the overwriting of existing content to FALSE so it is safer by default. If people do want to overwrite existing content they'll have to manually add the --update option.

I updated !24 with the latest commits from #2698425: Do not re-import existing entities and changed the default to FALSE.

Also "No, I think we should have this logic inside the Drush command. " but the importAllContent command was removed entirely. I haven't added it here but seems like it was probably a useful command.

When the extension list is left empty, the dcim command in the latest MR will import content from all installed extensions, so I think that is what the import all used to do, right?

We tested the MR 24 and it works perfectly. We noticed, though, that if a dependency file miss Default content simply ignore them, because the method Graph::searchAndSort from core doesn't add them to the array list returned and sorted.

The only trace of them is found in `vertexes`

We'd like to propose a small, but very useful, patch that adds a log warning when some dependencies are missing.

hi
i am trying to aplly the MR24 on D10.2 with php 8.2 without success can somebody help with this

Thanks

Hi!
I updated MR 24 with the latest code from 2.0.x: 542125c commit Issue #3428127 by deepakkm, ankitv18, Berdir: Automated Drupal 11 compatibility fixes for default_content