Problem/Motivation

Follow-up to #2725533: Add experimental content_moderation module.

For content moderation, there are the following concepts:

- state - configuration entity for a moderation state
- transition - configuration entity for the (one way) from and to relationship between two states
- valid states - the states that are allowed on a particular entity bundle, stored as a third party setting

This means you need to have two different config entity types, several individual entities of those types, as well as bundle third party settings to figure out what the workflow is for a particular bundle entity.

It also results in some trickiness with things like ordering states/transitions - since each needs to store its own weight relative to the others. This is usually a sign that data shouldn't be in configuration entities but rather a single object.

Also I can see people wanting to use this system for workflow on users (account pending, e-mail validated, approved, blocked etc.), even though they wouldn't use forward revisions for that, or even necessarily revisions. However those states are extremely user-specific, and something like translation editorial review is never going to apply to account entities too. So not sure how useful the global states and transitions are conceptually.

Proposed resolution

Consider consolidating this information into one or two objects per-entity or per-bundle (or per entity with per bundle overrides).

So something like (excuse the invalid YAML). This isn't a proposal as such, just something for comparison.

states:
 - draft:
       label: draft
 - published:
        label: published 
 - archived:
       label: archived
transitions:
  - draft_published:
       from: draft
       to: published

In chat @alexpott pointed out that http://symfony.com/blog/new-in-symfony-3-2-workflow-component exists, this has a concept of a single 'workflow' that contains all the information. Again not a proposal but something to compare against.

Current resolution

