Commerce entities implementing EntityOwnerInterface should always return a UserInterface object

Hi @mglaman, here is the path. Could you please check it.

Added some changes in tests, it was made the same way as in OrderTest for non-existent/deleted user ID case.

We have this in Product/Store:

      // If no owner has been set explicitly, make the anonymous user the owner.
      if (!$translation->getOwner()) {
        $translation->setOwnerId(0);
      }

Guessing it will no longer trigger. This was a fix for a crash (where saving a NULL as the uid wasn't accepted), our test coverage is either incomplete, or the crash is no longer a problem. Will need to doublecheck.

I suppose that for User::getAnonymousUser(); a crash it's not a problem for now. Do I need to change something according to that in the patch?

Ok, so the attached patch introduced an EntityOwnerTrait in Commerce that basically uses core's EntityOwnerTrait and simply override getOwner() so we don't have to duplicate this logic everywhere.

I also replaced:

      // If no owner has been set explicitly, make the anonymous user the owner.
      if (!$translation->getOwner()) {
        $translation->setOwnerId(0);
      }

by

      // Explicitly set the owner ID to 0 if the translation owner is anonymous
      // (This will ensure we don't store a broken reference in case the user
      // no longer exists).
      if ($translation->getOwner()->isAnonymous()) {
        $translation->setOwnerId(0);
      }

and added tests coverage for that logic that we were missing.

Should we rename our EntityOwnerTrait to CommerceEntityOwnerTrait? Or just keep it as is?
EntityOwnerTrait has been introduced in Drupal core 8.7 so it's safe to use.

Thanks, Committed!

Warning: call_user_func() expects parameter 1 to be a valid callback, class 'Drupal\commerce_product\Entity\Product' does not have a method 'getCurrentUserId' in Drupal\Core\Field\FieldConfigBase->getDefaultValue() (line 397 of core/lib/Drupal/Core/Field/FieldConfigBase.php).

I opened a products view, and got this warning, so this means we probably need an update hook.

I've been trying to fix this via an update hook and couldn't really manage, (because base fields are overridden on my install...

So I'm wondering if our EntityOwnerTrait shouldn't provide a getCurrentUserId() method for BC reason.

And if we do, should we still do the following?

  $definition_update_manager = \Drupal::entityDefinitionUpdateManager();
  $storage_definition = $definition_update_manager->getFieldStorageDefinition('uid', 'commerce_product');
  $storage_definition
    ->setDefaultValueCallback(Product::class . '::getDefaultEntityOwner');
  $definition_update_manager->updateFieldStorageDefinition($storage_definition);

  $storage_definition = $definition_update_manager->getFieldStorageDefinition('uid', 'commerce_product_variation');
  $storage_definition
    ->setDefaultValueCallback(ProductVariation::class . '::getDefaultEntityOwner');
  $definition_update_manager->updateFieldStorageDefinition($storage_definition);

Same patch, with update hooks for each entity type.

With that patch, I'm still experiencing issues because of the base fields overrides (that is without the getCurrentUserId() method.

Wrong update hook name for the payment hook.

Updated the deprecation notice to mention Commerce instead of core.

Not committing this yet until I figure out what to do with base field overrides.

Ok, the attached patch is taking care of the base field overrides which removes the need for getCurrentUserId(). I manually tested it on a dev install and the warning was gone after running the update and after clearing caches.

Hopefully the last iteration.

I was able to test this locally, so I went ahead and committed it.