Skip exporting core users by default

+++ b/drush/default_content.drush.inc
@@ -79,20 +80,55 @@ function drush_default_content_export_references($entity_type_id, $entity_id = N
+ * Removes default core users from the list of all entities.
+ *
+ * @param array $entities
+ *   Keyed array of entities by entity type.
+ */
+function _drush_default_content_remove_core_users(&$entities) {
+  if (empty($entities['user'])) {
+    return;
+  }
+  foreach ($entities['user'] as $uuid => $user) {
+    // Deserialize as array to iterate values.
+    $data = json_decode($user, TRUE);
+    foreach ($data['uid'] as $item) {
+      foreach ($item as $key => $value) {
+        if ($key === 'value' && ($value === '0' || $value === '1')) {
+          unset($entities['user'][$uuid]);
+        }
+      }
+    }
+  }
+  if (empty($entities['user'])) {
+    // Skip to create empty directory.

Rather than do this here, I think this should be a third argument to the managers's exportContentWithReferences method. That way we don't even serialize them.

Adding patch with logic in the exportContentWithReferences

Working from 2815051-exclude-uids-3.patch: added third argument to exportContentWithReference calls in DefaultContentManagerIntegrationTest.

Hello!
These patches look like great starts. Imo, it makes sense to default this skip_core_users to TRUE, since it seems like that'd be the most common use case as mentioned in the original ticket description (there was a todo in #2 indicating that this would eventually make sense as well).

Also, I think this could use a little bit of testing, which is included in this updated patch.

It was pointed out that we can get the result with --skip-core-users=0

I changed the drush option to include-core-users. This option allows the user to include the core users, but maintains the default of excluding users 0 and 1.

Hi,

the patch from #8 seems to do what it says drush default-content-export-references now do not export json files for the core users (in my case I have some content authored by admin (1)).

However I am not sure this is the right solution to the import problem, because the UUID of the users are still referenced in the node files, and on new sites the UUIDs of the core users may be different.

For example if I export some nodes authored by admin (1) with the patch in #8 and then provide the exported data in a content/ dir in a installation profile for a new site, the imported content results as authored by Anonymous and not by the original author.

I suspect this has to do with UUIDs.

Maybe the problem should be really solved at import time, fixing the import of core users, see #2698425-19: Do not reimport existing entities.

Thanks,
Antonio

Here is a re-roll of the patch, which did not apply cleanly anymore after the addition of the run-as option in commit 11fa54825b611dc280dd0621b090c2b53dea0f29.

I am also attaching a second patch to propose a solution for the problem of importing content authored by core users: the rationale is that the UUID of the users 0 and 1 in the data to import gets overridden by the UUIDs of these users on the current site.

With this second patch I can import content authored by user 1 exported from a previous incarnation of the site, and it still shows as authored by user 1 on the new site. I only noticed a problem with translations sill imported as authored by Anonymous even if they were authored by user 1 originally, but this can be worked around by passing the --run-as=1 when importing the content (I am using the patch in #2640734: Allow manual imports for that).

If the idea seems sound I can clean up the patch further, and investigate the problem with translations.

Thanks,
Antonio

Rebasing the patches on top of the latest 8.x-1.x, the previous ones do not apply anymore after #2857589: Remove the run-as option in drush commands and #2827140: Simple refactoring of drush dcer.

Patch 0001 is the one fixing this issue, and should be in pretty good shape.
Patch 0002 is only an experiment about fixing the import problem, this can be proposed in a different issue if necessary.

Ciao,
Antonio

1. +++ b/src/DefaultContentManager.php
   @@ -268,9 +268,14 @@ class DefaultContentManager implements DefaultContentManagerInterface {
   +      $skip = !$include_core_users && $entity->getEntityTypeId() == 'user' && in_array($entity->id(), [0, 1]);
   

   nits:
   ===
   and
   in_array((int) $entity->id(), [0, 1], TRUE)

   remember in PHP in_array('f', [0, 1]) returns TRUE because the 'f' is cast to 0.

   Also, no need to create the intermediate variable $skip, just put it in the if()

2. +++ b/src/DefaultContentManagerInterface.php
   @@ -46,11 +46,13 @@ interface DefaultContentManagerInterface {
   +   *   Whether or not to skip superuser and anonymous accounts when exporting.
   

   Can we add (optional) to the start of this doc comment, and , defaults to FALSE at the end.

   Is defaulting to FALSE the right behaviour? We're changing the defaults here - i.e, anyone with an existing workflow is going to be caught by this change, having it and the command default to TRUE would retain BC

Actually, I think ao2's "Patch 0002" makes more sense -- treat users 0 and 1 as special cases, and match imported content to those uids, circumventing UUID match.

We're changing the defaults here - i.e, anyone with an existing workflow is going to be caught by this change, having it and the command default to TRUE would retain BC

I can speak for my own workflow, which is to manually exclude user 0 and 1, because i kept getting conflicts since user 0 and 1 will always exist on an already-installed site. The existing behavior only works when importing content to the same site from which it was exported, which I think is not the intended use case for this module.

Also flagging for consideration #2689069: Do not import entity IDs (included in exports from Drupal 8.1.x)

If we are going to override the UUID for content authored by users 0 and 1 (like I proposed in patch "0002"), we may as well go one step further and also ignore user entities with uid 0 and 1 at import time, and stop caring at all about the behavior at export time.

Here's a simple re-roll of 0002-override_core_users_uuids-281505-12.patch that applies to the latest dev (After "Split DefaultContentManager into Exporter and Importer" was committed).

Here's another reroll that applies to commit 68c30918 (alpha6). I presume we still need tests for this?

Disclaimer: I haven't actually functionally tested this, just did the conflict resolutions that seemed to make most sense. I ran out of my alotted time to actually do some testing based on it. Setting to needs review to have existing tests ran.

Edit: leaving as-is, probably needs a review also.

Quick review (basically what PHPCS says upon closer inspection of the code):

1. +    $this->core_users_uuids = [
   +      0 => \Drupal::entityTypeManager()->getStorage('user')->load(0)->uuid(),
   +      1 => \Drupal::entityTypeManager()->getStorage('user')->load(1)->uuid()
   +    ];
   +    $this->overridden_uuids = [
   +      0 => "",
   +      1 => ""
   +    ];
   

   These assignments violate coding standard array syntax, multiline arrays need to have a comma after each line. Also, the member variables are dynamically defined. They need to be defined separately above the member functions, with a docblock comment explaining what they do. Naming using snake_case is nog consistent with the lowerCamelCase in the rest of the class.

2. +  private function overrideCoreUsersUuids(&$item) {
   +    // If the content is from a system user, override its UUID to match the
   +    // one of the current site.
   

   This function needs a docblock explaining what it does.

3. +        // match the end of the string
   

   Inline comments need to start with upper case and end in full-stops, exclamation marks, colons, question marks, or closing parentheses.

Here's a patch that addresses the above concerns. Also it introduces the notion that the core users might have multiple different UUIDs if the entities were exported from multiple different environments. I've also run into [#2698425: Do not reimport existing entities] during my testing, which means I need to see whether this issue is still relevant to me. Possibly, it solves this problem as well.

There's a module to help fix this issue.
https://www.drupal.org/project/default_content_extra

Every time someone creates a {some_module}_extras instead of collaborating on getting issues like this one to RTBC and committed, a kitten gets killed

Fixing the test in #24.

Fixing a small typo that caught my eye. Also, updating the issue summary with remaining tasks.

Looks good, works as exepcted. @eelkeblok would be good to test these issues as well please.

1. +++ b/src/Importer.php
   @@ -109,6 +127,53 @@ class Importer implements ImporterInterface {
   +      0 => \Drupal::entityTypeManager()->getStorage('user')->load(0)->uuid(),
   +      1 => \Drupal::entityTypeManager()->getStorage('user')->load(1)->uuid(),
   

   Please use $this->entityTypeManager or the passed in entity type manager instead of using the singleton

2. +++ b/src/Importer.php
   @@ -109,6 +127,53 @@ class Importer implements ImporterInterface {
   +  private function overrideCoreUsersUuids(&$item) {
   

   Let's make this protected instead of private

3. +++ b/src/Importer.php
   @@ -109,6 +127,53 @@ class Importer implements ImporterInterface {
   +          if (!is_array($this->coreUsersImportUuids[$uid])) {
   +            $this->coreUsersImportUuids[$uid] = [];
   +          }
   

   When would this not be an array?

4. +++ b/src/Importer.php
   @@ -109,6 +127,53 @@ class Importer implements ImporterInterface {
   +  private function replaceCoreUserUuidsInString(&$contents) {
   

   Let's make this protected too

5. +++ b/src/Importer.php
   @@ -140,6 +205,7 @@ class Importer implements ImporterInterface {
   +          $this->overrideCoreUsersUuids($decoded);
   
   @@ -164,6 +230,7 @@ class Importer implements ImporterInterface {
   +              $this->overrideCoreUsersUuids($item);
   

   Why do we call this twice, once with $decoded and again with $item - seems only need the second?

Just a functional observation: as it stands, the module including this patch still exports the users themselves. It you subsequently try to import the default content that will break because the user will still be imported and will now have a collission on its uid (which seems to be a problem in itself, although I have not checked if there is an open issues for that).

One solution would be to also apply #2698425: Do not reimport existing entities, but I think it would be cleaner if the user was (after all) not exported in the first place. I.e. I think we need to combine both patches; core users should not be exported by default and there should be some uid-based matching to get content to match to users that do exist in the target site.

This addresses the concerns in #33.

3. When would this not be an array?

When the particular uid has not been encountered in the import data yet. However, I've found this doesn't actually work as intended because the key will be invalid (and PHP wil complain). I changed it to !isset().

5. Why do we call this twice, once with $decoded and again with $item - seems only need the second?

AFAICT (this is a while ago), the first one is the item itself, whereas the second call handles the references to other entities.

Oops. Previous patch did not include one small last minute change I made to get this to actually work. Here is the updated patch. Only difference is that the previous version tried to use $this->entityTypeManager() as a method.

Adressing #34. I've included what is basically 0001-skip-exporting-core-users-by-default-2815051-12.patch from #12, except that that didn't apply anymore because of some structural changes. Also, the new method now has its default set to TRUE for including core users, so that if anyone has built their own code around this functionality, the code will continue to work the same way.

The default for the drush option is still to not include core users if the option is ommited and to include core users if the option is passed (--include-core-users). I don't think we should be too worried about backwards compatibility, the release is still marked alpha, after all.

Input welcome if the defaults make sense. I worry maybe it is confusing the code has one default and the drush command another.

I might take crack at some tests next.

Right. How enlightening writing tests can be (and sorry for the spam... seems this issue has become my own personal echo chamber). I've come to realize that if the default content does not include core users, everything works fine (sorry if I'm late to the party). It is only when the DC does include the core users, problems start. I would actually argue there is only two reasonable situations:

1. Your DC should be tied to the standard root user, or the anonymous user, regardless of their uuid (since these are determined when installing the site).
2. You actually want these users as part of the DC and they should be imported as "any" other content. It just so happens they have IDs that are more likely to conflict with pre-existing "content".

For 1, I would argue that the users should simply not be part of your default content, and having them anyway is messy. Introducing the behaviour that the users are not exported by default will help prevent this situation. Anyone who finds themselves in this situation should remove the exported users and stop exporting them (by applying this patch/waiting for a release that includes it).

For 2, I believe that is basically covered by #2698425: Do not reimport existing entities now (IIRC, it is now targeted to update content with the same UUID and add content with a different UUID, disregarding the actual ID). In the meantime, people finding themselves in this situation are probably already somehow dealing with it, e.g. by changing the ids in the DC to not conflict with IDs in a freshly installed site.

Please find attached a brave new patch that only implements the drush option and adds a test to make sure it works. No interdiff, as this patch stands on its own.

Thoughts?

That was the point of that patch, testbot.

Patch in #38 works for me.

Scratch that last comment. After applying this patch and using the command drush dcer taxonomy_term --folder=path/to/folder, I'm still seeing the admin user account .json file exported to the content/user directory.

The patch didn't work for me as well.
I ended up deleting the user and its references manually for now :)

Updated patch supports Drupal 9 too.

Please take a look!

patch from #45 is still exporting the admin user to a json file when I export a node (with paragraphs):
drush dcer node 1 --folder=modules/custom/myproject_default_content/content

+++ b/src/Commands/DefaultContentCommands.php
@@ -62,9 +62,10 @@ public function contentExport($entity_type_id, $entity_id, $options = ['file' =>
+  public function contentExportReferences($entity_type_id, $entity_id = NULL, $options = ['folder' => NULL, 'include-core-users' => TRUE]) {

I think the default should be FALSE. Setting --include-core-users will set it to TRUE, not setting it should not default to "enabled".
We already have $include_core_users = drush_get_option('include-core-users', FALSE); in place which also defaults to FALSE - so changing this in the method signature makes the default consistent.
I think this should also address the issue @flyke reported in #48, I suspect a difference in drush 8/9 handling.

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

User 0 and 1 are automatically and always referenced as target_id 0/1 instead of UUID, which should resolve this.

User 0 and 1 are automatically and always referenced as target_id 0/1 instead of UUID, which should resolve this.

How do you mean that? For me drush dcer user --folder="whatever" does export user 0 and 1.

Yes, the users themself are still exported, I suppose we could skip that too. What I mean is that if you export nodes or comments or whatever that have the author as 0/1 or another user, then that reference is hardcoded to 0/1 instead of the UUID and they can be imported correctly.

Oh I see. In any case exporting system users feels unnatural but we should at least allow the exportation of users 0/1 in some way.
Maybe we could allow the exportation, only if it they are exported directly with id and not by a mass exportation. I will link the relevant issue for the mass exportation.
Should we reopen this to tackle the issue, or archive this and create a more precise issue?

Agreed, ignoring them unless explicitly specify by ID seems fine, I don't think we need any special drush options for that.

Lets reopen this issue.