ContentEntity source plugin should exclude user with uid "0"

+++ b/core/modules/migrate_drupal/src/Plugin/migrate/source/ContentEntity.php
@@ -217,6 +217,12 @@ public function query() {
+    // @see \Drupal\user\Plugin\migrate\source\d6\User::query()
+    // @see \Drupal\user\Plugin\migrate\source\d7\User::query()

I was going to ask to add @see … but you already did 🤩

Looks like something we should be testing.

I'm hopeful we can test that relatively easily in \Drupal\Tests\migrate_drupal\Kernel\Plugin\migrate\source\ContentEntityTest 🤞

This issue is not specific to Drupal 7 sources, removing tags. One could argue that this could have a migrate-d8+-d8+ tag, but there are only two source plugins for the current version of Drupal so I don't think it is necessary.

I've created a merge request with @huzooka's patch and I've fixed ContentEntityTest as suggested by @Wim Leers.

ContentEntityTest::testUserSource() already has an assertion to check that only one user is in the $user_source (line 258) so I just added a second user with uid 0 which should be excluded.

Just a few changes and this will be ready.

@quietone all the changes are done as suggested. Please review it.

@sudiptadas19, Thanks for the updates. Since this issue is using a MR it would be easier if work continued on the MR and not reintroduce the patch system. When making a patch normally I expect an interdiff but this is so small I guess that can be skipped this time.

+++ b/core/modules/migrate_drupal/tests/src/Kernel/Plugin/migrate/source/ContentEntityTest.php
@@ -155,6 +155,12 @@ protected function setUp(): void {
+      'name' => 'anonymous',
...
+    $this->anonymousUser->save();

Why is this changed?

Added changes as suggested in MR 358. Please review it.

Ah, wow now there are two merge requests. That is a new situation for me.

@sudiptadas19, there is a question in #13.

@quietone

+++ b/core/modules/migrate_drupal/tests/src/Kernel/Plugin/migrate/source/ContentEntityTest.php
@@ -155,6 +155,12 @@ protected function setUp(): void {
+ 'name' => 'anonymous',
...
+ $this->anonymousUser->save();

We are creating an anonymous user object here, that is the reason I made those changes to be more accurate.
'name' => 'anon' to 'name' => 'anonymous',
$this->anonUser to $this->anonymousUser

any problem with these changes? please suggest.

There are now two branches and two merge requests for this and it is not making it easy to work on this. And besides, this is a small change and doesn't need multiple branches, in my opinion. If another person works on this issue which MR do they use/review? It is clear as mud.

The first, MR320 has unresolved issues. All but one of those has been resolved in the second MR358. I can not find a way to mark the unresolved issues and resolved. I would like to close MR320 but I can't find a way to do that.

I say work here is now on MR358, which looks good. I just have one question.

I will repeat the question for the one unresolved question from MR320. It is minor but I would like to know ...

The condition in the two queries referred to is different than what is done here.
->condition('u.uid', 0, '>');
Why the change?

The condition in the two queries referred to is different than what is done here.
->condition('u.uid', 0, '>');
Why the change?

@quietone in our case we are using $this->entityType->getKey('id'), this will return string|bool that is the reason we have used this condition. $query->condition($this->entityType->getKey('id'), '0', '<>')

Hmm. If it does return FALSE, then trying to set the condition will thrown a QueryException. Perhaps that could be handled better? Then again, just above is this,

    if (!empty($this->configuration['bundle'])) {
      $query->condition($this->entityType->getKey('bundle'), $this->configuration['bundle']);
    }

So, at least using getKey this way is consistent. I've search core and do see that this pattern is common.

But my question, which admittedly could have been clearer, is why use '<>' and not '>'? Is it possible for a uid to be less than zero? Or is this just being defensive.

@quietone we have put a condition for string '0'. To check uid is not equal to '0' we have used '<>'. If we go with int 0 then we can go for '>' instead of '<>'.

But my question, which admittedly could have been clearer, is why use '<>' and not '>'? Is it possible for a uid to be less than zero?

No, it is not possible that uid will be lesser than zero.

Can we go with condition($this->entityType->getKey('id'), 0, '>') instead of - condition($this->entityType->getKey('id'), '0', '<>') ?

Added suggested changes, available in MR358. Please review.

@sudiptadas19, thanks. That makes more sense to me because uid is an integer.

I think all that is left is to make 358 mergeable, I am sure the problem is because #3184650: ContentEntity migration source adds revision ID as source key, incompatible with Drupal 8.8 and earlier was comitted.

@anmolgoyal74 thanks for make MR 358 mergeable.

Please review the changes.

While it is good that 358 is now mergeable it also means I can't apply it as a patch locally, which I like to do so I can read the code in my editor with my colors and fonts. Oh well, that boat has sailed on this one. Then I found a line was missing. I found it in the other MR but could not get a link to that one line, what a nuisance. So, I copy/pasted.

The good news is, we are one step closer.

@quietone

$this->assertEquals(1, $user_source->count());

is there, now it is wrapped in a condition

if (!$configuration['include_translations']) {
$this->assertEquals(1, $user_source->count());
}

I have not added this condition. Please confirm whether need to do a modification.

@sudiptadas19, thanks! I clearly am having problem adjusting to gitlab interface and I forgot that #3184650: ContentEntity migration source adds revision ID as source key, incompatible with Drupal 8.8 and earlier was committed.

So, yes, MR 358 is RTBC. But I think that the unresolved issue needs to be closed first.

Thanks, @quietone for the confirmation. Moving this to review state.

Looks like the work here is to get MR 358 mergeable, it isn't showing up as mergeable.

@sudiptadas19, it looks like you haven't touched this for a while, remember to un-assign and set the status to Need Review. I would have noticed this much earlier otherwise.

After all the work MR358 is not mergeable. Let's go back to a patch please, there has been too much time wasted on fiddling with the MRs.

Marking this as a novice task for the creation of a patch from MR358.

Created a patch from MR358, as suggested by @quietone.
Not adding an interdiff, as this is a small change.

Added a new patch for the 9.3.x version.

@sudiptadas19, thank you!

I applied the patch and reviewed it. It took me a bit to remember why the assertion is not made when translations are involved. But it just takes a look at the ContentEntity::count to see why. Plus the same if block to avoid testing counts is used for other entities in the test so this is not doing anything new. And there is an issue to get counts when translations are involve. #2937166: Improve count() for ContentEntity source plugin, so there is no need for a followup.

Finally, back to RTBC.

Committed/pushed to 9.3.x and cherry-picked to 9.2.x, thanks!