Problem/Motivation

It seems the consensus right now is the ContentModerationState entity itself shouldn't be interacted with directly. One common use case (I've used it myself, also came up in #2860889: ContentModerationStateInterface additions) is using the entity hooks to respond to changes in content state.

Proposed resolution

Perhaps we can dispatch events for these changes instead of making developers aware of the entity in the background.

Remaining tasks

User interface changes

API changes

Data model changes

Comments

Sam152 created an issue. See original summary.

timmillwood’s picture

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?

Sam152’s picture

Yeah, if it's marked as internal, we don't want to expose that entity to developers if it can be helped. Implementing an update hook and using the fields for the @internal entity wouldn't be a stable API.

jhedstrom’s picture

Assigned: Unassigned » jhedstrom
jhedstrom’s picture

Assigned: jhedstrom » Unassigned
Status: Active » Needs review
FileSize
8.45 KB

This adds and fires create and update events.

Sam152’s picture

Status: Needs review » Needs work

Looking good, some feedback.

  1. +++ b/core/modules/content_moderation/src/Entity/ContentModerationState.php
    @@ -176,7 +177,15 @@ public function save() {
    +    $event = new ContentModerationStateEvent($this);
    

    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 :)

  2. +++ b/core/modules/content_moderation/src/Entity/ContentModerationState.php
    @@ -176,7 +177,15 @@ public function save() {
    +      \Drupal::service('event_dispatcher')->dispatch(ContentModerationStateEvent::CREATED, $event);
    ...
    +      \Drupal::service('event_dispatcher')->dispatch(ContentModerationStateEvent::UPDATED, $event);
    

    Can cache the event dispatcher.

  3. +++ b/core/modules/content_moderation/src/Event/ContentModerationState.php
    @@ -0,0 +1,60 @@
    +  /**
    +   * Name of the event fired when saving a new moderation state entity.
    +   *
    +   * @Event
    +   *
    +   * @var string
    +   */
    +  const CREATED = 'content_moderation.created';
    ...
    +  /**
    +   * Name of the event fired when updating an existing moderation state entity.
    +   *
    +   * @Event
    +   *
    +   * @var string
    +   */
    +  const UPDATED = 'content_moderation.updated';
    

    We should remove all entity-centric things that allude to the ContentModerationState entity from the event. Maybe this should be one event, but with a method to get the entity whose state has changed, so developers can check isNew()?

  4. +++ b/core/modules/content_moderation/tests/src/Kernel/Event/ContentModerationStateTest.php
    @@ -0,0 +1,53 @@
    +  /**
    +   * Tests events for adding and updating moderation state entities.
    +   */
    +  public function testCreateUpdateStates() {
    +    // Utilize the state system to track if the event was fired.
    +    $this->assertFalse(\Drupal::state()->get('content_moderation_test_event.created', FALSE));
    +    $state = ContentModerationState::create(['workflow' => 'editorial']);
    +    ContentModerationState::updateOrCreateFromEntity($state);
    +    // @see \Drupal\content_moderation_test_event\EventSubscriber\Test::onCreate
    +    $this->assertEquals($state->id(), \Drupal::state()->get('content_moderation_test_event.created', FALSE));
    

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

    Looks like symfony does something similar in \Symfony\Component\HttpKernel\Tests\Fragment\InlineFragmentRendererTest::testRenderExceptionIgnoreErrors, but I couldn't find an example in core.

jhedstrom’s picture

Issue tags: +Workflow Initiative

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.)

jhedstrom’s picture

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).

Sam152’s picture

#7 sounds good to me.

Sam152’s picture

Status: Needs work » Needs review
FileSize
20.13 KB
12.49 KB

Moving this along a bit. Right now it's red based on another bug I'll open as a blocker. Getting the original state was surprisingly annoying, but seemed to work.

I was able to test the event by replacing the dispatcher in the container, seemed to work pretty well.

Sam152’s picture

Status: Needs review » Needs work

The last submitted patch, 10: 2873287-10.patch, failed testing.

Sam152’s picture

Status: Needs work » Needs review
FileSize
2.94 KB
15.44 KB

This is still blocked, but fixing the fail introduced by the changes. Now that we fire an event which loads some extra information for the event, we can't just create phony ContentModerationState entities for the storage test.

Status: Needs review » Needs work

The last submitted patch, 13: 2873287-13.patch, failed testing. View results

timmillwood’s picture

timmillwood’s picture

Title: Dispatch events for changing content moderation states » [PP-1] Dispatch events for changing content moderation states
Status: Needs work » Postponed
Related issues: +#2881645: ContentModerationState 'workflow' column empty for all non-default translations.