Switch to entity owner in EntityContentBase during validation

Assigning to myself for review.

From May 7th:

Thanks for the patch.

1) Reviewed the code and it looks straight forward. I compared it to the AccountSwitcherInterfaceusage in core/lib/Drupal/Core/Cron.php and see they are doing dependency injection in the constructor but this is adding it via create. I'd need to look at more examples which I can't do at the moment (getting pulled away). I imagine the code is perfectly fine though. :)

2) Patch applies cleanly:

[mac:kristen:drupal-9.1.x-dev]$ patch -p1 < 3134470.patch
patching file core/modules/migrate/src/Plugin/migrate/destination/EntityContentBase.php

3) What is the best way to manually test this?

==

UPDATED (May 8th): I searched the codebase for something like:

protected function getX(): YInterface {

and didn't see anything. I like to review existing code that's similar when reviewing to help me validate the approach.

Ok, I was being too hasty yesterday and now see that this code is using some of the same code from the referenced migrate_tools module patch in #3134470: Switch to entity owner in EntityContentBase during validation which was based on the default_content module.

Looking at this code again and at other migrate code, unfortunately, it's not clear to me why accountSwitcher isn't part of the constructor and the getAccountSwitcher function is used.

Thanks for the explanation. Now I have dumb question...

Can't it not be added to the parameters in the constructor but still initialized in the constructor (presumably if people are extending the class they are calling the parent as this is a best practice, no?) and then the function wouldn't be required as it would always be set? Or we can't assume they would call the parent constructor?

Thanks for the test.

1) Patch applied cleanly:

[mac:kristen:drupal-9.1.x-dev]$ patch -p1 < 3134470-11.patch 
patching file core/modules/migrate/src/Plugin/migrate/destination/EntityContentBase.php
patching file core/modules/migrate/tests/src/Kernel/MigrateEntityContentValidationTest.php
Hunk #3 succeeded at 47 with fuzz 1.

2) Reviewed the code and could follow most of it though it would be nice to have some comments :) though there aren't many in the other tests.

1. +++ b/core/modules/migrate/tests/src/Kernel/MigrateEntityContentValidationTest.php
   @@ -33,9 +47,12 @@ class MigrateEntityContentValidationTest extends KernelTestBase {
   -    $this->installConfig(['system', 'user']);
   

   Unclear why this was deleted though I assume it wasn't needed.

2. +++ b/core/modules/migrate/tests/src/Kernel/MigrateEntityContentValidationTest.php
   @@ -33,9 +47,12 @@ class MigrateEntityContentValidationTest extends KernelTestBase {
   +
    
   

   Nitpick: extra new line.

3. +++ b/core/modules/migrate/tests/src/Kernel/MigrateEntityContentValidationTest.php
   @@ -140,6 +157,87 @@ public function test2() {
   +  /**
   +   * Tests validation where entity owner becomes the user who saves the entity.
   +   */
   +  public function testAccountSwitcher() {
   

   IMO this comment isn't clear. Not sure what's better but maybe something along the lines of the following, unless I'm not understanding the test:

   Tests validation where user without filter format permission saves content.

3) Still have question from #9.

4) Moving back to "Needs work" in case any of 2) should be addressed.

We crossposted so #14.3 was answered in #13. Thanks!

Thanks for the updates.

1) Reviewed the interdiff and the changes cover the items noted in #14.

The comments help a lot! :)

+++ b/core/modules/migrate/tests/src/Kernel/MigrateEntityContentValidationTest.php
@@ -33,9 +48,11 @@ class MigrateEntityContentValidationTest extends KernelTestBase {
+    $this->installSchema('system', ['sequences']);
+    $this->installConfig(['field', 'filter_test', 'system', 'user']);

Ah, see that installConfig was moved after installSchema. Thanks for pointing that out.

2) Patch applies to 8.9 (with offsets), 9.0, and 9.1.

