Dispatch events for changing content moderation states

As I mentioned in IRC we used to have events that fired when a moderation state was changed, this is how Workbench Moderation does it, but when we moved to store the state in a ContentModerationState entity we decided that the event wasn't needed because we have the create, update, and insert hooks. Although if we're not supposed to interface directly with ContentModerationState entities maybe we should add the events back in?

This adds and fires create and update events.

The point of the issue was that users wouldn't ever interact or know about the entity. Instead of passing this into the event, we should pull out what is relevant and put into the event. That can safely keep the entity type @internal while providing @api :)

Ah, good call. So perhaps the event can have something along these lines for methods:

• getModeratedEntity
• getWorkflow
• getNewState
• getOldState

(Those last 2 methods I haven't thought through if we have that information or not...I assume we do.)

I wonder if we can mock event dispatcher and put it into the container and assert the dispatch methods are called instead?

We could potentially do that, but calling the actual event listener seems valuable too, since, in this case, we can verify the event listener had access to the moderation state entity (when refactored, we could verify access to the moderated entity, the workflow, the transition, etc).

Added this as a should have in #2755073: WI: Content Moderation module roadmap

This is currently blocked on #2881645: ContentModerationState 'workflow' column empty for all non-default translations.

1) Can't we throw the event on the workflows module level?

Otherwise people using workflows without content_moderation will once again have the same need, and an issue will eventually get raised duplicating this work.

2) Would be great to have pre_transition and post_transition events, instead of a single changed event (the equivalent of post_transition).

@Sam152
I literally have the same event in state_machine: https://github.com/bojanz/state_machine/blob/8.x-1.x/src/Event/WorkflowT...
As you can see, it's almost the same, just some wording differences.
So, definitely generic enough.

(And the reason why I'm saying this is because I'd love to see workflows in core achieve feature parity with state_machine at some point, so that I can deprecate state_machine. The event is half of that).

Since Workflow is just a ConfigEntity now and the storage isn't handled in generic way (each WorkflowType implementation is responsible for it), it would be tricky/hacky to get $entity as context in the event,

Content Moderation uses bolted on ContentEntity for storage, having plenty of issues trying to keep it in sync with translations and revisions of the moderated entity (also views integration), this feels like a lot of effort just to duplicate what @FieldType would get out of the box.

Moving this back to 8.4.x, this is a "should have" Content Moderation issue and blocking it becoming stable in 8.4.0

Re-roll

Since this patch adds the official way to react to moderation state changes, it would be really good to get into the first stable version of content moderation (eg, 8.4.0), so that contrib can do things the correct way, and not have to refactor for 8.5.

Content moderation notifications is one such example.

The patch looks great to me, I just have one question:

--- a/core/modules/content_moderation/tests/src/Kernel/ContentModerationStateStorageSchemaTest.php
+++ b/core/modules/content_moderation/tests/src/Kernel/ContentModerationStateStorageSchemaTest.php

I'm very curious why this test needed to be changed because I don't see how these changes could affect the rest of the patch.

And another question for @bojanz: do you think that #20 and #21 address your concern properly?

This missed 8.4.0, but I'd still like to advocate for it getting into 8.4. Otherwise contrib modules will start to leverage entity events on the moderation state entity that we are trying to keep internal here, and then need to be re-written for 8.5, which will be difficult to maintain BC for both versions at that point.

Alright, I discussed this issue with @timmillwood and @Sam152 this morning, and based on that discussion I came to some decisions about this issue:

1. This isn't a bug, so recategorizing to task.
    
2. As an API addition to a beta module, it's minor-only, so moving to 8.5.x.
    
3. I don't think we can really claim that hooks provided by the entity API are internal, even if there's an @internal on the entity type itself. A developer implementing the hooks wouldn't have a very direct way of knowing, so if we truly considered them internal we'd have to raise that to the developer some way more explicitly, or even prevent them from being implemented somehow. @Sam152 said he'd like to explore this. However, I think that by their very nature hooks are API, so I don't think it's even good DX to try to make them internal.
    
