Omit workspaces entity presave and predelete hooks for internal entities

Needs review?

Working on an updated patch.

+++ b/core/modules/workspaces/src/EntityOperations.php
@@ -116,9 +116,9 @@ public function entityPreload(array $ids, $entity_type_id) {
-    if ($entity_type->getProvider() === 'workspaces' || $this->workspaceManager->getActiveWorkspace()->isDefaultWorkspace()) {
...
+    if ($entity_type->isInternal() || $this->workspaceManager->getActiveWorkspace()->isDefaultWorkspace()) {

This change makes the workspace entity type (which is not internal) unable to be changed in a non-default workspace, as the test fails show :)

Also, we need to skip the pre-delete hook for internal entity types as well, which shows that this code can be a little confusing. Let's introduce a dedicated method for the checks in pre-save and pre-delete, so we don't get into the same problem again.

Reworked the test coverage because a phpunit test method should set an expected exception only once.

+++ b/core/modules/workspaces/src/WorkspaceManager.php
@@ -254,6 +254,19 @@ public function shouldAlterOperations(EntityTypeInterface $entity_type) {
+  public function shouldSkipPreOperations(EntityTypeInterface $entity_type) {

Does workspaces have a notion of handlers? This feels like something we might want the entity providers to have a say in?

E.g. content moderation has handlers with a default and then node has its own with some extra sauce

@larowlan, nope, we didn't find the need to have per-entity type customizations so far.

+++ b/core/modules/workspaces/src/WorkspaceManager.php
@@ -254,6 +254,19 @@ public function shouldAlterOperations(EntityTypeInterface $entity_type) {
+  /**
+   * {@inheritdoc}
+   */
+  public function shouldSkipPreOperations(EntityTypeInterface $entity_type) {

Out-of-interest how come this is on WorkspaceManager and not a protected method on \Drupal\workspaces\EntityOperations? It seems odd for this implementation detail to be in the Workspace manager.

@alexpott, very good point :) The only reason I put that helper method on the workspace manager is to make it easier to override if anyone needs some custom behavior. But after your question, I realized that we instantiate the entity operations class with the class resolver service, which means that developers can provide a service with that class name and the override ability is not lost.

+++ b/core/modules/workspaces/src/EntityOperations.php
@@ -340,4 +343,24 @@ public static function entityFormEntityBuild($entity_type_id, RevisionableInterf
+    // - the entity type is internal, which means that it should not affect
+    //   anything in the default (Live) workspace;

I've read this a few times and I'm just not sure we can make this assumption. Here are docs for isInternal()

   * Indicates whether the entity data is internal.
   *
   * This can be used in a scenario when it is not desirable to expose data of
   * this entity type to an external system.
   *
   * The implications of this method are left to the discretion of the caller.
   * For example, a module providing an HTTP API may not expose entities of
   * this type or a custom entity reference field settings form may deprioritize
   * entities of this type in a select list.

I'm not sure that

which means that it should not affect anything in the default (Live) workspace;

is really an assumption that can be made. Or if it can be then we really need to explain why.

Discussed this issue with @alexpott at Dev Days Transylvania, and, after reviewing all the usages of internal in core and contrib, we agreed to move forward with the current patch and discuss improving the documentation of that property in a follow-up issue: #3061252: Improve the documentation of the 'internal' property of entity types

Rerolled after #3059090: Deprecate \Drupal\Tests\PhpunitCompatibilityTrait::setExpectedException().

Committed 76fa7e9 and pushed to 8.8.x. Thanks!

I'm going to commit #11 to 8.7.x once it has had a test run.

I just checked and #11 doesn't apply anymore to 8.7.x either, so here's a reroll for that.

Committed 37e4d4b and pushed to 8.7.x. Thanks!