Adds a new experimental Workflows module that provides

  • A workflow configuration entity type. This is where states and transitions are configured.
  • The concept of workflow type plugins. This is how modules can leverage workflows and extend the information stored against a state or a transition. If there are no workflow type plugins the user can not create workflows because there is nothing to apply them to.
  • Fluid API for workflow entities:
        $workflow = new Workflow(['id' => 'test', 'type' => 'test_type'], 'workflow');
        // By default states are ordered in the order added.
        $workflow
          ->addState('draft', 'Draft')
          ->addState('published', 'Published')
          ->addState('archived', 'Archived')
          ->addTransition('create_new_draft', 'Create new draft', ['draft', 'published], 'draft')
          ->addTransition('publish', 'Publish', ['draft', 'published'], 'published');
    
  • A workflow creation UI. A user interface that can create, edit and delete workflows and their states and transitions.

Changes content moderation module by:

  • Removing the state and transition configuration entities
  • Changing the content moderation state content entity to record the workflow and state used.
  • Adds a new workflow type to moderate content entities.
  • Changes the permission names to include the workflow label
  • Changes how workflows are added to bundles. Instead of arranging states and transitions on the bundle you just select a workflow.

Advantages

  • Just makes sense. Arranging a workflow on each bundle is a repetitive task and states and transitions not belonging to a workflow entity means that in order to create a workflow in HEAD you have to load all sorts of things. This also means we can totally get around issues with duplicate state names.
  • States are ordered on the workflow using table drag.
  • Allows additional configuration on both the state and transition level
  • Allows workflows to be applied to things other than content by contrib

Experimental module / release strategy

All the changes are in experimental code.

Remaining tasks

Review patch, make decisions, and create / find followups

Decisions:

  • What should the workflow provided by the content moderation be called. At the moment it has the ID 'typical' and the label 'Typical workflow' changed to 'Editorial workflow' / editorial see #117
  • Decide how much of the UI work to include here - created follow up #2830584: Use modals for creating, updating, and deleting workflows, with a new DialogFormTrait
  • Should we continue to use the word "State" or use the more generic and common in workflow theory "Place"? Chose state since the arguments in #95 are compelling
  • Should the new module by called "Workflow"? This clashes with a 8.x contrib module. Ee could use "Workflows" just to avoid the issue. We called the experimental module "Workflows"
  • Should transitions have multiple from states? And should transitions have multiple from states? We implemented multiple from states and a single to state - see #107

Followups:

User interface changes

New UI for creating workflows - see #22 for a flavour.
Changed UI for attaching workflows to content entities.

API changes

New workflow configuration entity
New workflow type plugin type
Removed ModerationState config entity
Removed ModerationTransition config entity

Data model changes

The content moderation state content entity no longer references a ModerationState config entity by references a Workflow config entity and records a state ID.

CommentFileSizeAuthor
#157 Screen Shot 2016-12-09 at 10.32.44 AM.png85.56 KBSam152
#156 create-workflow.png117.19 KByoroy
#153 2779647-2-153.patch373.56 KBalexpott
#153 148-153-interdiff.txt14.17 KBalexpott
#148 146-148-interdiff.txt653 bytesalexpott
#148 2779647-2-148.patch369.86 KBalexpott
#146 2779647-2-146.patch370.5 KBalexpott
#144 2779647-2-144.patch369.73 KBalexpott
#135 2779647-2-126.patch371.73 KBalexpott
#135 2779647-2.content_moderation.do-not-test.patch213.62 KBalexpott
#135 2779647-2.workflow_ui.do-not-test.patch62.71 KBalexpott
#135 2779647-2.core_.do-not-test.patch45.52 KBalexpott
#126 2779647-2-126.patch371.73 KBalexpott
#126 123-126-interdiff.txt11.71 KBalexpott
#123 2779647-2-123.patch369.94 KBalexpott
#117 2779647-2-117.patch369.99 KBalexpott
#117 111-117-interdiff.txt26.27 KBalexpott
#111 2779647-2-111.patch369.98 KBalexpott
#111 110-111-interdiff.txt17.13 KBalexpott
#110 2779647-2-110.patch365.87 KBalexpott
#110 108-110-interdiff.txt21.51 KBalexpott
#108 2779647-2-108.patch361.99 KBalexpott
#108 107-108-interdiff.txt5.71 KBalexpott
#107 2779647-2-107.patch357.43 KBalexpott
#107 102-107-interdiff.txt65.59 KBalexpott
#102 2779647-2-100.patch354.98 KBalexpott
#102 99-100-interdiff.txt10.58 KBalexpott
#99 2779647-2-99.patch354.6 KBalexpott
#99 97-99-interdiff.txt545 bytesalexpott
#97 2779647-2-97.patch354.07 KBalexpott
#96 83-96-interdiff.txt5.12 KBalexpott
#96 2779647-2-96.patch350.27 KBalexpott
#88 Screen Shot 2016-11-02 at 11.24.20 AM.png51.83 KBcatch
#88 Screen Shot 2016-11-02 at 11.25.17 AM.png81.17 KBcatch
#87 content_moderation-do-not-test.patch209.41 KBcatch
#87 workflow-do-not-test.patch136.24 KBcatch
#83 2779647-2-83.patch345.66 KBalexpott
#83 80-83-interdiff.txt6.49 KBalexpott
#80 2779647-2-80.patch344.72 KBalexpott
#80 79-80-interdiff.txt22.96 KBalexpott
#79 2779647-2-79.patch339.62 KBalexpott
#79 75-79-interdiff.txt10.07 KBalexpott
#75 2779647-2-75.patch338.36 KBalexpott
#75 72-75-interdiff.txt509 bytesalexpott
#72 2779647-2-72.patch338.36 KBalexpott
#72 71-72-interdiff.txt15.61 KBalexpott
#71 2779647-2-71.patch327.55 KBalexpott
#71 69-71=interdiff.txt2.43 KBalexpott
#69 2779647-2-69.patch327.81 KBalexpott
#67 2779647-2-67.patch318.42 KBalexpott
#64 Simple solution.png177.12 KBpericxc
#61 2779647-2-61.patch304.32 KBalexpott
#55 2779647-2-55.patch285.22 KBalexpott
#51 2779647-2-51.patch266.73 KBalexpott
#47 2779647-47.patch293.81 KBSam152
#47 interdiff.txt45.73 KBSam152
#45 interdiff.txt402 bytesSam152
#45 2779647-45.patch261.01 KBSam152
#43 2779647-2-43.patch260.99 KBalexpott
#36 2779647-2-36.patch244.74 KBalexpott
#35 scookie1.png34 KBscookie
#29 2779647-2-29.patch222.49 KBalexpott
#23 2779647-2-23.patch214.45 KBalexpott
#22 5.png82.24 KBSam152
#22 4.png126.46 KBSam152
#22 3.png134.39 KBSam152
#22 2.png67.63 KBSam152
#22 1.png20.85 KBSam152
#21 2779647-2-21.patch212 KBalexpott
#14 2779647-2-14.patch189.01 KBalexpott
#12 2779647-2-12.patch189.84 KBalexpott
#10 2779647-2-10.patch95.52 KBalexpott
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

catch created an issue. See original summary.

timmillwood’s picture

The Symfony Workflow bundle does look pretty interesting for this. A few people have mentioned it now and states and transitions seem to be what it does well. We would have to look in a bit more detail how we can create and manage these states and transitions. Obviously the advantage of using config entities is it has the whole config system and entity api in Drupal to help manage it. For either a custom object or a Symfony Workflow we would need to implement a UI to work with it.

alexpott’s picture

@timmillwood as @catch wrote in IRC:

timmillwood: alexpott: sorry yeah was thinking about more or less config-entity-per-workflow (then potentially workflow per entity type with per-bundle overrides or something)

So we're not suggesting using Symfony's workflow - what we're suggesting is rearranging the config to be more similar. I think the current global states and transitions is not how a person thinks and is causing problems:

#2750365-50: Permissions: moderation states with the same name is unclear for user to assign to a role writes...

Sites with many different workflows can have the same label (eg, Draft) for multiple states since these are site wide, and a state transition for one entity type might have completely different permissions than a state for another entity type.

I think that this is precisely why this issue is a good idea.

alexpott’s picture

Whilst talking about this on IRC @bojanz mentioned this sounds like https://www.drupal.org/project/state_machine

It does. Scarily so. See https://github.com/bojanz/state_machine/blob/8.x-1.x/README.md - the structure matches @catch's structure almost perfectly.

alexpott’s picture

Also I think we should consider moving where the default revision / is published logic is stored to the bundle where the workflow is applied. This is because these actions are specific to where the workflow is being applied. Or maybe workflows can have plugins that allow extra things to be configured on each state when a workflow is created for a particular entity type. Does it make sense to have the same workflow for different entity types?

catch’s picture

I hadn't reviewed 8.x state_machine yet, encouraging that it ended up at the same place though (and good to find out now, not in six months, thanks bojanz!). For some reason I thought 7.x workbench_moderation used (or could use) state_machine but looks like that was only in a dev branch.

Does it make sense to have the same workflow for different entity types?

I can see that different entity types on a site having the same workflow (i.e. if you have a site with multiple custom entity types) - but not sure we need to support that vs. just configuring identical workflows.

timmillwood’s picture

I agree with #5 in that all moderation settings currently stored on the bundle should be stored against the workflow.

I think a workflow would be used across multiple bundles and multiple entity types. I can't see of a use case where there would be a set of moderation states and moderation transitions with different moderation settings.

scookie’s picture

In production, we are currently on Drupal 6 and are in the midst of a big project to migrate to Drupal 8.
In Drupal 6, we have this:

One workflow that is used by 7 different content types.
8 other content types that each have their own workflow.
These workflows are actual "objects" with a name.

Those 7 content types can share a common workflow because the states are the same, the state transitions are the same, and the permissions are the same.

In the other 8 content types, some of the states are the same, some of those workflows either have fewer states or have additional states, and the roles that are allowed to use the transitions are different. In Workbench Moderation, since states were global, we had to define many states with duplicate state names for that reason. If we now specify states for each type of content (or each custom entity type), then we will have to duplicate states, transitions, and permissions unnecessarily (in the case of content types that share a common workflow). It seems to me that if we treat a workflow as an actual thing (object? sorry, I'm not technical, so I'm terminology-challenged) that has states, transitions, and permissions for those transitions, then any type of entity that uses that particular workflow can be associated with that workflow. We won't have to duplicate anything. If some other type of entity requires a different workflow due to different states, different transitions, and/or different permissions, then we would define that workflow and associate that other type of entity with that other workflow, and so on.

Does that make sense?

timmillwood’s picture

alexpott’s picture

Category: Plan » Task
Priority: Normal » Major
Status: Active » Needs review
FileSize
95.52 KB

I've started to work through this. The resulting code is much much nicer. Way less wtf. Note the patch attached here is not for review. It is just a current state of play. Many of the tests will be horribly borken. This is a massive change.

Status: Needs review » Needs work

The last submitted patch, 10: 2779647-2-10.patch, failed testing.

alexpott’s picture

Status: Needs work » Needs review
FileSize
189.84 KB

Now with a working UI - which because you are configuring an entire workflow at once is way easier to grok.

Status: Needs review » Needs work

The last submitted patch, 12: 2779647-2-12.patch, failed testing.

alexpott’s picture

Status: Needs work » Needs review
FileSize
189.01 KB

Move the initial state from bundle configuration to workflow because a workflow should define it's starting point - also makes configuring it sensible.

I really think we should pull all the workflow stuff out of content_moderation and have a new module - and just make content moderation about applying workflows to content entities.

timmillwood’s picture

Whoa! That's a pretty big patch, and a great start.

Thinking about:

+++ b/core/modules/content_moderation/config/install/content_moderation.workflow.typical.yml
@@ -0,0 +1,34 @@
+label: 'Typical workflow'

Maybe "default" or "standard" instead of "typical"?

The workflow yml file seems so close the that of the Symfony Workflow component I fear we're missing out if we don't use it. Yes, some bits of it are a bit weird (like the naming), but like many things in D8, are we best to get off the island?

Status: Needs review » Needs work

The last submitted patch, 14: 2779647-2-14.patch, failed testing.

alexpott’s picture

@timmillwood I'm not sure that it is worth it because we'd still to bridge it and configuration - so why not just make the configuration have its capabilities. What I have been thinking about is how to untangle the default_revision and published settings from the workflow. Basically so we can untangle workflows from entities. So we could have a workflow module that supplies a pluggable configuration entity that defines workflows ie states and transitions and content_moderation module that defines a workflow plugin that can attached workflow to a bundle and give each state meaning to the entity system.

Another though is that we should ditch the initial state configuration and just have it be the workflow state with the least weight (ie. top of the list).

alexpott’s picture

Oh and 'typical' was a five second choice knowing that changing it was certain and I had no better ideas at the time :)

timmillwood’s picture

In another issue I've been discussing the idea of a default or initial state for bother unpublished and published. So that when content_moderation is first enabled we can assign an initial state for all existing revisions regardless of if they are published or not. Currently when content_moderation is enabled existing nodes get the default state (draft) even if they are published based on the 'status' field of the node.

alexpott’s picture

@timmillwood well yeah - that should be part of the attaching a workflow to a bundle. If there are existing nodes.

alexpott’s picture

Status: Needs work » Needs review
FileSize
212 KB

More of the webtests green. I'm not gonna to bother with interdiffs till the patch is green - or there is a code review - which the patch is not really ready for :)

Sam152’s picture

FileSize
20.85 KB
67.63 KB
134.39 KB
126.46 KB
82.24 KB

For the benefits of watchers, #21 gives you:

alexpott’s picture

Fixing the UI a bit so new workflows can be created.

The last submitted patch, 21: 2779647-2-21.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 23: 2779647-2-23.patch, failed testing.

Sam152’s picture

I really like the direction of this. As someone who will be rolling out a production site that heavily revolves around content_moderation, I'd be interested in contributing to make sure large schema changes like this are committed quickly. If we can nail down the schema at least, it should make upgrading less painful for myself and others who are using it at this early stage.

pericxc’s picture

Two suggestions:

  1. Can you please ensure that different workflows could share the same state? In 3.png, add an option to select existing state.
  2. Once you select an existing state all "To state" would be automatically populated for a give workflow.
timmillwood’s picture

I'm not sure there's a need for two workflows to share the same state, is there?
You could have two workflows, both with a state that has the same name, but from a data / storage point of view they won't be the same state entity anymore as this new approach consolidates everything into one workflow entity.

I think the new config file shows well how the new workflow entities will be stored. There is now no need to define transitions and moderation states separately, it's all just done in one workflow.

langcode: en
status: true
dependencies: {  }
id: typical
label: 'Typical workflow'
states:
  archived:
    label: Archived
    published: false
    default_revision: true
    weight: 5
  draft:
    label: Draft
    published: false
    default_revision: false
    weight: -5
  published:
    label: Published
    published: true
    default_revision: true
    weight: 0
initial_state: draft
transitions:
  archived:
    draft: 'Un-archive to Draft'
    published: 'Un-archive'
  draft:
    draft: 'Create New Draft'
    published: 'Publish'
  published:
    archived: 'Archive'
    draft: 'Create New Draft'
    published: 'Publish'
applies: { }
alexpott’s picture

Status: Needs work » Needs review
FileSize
222.49 KB

@Sam152 totally agree that we want to get this done ASAP. I've been asked about using content_moderation in production several times (vs workbench_moderation). Content moderation is an experimental module - and an alpha at that so depending on when you site launches I might be tempted to go for workbench_moderation. And then help me and @timmillwood create an upgrade path :)

Along the same lines - discussed this a bit with @catch and both of us feel that workflows should be decoupled from content entities and we should have a workflow module that provides the UI and the Workflow entity. Here's the first cut of doing that. Next up is to fully test the Workflow UI and Workflow entity as part of the new Workflow module and then make them pluggable so content moderation can provide a plugin than configures how a workflow affects content entities.

The end goal end is to allow other modules to leverage workflows without content_moderation. For example, the Workflow Initiative is pretty keen to add a workflow to their workspace concept.

Again no interdiff because it'd be larger than the patch.

Sam152’s picture

I really like this direction. Another generic building block that will go a long way to standardising patterns of interaction between other building blocks.

Could this possibly benefit from a sandbox to split up and track some of the remaining effort?

jibran’s picture

  1. +++ b/core/modules/workflow/src/Form/WorkflowAddForm.php
    @@ -0,0 +1,69 @@
    +      '#disabled' => !$workflow->isNew(),
    

    It's always FALSE for AddForm.

  2. +++ b/core/modules/workflow/src/Form/WorkflowEditForm.php
    @@ -0,0 +1,127 @@
    +      '#disabled' => !$workflow->isNew(),
    

    It's always TRUE for EditForm.

Status: Needs review » Needs work

The last submitted patch, 29: 2779647-2-29.patch, failed testing.

timmillwood’s picture

There's already a Workflow module, with a D8 release https://www.drupal.org/project/workflow

jhedstrom’s picture

Along the same lines - discussed this a bit with @catch and both of us feel that workflows should be decoupled from content entities and we should have a workflow module that provides the UI and the Workflow entity.

This is a great direction. It will allow re-use of common workflows, and then specific or outlying workflows as-needed and minimize configuration duplication (as opposed to separating states to be entity-specific, which would allow the same end-goal, but result in lots of duplication).

scookie’s picture

FileSize
34 KB

Wonderful!! So glad that my original comment (#8) made sense. I really was having a hard time wrapping my head around the fact that in all of my experience with Drupal 6 and other platforms, workflow was always an object - and suddenly in Workbench/Content Moderation, it wasn't.

Regarding item 1 in #27 (have workflows "share" states), the response to that in #28 makes sense. But from an ERD point of view, I understand #27. I could see workflow as an entity, state as an entity, and there is a many-to-many relationship between them, producing an associative entity called workflow state. That would allow you to define a state once, including whether it's an unpublished or published state, then when you're defining a workflow, you would select a state to associate with that workflow. That makes sense for a logical data model - not so sure about a physical data model. Some denormalization might be an OK thing for performance reasons. Here's what I'm talking about:

ERD

alexpott’s picture

Status: Needs work » Needs review
FileSize
244.74 KB

Patch attached completes the decoupling of workflow and content moderation by creating a new plugin type called Workflow Type. Modules that want to attach a workflow to something provide one of these and then you can make workflows for it. And there is 0 content entity logic in the workflow module.

Patch addresses #31 - not really time for that level of review yet. Still need to agree architect and names.

@scookie - well it is just as viable to view states as a property of a workflow - sharing states across workflows seems very strange to me. And we have several bugs in this area already because when you do want the states to be different but have the same name there's nothing you can do. I think the patch attached shows the benefits of managing a workflow as a single configuration entity without having to have the entities that exist in head. Also the objects in your ERD do exist - they are just value objects that the workflow can create.

Status: Needs review » Needs work

The last submitted patch, 36: 2779647-2-36.patch, failed testing.

tacituseu’s picture

Decoupling workflow and content moderation would be great.

Most of my use cases for workflows involve lists with couple of different types (bundles/content types) of Assets with a state assigned from shared AssetWorkflow, each one of them belonging to an Owner, which could be one of 2 other content types (e.g. person or company) that are in another specific (fixed) state in shared workflow OwnerWorkflow, with AssetWorkflow exposed as a filter.

my observations:
- workflows shared between bundle types makes code/forms/queries much cleaner
- workflows often share same state names (mostly Draft and Archived)
- so far found no need to have a state shared between two workflows or to have workflow shared between different entity types

additional things I find useful working with workflows (might be out of scope of content moderation, but a provision to do so would be appreciated):
- ability to block state transition (e.g. until specific field is filled or file attached)
- calling an Action on an entity after successful transition (configurable per transition, e.g. publishing action for content moderation)

scookie’s picture

@alexpott Yes, it's absolutely viable to make state a property of workflow.
You said that sharing states across workflows seems strange to you. In my example, I think “sharing states” is not quite the right phrase. It’s more like “sharing state labels”. In my ERD, state doesn’t have much meaning by itself; it only has meaning when associated with a particular workflow. When a node is in moderation, the handling of its state comes from the Workflow State entity, not the State entity. I added the Published property to the State entity, but if a given state label can represent a published state in one workflow, and an unpublished state in a different workflow, then that property should live in the Workflow State entity, not the State entity. Note that the other information about a state, i.e., Default revision, is in the Workflow State entity because it is specific to a given workflow.

I'm not sure what you mean by "...when you do want the states to be different but have the same name there's nothing you can do.". What do you mean by wanting the states to be different? In the current Content Moderation, we have to define states globally, then enable them for different content types. Because we need a Draft state for 3 different content types, but that Draft state behaves differently for each content type, we have defined 3 states that have a label of Draft. But we have edited the machine name for each of those Draft states so that we CAN have 3 different states called Draft, and each content type has its own "Draft" enabled. That's why we had to make some changes to show the machine name along side of the label when duplicate labels exist. We needed to know which "Draft" we were talking about; which "Draft" we were enabling for a given content type. But in my example ERD, we're only using the State entity for purposes of creating consistent labels. The suggestion that @pericxc made in #27 just provides a way for the user who is configuring the workflows to specify a state label once, then have that label reused by any workflow that needs it. It provides consistency across workflows and saves the workflow administrator a bit of time. It’s kind of like using a taxonomy vocabulary for a field that is used by different content types. In my example, the State entity is like the taxonomy vocabulary and the state's Label property is like the taxonomy term.

scookie’s picture

We still need the ability to have 2 different states with the same label to apply to a single workflow. This is required in order to provide the user with the option to either create a new draft when an item is published, or to "unpublish" the item and return it to a draft state. To handle this, we have 2 Draft states; Draft (draft_1) has its Default revision field selected, Draft (draft_2) has that field unselected. Both of these states are enabled for a given content type.

We have these transitions defined:
Keep Published: From Published to Published
Create New Draft: From Published to Draft (draft_1)
Unpublish: From Published to Draft (draft_2)

When the workflow participant is on the New Draft tab of a node for this content type, s/he will see this:
Save and Keep Published
Save and Create New Draft
Save and Unpublish

tacituseu’s picture

Just to expand on #40, not only what state entity goes into matters, but also how it got there (by which transition), that is why I asked in #38 to have ability to define actions per transition, not per state as it is now.

alexpott’s picture

@scookie the comment

And we have several bugs in this area already because when you do want the states to be different but have the same name there's nothing you can do

Should have come with a link :) #2750365: Permissions: moderation states with the same name is unclear for user to assign to a role is an example of the kind of problems the current approach in HEAD and workbench_moderation is causing.

Personally I wouldn't have 2 draft states for the workflow described in #40 I'd just label the transitions from draft -> published and published -> draft differently. However there is nothing in the current patch that will stop you having multiple states in the same (or workflows) with the same label. The only thing that has to be different is the state's ID and this only has to be unique as port of that workflow.

alexpott’s picture

Status: Needs work » Needs review
FileSize
260.99 KB

Move work

Status: Needs review » Needs work

The last submitted patch, 43: 2779647-2-43.patch, failed testing.

Sam152’s picture

Status: Needs work » Needs review
FileSize
261.01 KB
402 bytes

Missing @group annotation. Keen to see what the bot says.

Status: Needs review » Needs work

The last submitted patch, 45: 2779647-45.patch, failed testing.

Sam152’s picture

Status: Needs work » Needs review
FileSize
45.73 KB
293.81 KB

I've fixed some of the fails. I also converted the functional tests to BTB. If working on this without other contributors is simply easier at this formative stage, feel free to ignore. Don't want to stop momentum on this issue.

Re: the new view, I suspect perhaps the latest revision filter was broken previously which was causing "views.view.latest" to show all revisions instead of just the ones with the filter applied. Now the view which requires access to all the revisions has a dedicated one without any filtering.

bojanz’s picture

This is a great improvement! I'm very happy to see it.

A few questions:
1) What's your strategy for editing workflows after they're in use? Disable the whole form if there are any entities?
Right now there are no safeguards, which means that an edited workflow could result in invalid entities (non-existent states, etc).