4. I think having both the event and the hooks is bad DX and redundant. It should be one or the other.
    
5. We do need to provide full BC and upgrade paths for the module now that it's in beta. I think we might even need to do so for the entity type despite it being marked as internal -- it'd be disruptive. And anyway, this issue isn't about removing or changing the moderation state entity type (and an issue for that doesn't even exist yet). If we do go the route of deprecating (for removal in D9) the entity in the future, we could reopen this issue, but I don't think we should add the event based on some future direction we haven't even explored yet.
    
6. @timmillwood said that he thought that we should not be using hooks and that the direction for D9/D10 should be to replace hooks with events, but we haven't made such a policy yet. @catch also raised that he's strongly opposed to replacing the hook system with events as they are. Quoting @catch:
   

   I’m completely opposed to using event dispatcher everywhere, the DX is bad.
   You have to have an Event class for everything, even when there’s no information to pass around.
   Also the way we use Event::CONSTANT now is more fragile than the hook system.
   Event dispatcher also only has priority, there’s no before/after which hook_module_implements_alter(), ugly as it is, provides.
   So it’s just not an adequate replacement, and it wouldn’t be an improvement except for it being OOP.
   We could do #2237831: Allow module services to specify hooks
   Then invoke a hook everywhere we currently invoke an event (including adapters for events that symfony provides), and deprecate all the events.

   I'm less sure about that, and it's not in scope here, but it's therefore also not in scope to decide events are preferable without that having been adopted core-wide already. We shouldn't make decisions based on a policy/best practice we haven't agreed to adopt. (Put otherwise, #1972304: Add a HookEvent and #1509164: Use Symfony EventDispatcher for hook system do not have consensus, so we should not make decisions as if they do, and we should not add events in a fragmented way so that it's a mixture of hooks and events and you can't know whether to expect one or the other except based on the subsystem maintainers' or patch authors' preferences).

So, TLDR, let's stick with the hooks for now. I recommend either postponing this issue on an issue to deprecate the moderation state entity and its hooks, or probably wontfixing it entirely. Setting NR for now in the absence of those issues and to give others a chance to review this.

Thanks everyone! Sorry for the delay; it took me some time to get to the bottom of what the issues were surrounding this.

Edit: Adding whitespace for readability.

I think we still either need something like the event proposed here, or to make the ContentModerationState not internal only. The content moderation notifications module has to do some acrobatics to get the previous state of a moderated entity during node save, whereas with this event, that would be much more straight-forward. Alternatively, if the content moderation state entity implemented a public interface for read-only operations (eg, the moderation state has changed from x to y, etc), that would work as well.

I'm working on adding Content Moderation support to Workbench Email in #2931097: Support the core Content Moderation module and they rely on the existing events to know when a state has changed.

Patch is still green and was already rtbc.

Is there discussion somewhere regarding why the entity is considered internal?

I have a client wanting transitions to be able to require comments on the reason for the transition; it sounds to me like having a fieldable transition entity is exactly what I'd be after.

+1 for this, it will be really useful, especially for people struggling with the $entity->original weirdness. #2914901: content moderation state entity with no original value is an example of a scenario where having this event would make developers lives much easier.

Patch #45 didn't apply cleanly anymore. Attached re-roll.

Arrived here from this rules task #2923001: Support new core Workflows events - which is blocked on this.

I had a look a the latest patch, the test was failing because we now need to use the ContentModerationTestTrait to create the editorial workflow, which was introduced in #2952307: Move workflow config into standard profile

Should hopefully come back green now :)

@xjm in #39 suggested this should be won't fix or postponed, but asked for feedback on that.

#40, #41, #45 and #48 have some experienced core devs pushing back in favour of this issue, so setting for RTBC for @xjm to reconsider.

