Fix ContentModeration workflow type to calculate correct dependencies

Didn't seem to be a plugin interface that also had onDependencyRemoval, considered extending DependentPluginInterface into RemovableDependentPluginInterface but decided it was also fine directly on the workflow type plugin interface.

1. Yeah, neato.
2. Done.
3. Good catch.
4. Done.

1. Done.
2. Done.
3. My reasoning was if some 3rd party added or enforced additional config dependencies that were simple config or otherwise not entities, it would skip over them. Digging deeper it looks like they don't trigger the same dependency removal (maybe it's invalid in the first place?), so it is indeed superfluous and can be removed.
4. Done.

Good to know about depending on config entities only. FYI the patch in #8 doesn't include the instanceof check.

Simple re-roll to take into effect changes to ContentModerationStateTest.

Seeing as it was just a re-roll this can go straight to RTBC.

(Posting a partial review and leaving RTBC for the moment since my internet connection is unreliable.)

+++ b/core/modules/content_moderation/src/Plugin/WorkflowType/ContentModeration.php
@@ -88,10 +118,11 @@ public function getEntityTypes() {
-   *   The bundles of the entity type the workflow is applied to.
+   *   The bundles of the entity type the workflow is applied to or an empty
+   *   array if the entity type is not applied to the workflow.
    */
...
-    return $this->configuration['entity_types'][$entity_type_id];
+    return isset($this->configuration['entity_types'][$entity_type_id]) ? $this->configuration['entity_types'][$entity_type_id] : [];

So this change made me question why we would be looking up the bundles for an entity type that's not used for a workflow, or if there's a valid scenario for an entity type that is used for a workflow to have an empty list of bundles. The reason for my concern was that this is changing the method to silently return an empty array if the entity type ID is not valid. In HEAD, this method is only used within a foreach of getEntityTypes() so it is impossible that the entity type would not be valid. The method appliesToEntityTypeAndBundle() already exists, so it seemed to me that this API change was unnecessary. Instead, the calling code could just check appliesToEntityTypeAndBundle() before calling this method. @alexpott made the case that this API change makes the calling code more robust by always returning an array.

I also wondered if a developer would instead expect an explicit failure (exception, FALSE return value, or whatever) from this method for an unsupported entity type. @alexpott pointed out that \Drupal\field\Entity\FieldConfig::__construct() (for example) doesn't validate that the bundle matches the entity type, so maybe the rest of core does not let developers expect such explicit failures.

At the least, we need to change the one-line summary of the function to allow that it might not return any values (in addition to updating the parameter docs as the patch does). @alexpott and I came up with:

Gets any bundles the workflow is applied to for the given entity type.

Still, it feels borderline out of scope to me to make this change/improvement here. In an ideal world I'd prefer that such an API improvement happen in a separate issue.

@alexpott also suggested that appliesToEntityTypeAndBundle() could be changed in a followup to be simpler and call this updated version of getBundlesForEntityType().

I think the change to \Drupal\content_moderation\Plugin\WorkflowType\ContentModeration::getBundlesForEntityType() is fine - it is required because without this change the following code would produce a PHP warning:

No, as we discussed, the method could instead check appliesToEntityTypeAndBundle(). That would use the existing API. If we want to change an API, we ideally should do so deliberately in a dedicated issue so that it is clear what the intent is, rather than it being a hack to make new code work. If it needs to happen beforehand, then that other issue can happen first.

I also asked alexpott if it was always expected that a workflow was applied to a specific bundle of an entity type, and whether workflows were supported for non-bundleable entity types, because there seem to be a lot of assumptions about bundles (both in the patch and in HEAD). At first @alexpott thought that non-bundleable entities were supported by content_moderation and even tested, but then realized #2799785: Entity types with non-config bundles can not be moderated actually added testing and support for bundles that were not backed by config entities. He said he was not sure that anyone had tried applying moderation workflows to non-bundleable entity types.

This made me wonder about this part of the dependency removal method:

+++ b/core/modules/content_moderation/src/Plugin/WorkflowType/ContentModeration.php
@@ -156,11 +187,59 @@ public function defaultConfiguration() {
+    foreach ($dependencies['config'] as $removed_config) {
+      if ($entity_type_id = $removed_config->getEntityType()->getBundleOf()) {
+        $bundle_id = $removed_config->id();
+        $this->removeEntityTypeAndBundle($entity_type_id, $bundle_id);
+        $changed = TRUE;

This only removes bundles that are defined as the bundle providers for the removed config, and not bundles like those supported in #2799785: Entity types with non-config bundles can not be moderated. So how do we clean up the workflow config if a bundle that is not provided by a config entity goes away? Maybe core doesn't support that yet, but it seems #2799785: Entity types with non-config bundles can not be moderated tries to.

@alexpott suggested:

\Drupal\Core\Entity\EntityTypeListener::onEntityTypeDelete would be what we’d need to listen to - and yes I think a lot of things would break

At the least, if we add information about non-config bundles to workflow config, we should confirm it's cleaned up on uninstall in a followup. I couldn't figure out how to test this manually with core.

I don't think we should try to fix that here, since I think it's a real edgecase and this issue fixes an actual data integrity bug in HEAD for everyone. But it might be another followup.

From manual tests moderation does work on entity types without a bundle, but there is no way to set them as moderated via the UI, because the UI lives in the bundle settings. #2843083: Select entity type / bundle in workflow settings is working to resolve this.

Thanks @timmillwood.

Maybe @amateescu can clarify/test in the as-yet unfiled potential followup whether this also successfully removes workflows for the non-config bundles he is using that led to posting #2799785: Entity types with non-config bundles can not be moderated, and if there are any cases where those non-config bundle definitions would go away other than module uninstallation.

@alexpott and I agreed to split the (small) API change out from this into another issue. This one may be briefly postponed as we look at that. However, I'm in the process of manually testing some different scenarios for this patch so assigning this issue to myself while I do that.

#2850341: Improve \Drupal\content_moderation\Plugin\WorkflowType\ContentModeration::appliesToEntityTypeAndBundle() and ::getBundlesForEntityType() is the new blocking issue.

The first scenario I tested:

1. Install standard from HEAD.
2. Install Workflows and Content Moderation
3. Apply the Editorial workflow to articles
4. Uninstall taxonomy and history
5. Uninstall node
6. The workflow still contains:
   

     entity_types:
       node:
         - article

7. Reinstall node.
8. Create a new content type called "article".
9. The Editorial workflow is already applied to it by MAGIC, even though your article type might not be the same thing. Since the method to add moderation to a given entity type and bundle works even if it is already added, nothing fatals, but it's still a wrong behavior.

With the patch applied, the workflow is correctly updated in step 6:

 langcode: en
 status: true
 dependencies:
-  config:
-    - node.type.article
   module:
     - content_moderation
 _core:
@@ -65,6 +63,4 @@ type_settings:
     published:
       published: true
       default_revision: true
-  entity_types:
-    node:
-      - article
+  entity_types: {  }

And, in step 9, the new article type is correctly created with no workflow pre-applied.

So far so good!

Forgot to mention in #25. The patch also fixes the config entity to actually include the dependency for the bundle.

In HEAD, after adding the moderation workflow to the article bundle

diff --git a/workflows.workflow.editorial.yml b/workflows.workflow.editorial.yml
index 5fe1957..c3f4b9d 100644
--- a/workflows.workflow.editorial.yml
+++ b/workflows.workflow.editorial.yml
@@ -63,4 +63,6 @@ type_settings:
     published:
       published: true
       default_revision: true
-  entity_types: {  }
+  entity_types:
+    node:
+      - article

With patch

diff --git a/workflows.workflow.editorial.yml b/workflows.workflow.editorial.yml
index d34708c..518ebb0 100644
--- a/workflows.workflow.editorial.yml
+++ b/workflows.workflow.editorial.yml
@@ -2,6 +2,8 @@ uuid: cc0abc72-7e33-4098-ab3f-bfe77aa8168c
 langcode: en
 status: true
 dependencies:
+  config:
+    - node.type.article
   module:
     - content_moderation
 _core:
@@ -63,4 +65,6 @@ type_settings:
     published:
       published: true
       default_revision: true
-  entity_types: {  }
+  entity_types:
+    node:
+      - article

I also tested the patch with leaving the node module installed, but deleting the article type. It is fixed in the same way as #25. The expectations that the workflow will be updated are also correctly communicated in the UI after the patch, e.g.:

CONFIGURATION UPDATES
The listed configuration will be updated.
Workflow
* Editorial workflow

I then tested adding the same workflow to both page and article content type to ensure they could be added and removed independently and the config would be updated correctly. That also worked correctly, but I did notice a strange schema change in the process of testing this. By:

1. Deleting the article type.
2. Adding a new article type.
3. Adding the workflow to articles.
4. Adding the workflow to pages.
5. Removing the workflow from articles.
6. Re-adding the workflow to articles.

(Steps 1 and 2 may not be necessary to reproduce, but that is what I did.)

I got this diff:

diff --git a/workflows.workflow.editorial.yml b/workflows.workflow.editorial.yml
index 8846c55..1713f6c 100644
--- a/workflows.workflow.editorial.yml
+++ b/workflows.workflow.editorial.yml
@@ -3,6 +3,7 @@ langcode: en
 status: true
 dependencies:
   config:
+    - node.type.article
     - node.type.page
   module:
     - content_moderation
@@ -67,4 +68,5 @@ type_settings:
       default_revision: true
   entity_types:
     node:
-      - page
+      1: article
+      0: page

Are those serial keys being exported accidentally? Are they weights? It feels like the weights should not be changed since I did not reorder anything, and if they are weights, shouldn't we always store them for a clean diff?

There are still some scenarios I haven't tested that I might (actually having content in these workflows, taxonomy, etc.), but setting NR for this last issue.

(BTW the committer crediting bug appeared on this issue and alexpott is currently unintentionally uncredited. drumm wanted an example of this bug happening so not fixing that yet.)

(drumm says it is okay to correct the credit.)

#2850341: Improve \Drupal\content_moderation\Plugin\WorkflowType\ContentModeration::appliesToEntityTypeAndBundle() and ::getBundlesForEntityType() is in now so we can update this on top of it. Thanks @alexpott and @timmillwood for accommodating the scope fix.

I just retested and found the bug from #25 #26 also happens before the patch, so not in scope here. It definitely seems like a bug somewhere in the config entity implementation though, so let's file a followup for it.

Regarding #22 about non-config bundles. In this diff:

diff --git a/workflows.workflow.editorial.yml b/workflows.workflow.editorial.yml
index d34708c..518ebb0 100644
--- a/workflows.workflow.editorial.yml
+++ b/workflows.workflow.editorial.yml
@@ -2,6 +2,8 @@ uuid: cc0abc72-7e33-4098-ab3f-bfe77aa8168c
 langcode: en
 status: true
 dependencies:
+  config:
+    - node.type.article
   module:
     - content_moderation
 _core:
@@ -63,4 +65,6 @@ type_settings:
     published:
       published: true
       default_revision: true
-  entity_types: {  }
+  entity_types:
+    node:
+      - article

The first part does not happen in HEAD, but the second part already does. The first part is only relevant for bundles defined via a config entity; the second is relevant for any. So hopefully this patch is also fine for those non-config-entity bundles too.

Edit: Except the second part does not happen on uninstallation in HEAD, only on installation. Right. So this patch should still also be fixing that for the non-config bundles as well. I think maybe it does? Edit 2: let's still file the followup for that as well, I guess, something like "Ensure non-configuration bundles are correctly removed when the bundles go away".

I wanted to test this for taxonomy terms (as the other core example of a config entity defining a bundle for a content entity) but could not apply a workflow to taxonomy terms through the UI. Is this because taxonomy terms are technically revisionable but not yet revisioned due to #2721313: Upgrade path between revisionable / non-revisionable entities being outstanding?

Or maybe #35 is also fixed by #2843083: Select entity type / bundle in workflow settings. Edit: Looks like no; that issue is showing taxonomy in that UI with help from a contrib module. But it does make it clear that core does not support it yet, at least. Is there an existing issue for supporting content moderation for taxonomy etc. in core?

Ah, looks like I can maybe test it with content blocks? (I always forget about content blocks.)

The requirement right now is that entities are revisionable. This has sort of been floating around as a task on the radar, but without an issue. Opened #2850627: Do not require a revisionable entity type when using content_moderation..

I tested with content blocks as well, and they also work fine in the same scenarios I tested above. E.g.:

diff --git a/workflows.workflow.editorial.yml b/workflows.workflow.editorial.yml
index eaeadf1..fa6e70e 100644
--- a/workflows.workflow.editorial.yml
+++ b/workflows.workflow.editorial.yml
@@ -3,7 +3,6 @@ langcode: en
 status: true
 dependencies:
   config:
-    - block_content.type.basic
     - node.type.page
   module:
     - content_moderation
@@ -67,7 +66,5 @@ type_settings:
       published: true
       default_revision: true
   entity_types:
-    block_content:
-      - basic
     node:
       - page

Maybe @amateescu can clarify/test in the as-yet unfiled potential followup whether this also successfully removes workflows for the non-config bundles he is using that led to posting #2799785: Entity types with non-config bundles can not be moderated

FWIW, I don't have a specific use-case for moderating entities with non-config bundles. I was just reviewing a Content Moderation patch, saw that the settings were stored in config bundles and thought it's whack so I opened that issue and started to work on it :)

1. Re: #41, thanks @amateescu! Based on that, @alexpott and I agreed to file a followup meta about the matrix of what entity type properties (revisionable, bundleable, publishable, etc.) the module does and doesn't support and ensuring validation, config management etc. is all robust, as we add support and testing. There is #2850627: Do not require a revisionable entity type when using content_moderation. already (added above) and a few other issues connected to this, but no overall discussion that I could find. The needs followup tag is on for that.

2. +++ b/core/modules/content_moderation/src/Plugin/WorkflowType/ContentModeration.php
   @@ -161,11 +191,59 @@ public function defaultConfiguration() {
      public function calculateDependencies() {
   ...
   +  public function onDependencyRemoval(array $dependencies) {
   

   One thing I have gone back and forth about is whether we should improve the inline docs for these two methods to better explain what's happening in each case, since it's always so confusing to understand what's going on with entity types that define bundles for entity types etc. It took me a long time parsing code in my head and reading API docs to verify what each bit was doing, even though each one is only a few lines. I may post some suggestions for better docs (which can also be elucidating if I mis-explain what's happening) or someone else can if they like.
    

3. I also just tested to confirm that there are no circular dependencies -- no entity type configuration is harmed when a workflow is deleted. (I did find a different bug while testing that, but confirmed said bug is also in HEAD so I'll file it separately.)

   Similarly, I tested that there aren't leaky dependencies from other things like fields and displays (which could cause data loss), by uninstalling the taxonomy module which futzes with articles, but not the bundle itself.

   No other concerns from me at this point. I've tested several core scenarios and not sure I have the energy to do much more reinstallation and config exporting and diffing. ;) I do like to test config dependency patches thoroughly since they are so important for data integrity. A couple other scenarios someone could examine if they like, not blocking:

   • Using Multiversion to test entity types that aren't supported out of the box in core, as @timmillwood did for #2843083: Select entity type / bundle in workflow settings
   • Making the bundle depend on something and confirming the workflow cleanup happens properly after the deletion of whatever the bundle depends on. (I couldn't come up with any scenario for core where a bundle depends on anything other than the module providing it, but maybe contrib has one.) This is a low priority since the config system should handle that for us anyway; maybe not worth anyone's time.
   • The thing I mentioned previously about testing with content is irrelevant because you can't remove a bundle or module if there is data in it, so ignore that suggestion.

   So really just the Multiversion one is worthwhile I guess.

Edit: putting my comment in a numbered list for readability.

Forgot to mention, the docs improvements I suggest in #42.2 could also be a followup since the code obviously works and someone who does know the entity system inside out can follow it. I realize it's vague "make it better" feedback. So nothing in #42 is really a hard blocker; we just need reviewer to do a final review of the latest patch and to make sure none of my feedback raises flags, and then there are followups.

Sorry, two more things I had in a note on my laptop from an airport:

1. +++ b/core/modules/workflows/src/Entity/Workflow.php
   @@ -506,4 +506,13 @@ public function status() {
   +  public function onDependencyRemoval(array $dependencies) {
   +    $changed = $this->getTypePlugin()->onDependencyRemoval($dependencies);
   +    // Call parent first to ensure it is always called regardless of $changed.
   +    return parent::onDependencyRemoval($dependencies) || $changed;
   

   I don't understand what this is doing or why it is needed, despite the comment.

2. +++ b/core/modules/workflows/src/WorkflowTypeInterface.php
   @@ -117,4 +117,17 @@ public function buildStateConfigurationForm(FormStateInterface $form_state, Work
   +  /**
   +   * Informs the plugin that a dependency of the workflow will be deleted.
   +   *
   +   * @param array $dependencies
   +   *   An array of dependencies that will be deleted keyed by dependency type.
   +   *
   +   * @return bool
   +   *   TRUE if the workflow settings have been changed, FALSE if not.
   +   *
   +   * @see \Drupal\Core\Config\ConfigEntityInterface::onDependencyRemoval()
   +   */
   +  public function onDependencyRemoval(array $dependencies);
   

   I was surprised that this isn't provided by some generic interface since it seems like it's going to be the same for every config entity ever. Is there a similar example I can look at on a different core entity interface?

Ahh. Maybe #44.2 is already mentioned in #3 and covered by #2579743: Config entities implementing EntityWithPluginCollectionInterface should ask the plugins to react when their dependencies are removed? If so can we stick an @todo on the method for it?

NW for #44 I think. Sorry for the comment-monologue.

1. The interface calls for:

   * @return bool
   *   TRUE if the entity has been changed as a result, FALSE if not.

parent::onDependencyRemoval() sorts out things like 3rd party settings getTypePlugin()->onDependencyRemoval(), as you know, sorts out the entity types and bundles. Returning $parent_changed || $plugin_changed ensures that if either of those two things change a dependency, we return the correct value.

Better comment needed?

2. PluginSettingsInterface contains it, but that is deprecated. Other than that it's found (or something similar) in these places:

\Drupal\Core\Config\Entity\ConfigEntityInterface::onDependencyRemoval
\Drupal\Core\Field\FieldItemInterface::onDependencyRemoval

Unlike calculateDependencies which has a neat DependentPluginInterface. Perhaps RemovalDependentPluginInterface that extends DependentPluginInterface? Possibly follow-up territory.

Only other potential followup is @xjm's vague "please hold my hand with inline comments to help me understand how the entity system works".

#49 resolves #45 for me, especially with the tests. Followups can be added before or after RTBC since they are not directly in scope.

Looks ready to me.

Posted this stub for now: #2851603: Add inline documentation to content moderation onDependencyRemoval() that describes what's happening

#53 is the same issue twice but I assume the second is #2851228: Warn users of what features are not available on a given entity type.

It's not applying to 8.4.x; I guess needs a reroll for the other content moderation patch?

Rerolled.

Some coding standards errors. I can probably fix these on commit, but a bit frayed, so bear with me here.

FILE: .../maintainer/core/modules/workflows/src/WorkflowTypeInterface.php
----------------------------------------------------------------------
FOUND 3 ERRORS AFFECTING 3 LINES
----------------------------------------------------------------------
 28 | ERROR | [x] Return type must not contain variable name
    |       |     "$workflow"
 65 | ERROR | [x] Return type must not contain variable name "$state"
 83 | ERROR | [x] Return type must not contain variable name
    |       |     "$transition"
----------------------------------------------------------------------
PHPCBF CAN FIX THE 3 MARKED SNIFF VIOLATIONS AUTOMATICALLY
----------------------------------------------------------------------

diff --git a/core/modules/workflows/src/WorkflowTypeInterface.php b/core/modules/workflows/src/WorkflowTypeInterface.php
index cc92cad173..0fe668547c 100644
--- a/core/modules/workflows/src/WorkflowTypeInterface.php
+++ b/core/modules/workflows/src/WorkflowTypeInterface.php
@@ -25,7 +25,7 @@
    * @param \Drupal\workflows\WorkflowInterface $workflow
    *   The workflow to initialize.
    *
-   * @return \Drupal\workflows\WorkflowInterface $workflow
+   * @return \Drupal\workflows\WorkflowInterface
    *   The initialized workflow.
    *
    * @see \Drupal\workflows\Form\WorkflowAddForm::save()
@@ -62,7 +62,7 @@ public function checkWorkflowAccess(WorkflowInterface $entity, $operation, Accou
    * @param \Drupal\workflows\StateInterface $state
    *   The state object to decorate.
    *
-   * @return \Drupal\workflows\StateInterface $state
+   * @return \Drupal\workflows\StateInterface
    *   The decorated state object.
    */
   public function decorateState(StateInterface $state);
@@ -80,7 +80,7 @@ public function deleteState($state_id);
    * @param \Drupal\workflows\TransitionInterface $transition
    *   The transition object to decorate.
    *
-   * @return \Drupal\workflows\TransitionInterface $transition
+   * @return \Drupal\workflows\TransitionInterface
    *   The decorated transition object.
    */
   public function decorateTransition(TransitionInterface $transition);

It's a patch for a thing that fixes things. And the things that it fixes make data integrity bug-magic so I backported it. Thanks!

Hm the coding standards issues were outside of this patch's additions, just in a changed file, and apparently I am using coder 8.2.10 instead of 8.2.8 so I accidentally cleaned up out-of-scope lines with my pre-commit hooks. Sorry for the noise -- going to leave the commit though.

I think this issue was a regression from 8.2.0 so originally I did not tag it for that reason, but we should at least mention it in the beta release note and probably the content moderation section of the overall notes as well.

I just spent 5 minutes clicking in related issue circles looking for this one. ;)

@alexpott and I agreed that this does not need to go in the 8.3.0 release notes/changelog, since it is just a followup to the workflow conversion that happened in 8.3.x only and so was a regression since 8.2.x, but I am mentioning it in the beta release notes since the bug affected the alpha.