2) Why store the workflow ID inside the bundle info, and bless it as the "one true workflow"? Why not simply have a state_item field type that holds the state, and references the workflow? It would be convenient, and more natural when allowing multiple workflows.
This also makes sense when formatting the state, we currently store only the state ID, but will need to show the state label in various places. There's also a use case for a formatter that renders transition buttons (Jira-style), which would benefit from a field type.

3) ModerationStateFieldItemList appears to be unused?

4) I'm not a big fan of keying transitions by from_state, and found it a bit difficult to parse.
There are cases when a transition can have multiple from states, and most state machines make from_state an array because of that, for example cancelling an order that's in various states. Here that would require duplication the transition definition for each from state. Guessing this was done to avoid having a transition ID, even though that's usually a more common implementation.

Status: Needs review » Needs work

The last submitted patch, 47: 2779647-47.patch, failed testing.

alexpott’s picture

@Sam152 thanks for the fixes - I'll merge some of them in. I don't think we should be converting all the tests to BTB since that is not part of the scope of the issue.

@bojanz thanks for the review:

  1. Yep this is painful in HEAD right now - need to create an issue for this. It'll will be way easier to solve once this lands. But it does hint that WorkflowType plugins need to be able to determine if a state is in use and prevent deleting.
  2. That sounds like a really good idea for a follow-up to make something being in multiple workflows way simpler. We should address all of this in a followup. I also think that ModerationStateFieldItemList should return the ContentModerationState value object which would make implementing the jira-like functionality very simple.
  3. It is definitely still used - see \Drupal\content_moderation\EntityTypeInfo::entityBaseFieldInfo()
  4. Yeah up for changing the implementation to something more similar to symfony's definition... it doesn't matter really because nothing interacts directly with the data as it is stored.
alexpott’s picture

Status: Needs work » Needs review
FileSize
266.73 KB

More tests. Once I've fixed with the tests of the workflow module I'll post an issue summary update that describes the solution and compares to the current situation in HEAD. And try to put together a plan for getting this done.

jhedstrom’s picture

I spoke with @alexpott at Badcamp, and wanted to post a few notes:

  • Workflows with more complex requirements will be able to easily decorate the states provided, for instance, by Content Moderation, to customize transition logic as needed.
  • The example of 3 workflows being the same except for one transition could actually rather be a single workflow with customized logic (eg, check bundle being applied to, etc) at that particular transition.
  • The separation of workflow from content will allow quite cool things in the future, such as moderation of configuration (eg, a Views publishing workflow), user approval workflows, and many other things that are currently not possible with the tight coupling to content entities.
scookie’s picture

@alexpott regarding your comments in #42

First of all, I’m painfully aware of the issue reported in #2750365: Permissions: moderation states with the same name is unclear for user to assign to a role as well as https://www.drupal.org/node/2738903 - I’m the person who prompted the reporting of these issues. I work with @pericxc and @jhedstrom. ☺ The solution is to display the machine name when duplicate state names and transition names exist.

Second, regarding your comment “Personally I wouldn't have 2 draft states for the workflow described in #40 I'd just label the transitions from draft -> published and published -> draft differently.”

Um… not sure what you mean by labeling the transitions from draft -> published and published -> draft differently. I need 2 different transitions, both of which go from Published to some draft state. One transition goes from published to the draft state that is NOT the default revision (that transition is labeled Create new draft), and the other transition goes from published to the draft state that IS the default revision (that transition is labeled Unpublish). So yeah, I need 2 draft states. How is your “labeling” solution going to work given my use cases?

Thanks!

Status: Needs review » Needs work

The last submitted patch, 51: 2779647-2-51.patch, failed testing.

alexpott’s picture

Status: Needs work » Needs review
FileSize
285.22 KB

@scookie #53 is interesting. Yes my labelling solution is not going to work for that case - but the solution in the patch does not stop you have two states with the same label in the same workflow. So your current solution is still viable. But it hints that we should change content moderation to add the actions to the transition rather than the state. Which means that we should make it simple for workflow types to decorate transitions as well as states. Considering this has come up several times going to incorporate that into the patch.

Patch attached has full testing of the Workflow and State objects provided by the workflow module. Still need to add more tests of the integration points.

scookie’s picture

@alexpott - Thanks for your response! I just wanted to make sure that in this new incarnation of workflow, we'll still be able to have duplicate state labels (and duplicate transition labels) and that we'll be able to differentiate between them by displaying their machine names when appropriate.

Also, currently in our Drupal 6 production environment, we have triggers defined to perform actions based on events that are state transitions. One of these triggers sends email notifications out to people with certain roles when a particular state transition occurs. I'm not entirely sure those are the kinds of actions you're talking about - I may be mixing apples and oranges here.

Status: Needs review » Needs work

The last submitted patch, 55: 2779647-2-55.patch, failed testing.

timmillwood’s picture

diff --git a/core/phpunit.xml.dist b/core/phpunit.xml.dist
index 9658f2c..463f080 100644
--- a/core/phpunit.xml.dist
+++ b/core/phpunit.xml.dist
@@ -15,7 +15,7 @@
  result printer that links to the html output results for functional tests.
  Unfortunately, this breaks the output of PHPStorm's PHPUnit runner. However, if
  using the command line you can add