1. Still needs a CR

2. +++ b/core/modules/content_moderation/src/Entity/ContentModerationState.php
   @@ -194,7 +207,25 @@ public function save() {
   +      $original_content_moderation_state = \Drupal::entityTypeManager()
   +        ->getStorage($this->getEntityTypeId())
   +        ->loadRevision($this->getLoadedRevisionId());
   

   Note this is an uncached read. Is ->original never populated?

3. +++ b/core/modules/content_moderation/src/Entity/ContentModerationState.php
   @@ -194,7 +207,25 @@ public function save() {
   +    \Drupal::service('event_dispatcher')->dispatch(ContentModerationEvents::STATE_CHANGED, $event);
   

   Does dispatching an changed event for a creation really make sense? In order situations we're defining the before state as the initial state which is confusing. See #2914873: The "from state" used for calculating available transitions is changed when using the content moderation state widget and preview mode.

4. +++ b/core/modules/content_moderation/src/Event/ContentModerationStateChangedEvent.php
   @@ -0,0 +1,103 @@
   +   * @return string
   +   *   The state the content was before.
   +   */
   

   Can be FALSE

Also #39 has not been addressed? There have been a few comments in favor of the idea of this issue, but no changes to the patch nor specific response to my points.

I vote event instead of hooks, what is required to commit to core beyond #2873287-50: Dispatch events for changing content moderation states? Removal and upgrade path for hooks?

What is required are specific (and persuasive) responses to @xjm's points 3-6 in #39

Just a minimal update for D10, changed event base class.

Fixed failed commands on #65
Attached interdiff patch

I have fixed CS error. Please review it

Try to fix PHPStan error.

Fixing the dispatch call.

That also needed to happen in tests.
Fixed that, and the method signature forces us to return something (usually the event itself). This should be green.

Next step should be fixing the php10 deprecation warnings.

That was the interdiff only... not even properly named.

Rewritten the test based on #3306554: InvocationMocker::withConsecutive() is deprecated in PHPUnit 9.6 and removed from PHPUnit 10. Added a todo if this lands before that one. No deprecations anymore.
I feel like writing tests will be even harder now :-(

Mostly code standards, but I changed an exactly(5) to any() in previous patch and I reverted that back.

Per #39 still NW.

Just another rebase, conflict on the save method changes.

Updated remaining tasks to include #39 + change record.

patch from #76 does not apply to 10.2.2

for 10.2.x use patch from #79

Hi, I was looking for this functionality, its great for being informed of a transition event, but what about being able to veto the transition?

Use case: Product publication.

A user wants to validate a product ready for publication. The system needs to check that the product is really ready. Maybe they haven't filled in all the required fields for publication (fields which cannot be required to create the initial entity/node). Maybe they haven't prepared related content which is required for publication.

The system should be able to validate certain data before allowing the product to pass to a validated state.

Previously we used the "Workflow" module but we decided against it because the module doesn't seem to be maintained and the code base is still in a state of temporary migration from Drupal 7 style code. Content Moderation seemed to be the more accepted option however it is limited in this respect.

@willeaton: You might find State Machine suitable for your needs. It provides / supports "Guards" which are plugins you can define to prevent specific state transitions if certain conditions are not met. But we're getting a bit off topic from this issue.

I also needed something like this for a project, but this issue seems a potentially doomed in deadlock between release manager and subsystem maintainer. I'm not sure I can be more persuasive than what @Sam152 already said in #40 and #45. Yes, a mix of hooks and events is somewhat crappy DX, but so is the current situation. I'm not sure how we provide 100% BC without a mix of both unless we want to make ContentModerationState public API. Drupal seems to love to provide a lot of different ways to alter the same thing 😅 so I'm not totally convinced that having both Events and hooks is really all that terrible. 😂

+1 to doing this. If I can find more time for a direct (and hopefully persuasive) response to every point in #39, I'll try.