[mac:kristen:drupal-8.9.x-dev]$ patch -p1 < 3134470-16.patch
patching file core/modules/migrate/src/Plugin/migrate/destination/EntityContentBase.php
Hunk #1 succeeded at 9 (offset 1 line).
Hunk #2 succeeded at 19 (offset 1 line).
Hunk #3 succeeded at 111 (offset 7 lines).
Hunk #4 succeeded at 149 (offset 7 lines).
Hunk #5 succeeded at 159 (offset 7 lines).
Hunk #6 succeeded at 202 (offset 7 lines).
Hunk #7 succeeded at 403 (offset 7 lines).
patching file core/modules/migrate/tests/src/Kernel/MigrateEntityContentValidationTest.php
Hunk #3 succeeded at 48 with fuzz 1.
[mac:kristen:drupal-8.9.x-dev]$ cd90
[mac:kristen:drupal-9.0.x-dev]$ patch -p1 < 3134470-16.patch
patching file core/modules/migrate/src/Plugin/migrate/destination/EntityContentBase.php
patching file core/modules/migrate/tests/src/Kernel/MigrateEntityContentValidationTest.php
[mac:kristen:drupal-9.0.x-dev]$ cd91
[mac:kristen:drupal-9.1.x-dev]$ patch -p1 < 3134470-16.patch
patching file core/modules/migrate/src/Plugin/migrate/destination/EntityContentBase.php
patching file core/modules/migrate/tests/src/Kernel/MigrateEntityContentValidationTest.php

3) Reviewed the patch again. Only a couple nitpicks I missed before caught my eye. Not sure they need to be changed but I'll move back to needs work in case they should be.