- - -printerClass="\Drupal\Tests\Listeners\HtmlOutputPrinter" to use it (note
+ - -printer="\Drupal\Tests\Listeners\HtmlOutputPrinter" to use it (note
  there should be no spaces between the hyphens).
 -->
   <php>
@@ -28,7 +28,7 @@
     <!-- Example SIMPLETEST_DB value: mysql://username:password@localhost/databasename#table_prefix -->
     <env name="SIMPLETEST_DB" value=""/>
     <!-- Example BROWSERTEST_OUTPUT_DIRECTORY value: /path/to/webroot/sites/simpletest/browser_output -->
-    <env name="BROWSERTEST_OUTPUT_DIRECTORY" value=""/>
+    <env name="BROWSERTEST_OUTPUT_DIRECTORY" value="/Users/alex/dev/sites/drupal8alt.dev/sites/simpletest"/>
   </php>
   <testsuites>
     <testsuite name="unit">

Don't think these changes are needed.

I've had a good read of the patch, not sure I fully understand how the plugins are used, but it all looks pretty awesome.

I still have the concern from #33 about it being called "Workflow" module. How about "Workflows", "Workflow State" or "Workflow States"?

jhedstrom’s picture

I still have the concern from #33 about it being called "Workflow" module. How about "Workflows", "Workflow State" or "Workflow States"?

I think it would be preferable for core to take the most sensible name, which in this case is 'Workflow'. That contrib project seems to be in a limbo state for 8.x at this time.

alexpott’s picture

@timmillwood workflow type plugins are the way a module can leverage workflow and attach them to something. So in the current patch content_moderation provides such a plugin to be able to create workflows for content entities.

alexpott’s picture

Status: Needs work » Needs review
FileSize
304.32 KB

The patch attached separates the transitions from states so that workflow types can also decorate transitions and do special things. The separation of concerns takes us to a config that looks very like Symfony and what @bojanz was saying about other state machine implementations.

Status: Needs review » Needs work

The last submitted patch, 61: 2779647-2-61.patch, failed testing.

Sam152’s picture

@alexpott, what are you looking for to move this forward at this stage? Detailed code review? Manual testing?

pericxc’s picture

FileSize
177.12 KB

I would like to let you know that simple solution I am presenting here would solve all our problems and I guess all sites that dealing with large number of different workflows and states:

1. Enforce unique entity state labels
2. Adding state transitions to the bundle's workflow ( or Workflow entity)
======
This way workflow( states and transitions) are reusable, the number of states and transitions are keep to the minimum, (critical for sites with large number of workflows) including permissions.

Seems like you are going in different direction but current CM with small changes changes looks good.

my simple solution

scookie’s picture

HI @pericxc,

I'm concerned about the restriction of enforcing entity state labels that you discuss in #64.
In order to allow the option of either creating a new draft or unpublishing and returning the published version to a draft state, we need 2 draft states - one being the default revision, the other not being the default revision. I don't want to have 2 different labels for them, I want both of them to be called "Draft". So we would continue to expose the machine name when duplicate labels exist.

alexpott’s picture

@Sam152 I'm still working on the code. I need to update the issue summary to reflect the nature of the change so people don't have to read the code to find out.

alexpott’s picture

Status: Needs work » Needs review
FileSize
318.42 KB

More work on the UI and form classes for workflow states and transitions.

Status: Needs review » Needs work

The last submitted patch, 67: 2779647-2-67.patch, failed testing.

alexpott’s picture

Status: Needs work » Needs review
FileSize
327.81 KB

This should be the first green patch - took @bojanz suggestion and added the workflow to the ContentModerationState which potentially opens up multi-workflow possibilities in future patches.

Code reviews of the workflow module that's part of this patch are now welcome - especially those that concentrate on architectural questions and discussion - of which there has been plenty already on this issue (which is fantastic).

To do:

  • Add more test coverage of workflow type plugins abilities to decorate state and transitions and add forms to the workflow UI.
  • Discuss whether we want to call states "states". In Symfony land and the maths of workflows they are called places. A workflow consists of places and transitions. See https://en.wikipedia.org/wiki/Petri_net - technically this could be a follow up but I think it is worth getting the basic names right first up
  • Update issue summary with approach from patch
  • Open followups from @todo's in workflow module

Status: Needs review » Needs work

The last submitted patch, 69: 2779647-2-69.patch, failed testing.

alexpott’s picture

Status: Needs work » Needs review
FileSize
2.43 KB
327.55 KB

Boo uasort!

alexpott’s picture

Adding tests of how a workflow type can decorate transitions and states with additional information and display forms to collect this information in the UI.

Status: Needs review » Needs work

The last submitted patch, 72: 2779647-2-72.patch, failed testing.

alexpott’s picture

Issue summary: View changes
alexpott’s picture

Status: Needs work » Needs review
FileSize
509 bytes
338.36 KB

Fixing test

alexpott’s picture

Issue summary: View changes
alexpott’s picture

Issue summary: View changes

Status: Needs review » Needs work

The last submitted patch, 75: 2779647-2-75.patch, failed testing.

alexpott’s picture

Status: Needs work » Needs review
FileSize
10.07 KB
339.62 KB

The fails in #75 are unrelated. Patch attached does a bit more cleaning up and adding missing explicit test coverage of additions to content moderation.

alexpott’s picture

Standardising on the short array syntax in the new workflow module and ensuring we have complete test coverage of the State and Workflow objects.

timmillwood’s picture

Currently it's possible to delete a workflow that's in use.

If you do this then try to edit a moderated node you get:

Error: Call to a member function id() on null in drupal/core/modules/content_moderation/src/Plugin/Field/ModerationStateFieldItemList.php on line 35

If you do this then try to add a new moderated node you get:

Error: Call to a member function getInitialState() on null in drupal/core/modules/content_moderation/src/Plugin/Field/FieldWidget/ModerationStateWidget.php on line 120

We could either prevent workflows from being deleted, or update content moderation to ignore a defined workflow if it doesn't exist.

alexpott’s picture

@timmillwood currently is it possible to delete transitions and states in Content moderation - imo that's for a followup. See section on followups.

alexpott’s picture

Added a Workflow::loadMultipleByType() to make life easy. It's now used by content_moderation.

timmillwood’s picture

Just finished the weekly UX call where I demo'd and talked about @alexpott's Workflow module from this issue.

Jozef has been working on some prototypes for this, which again brought up the question of moderation states being outside a workflow so they can be used in multiple workflows. We discussed if you wanted to edit a moderation state and wanted it to affect all workflows with that state, or if you wanted to edit a moderation state and not affect all workflows with that state. There was no clear answer, but personally I am thinking there will be more times a user will only want to affect one workflow.

We also talked about attaching workflows to things within the workflow. For example the content moderation workflow type plugin could list all bundles within the workflow edit page. This would save users having to go off to the bundle settings page.

Finally we talked about the module name, Kevin suggested "Flows". As "Workflows" is specific to work flows, but what we are defining could be used for any sequential process.

I will ask Jozef to post his prototypes here, as he had a few other neat ideas about adding and editing moderation states and transitions.

catch’s picture

It would be useful for review to split the patch into content_moderation changes and adding the workflow module - makes sense to commit all in one go however.

This is a big change, but it's not as big as the initial experimental content_moderation commit, so I'd support getting it sooner and later and dealing with any further fallout in follow-up issues.

Wil try to have a proper look tomorrow.

One quick question from the top of the patch:

+++ b/core/modules/content_moderation/src/ContentModerationState.php
@@ -0,0 +1,114 @@
+  public function isPublished() {

Should this be isPublishedState() or something? The workflow itself does not get published. Same with isDefaultRevision().

timmillwood’s picture

I've found another potential issue which is kinda related to #81. If a content type has Workflow Y with state A and there is content with state A, the content type can be changed to Workflow X which might not have state A.

catch’s picture

Here's the split patches.

catch’s picture

Did a first pass through, tried not to nitpick and instead look at higher level things.

Generally this looks great, the config entities make a lot more sense, content_moderation has 1,000 lines of code removed etc.

The main thing that stuck out negatively is the workflow UI and how it relates to content_moderation:

- the UI is under admin/config/workflow in its own category
- When you create a UI, you start with this form:

- the UI for selecting a workflow is accessibly only via the content type overview select list:

If you don't have any workflow-enabling modules enabled, you get a UI with a message telling you to enable a module.

Workflows are tied to workflow types (you can't create a workflow for content_moderation, then start using it with some other type of workflow-reliant module). Also after you create a workflow, you have to go from admin/config/workflow to admin/structure/types, then find the 'content moderation' option in the select list on the content type to associate it. Given all this, I'm not sure how the centralized UI is useful here.

It seems more like we'd want a UI per-workflow-type. However given the routes are defined on the config entity, I'm not sure how that can be done without essentially subclassing the config entity in content_moderation (and the entity that workflow provides just being a stub with no UI exposed). This all leads to it being less generic again. If we did do something like that though, then workflow could probably move to \Drupal\Core (with everything @internal for now) rather than being a module, which would in turn help with the contrib workflow module conflict.

One question on the API:

+++ b/core/modules/workflow/src/WorkflowInterface.php
@@ -0,0 +1,240 @@
+
+  /**
+   * Sets a state's label.
+   *
+   * @param string $state_id
+   *   The state ID to set the label for.
+   * @param string $label
+   *   The state's label.
+   *
+   * @return \Drupal\workflow\WorkflowInterface
+   *   The workflow entity.
+   */
+  public function setStateLabel($state_id, $label);
+

It looks slightly odd having individual methods here vs. the state value object. But then given state objects are immutable, not sure adding an mutable version just to put these methods on there would be an improvement either.

Also wondered why the workflow object has to store weights vs. just relying on the order in the YAML.

amateescu’s picture

workflow could probably move to \Drupal\Core (with everything @internal for now) rather than being a module, which would in turn help with the contrib workflow module conflict.

Big +1. That's what we discussed in Dublin and I was a bit surprised to see it as a module in the initial patch.

Fabianx’s picture

+1 to workflow being part of core instead of being a module

tacituseu’s picture

- the UI for selecting a workflow is accessibly only via the content type overview select list:

There should also be a local task defined for each bundle (content_moderation\src\Plugin\Derivative\DynamicLocalTasks.php) in addition to select list entry under operations (last checked on 2779647-2-79.patch).

Now that there can be multiple workflows, it feels more like it belongs under "admin/structure" than "admin/config/workflow", maybe move it as a follow-up ?

alexpott’s picture

Thanks for all of the reviews.

There's been a bit of debate about using @internal to mean experimental - in some ways it means exactly the opposite because the thing is we want modules to try out the workflow entities and use them if they are about making workflows. Personally I think some sort of centralised UI is useful because an overview of all the complex workflows available and in use on your site is intrinsically useful to understanding what is going on. That said I agree that the current UI can be improved on - however I challenge people to use HEAD and think it is better. Maybe a better experience would be to move the type selection further forward in the UI and into the URL so /admin/config/workflow/workflows/add would become /admin/config/workflow/{type}/add and /admin/config/workflow/{type} would list workflows of that type. That would make the UI seem to be more part of content moderation and not result in unrelated workflows being listed together.

+1 to workflow being part of core instead of being a module

With respect to moving the entity and plugin into core - I'm completely neutral on this. I guess if we do this and the above thing we could then rename the module to be workflow_ui which might be a very good thing because changing workflows once a site is up and running is probably not something we want to encourage.

Also wondered why the workflow object has to store weights vs. just relying on the order in the YAML.

The weight key exists because it diffs way better in configuration than moving states. In fact Workflow::addState and Workflow::addTransition should sort be key to ensure that config diffs nicely.

It looks slightly odd having individual methods here vs. the state value object.

It think the value of immutable states and transitions outweighs the awkwardness of changing their values ie Workflow::setStateLabel / etc... because the common case of interacting with a workflow is not changing a label or weight.

Now that there can be multiple workflows, it feels more like it belongs under "admin/structure" than "admin/config/workflow"

Wrt to moving to admin/structure vs admin/config - I'm not sure that workflow affect the structure of your site so I ponder if moving there is a good idea. Happy for one of the UX specialists to have their say - plus as @tacituseu notes this is something that could be done post this change.

Workflows are tied to workflow types (you can't create a workflow for content_moderation, then start using it with some other type of workflow-reliant module)

Whilst working on the architecture for this change I thought quite a bit about this. I just could conceive of a use-case for multi-type workflows - a content publishing workflow is quite different from config deployment workflow for example. But OTOH if we think this is useful we could change it to allow a workflow to have multiple plugins it is just I think this decision makes sense.

Fabianx’s picture

+1 to moving the parts into an experimental workflow_ui module and moving the rest centralized into core

Sam152’s picture

Thank you for working on this. This is looking really good, I've installed this patch on a fresh copy of Drupal 8 and also applied it against a project I am working on which heavily uses content_moderation. After wiping the content and upgrading the config, I was able to get a lot of the project test suite green by simply updating labels, buttons and UI assertions where required.

If the major items of feedback left are usability and other non-schema related changes, I think it would be great to push ahead with getting this in. That will also help unblock effort from other contributors for different parts of the module (bug reports, UX, follow ups), bit tricky multiple people working on different parts in isolation at this stage.

It looks like the architectural feedback is largely on the table at this stage?

bojanz’s picture

Great progress since my review in #48.

Discuss whether we want to call states "states". In Symfony land and the maths of workflows they are called places. A workflow consists of places and transitions. See https://en.wikipedia.org/wiki/Petri_net - technically this could be a follow up but I think it is worth getting the basic names right first up

I think states make the most sense. What sounds more logical to an everyday developer: "The node's state" or "The node's place"? $node->getState() or $node->getPlace()? I am not knowledgeable enough about theory to know how petri net theory differs from the general state machine one, but solutions before tended to cover state machines and states, I've only seen places once or twice (primarily in the symfony library).

1) It feels limiting that we don't allow multiple "from" states in a transition. That requires transitions to be duplicated for each from state. A node could be published from different moderation states, if allowed. An order might be cancelled from any part of a workflow. Etc. You can see how this looks in both winzou/state-machine(which was Symfony's most prominent state machine library prior to the official one) and in my state_machine module.

EDIT: Just checked symfony/workflow, and they allow both multiple "from" states and multiple "to" states, so I guess we didn't inherit this limitation from their approach.

2)

+  published-published:
+    from: published
+    to: published
+    label: Publish

This label doesn't feel right. If I save an already published Node, I'm not publishing it again. I would expect the verb to be a regular "Save" cause I'm not actually transitioning to a different state.

3) We're still missing a state field type. Doesn't need to be added in this issue, but we really don't want every module to reinvent one, just to be able to render a state label and show a transition form on the rendered entity.

4) The State object has id(), label(), weight(), getTransitionTo(), getTransitions(). Why don't the first three methods have a "get" prefix? We have no reason to be inconsistent here, and we stopped naming getters without a prefix in core a long time ago.
Same with Transition, which has id(), label(), from(), to() instead of getId(), getLabel(), getFromState(), getToState().

Nitpicks:

+class Workflow extends ConfigEntityBase implements WorkflowInterface, EntityWithPluginCollectionInterface {
+
+  /**
+   * The Moderation state ID.
+   *
+   * @var string
+   */
+  protected $id;

Not the moderation state ID.

+  /**
+   * The states of the workflow.
+   *
+   * The array key is the machine for the state. The structure of each array
+   * item is:

the machine name for the state.

+  /**
+   * @var array
+   */
+  protected $type_settings = [];
+
+  /**
+   * @var \Drupal\Component\Plugin\LazyPluginCollection
+   */
+  protected $pluginCollection;

Missing first line of docs.

+class WorkflowAddForm extends EntityForm {
+
+  /**
+   * WorkflowEditForm constructor.
+   *
+   * @param \Drupal\Component\Plugin\PluginManagerInterface $workflow_type_plugin_manager
+   *   The workflow type plugin manager.
+   */

Wrong class name in constructor docblock.

alexpott’s picture

alexpott’s picture

Moving all the things into core but leaving the UI behind. Now will work on the rest of the feedback. No interdiff because all moves would make for something massive.

Status: Needs review » Needs work

The last submitted patch, 97: 2779647-2-97.patch, failed testing.

alexpott’s picture

Status: Needs work » Needs review
FileSize
545 bytes
354.6 KB

So in order to do this we need to do an update :(

This moving to core is becoming a bad idea for me.

Status: Needs review » Needs work

The last submitted patch, 99: 2779647-2-99.patch, failed testing.

timmillwood’s picture

Why is the update such a bad thing? because we're enforcing the installation of an entity marked @internal?

alexpott’s picture

Issue summary: View changes
Status: Needs work » Needs review
FileSize
10.58 KB
354.98 KB

Just spent 20 minutes in my head thinking about the transitions and @bojanz comments on allowing multiple to's and from's. My initial thoughts were this just makes everything more complex. But then I had a realisation - allowing multiple tos and froms means that we can have less transitions. And less transitions means that we can have less permissions per workflow in content moderation. Which will make configuring and maintaining a site simpler which is a good thing. Because then you can create complex workflows and chose how complex the permissions get by defining different amounts of transitions - regardless of the the number of states. One fly-in-the-ointment is that this makes transition labelling complex so perhaps the best solution is the halfway house of https://github.com/winzou/state-machine and just allow multiple froms.

Before I start implementing transitions based on this I'd like some feedback since this change is likely to be quite extensive.

Patch attached addresses @bojanz's nits in #95 and @catch's comments about ContentModerationState's method names in #88.

Still to address the workflow UI per type question but I think it is a really good idea and one that we should attempt to implement here.

@timmillwood the update is bad because as an experimental thing making changes to live sites is bad form.

timmillwood’s picture

One other thought. Now workflows are in core, content_moderation doesn't really *need* to depend on workflow_ui. Content_moderation defines the typical workflow, and typically that's all that's needed. It can be attached to a bundle via the bundle settings, and be very functional without workflow_ui.

catch’s picture

Just spent 20 minutes in my head thinking about the transitions and @bojanz comments on allowing multiple to's and from's.

So a common use-case is you want regular content creators to be able to transition from draft to review, but editors to go straight from draft (or any intermediate state) to published. And then even more common is to go from any review state back to draft.

So if we take:

draft -> review

draft -> published
review -> published

review -> draft
published -> draft

It sounds like you'd still need two transitions if you wanted to restrict the draft -> published transition to regular content editors? But if both groups can set things back to draft, that's still a net reduction in transitions and permissions so seems sensible.

tacituseu’s picture

Just wanted to note/clarify that there is a difference between what was discussed here to this point and how multiple 'froms' and 'tos' work in Symfony's Workflow implementation (as it seems to be the source of this idea).

from https://github.com/symfony/workflow/blob/master/Workflow.php:

/**
 * Return the first Transition in $transitions that is valid for the
 * $subject and $marking. null is returned when you cannot do any Transition
 * in $transitions on the $subject.
 *
 * @param object       $subject
 * @param Marking      $marking
 * @param Transition[] $transitions
 *
 * @return Transition|null
 */
private function getTransitionForSubject($subject, Marking $marking, array $transitions)
{
		foreach ($transitions as $transition) {
				foreach ($transition->getFroms() as $place) {
						if (!$marking->has($place)) {
								continue 2;
						}
				}
				if (true !== $this->guardTransition($subject, $marking, $transition)) {
						return $transition;
				}
		}
}

it means the multiple 'from' transition is not available if given subject doesn't have _both_ markings (states, see comment in: https://github.com/symfony/workflow/blob/master/MarkingStore/MultipleSta...)

See sample 'or' and 'and' workflows from: https://github.com/lyrixx/SFLive-Paris2016-Workflow/blob/master/app/conf..., the multiple 'froms' there aren't meant to reduce number of transitions.

What was discussed here to this point is the 'or' type, (e.g. you want one transition to be able to get you from either draft or review to published), what Symfony's multiple 'froms' and 'tos' is about is the 'and' type (e.g. transition will take you to published only when you reached both approved_by_journalist and approved_by_spellchecker see: http://symfony-workflow-demo.herokuapp.com/article for nice graph at the bottom).

catch’s picture

So the update is unfortunate, but I think we've worked ourselves into a corner a bit:

- making the change in 8.3.x-only undermines the goal of an alpha experimental module
- the update doesn't actually do anything really, it just puts the entity type definition into k/v. This matters for content entities, discussed with Alex Pott and neither of us can see a reason to do it for config entities except that it means we do the same thing as content entities.
- we could do a patch to stop keeping configuration entities in state, but that'd be even more of an 8.3.x change. Although we should open an issue to change things so that we don't run into this again.

My suggestion would be to add a post update to do the same as the update function here, and run that in a patch release. That's very low risk and the post update means we don't have to worry about update numbering issues between the branches.

alexpott’s picture

After discussing with @bojanz we looked at both https://github.com/aasm/aasm and https://github.com/winzou/state-machine (https://github.com/winzou/state-machine/blob/master/src/SM/StateMachine/...) which both have a more similar understanding of workflow as a state machine where the entity can only be in one state (per workflow). Therefore I've pursued the multiple from states solution as detailed in several of @bojanz's comments.

The patch also moved the update function to a post update as recommended by #106.

Now the only test is to resolve some of the UI difficulties by tying it more into the workflow type providers. We could choose to delay this to a follow-up as I don't think that this work will influence the core workflow, state or transition objects. However I do think it will affect \Drupal\Core\Workflow\WorkflowTypeInterface as I think one of the likely things is to add a buildWorkflowConfigurationForm method so that the workflow type can add stuff to the main workflow form. In content moderation's case this will be a way of attaching workflows to particular content entities.

@timmillwood the patch also proves the decoupling of workflow_ui and content_moderation by removing the dependency.

alexpott’s picture

Discussed two more issues with @bojanz:

  1. How to sort transitions - atm we sort on label and to state weight - we agreed to add a weight property to transitions and sort using that.
  2. Whether or not to allow transitions that duplicate another transition - we agreed that this should not be allowed.

Patch attached improves test coverage.

alexpott’s picture

Issue summary: View changes
alexpott’s picture

Patch attached prevent transitions from duplicating a transition made possible by another already existing transition.

Status: Needs review » Needs work

The last submitted patch, 111: 2779647-2-111.patch, failed testing.

The last submitted patch, 111: 2779647-2-111.patch, failed testing.

The last submitted patch, 111: 2779647-2-111.patch, failed testing.

tacituseu’s picture

Status: Needs work » Needs review

Just a nitpick:

-      '#published' => $default ? $default_state->isPublishedState() : FALSE,
+      '#default_value' => $default->id(),
+      '#published' => $default->isPublishedState(),

'#published' got changed to '#published_status' in #2498919: Node::isPublished() and Node::getOwnerId() are expensive, looks as if this is not used anyway, it seems to be handled in NodeModerationHandler::onPresave now.
Back to needs review because change was caused by unrelated problem.

Sam152’s picture

This is looking really good, hard to fault the code or the approach. I've reviewed the patch with review_level(E_NIT). A lot of these are just questions or matter of style so feel free to ignore. If the major architectural discussions have taken place, I fully expect the next step to be RTBC and commit. It's experimental and blocks/conflicts pretty much all other content moderation issues, lets get this in.

Leaving this NR for others to kick in any comments.

  1. +++ b/core/lib/Drupal/Core/Workflow/Entity/Workflow.php
    @@ -0,0 +1,485 @@
    +    if (preg_match('/[^a-z0-9_]+/', $state_id)) {
    ...
    +    if (preg_match('/[^a-z0-9_]+/', $transition_id)) {
    

    Can be made DRY with perhaps a validateLowercaseMachineName method?

  2. +++ b/core/lib/Drupal/Core/Workflow/Entity/Workflow.php
    @@ -0,0 +1,485 @@
    +    // Always add to the end.
    ...
    +    // Always add to the end.
    

    Perhaps some protected method to get the max weight out of a certain property? Could be used in both places.

  3. +++ b/core/lib/Drupal/Core/Workflow/Entity/Workflow.php
    @@ -0,0 +1,485 @@
    +      array_multisort(
    +        $weights, SORT_NUMERIC, SORT_ASC,
    +        $labels, SORT_NATURAL, SORT_ASC
    +      );
    

    Can we create a protected method to sort these by weight and label and call from both getStates and getTransitions?

    Maybe a candidate for \Drupal\Component\Utility\SortArray?

  4. +++ b/core/lib/Drupal/Core/Workflow/WorkflowAccessControlHandler.php
    @@ -0,0 +1,74 @@
    +      $container->get('core.workflow_type_manager')
    ...
    +    $workflow_types_count = count(\Drupal::service('core.workflow_type_manager')->getDefinitions());
    

    This is injected, no need for \Drupal

  5. +++ b/core/lib/Drupal/Core/Workflow/WorkflowAccessControlHandler.php
    @@ -0,0 +1,74 @@
    +    return $admin_access->andIf(AccessResult::allowedIf($workflow_types_count > 0))->addCacheTags(['config:core.extension']);
    

    This might need a comment, don't understand this. If there are multiple workflow types the access result becomes neutral?

  6. +++ b/core/lib/Drupal/Core/Workflow/WorkflowInterface.php
    @@ -0,0 +1,289 @@
    +  public function getTransitionsForState($state_id, $direction = 'from');
    

    Maybe we should have directional constants?

  7. +++ b/core/modules/content_moderation/config/install/core.workflow.typical.yml
    @@ -0,0 +1,63 @@
    +label: 'Typical workflow'
    

    Seems like a good as time as any to bikeshed this. "Basic Editorial Workflow", "Editorial Workflow"?

  8. +++ b/core/modules/content_moderation/config/install/core.workflow.typical.yml
    @@ -0,0 +1,63 @@
    +type: content_moderation
    +type_settings:
    +  states:
    +    archived:
    +      published: false
    +      default_revision: true
    +    draft:
    +      published: false
    +      default_revision: false
    +    published:
    +      published: true
    +      default_revision: true
    

    Reading this makes the approach to the decoupling aspect of the patch make a lot of sense to me.

  9. +++ b/core/modules/content_moderation/src/ContentModerationState.php
    @@ -0,0 +1,114 @@
    +  public function isPublishedState() {
    +    return $this->published;
    +  }
    

    Do we need an interface that extends StateInterface to house these?

  10. +++ b/core/modules/content_moderation/src/Form/BundleModerationConfigurationForm.php
    @@ -50,128 +49,64 @@ public function getBaseFormId() {
    +      '#type' => 'hidden',
    

    Should probably be type => value

  11. +++ b/core/modules/content_moderation/src/Form/BundleModerationConfigurationForm.php
    @@ -50,128 +49,64 @@ public function getBaseFormId() {
    +      '#type' => 'hidden',
    

    Should probably be type => value

  12. +++ b/core/modules/content_moderation/src/Form/BundleModerationConfigurationForm.php
    @@ -50,128 +49,64 @@ public function getBaseFormId() {
    +      '#type' => 'hidden',
    

    Should probably be type => value

  13. +++ b/core/modules/content_moderation/src/Form/BundleModerationConfigurationForm.php
    @@ -50,128 +49,64 @@ public function getBaseFormId() {
    +        //      @todo make this work with new select
    +        //        '#states' => [
    +        //          'visible' => [
    +        //            ':input[name=enable_moderation_state]' => ['checked' => FALSE],
    +        //          ],
    +        //        ],
    

    Need a follow up issue link.

  14. +++ b/core/modules/content_moderation/src/Plugin/Field/FieldFormatter/ContentModerationStateFormatter.php
    @@ -0,0 +1,49 @@
    + * Plugin implementation of the 'uri_link' formatter.
    ...
    + *   id = "content_moderation_state",
    

    Docblock incorrect

  15. +++ b/core/modules/content_moderation/src/Plugin/Field/FieldFormatter/ContentModerationStateFormatter.php
    @@ -0,0 +1,49 @@
    +  public static function isApplicable(FieldDefinitionInterface $field_definition) {
    +    // By default, formatters are available for all fields.
    +    return TRUE;
    +  }
    +
    

    I assume this needs a @todo to make it only format the moderation string fields.

  16. +++ b/core/modules/content_moderation/src/Plugin/Field/FieldWidget/ModerationStateWidget.php
    @@ -23,7 +21,7 @@
    - *     "entity_reference"
    + *     "string"
    

    Does this class have an isApplicable as well or need one?

  17. +++ b/core/modules/content_moderation/src/Plugin/WorkflowType/ContentModeration.php
    @@ -0,0 +1,164 @@
    +    // TODO: Implement calculateDependencies() method.
    

    @todo for easy grep? Also lets add a follow up.

  18. +++ b/core/modules/workflow_ui/src/Form/WorkflowEditForm.php
    @@ -0,0 +1,204 @@
    +      '#markup' => $this->t('@todo describe what a state is.'),
    

    Maybe this is implied knowledge if you get this far?

  19. +++ b/core/modules/workflow_ui/src/Form/WorkflowStateEditForm.php
    @@ -0,0 +1,168 @@
    +      '#access' => $this->entity->access('edit'),
    

    heh, good thinking

  20. +++ b/core/modules/workflow_ui/src/Form/WorkflowTransitionAddForm.php
    @@ -0,0 +1,150 @@
    +    // @todo add some ajax to ensure that only valid transitions are selectable.
    

    Lets create an issue.

  21. +++ b/core/modules/workflow_ui/src/Form/WorkflowTransitionEditForm.php
    @@ -0,0 +1,170 @@
    +      '#type' => 'hidden',
    

    type => value?

alexpott’s picture

  1. I don't think this is worth it I think the default regex from MachineName should be available as a constant and we should be using that.
  2. Done - looks nice
  3. SortArray has problems with objects that are only fixed in PHP7 and I don't think a generic helper helps because the typehint would be StateInterface[]|TransitionInterface{}
  4. Fixed
  5. Fixed
  6. I thought about this but then we'd have to replace 'from' everywhere and that just wouldn't look right.
  7. Changed to 'editorial' it's a good name
  8. Yep
  9. I don't think so because a workflow type can change the state and transition object we need the interfaces for the value objects... but nothing can replace their replacements... so we should just treat these are value objects and typehint against the concrete class.
  10. Fixed
  11. Fixed
  12. Fixed
  13. Fixed by just changing to #acccess - #states actually didn't make much sense there.
  14. Fixed - nice spot
  15. Fixed properly by copying what's in the widget :)
  16. It has one :)
  17. Fixed - created #2830581: Fix ContentModeration workflow type to calculate correct dependencies
  18. Removed and created #2830584: Use modals for creating, updating, and deleting workflows, with a new DialogFormTrait to follow up on the UI - maybe states do need explaining but we should work that out there
  19. Fixed
  20. Added the followup #2830584: Use modals for creating, updating, and deleting workflows, with a new DialogFormTrait
  21. Fixed

I (or someone else) needs to create all the other followups listed in the issue summary.

alexpott’s picture

Issue summary: View changes
Sam152’s picture

Issue summary: View changes
Sam152’s picture

Follow up issues created.

timmillwood’s picture

Title: Content moderation states/transitions/allowed states/transitions » Add a workflow component, ui module, and implement it in content moderation
Assigned: Unassigned » xjm
Issue summary: View changes
Status: Needs review » Reviewed & tested by the community
Issue tags: +Needs release manager review

Every time I review it I'm shocked by the size of the patch. I don't see any major issues, everything I see is or can be covered by follow up issues.

Assigning to XJM for review.

Also updating issue title as the old one didn't mean much.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 117: 2779647-2-117.patch, failed testing.

alexpott’s picture

Status: Needs work » Reviewed & tested by the community
FileSize
369.94 KB

There was a conflict in core/modules/system/system.post_update.php because a recent issue added one.

The last submitted patch, 117: 2779647-2-117.patch, failed testing.

tstoeckler’s picture

Read through the entire patch except for the test coverage and this really looks great. Thanks for the incredible effort on this @alexpott but also everyone else for the reviews, etc. We have actually implemented something similar for our project, so I've been spending a lot of thought and time on all the different parts that make up a workflow engine and I must say that even though ours is actually pretty cool, as well ;-) I'm thoroughly impressed. Those architectural decisions that are nice in our implementation you have done similarly and those that I have come not to like as much you have solved waaaay more elegantly. And at the same time this is orders of magnitude more generic that what we have. Wow, really impressive!

Some notes, nothing that makes me want to un-RTBC this, though. I will open follow-ups for anything that's not fixed when this goes in and also for the architectural questions assuming they are not shot down as nonsense.

  1. +++ b/core/lib/Drupal/Core/Workflow/Entity/Workflow.php
    @@ -0,0 +1,494 @@
    +  public function getTransitionsForState($state_id, $direction = 'from') {
    

    So everywhere else you are very strict about validating input, so maybe we can validate $direction as well?

  2. +++ b/core/lib/Drupal/Core/Workflow/Entity/Workflow.php
    @@ -0,0 +1,494 @@
    +  public function setTransitionFromStates($transition_id, array $from_state_ids) {
    ...
    +      if ($this->hasTransitionFromStateToState($from_state_id, $this->transitions[$transition_id]['to'])) {
    +        $transition = $this->getTransitionFromStateToState($from_state_id, $this->transitions[$transition_id]['to']);
    +        if ($transition_id !== $transition->id()) {
    +          throw new \InvalidArgumentException("The '{$transition->id()}' transition already allows '$from_state_id' to '{$this->transitions[$transition_id]['to']}' transitions in workflow '{$this->id()}'");
    

    This one is interesting. I can see why we do this, but I can also see two reasons against it:

    1. Having a transition that is a superset (in terms of the from states) of another transition that is only available to privileged users. This can also be solved by making to transitions where the from states do not overlap, so this is only a half-reason, but it's not completely invalid because the superset could possibly make for a better UI in terms of labelling, etc.
    2. We actually have the concept of "Change classes" in our project, i.e. if you only fix a typo it triggers something different than if you only change content. This could also be solved by adding additional states (possibly with auto-transitions) so is also only a half-reason, but I wanted to bring it up.

    Can also be discussed in a follow-up but I thought would be worthwhile to bring up.

  3. +++ b/core/lib/Drupal/Core/Workflow/State.php
    @@ -0,0 +1,118 @@
    +    return $this->workflow->hasTransitionFromStateToState($this->id, $to_state_id);
    ...
    +    return $this->workflow->getTransitionFromStateToState($this->id(), $to_state_id);
    

    Super minor, but we should be using $this->id() or $this->id consistently.

  4. +++ b/core/lib/Drupal/Core/Workflow/WorkflowInterface.php
    @@ -0,0 +1,289 @@
    +  public function setTransitionFromStates($transition_id, array   $from_state_ids);
    

    Triple space after the array typehint.

  5. +++ b/core/modules/content_moderation/src/ContentModerationState.php
    @@ -0,0 +1,114 @@
    +class ContentModerationState implements StateInterface {
    

    Why not extend State?

  6. +++ b/core/modules/content_moderation/src/Entity/ContentModerationState.php
    @@ -55,10 +55,17 @@ public static function baseFieldDefinitions(EntityTypeInterface $entity_type) {
    -    $fields['moderation_state'] = BaseFieldDefinition::create('entity_reference')
    +    $fields['workflow'] = BaseFieldDefinition::create('entity_reference')
    ...
    +    $fields['moderation_state'] = BaseFieldDefinition::create('string')
    

    We should add an entity-level constraint that the moderation state is a valid workfow state of the referenced workflow.

  7. +++ b/core/modules/content_moderation/src/EntityOperations.php
    @@ -241,11 +255,13 @@ public function entityView(array &$build, EntityInterface $entity, EntityViewDis
    +   * @param \Drupal\Core\Workflow\WorkflowInterface $workflow
    +   *   The workflow being applied to the entity.
    ...
    -  protected function isDefaultRevisionPublished(EntityInterface $entity) {
    +  protected function isDefaultRevisionPublished(EntityInterface $entity, WorkflowInterface $workflow) {
    
    @@ -260,7 +276,7 @@ protected function isDefaultRevisionPublished(EntityInterface $entity) {
    +    return $default_revision && $workflow->getState($default_revision->moderation_state->value)->isPublishedState();
    

    Couldn't this use $this->moderationInfo->getWorkflowForEntity() instead of passing in the workflow?

  8. +++ b/core/modules/content_moderation/src/EntityTypeInfo.php
    @@ -291,9 +301,9 @@ public function entityBaseFieldInfo(EntityTypeInterface $entity_type) {
    -    $fields['moderation_state'] = BaseFieldDefinition::create('entity_reference')
    -      ->setLabel($this->t('Moderation state'))
    -      ->setDescription($this->t('The moderation state of this piece of content.'))
    +    $fields['moderation_state'] = BaseFieldDefinition::create('string')
    +      ->setLabel(t('Moderation state'))
    +      ->setDescription(t('The moderation state of this piece of content.'))
    ...
           ->setSetting('target_type', 'moderation_state')
    

    If this is now a string the target type setting is obsolete.

    Also should still be using $this->t().

    More importantly, why is this not and entity reference to a content moderation state? I guess this question would have been valid before, as well, so maybe a follow-up. I do think that would make a lot of other areas more self-documenting. I've highlighted some below.

  9. +++ b/core/modules/content_moderation/src/Form/BundleModerationConfigurationForm.php
    @@ -50,128 +49,59 @@ public function getBaseFormId() {
    +    $options = array_map(function (WorkflowInterface $workflow) {
    +      return $workflow->label();
    +    }, array_filter($workflows, function (WorkflowInterface $workflow) {
    +      return $workflow->status() && $workflow->getTypePlugin() instanceof ContentModeration;
         }));
    ...
    +    $selected_workflow = array_reduce($workflows, function ($carry, WorkflowInterface $workflow) use ($bundle_of_entity_type, $bundle) {
    +      $plugin = $workflow->getTypePlugin();
    +      if ($plugin instanceof ContentModeration && $plugin->appliesToEntityTypeAndBundle($bundle_of_entity_type->id(), $bundle->id())) {
    +        return $workflow->id();
    +      }
    +      return $carry;
    +    });
    

    I think this could use some code comments why the logic is different in the two cases.

  10. +++ b/core/modules/content_moderation/src/Form/BundleModerationConfigurationForm.php
    @@ -50,128 +49,59 @@ public function getBaseFormId() {
    +    if ($selected_workflow) {
    +      $form['enable_workflow_note'] = [
    ...
    +        '#access' => !empty($selected_workflow)
    

    Isn't this check duplicated? I think the #access check should suffice. Also casting to bool would be clearer than !empty(), but I guess that's arguable.

  11. +++ b/core/modules/content_moderation/src/ModerationInformation.php
    @@ -123,10 +130,22 @@ public function hasForwardRevision(ContentEntityInterface $entity) {
    +  public function getWorkFlowForEntity(ContentEntityInterface $entity) {
    +    $bundles = $this->bundleInfo->getBundleInfo($entity->getEntityTypeId());
    +    if (isset($bundles[$entity->bundle()]['workflow'])) {
    +      return $this->entityTypeManager->getStorage('workflow')->load($bundles[$entity->bundle()]['workflow']);
    +    };
    +    return NULL;
       }
    

    If - instead of the string state ID - the moderated entity actually referenced a content moderation state we could fetch the workflow from that. This would actually allow having different workflows for entities of the same bundle. That would be nice to support in the data model even if the core UI would not allow setting it up that way.

  12. +++ b/core/modules/content_moderation/src/Plugin/Field/FieldFormatter/ContentModerationStateFormatter.php
    @@ -0,0 +1,48 @@
    + *   field_types = {
    + *     "string",
    + *   }
    ...
    +          '#markup' => $workflow->getState($item->value)->label(),
    ...
    +  public static function isApplicable(FieldDefinitionInterface $field_definition) {
    +    return $field_definition->getName() === 'moderation_state';
    +  }
    

    The fact that we have to provide such a formatter is pretty clear evidence in my opinion that what we actually want is a reference to the content moderation state on the moderated entity instead of a string field with the state ID.

  13. +++ b/core/modules/workflow_ui/src/Form/WorkflowAddForm.php
    @@ -0,0 +1,107 @@
    +    // This form can only set the workflow's ID, label and the weights for each
    +    // state.
    

    This comment seems to be falsely copied from the edit form.

  14. +++ b/core/modules/workflow_ui/src/Form/WorkflowEditForm.php
    @@ -0,0 +1,194 @@
    +        'attributes' => ['aria-label' => $this->t('Edit \'@transition\' transition', ['@transition' => $transition->label()])],
    ...
    +        'attributes' => ['aria-label' => $this->t('Delete \'@transition\' transition', ['@transition' => $transition->label()])],
    

    Let's use double quotes for these strings.

  15. +++ b/core/modules/workflow_ui/src/Form/WorkflowStateAddForm.php
    @@ -0,0 +1,115 @@
    +        $configuration['states'][$values['id']] = $values['type_settings'][$entity->getTypePlugin()->getPluginId()];
    +        $entity->set('type_settings', $configuration);
    
    +++ b/core/modules/workflow_ui/src/Form/WorkflowStateEditForm.php
    @@ -0,0 +1,168 @@
    +      $configuration['states'][$values['id']] = $values['type_settings'][$entity->getTypePlugin()->getPluginId()];
    +      $entity->set('type_settings', $configuration);
    

    I guess this should sort $configuration['states'] to minimize config diffs.

  16. +++ b/core/modules/workflow_ui/src/Form/WorkflowStateAddForm.php
    @@ -0,0 +1,115 @@
    +  protected function actions(array $form, FormStateInterface $form_state) {
    +    $actions['submit'] = [
    +      '#type' => 'submit',
    +      '#value' => $this->t('Save'),
    +      '#submit' => ['::submitForm', '::save'],
    +    ];
    +    return $actions;
    

    wouldn't it be nicer to call the parent and unset $actions['delete']?

  17. +++ b/core/modules/workflow_ui/src/Form/WorkflowTransitionAddForm.php
    @@ -0,0 +1,151 @@
    +      $configuration['transitions'][$values['id']] = $values['type_settings'][$entity->getTypePlugin()->getPluginId()];
    +      $entity->set('type_settings', $configuration);
    
    +++ b/core/modules/workflow_ui/src/Form/WorkflowTransitionEditForm.php
    @@ -0,0 +1,171 @@
    +      $configuration['transitions'][$values['id']] = $values['type_settings'][$entity->getTypePlugin()->getPluginId()];
    +      $entity->set('type_settings', $configuration);
    

    Same as above: I guess we should sort this?

  18. +++ b/core/modules/workflow_ui/workflow_ui.permissions.yml
    index 0000000..f1cfabc
    --- /dev/null
    
    --- /dev/null
    +++ b/core/modules/workflow_ui/workflow_ui.routing.yml
    

    Would be nice to add (a) route provider(s), but we can also discuss that in a follow-up as I guess that (sadly) is somewhat contentious.

  19. +++ b/core/modules/workflow_ui/workflow_ui.routing.yml
    @@ -0,0 +1,47 @@
    \ No newline at end of file
    

    This should not be here.

alexpott’s picture

Thanks for the review @tstoeckler - good to know that this is very similar to what you came up with!

  1. Fixed and tested
  2. Yeah let's open a follow to discuss this - @dawehner also expressed something similar.
  3. Fixed
  4. Fixed
  5. Because this is decorating state ie wrapping it so extending is not necessary and doesn't actually work.
  6. Yes - let's do that in a followup - we didn't have that validation before.
  7. Yes but the whole method \Drupal\content_moderation\EntityOperations::isDefaultRevisionPublished() just exists to split up logic in \Drupal\content_moderation\EntityOperations::entityPresave() - imo there's no point in getting the workflow twice when we've just got it.
  8. Fixed t() and removed the obsolete setting (nice spot). Before this was a reference to state configuration entity they no longer exist so I just replaced it correctly. The problem with referencing the content entity is that we need entity reference revisions in core then :)
  9. Tidied this up - a bit less functional but easier to read.
  10. Removed if - nice spot.
  11. Let's handle multiple workflows in a followup - this is a tricky subject given the current implementation using the drop down save button
  12. Let's handle this in a followup because of the entity reference revisions implications
  13. And the comment is not particularly helpful either :)
  14. Fixed - just removed the single quotes in the string so it matches the state arias
  15. Well the sorting should occur in \Drupal\Core\Workflow\Entity\Workflow::addState() and \Drupal\Core\Workflow\Entity\Workflow::addTransition() - let's add this as a followup so we can concentrate on it. I think a simple sort on the machine name should suffice but sorting can sometimes get complex quickly.
  16. I dunno - I'm not a huge fan of doing unnecessary work.
  17. See answer to #15
  18. Yeah definitely followup material
  19. Fixed
tstoeckler’s picture

Yay, awesome. Thanks for the quick reply.

((($RTBC++)++)++);

Only answering those where anything is left to say.

7. Totally missed that method is protected. In that case it doesn't really matter (to me) and your explanation totally makes sense.

8.

The problem with referencing the content entity is that we need entity reference revisions in core then :)

LOL, yes, I completely missed that nice little detail. I do think this makes a lot of sense, though, so when I have a moment of feeling particularly masochistic I will open an issue for that.

Will try to find some time to open follow-ups for 2/6/8/15/18.

xjm’s picture

Status: Reviewed & tested by the community » Needs review

So, in #2296423-150: Implement layout plugin type in core and #2296423-158: Implement layout plugin type in core, I've pushed back really hard on why that code belonged in core/lib instead of a module.

The only reason I see in the summary for this to be in core/lib is to avoid a namespace collision with a contrib module. To me, that is not a compelling reason by itself; we can always come up with a different name. Can we update the summary with the reasoning for it to be in core/lib, along the lines of the questions I asked in the comments linked above?

xjm’s picture

Issue tags: +Needs change record
alexpott’s picture

Here are the questions and my thoughts of them:

  1. Does the code for this API belong in a module, or in \Drupal\Core?
  2. How do we denote that the code is experimental for developers and site owners?
  3. How do we avoid namespace collisions with contributed and custom projects?
  4. How do we minimize disruption for code, configuration, and data that may depend on the API?

My thoughts:

  1. At the moment this patch makes extensive changes to the content_moderation experimental module. Adds a workflow_ui module that is experimental and adds a core component that is marked as internal and provides an entity type and plugin system. I only moved this to core because it was what @amateescu and @catch asked for. But thinking forward, I think workflow should be part of all content entities in Drupal 9. I.e. a base field or part of an entity metadata system and not something separate that we add on. Hence the workflow subsystem should be in core because in future the content entity subsystem should depend on it. I hope that the basic draft/published part of content entities ships as a basic workflow in Drupal 9 so all entity state transitions are powered through the same system. This would mean that as a site grows and adds editorial practice such as a needs review state you won;'t need to rework all the content views etc.
  2. I have no idea what site owners have to do with this - I guess you mean the UI. In this instance the only way that a site owner gets to see the affects of the code is by enabling experimental modules. For developers all core code is marked as @internal and has a comment. In this case I think that this is a good way because this is internal code to the content_moderation and workflow_ui experimental modules. It is not meant yet for other modules.
  3. Currently the patch has no namespace collisions with any contributed projects - if it provided a workflow module it would have. If this patch is
  4. We minimize disruption by stating that the workflow entity is only to support the workflow_ui and content_moderation experimental modules

On splitting up the patch. We could add the core stuff, then the workflow_ui and then content_moderation but in my opinion doing that would be a mistake because only in doing them all together do we prove the system works and is fit for purpose. If it helps I can post separate patches for all three things as review patches. @catch did this once.

alexpott’s picture

Issue summary: View changes
alexpott’s picture

Issue summary: View changes
catch’s picture

Right there's a use-case for workflow around the published field (just the 2/3 core states of not-yet-published/published/unpublished) without content_moderation.

If we at some point integrate that with publishable content entities, that'd make the workflow module a dependency of node for example. Users could also have workflow for new/pending-registration/admin-approved/blocked etc. and it'd become a dependency of that too.

It's unfortunate that we need an update here, but that's a core bug/task (including configuration entities in the state entry unnecessarily) that we could have a separate bug fix for. The gotcha with that is taking them out of state in 8.2.x would be more disruptive than this update, but it'll help next time.

alexpott’s picture

Created #2831714: Remove configuration entities from entity schema repository to discuss the entity repository issue.

alexpott’s picture

Here's the patch split up into different parts to focus on for reviews. Core, worklfow_ui and content moderation. One thing to remember about the "size of this patch is the diff stat for the content moderation change 74 files changed, 1066 insertions(+), 2506 deletions(-) - so nearly 1500 lines being deleted :)

Reuploading #126 too to keep the last patch the one to commit.

Sam152’s picture

Started on a CR here: [#2833359]

Should this be RTBC based on the concerns in #128 being addressed in #130?

timmillwood’s picture

Status: Needs review » Reviewed & tested by the community
Issue tags: -Needs change record

Yes, I think this should be RTBC.

Almost all content_moderation.module issues are waiting on this and things like Trash that we would like to get into 8.3.x is waiting on this too.

Change record looks good!
https://www.drupal.org/node/2833359

Sam152’s picture

I agree, this blocks tonnes of work, so it would be great to rally some effort around it to get it over the line. I think the core/module decision is the only thing left blocking it.

To quote the linked issue:

I think things should be in modules if:

  • They are optional.
  • A site administrator might want to disable them.
  • They provide a specific user interface (this is kind of a sub-point of the previous point).

I think things should be in core/lib if:

  • If there are other things in core/lib that might need to depend on them. (Circular but true.)
  • They are required for all sites.
  • There is never a case for a site administrator to disable them.

Lets double check we're satisfying that criteria and then lets get this in. We're experimental after all. :)

xjm’s picture

Assigned: xjm » alexpott
Status: Reviewed & tested by the community » Needs review

@alexpott and I discussed this issue at length yesterday and the committers also discussed it as a group afterward.

  1. #130 still did not really make it clear to me why this needed to be in core/lib, but @alexpott made the case that workflows should be like languages in that they become a basic property of entities, for the same reasons. Even if you only use one and we fall back to that default case, the entity system should always be able to handle it or otherwise most contrib will not support it properly. There is also the notion that this might eventually replace or be related to entity status, which is a base part of the entity system. I'm not sure I'm 100% convinced of this, but those are both reasons for it to be in a core library rather than a module.
  2. Based on the shared concerns between this issue and #2296423: Implement layout plugin type in core, we discussed whether to allow experimental core libraries and how to do so effectively. @alexpott filed #2833347: Allow experimental modules to rely on \Drupal\Core\ namespaced classes that are only loadable when the module is enabled for that, and we are going to have a separate issue for how we document experimental APIs (still to be filed). If necessary, both this issue and the layout plugin issue can have followups for adopting the best practice that comes out of that discussion, but we could also use contributors' input and feedback there now.
  3. An important difference between this issue and #2296423: Implement layout plugin type in core is that the layout plugin only truly provides an API in a core library, and the service for the plugins is still in an experimental module (so the code is guaranteed to not be used unless a module specifically declares an experimental dependency). In this issue, by contrast, we are adding a new core service (which we have no way of marking experimental), a data storage in the form of a config entity with a schema (which there are very few of in core/lib), etc. This means there is no way to ensure it can be "turned off". @alexpott and I agreed to assign the issue to him to explore these points more and see if we can come up with a solution.

Thanks everyone!

yoroy’s picture

Question from UX team that came up while reviewing designs by jozef and comparing those with the current patch: https://youtu.be/TJsj7MDvYec?t=17m14s

In this patch, states are unique to a specific workflow. Is there still the option/desire to make states reusable across workflows? Maybe I could deduce that from the issue summary but it's a bit too technical for me to be sure :)

---

While testing the patch in simplytest: after assigning a workflow to content type Article and then going to the node form for article I got:

Fatal error: Call to a member function id() on a non-object in /home/r18kz/www/core/modules/content_moderation/src/Plugin/Field/ModerationStateFieldItemList.php on line 65

You can see that happen here: https://youtu.be/TJsj7MDvYec?t=38m42s

Gábor Hojtsy’s picture

Status: Needs review » Needs work

Needs works for the fatal in #140.

alexpott’s picture

This comment addresses the core/lib question.

So I explored trying to use the autoloader to magic in experimental code in core/lib BUT the problem is that entity type discovery wouldn’t find the experimental entity type. I don’t think there is a way we can proceed with the workflow entity in core. Any module that integrates with all entity types won’t be able to avoid integrating with experimental code. Given that a key promise of experimental code is that it shouldn’t affect live sites this means the entity type can not be in core/lib. Given that I propose that we move the entity type, I think we should also move the plugin manager and plugin type. These should move to a Workflows module and we should also move the the UI there too. This way we don’t clash with contrib. Also as this workflow entity and plugin type are completely new we don’t have the problems of layout plugin where we would break every 2 times if we don’t start with everything in core/lib. If once all this code is stable we choose to move it to core then we can do that with BC layers and hook_update_N().

I will open a follow up issue to discuss when, how and if the functionality is put into core/lib.

I've discussed this with @xjm and @catch and we've agreed that this is the best approach given the necessary restrictions placed upon experimental code.

alexpott’s picture

@yoroy / # #140 - there is no desire to share states across workflows. Thanks for the bug report!

alexpott’s picture

Status: Needs work » Needs review
FileSize
369.73 KB

Here's a start - renaming workflow_ui to workflows - still need to move everything out of core/lib

Status: Needs review » Needs work

The last submitted patch, 144: 2779647-2-144.patch, failed testing.

alexpott’s picture

Status: Needs work » Needs review
FileSize
370.5 KB

Moved all the code... now on to try and address the bug found in #140

Status: Needs review » Needs work

The last submitted patch, 146: 2779647-2-146.patch, failed testing.

alexpott’s picture

Status: Needs work » Needs review
FileSize
369.86 KB
653 bytes

The update function is no longer necessary.

Fabianx’s picture

I still think it would be a good idea to split the module in anticipation of a future move-to-core way:

- workflow_ui (will be kept as module, can be disabled)
- workflow_api (will move to core/lib/ one day, can/must be depended upon from contrib)

That is clean and nice and could be done regardless if the classes of workflow_api are registered in core/lib or not.

And as already said, its also possible to put them in core/lib already and extend the workflow_api classes for now with a clean exception message if that class does not exist on loading time.

But a split is a must IMHO.

Fabianx’s picture

Status: Needs review » Needs work
alexpott’s picture

@Fabianx If we move the workflow API to core then it is fine for a workflows module to just provide a UI. The views/views_ui is the only example of this in core. Actions for example just provides a UI not an API. Making users choose between _api and _ui seems unnecessary noise in the already very noisy module installation page.

alexpott’s picture

So the problem that #140 has found is that we can have workflows without states. This doesn't make sense. So I propose that we add workflow state form to the workflow add form so on creation you have an initial state. And in the API we thrown an exception on save() if there are no states.

The other way around this is to only allow workflows to return TRUE for status if there is at least one state. And only list enabled workflows.

alexpott’s picture

Status: Needs work » Needs review
FileSize
14.17 KB
373.56 KB

Here's a patch to address #140.

  • After creating a workflow you are taken to the add state form because it is the logical next step.
  • If you don't create a state and you navigate to the workflow edit form you are warned the workflow is disable because there are no states
  • You cannot delete the last state either via the API or UI
  • Content moderation does not list disabled workflows when attaching to a bundle.
  • There are tests for all of this.
Sam152’s picture

Status: Needs review » Reviewed & tested by the community

Only being enabled if a workflow has a state sounds very reasonable. I'm going to go out on a limb and say we probably don't want to attach a draft/published workflow to our workflow UI ;)

Interdiff LGTM.

alexpott’s picture

Assigned: alexpott » Unassigned

This got assigned to me in #139 - that's been answered by #142

yoroy’s picture

Status: Reviewed & tested by the community » Needs work
FileSize
117.19 KB

I can't find how to create my own workflow anymore because the button to create a new one seems to have disappeared :)

Sam152’s picture

Issue summary: View changes
Status: Needs work » Reviewed & tested by the community
FileSize
85.56 KB

That is working for me and also has test coverage, so it might be an issue with your environment?

yoroy’s picture

Maybe. I compared both patches in separate simplytest installs.

yoroy’s picture

Ok, tested #153 again in simplytest and the button is there.

jojototh’s picture

Here is a new prototype for workflow:
- keeps the same functionality and technical implementation
- user doesn't leave page before workflow and moderation configuration is done as it all happens on same page instead of 4 different pages - reduces the amount of clicks and pages needed
- simplifies the UX and reduces "up to 25%" of time needed for creating and administering workflows and moderation
- improves the Workflows listing by showing important information such as workflow type, which entity types it affects and states available

Below is a video comparing creating workflow, 1 new state and 1 new transition and configuring moderation for 1 content type in the current and proposed versions.
Video: https://youtu.be/xB-AbupfDxY

Interactive prototype: https://marvelapp.com/1124911

Sam152’s picture

In #160, I don't know if "Entity Types" is always going to be relevant for all workflow types. Probably something that would appear after "Content moderation" was selected. This sounds like it would be good to explore in a follow up, as it probably doesn't impact schema and this issue is blocking a lot of work.

alexpott’s picture

Issue summary: View changes

Edited the CR https://www.drupal.org/node/2833359 and updated the IS for the latest changes.

alexpott’s picture

Issue tags: +8.3.0 release notes
catch’s picture

Status: Reviewed & tested by the community » Fixed

OK. I've reviewed this several times over the lifetime of the issue (and opened it in the first place).

I think we've got as far as we can here. This is a massive change and one we'd normally have blocked initial commit of the module on without experimentalness to rely on.

Committed/pushed to 8.3.x, thanks!

  • catch committed 0f13905 on 8.3.x
    Issue #2779647 by alexpott, Sam152, catch, scookie, yoroy, pericxc,...
jibran’s picture

Published the CR.

tacituseu’s picture

xjm’s picture

Release manager signoff is implied by @catch's commit. :) (I myself did not review this.)

samuel.mortenson’s picture

Was it intentional to not have a Published -> Draft ("Unpublish") transition by default in the Editorial workflow? If yes - this means that if someone wants to create a new draft of a published Node, they have to first "Archive" it, then "Un-archive to Draft". This seems awkward, unless I'm missing something.

I have no clue how I missed that "Create New Draft" lets you switch from Published -> Draft, my mistake.

TR’s picture

Where is the documentation describing how all this is supposed to work?

alexpott’s picture

@TR this is an experimental module and as such is not such to the documentation gates - please open a follow up issue to document what you find lacking and then we all can address it.

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.

scookie’s picture

I have a question about this. In pre-8.3 Content Moderation, you can edit the machine name of the states because you can have duplicate state labels. In this version, I don't see the means by which you can edit the state machine names when you add a state. Will we be able to have duplicate state labels? This is important, because I need the ability to create a new draft of a published item, as well as un-publish a published item and put it back to a draft state. I don't want to archive it - archive has a very different business meaning from un-publishing something that was published erroneously. In order to accomplish that, I would have 2 different Draft states - one is the default revision, the other is not. But I want both to have a label of Draft.

cilefen’s picture

@scookie: please open new issues with those questions now that this is finished.

This reminds me to ask: do we need a workflows issue component or should stuff remain generally under content moderation?

Gábor Hojtsy’s picture

@cilefen: well this component/module was added in part to support use cases other than content moderation, so IMHO we should have a separate component yes.

tstoeckler’s picture

So I waaay missed the two-week mark before this was marked closed, so I guess not many people will see this, but I finally got around to opening the follow-ups that came up in my review in #125. Also found some other issues when looking into this again. On the other hand I couldn't get myself to open an issue for the route provider stuff, that's currently stalled in various different issues, so is a couple miles off either way.

Here's the list of issues:

nlisgo’s picture

I'm not sure if this was expected but when trying to upgrade to 8.3.0 from 8.2.7 I had issues when running "drush updatedb" because there was no update hook to enable the workflow module. Was this expected to be disruptive because content_moderation was not stable in 8.2.7?

The only way I got around it was to uninstall content_moderation before upgrading to 8.3.0 and then installing it again and reconfiguring the moderation settings.

nlisgo’s picture

If it is possible to avoid the trouble I had by introducing an update hook we should do it.

Sam152’s picture

@nlisgo, this is documented in the change record: https://www.drupal.org/node/2833359. The module is experimental so disruptions are expected.

There is an unofficial update hook being worked on here #2846618: Unofficial content_moderation 8.2.x to 8.3.0 upgrade path.

nlisgo’s picture

@Sam152, thanks for the updated. Good to know. I should have read the change record more carefully but as mentioned it wasn't too disruptive in my case.

Sam152’s picture

I don't think we ever got around to creating workflows as its own component. I think it makes sense to give it it's own queue before it hits stable. Once it's stable, changes in content_moderation shouldn't really need to affect workflows and vice versa.

yoroy’s picture

Probably best as a new issue :)