1. +++ b/core/modules/migrate/src/Plugin/migrate/destination/EntityContentBase.php
   @@ -184,8 +195,18 @@ public function isEntityValidationRequired(FieldableEntityInterface $entity) {
   +    if (($entity instanceof EntityOwnerInterface) && $user = $entity->getOwner()) {
   

   Sorry, should have caught this before.

   Nitpick: over 80 characters.

2. +++ b/core/modules/migrate/src/Plugin/migrate/destination/EntityContentBase.php
   @@ -184,8 +195,18 @@ public function isEntityValidationRequired(FieldableEntityInterface $entity) {
   +    if (($entity instanceof EntityOwnerInterface) && $user = $entity->getOwner()) {
   

   Same as above.

4) Not sure this will need manual testing before this can considered for RTBC.

Sorry, lack of sleep is becoming apparent here! Ignore me ;)

@heddn In your opinion, does this need manual testing prior to RTBC? Thanks.

Considering all the above, marking RTBC. Thanks for your patience!

I'd really like to see a failing patch and I decided to make it myself. I started from the patch in #16 and made a failing patch and re-uploaded 16.

And then I found I have some questions.

1. +++ b/core/modules/migrate/src/Plugin/migrate/destination/EntityContentBase.php
   @@ -184,8 +195,18 @@ public function isEntityValidationRequired(FieldableEntityInterface $entity) {
   +    if (($entity instanceof EntityOwnerInterface) && $user = $entity->getOwner()) {
   

   This test is done twice in the same method. Can't we do it just once? Should we check if the entity owner is the same as the current user before switching and switching back?

2. +++ b/core/modules/migrate/src/Plugin/migrate/destination/EntityContentBase.php
   @@ -375,4 +396,17 @@ public function getHighestId() {
   +   * Get the account switcher service.
   

   Should be 'Gets'.

3. +++ b/core/modules/migrate/tests/src/Kernel/MigrateEntityContentValidationTest.php
   @@ -140,6 +157,95 @@ public function test2() {
   +    // Filter formats have permissions tied to user/user roles.
   

   Does this mean user and user roles?

4. +++ b/core/modules/migrate/tests/src/Kernel/MigrateEntityContentValidationTest.php
   @@ -140,6 +157,95 @@ public function test2() {
   +            ]
   ...
   +            ]
   

   Missing trailing ','

5. +++ b/core/modules/migrate/tests/src/Kernel/MigrateEntityContentValidationTest.php
   @@ -140,6 +157,95 @@ public function test2() {
   +    // The second, non-permissioned user import should fail validation.
   

   I must be reading this wrong. The user is set to $normal_user but the content type that is owned by $normal user fails to migrate?

Nice! Glad this is getting more eyes on it :)

Setting to NW for #22

Thanks for the update.

1) Reviewing the interdiff in #26 and the items in #22, it appears that they are all addressed. I noticed one missing Oxford comma but I saw this so I don't want to hold this up further:

https://www.drupal.org/drupalorg/style-guide/words-phrases

But it's not doctrine. No one will be shamed for omitting an Oxford comma. (Editors will probably add that for you anyway.) And it doesn't create any grammar robot overlords (we hope).

1. +++ b/core/modules/migrate/tests/src/Kernel/MigrateEntityContentValidationTest.php
   @@ -161,7 +160,7 @@ public function test2() {
   +    // Filter formats have permissions tied to user, user roles and permissions.
   

   If this patch ends up being updated again, add Oxford comma after "roles".

2. +++ b/core/modules/migrate/tests/src/Kernel/MigrateEntityContentValidationTest.php
   @@ -241,7 +240,8 @@ public function testEntityOwnerValidation() {
   -    // The second, non-permissioned user import should fail validation.
   +    // The second, non-permissioned user import should fail validation because
   +    // they do not have access to use "filter_test" filter.
   

   Reading again, although I understand what "non-permissioned" means here, it turns out it's not commonly used. Only 4,800 results in Google for that. It's not used in core anywhere. Probably want to reword that. Perhaps just remove it since IMO the expanded text covers it.

   The second user import should fail validation because they do not have access to use "filter_test" filter.

2) Patch applies cleanly to 8.9, 9.0, and 9.1.

[mac:kristen:drupal-8.9.x-dev]$ patch -p1 < 3134470-26.patch
patching file core/modules/migrate/src/Plugin/migrate/destination/EntityContentBase.php
Hunk #1 succeeded at 9 (offset 1 line).
Hunk #2 succeeded at 19 (offset 1 line).
Hunk #3 succeeded at 111 (offset 7 lines).
Hunk #4 succeeded at 149 (offset 7 lines).
Hunk #5 succeeded at 159 (offset 7 lines).
Hunk #6 succeeded at 202 (offset 7 lines).
Hunk #7 succeeded at 404 (offset 7 lines).
patching file core/modules/migrate/tests/src/Kernel/MigrateEntityContentValidationTest.php
Hunk #3 succeeded at 47 with fuzz 1.
[mac:kristen:drupal-8.9.x-dev]$ cd90
[mac:kristen:drupal-9.0.x-dev]$ patch -p1 < 3134470-26.patch
patching file core/modules/migrate/src/Plugin/migrate/destination/EntityContentBase.php
patching file core/modules/migrate/tests/src/Kernel/MigrateEntityContentValidationTest.php
[mac:kristen:drupal-9.0.x-dev]$ cd91
[mac:kristen:drupal-9.1.x-dev]$ patch -p1 < 3134470-26.patch
patching file core/modules/migrate/src/Plugin/migrate/destination/EntityContentBase.php
patching file core/modules/migrate/tests/src/Kernel/MigrateEntityContentValidationTest.php

Moving to "Needs work" #27 as I think the rewording in 27.1.2 would make it easier to read.

+++ b/core/modules/migrate/src/Plugin/migrate/destination/EntityContentBase.php
@@ -184,8 +195,19 @@ public function isEntityValidationRequired(FieldableEntityInterface $entity) {
+    $switch_user = (($entity instanceof EntityOwnerInterface) && $user = $entity->getOwner());
+    if ($switch_user) {
+      $this->getAccountSwitcher()->switchTo($user);
+    }

Instead of having two variables here, $switch_user as a boolean flag and then $user as the object, why not just use $user as both the flag and the object?

$user = $entity instanceof EntityOwnerInterface ? $entity->getOwner() : NULL;
if ($user) {
  ...
}

Also I think we tend to use $account for generic account objects and $user as the actual "current user" though not sure this is set in stone, not sure if that would make it clearer here or not.

Altered the comment as suggested in #32.

Thanks for the update.

1) Reviewed the interdiff in #33 and see it addresses item from #32.

2) Patch applies cleanly to 9.1.

[mac:kristen:drupal-9.1.x-dev]$ patch -p1 < 3134470-33.patch
patching file core/modules/migrate/src/Plugin/migrate/destination/EntityContentBase.php
patching file core/modules/migrate/tests/src/Kernel/MigrateEntityContentValidationTest.php

3) Tests pass.

4) Marking RTBC based on this review and the comments above.

@heddn if they override the constructor then they'll be calling parent::__construct() - if we add AccountSwitcher to our __construct as $account_swticher = NULL and then trigger a deprecation when called without it we're fine and we'll break no one.

This pattern of doing pseudo setter inject in ::create() methods is okay for custom or contrib where you want to insulate yourself from potentially breaking upstream changes but I don't think we should use it all the time in core.

@heddn There's only an interdiff. Did you want to post the patch too or are you looking for feedback on the changes first?

Yeah we shouldn't re-order the arguments. In an ideal world the comment and user classes would be final and their constructor marked as @internal so we can change the order. But that's not the world we're in.

1. +++ b/core/modules/migrate/src/Plugin/migrate/destination/EntityContentBase.php
   @@ -375,4 +403,17 @@ public function getHighestId() {
   +  /**
   +   * Gets the account switcher service.
   +   *
   +   * @return \Drupal\Core\Session\AccountSwitcherInterface
   +   *   The account switcher service.
   +   */
   +  protected function getAccountSwitcher(): AccountSwitcherInterface {
   +    if (!isset($this->accountSwitcher)) {
   +      $this->accountSwitcher = \Drupal::service('account_switcher');
   +    }
   +    return $this->accountSwitcher;
   +  }
   

   This is no longer needed.

2. +++ b/core/modules/migrate/src/Plugin/migrate/destination/EntityContentBase.php
   @@ -184,8 +201,19 @@ public function isEntityValidationRequired(FieldableEntityInterface $entity) {
   +    // Entity validation can require the user that owns the entity. Switch to
   +    // use that user during validation.
   +    $account = $entity instanceof EntityOwnerInterface ? $entity->getOwner() : NULL;
   

   Do we have to be concerned about migration dependencies. Like is the user guaranteed to exist at this point?

3. +++ b/core/modules/user/src/Plugin/migrate/destination/EntityUser.php
   @@ -95,9 +96,15 @@ class EntityUser extends EntityContentBase {
   -  public function __construct(array $configuration, $plugin_id, $plugin_definition, MigrationInterface $migration, EntityStorageInterface $storage, array $bundles, EntityFieldManagerInterface $entity_field_manager, FieldTypePluginManagerInterface $field_type_manager, PasswordInterface $password) {
   -    parent::__construct($configuration, $plugin_id, $plugin_definition, $migration, $storage, $bundles, $entity_field_manager, $field_type_manager);
   +  public function __construct(array $configuration, $plugin_id, $plugin_definition, MigrationInterface $migration, EntityStorageInterface $storage, array $bundles, EntityFieldManagerInterface $entity_field_manager, FieldTypePluginManagerInterface $field_type_manager, PasswordInterface $password, AccountSwitcherInterface $account_switcher = NULL) {
   +    if ($account_switcher === NULL) {
   

   I was pondering about this from a migrate dependencies perspective and then realised that actually injecting this service is pointless because the user entity is not a EntityOwnerInterface entity.

   Kinda amusing to have to make this change for something that for this object will never be used.

@heddn re #41.2 probably worth adding a comment in the code about that then.

A brief look at the recent changes, since the RTBC.

+++ b/core/modules/comment/src/Plugin/migrate/destination/EntityComment.php
@@ -53,9 +54,15 @@ class EntityComment extends EntityContentBase {
+      @trigger_error('The account_switcher service must be passed to ' . __NAMESPACE__ . '\EntityComment::__construct(). It was added in drupal:9.1.0 and will be required before drupal:10.0.0.', E_USER_DEPRECATED);

If I am reading the policy correctly this needs to have when it will be removed and a CR.

%thing% is deprecated in %deprecation-version% (free text describing what will happen) %removal-version%. %extra-info%(optional). See %cr-link%

+++ b/core/modules/migrate/src/Plugin/migrate/destination/EntityRevision.php
@@ -114,11 +115,11 @@ class EntityRevision extends EntityContentBase {
+  public function __construct(array $configuration, $plugin_id, $plugin_definition, MigrationInterface $migration, EntityStorageInterface $storage, array $bundles, EntityFieldManagerInterface $entity_field_manager, FieldTypePluginManagerInterface $field_type_manager, AccountSwitcherInterface $account_switcher) {

Doesn't this need a trigger_error as well?

I'm wondering if this behaviour should be optional. For example, imagine permissions are being changed as part of the migration. Also maybe for performance reasons or other reasons someone if running the migration as user 1.

I read the CR and it looks fine to me and does the patch. I don't see anything still to do.

However, I have one question. I applied that patch and ran phpcs on migrate/destination/EntityContentBase.php and get the following error and there are not coding standard errors reported by drupalci. Does the message need to change?

----------------------------------------------------------------------
 141 | ERROR   | The trigger_error message 'The account_switcher
     |         | service must be passed to  __NAMESPACE__
     |         | \EntityContentBase::__construct(). It was added in
     |         | drupal:9.1.0 and will be required before
     |         | drupal:10.0.0. See
     |         | https://www.drupal.org/node/3142975.' does not match
     |         | the relaxed standard format: %thing% is deprecated
     |         | in %deprecation-version% any free text
     |         | %removal-version%. %extra-info%. See %cr-link%

But if you want entity validation, then you need to use the right user ID.

Is this the case? We don't switch before saving a node that was created by someone else.

@heddn yeah I think this is a tricky thing. It's going to depend so much on how your new site is configured as to what is the correct thing to do. I'm not sure it is possible for there to be a right way.

1. +++ b/core/modules/migrate/src/Plugin/migrate/destination/EntityContentBase.php
   @@ -184,7 +201,18 @@ public function isEntityValidationRequired(FieldableEntityInterface $entity) {
   +    // Entity validation can require the user that owns the entity. Switch to
   +    // use that user during validation.
   

   Can we point to an example validation constraint that requires this? (using @see)

2. +++ b/core/modules/migrate/src/Plugin/migrate/destination/EntityContentBase.php
   @@ -184,7 +201,18 @@ public function isEntityValidationRequired(FieldableEntityInterface $entity) {
   +    $account = $entity instanceof EntityOwnerInterface ? $entity->getOwner() : NULL;
   

   🤔 This implies that every migration using a EntityContentBase subclass as a destination for which the entity type implements EntityOwnerInterface MUST have a required migration dependency on the d7_user migration.

   Is it at all possible to add an assert() that adds the logic that checks that?

   Preferably in \Drupal\migrate\Plugin\migrate\destination\EntityContentBase::__construct(), where a MigrationInterface object is readily available, and also to avoid us asserting that for every entity we validate, which is pointless.

3. +++ b/core/modules/migrate/tests/src/Kernel/MigrateEntityContentValidationTest.php
   @@ -140,6 +156,96 @@ public function test2() {
   +    // Filter format access is impacted by user permissions.
   ...
   +    // Add a "body" field with the filter format.
   

   🤓 s/filter format/text format/

4. +++ b/core/modules/migrate/tests/src/Kernel/MigrateEntityContentValidationTest.php
   @@ -140,6 +156,96 @@ public function test2() {
   +    // Attempt to migrate entities. One owned by the admin user, the other is a
   +    // non-permissioned user.
   

   🤓 The second sentence here doesn't really make sense.

5. +++ b/core/modules/migrate/tests/src/Kernel/MigrateEntityContentValidationTest.php
   @@ -140,6 +156,96 @@ public function test2() {
   +    $this->assertSame(sprintf('2: [entity_test: 2]: user_id.0.target_id=This entity (<em class="placeholder">user</em>: <em class="placeholder">%s</em>) cannot be referenced.||%s.0.format=The value you selected is not a valid choice.', $normal_user->id(), $field_name), $this->messages[0], 'First message should have 2 validation errors.');
   

   👎 Not specifying a failure message here (that second parameter to assertSame()) yields far more helpful failing test output.

   Could you please remove that? 🙏😊

Back to needs work per #58.

#61: RE #58.2: Right, that makes sense! 👍

My key remaining concern is then that we ensure that the test coverage actually tests what we need it to test. @quietone already asked this question and proved it in #22 👍

I would RTBC this … but unfortunately at least the first of the following remarks must be addressed before this can get committed. The remainder are nitpicks that I am fine with not being addressed, since they're only in tests.

1. +++ b/core/modules/migrate/src/Plugin/migrate/destination/EntityContentBase.php
   @@ -122,11 +131,18 @@ class EntityContentBase extends Entity implements HighestIdInterface, MigrateVal
   +      @trigger_error('The account_switcher service must be passed to ' . __NAMESPACE__ . '\EntityContentBase::__construct(). It was added in drupal:9.1.0 and will be required before drupal:10.0.0. See https://www.drupal.org/node/3142975.', E_USER_DEPRECATED);
   
   +++ b/core/modules/migrate/tests/src/Kernel/MigrateEntityContentBaseTest.php
   @@ -301,4 +302,24 @@ public function testStubRows() {
   +    $this->expectDeprecation('The account_switcher service must be passed to Drupal\migrate\Plugin\migrate\destination\EntityContentBase::__construct(). It was added in drupal:9.1.0 and will be required before drupal:10.0.0. See https://www.drupal.org/node/3142975.');
   

   This now needs to be updated to 9.2.0.

2. +++ b/core/modules/migrate/tests/src/Kernel/MigrateEntityContentValidationTest.php
   @@ -140,6 +156,95 @@ public function test2() {
   +    // Filter format access is impacted by user permissions.
   

   Missed this spot in #58.

3. +++ b/core/modules/migrate/tests/src/Kernel/MigrateEntityContentValidationTest.php
   @@ -140,6 +156,95 @@ public function test2() {
   +    // Create 2 users, an admin user who has filter permission and another who
   

   Related nit: s/who has filter permission/who has permission to use this text format/

What about the points raised in #56 about files and path aliases? In that comment heddn suggests that path alias should be owned by uid 1. Will enabling validation on path alias migration break things for people?

I re-read the issue and reviewed the patch, finding a few things, showing that I have been dabbling in coding standards issues. The comments in test, MigrateEntityContentValidationTest::testEntityOwnerValidation have improved since my last look at this and it is much easier to understand. And along with the suggestion by Wim Leers it will be even better.

1. +++ b/core/modules/comment/src/Plugin/migrate/destination/EntityComment.php
   @@ -53,9 +54,11 @@ class EntityComment extends EntityContentBase {
   +   * @param \Drupal\Core\Session\AccountSwitcherInterface $account_switcher
   

   Null is allowed here. So methinks this should be
   @param \Drupal\Core\Session\AccountSwitcherInterface|null $account_switcher

2. +++ b/core/modules/user/src/Plugin/migrate/destination/EntityUser.php
   @@ -95,9 +96,11 @@ class EntityUser extends EntityContentBase {
   +   * @param \Drupal\Core\Session\AccountSwitcherInterface $account_switcher
   

   Same here

Next I read the CR. I do like that it is concise but in this case perhaps too concise. I think that the phrase , "When validation occurs during a migration" needs explanation. I suggesting adding an example, as in the IS here that shows how one enables validation. And even a link to the CR that added validation would be nice, https://www.drupal.org/node/3073707.

I remembered there was a tag for updating the change record. See #66

All the feedback from #65 and #66 addressed.

Fixed PHPCBF issue.

Fixed PHPCBF whitespace issues.

Nice! This test case demonstrates exactly the same issue that I encountered myself and this is exactly the solution I was going to propose before I found this issue :D

+++ b/core/modules/migrate/tests/src/Kernel/MigrateEntityContentBaseTest.php
@@ -301,4 +302,24 @@ public function testStubRows() {
+    $this->expectDeprecation('The account_switcher service must be passed to Drupal\migrate\Plugin\migrate\destination\EntityContentBase::__construct(). It was added in drupal:9.3.0 and will be required before drupal:10.0.0. See https://www.drupal.org/node/3142975.');

Nit: it won't be required before Drupal 10. The phrasing below matches other, similar deprecation examples.

Calling Drupal\migrate\Plugin\migrate\destination\EntityContentBase::__construct() without the $account_switcher argument is deprecated in drupal:9.3.0 and will be required in drupal:10.0.0. See https://www.drupal.org/node/3142975

The overwhelming majority of them do not have a period after the See <url> so I removed that dot as well.

IMHO, this can be RTBC'd because everything else looks good here. I made the change above myself so that this doesn't die by a death of a 1000 nits.

I've updated the CR

IMO, this is a bug since it is almost impossible to validate imported entities which have a text format that the anonymous user isn't allowed to use (which is often the case)

One question and a couple of nits

1. +++ b/core/modules/migrate/src/Plugin/migrate/destination/EntityContentBase.php
   @@ -185,7 +202,20 @@ public function isEntityValidationRequired(FieldableEntityInterface $entity) {
        $violations = $entity->validate();
   

   should we wrap this in a try catch and then do the switch back in a finally?

   just in case something in validation throws an exception?

2. +++ b/core/modules/migrate/tests/src/Kernel/MigrateEntityContentValidationTest.php
   @@ -140,6 +156,99 @@ public function test2() {
   +    /**
   +     * @var \Drupal\filter\FilterFormatInterface $filter_test_format
   +     */
   

   nit; should we use assert($filter_test_format instanceof FilterFormatInterface) instead?

3. +++ b/core/modules/migrate/tests/src/Kernel/MigrateEntityContentValidationTest.php
   @@ -140,6 +156,99 @@ public function test2() {
   +    /**
   +     * @var \Drupal\user\RoleInterface $role
   +     */
   

   nit: similarly here?

Thanks @larowlan!

should we wrap this in a try catch and then do the switch back in a finally?

just in case something in validation throws an exception?

This is wise. I added a finally block but left off the catch block for the reasons in the patch comment. This means that the account will be switched back whether there is an exception or not and the exception will bubble up just as it does today.

Every row import() is wrapped in a try/catch in MigrateExecutable, which captures the exception and stores it in the migration messages table. If I caught it, I would have had to either silence the exception or duplicate that logic.

This looks like probably the only fix available within migrate, but feels like it's an issue with filter format validation in general.

FilterFormat::access() accepts an $account parameter, however we can't pass that to ::validate().

This wouldn't have come up when we were doing validation in form API, it's because we're also trying to handle REST requests, but migrate is neither and feels like this kind of use case has been missed.

There's another problem here as well. There's no guarantee that the entity owner has access to the text format at the point the content is migrated, for various reasons:

1. The target site has different permissions for the text format.

2. The user is no longer in the role that has permissions for the text format.

3. The content was edited by an admin user, and the text format was changed to one that the entity owner doesn't have access too.

... and etc.

So I think this means we'd need a way for validation to bypass the access check in text format validation altogether. Moving back to needs review for more discussion.

This wouldn't have come up when we were doing validation in form API, it's because we're also trying to handle REST requests, but migrate is neither and feels like this kind of use case has been missed.

Migrate feels like an exceptional case. Validating entities instead of specific forms means Drupal can safely handle updates by users via HTTP requests (which includes form POSTs), that seems like right use case to optimize for. Everything else I can think of falls into developer-only scenarios. E.g. drush hooks, cron jobs, migrations, etc.

This is a best-effort resolution which makes a reasonable assumption IMO. Entity validation during migrations is an opt-in feature too, which makes it even more acceptable. Just disable it if its an enormous problem.

1 & 2: most Drupal-to-drupal migrate stuff in core assumes you're upgrading from D7 to D8+ without making any changes so this is not out of the ordinary. Whether that was a good idea is a much, much bigger fish to fry.

3: This is likely a one-off situation. The DX will be:

• see the validation error in the migrate messages
• decide whether it's too much work to resolve
• if it is
  • turn off validation for that migration
• it it isn't, either
  • go into the source site and change the format/owner
  • or write a process plugin

Setting this back to RTBC. I believe @catch's concerns were addressed by @gabesullice's and @heddn's comments.

Reroll

Small +1 on the re-roll, I arrived to exactly the same patch, and it looks good, even if I have not examined it in details, so it testbot confirms it is likely ready.

#82 and #83 makes sense - either don't validate, or validate with the correct user.

Committed 8065ee3 and pushed to 9.3.x. Thanks!

Publish CR