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 #117Decide how much of the UI work to include here - created follow up #2830584: Use modals for creating, updating, and deleting workflows, with a new DialogFormTraitShould we continue to use the word "State" or use the more generic and common in workflow theory "Place"?Chosestate
since the arguments in #95 are compellingShould 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:
- #2830584: Use modals for creating, updating, and deleting workflows, with a new DialogFormTrait
- #2830735: Add possible but not existent transitions to the state edit form
- #2830736: Add indication of where workflow is being used to workflow edit form
- #2830737: Add link to workflow on bundle edit screen
- #2830739: Discuss whether content moderations actions should be on the state or transition level
- #2830740: Allow workflow types to lock certain changes to workflows once things are in use
- #2830741: Decide if the workflow type should be able to rename the labels "State/Place" and "Transition" so it can call them what they like
- #2830581: Fix ContentModeration workflow type to calculate correct dependencies
- #2835545: Provide a Workflow FieldType that references a workflow entity and tracks the current state of an entity
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.
Comment | File | Size | Author |
---|---|---|---|
#157 | Screen Shot 2016-12-09 at 10.32.44 AM.png | 85.56 KB | Sam152 |
#156 | create-workflow.png | 117.19 KB | yoroy |
#153 | 2779647-2-153.patch | 373.56 KB | alexpott |
#153 | 148-153-interdiff.txt | 14.17 KB | alexpott |
#148 | 146-148-interdiff.txt | 653 bytes | alexpott |
Comments
Comment #2
timmillwoodThe 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.
Comment #3
alexpott@timmillwood as @catch wrote in IRC:
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...
I think that this is precisely why this issue is a good idea.
Comment #4
alexpottWhilst 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.
Comment #5
alexpottAlso 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?
Comment #6
catchI 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.
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.
Comment #7
timmillwoodI 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.
Comment #8
scookie CreditAttribution: scookie at Workday, Inc. commentedIn 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?
Comment #9
timmillwood#2799785: Entity types with non-config bundles can not be moderated has been postponed in favor of this.
Comment #10
alexpottI'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.
Comment #12
alexpottNow with a working UI - which because you are configuring an entire workflow at once is way easier to grok.
Comment #14
alexpottMove 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.
Comment #15
timmillwoodWhoa! That's a pretty big patch, and a great start.
Thinking about:
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?
Comment #17
alexpott@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
andpublished
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).
Comment #18
alexpottOh and 'typical' was a five second choice knowing that changing it was certain and I had no better ideas at the time :)
Comment #19
timmillwoodIn 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.
Comment #20
alexpott@timmillwood well yeah - that should be part of the attaching a workflow to a bundle. If there are existing nodes.
Comment #21
alexpottMore 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 :)
Comment #22
Sam152 CreditAttribution: Sam152 as a volunteer and at PreviousNext commentedFor the benefits of watchers, #21 gives you:
Comment #23
alexpottFixing the UI a bit so new workflows can be created.
Comment #26
Sam152 CreditAttribution: Sam152 as a volunteer and at PreviousNext commentedI 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.
Comment #27
pericxc CreditAttribution: pericxc commentedTwo suggestions:
Comment #28
timmillwoodI'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.
Comment #29
alexpott@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.
Comment #30
Sam152 CreditAttribution: Sam152 as a volunteer and at PreviousNext commentedI 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?
Comment #31
jibranIt's always FALSE for AddForm.
It's always TRUE for EditForm.
Comment #33
timmillwoodThere's already a Workflow module, with a D8 release https://www.drupal.org/project/workflow
Comment #34
jhedstromThis 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).
Comment #35
scookie CreditAttribution: scookie at Workday, Inc. commentedWonderful!! 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:
Comment #36
alexpottPatch 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.
Comment #38
tacituseu CreditAttribution: tacituseu commentedDecoupling 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)
Comment #39
scookie CreditAttribution: scookie at Workday, Inc. commented@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.
Comment #40
scookie CreditAttribution: scookie at Workday, Inc. commentedWe 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
Comment #41
tacituseu CreditAttribution: tacituseu commentedJust 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.
Comment #42
alexpott@scookie the comment
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.
Comment #43
alexpottMove work
Comment #45
Sam152 CreditAttribution: Sam152 as a volunteer and at PreviousNext commentedMissing @group annotation. Keen to see what the bot says.
Comment #47
Sam152 CreditAttribution: Sam152 as a volunteer and at PreviousNext commentedI'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.
Comment #48
bojanz CreditAttribution: bojanz at Centarro commentedThis 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.
Comment #50
alexpott@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:
Comment #51
alexpottMore 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.
Comment #52
jhedstromI spoke with @alexpott at Badcamp, and wanted to post a few notes:
Comment #53
scookie CreditAttribution: scookie at Workday, Inc. commented@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!
Comment #55
alexpott@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.
Comment #56
scookie CreditAttribution: scookie at Workday, Inc. commented@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.
Comment #58
timmillwoodDon'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"?
Comment #59
jhedstromI 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.
Comment #60
alexpott@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.
Comment #61
alexpottThe 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.
Comment #63
Sam152 CreditAttribution: Sam152 as a volunteer and at PreviousNext commented@alexpott, what are you looking for to move this forward at this stage? Detailed code review? Manual testing?
Comment #64
pericxc CreditAttribution: pericxc commentedI 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.
Comment #65
scookie CreditAttribution: scookie at Workday, Inc. commentedHI @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.
Comment #66
alexpott@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.
Comment #67
alexpottMore work on the UI and form classes for workflow states and transitions.
Comment #69
alexpottThis 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:
Comment #71
alexpottBoo uasort!
Comment #72
alexpottAdding tests of how a workflow type can decorate transitions and states with additional information and display forms to collect this information in the UI.
Comment #74
alexpottComment #75
alexpottFixing test
Comment #76
alexpottComment #77
alexpottComment #79
alexpottThe fails in #75 are unrelated. Patch attached does a bit more cleaning up and adding missing explicit test coverage of additions to content moderation.
Comment #80
alexpottStandardising on the short array syntax in the new workflow module and ensuring we have complete test coverage of the State and Workflow objects.
Comment #81
timmillwoodCurrently it's possible to delete a workflow that's in use.
If you do this then try to edit a moderated node you get:
If you do this then try to add a new moderated node you get:
We could either prevent workflows from being deleted, or update content moderation to ignore a defined workflow if it doesn't exist.
Comment #82
alexpott@timmillwood currently is it possible to delete transitions and states in Content moderation - imo that's for a followup. See section on followups.
Comment #83
alexpottAdded a
Workflow::loadMultipleByType()
to make life easy. It's now used by content_moderation.Comment #84
timmillwoodJust 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.
Comment #85
catchIt 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:
Should this be isPublishedState() or something? The workflow itself does not get published. Same with isDefaultRevision().
Comment #86
timmillwoodI'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.
Comment #87
catchHere's the split patches.
Comment #88
catchDid 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:
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.
Comment #89
amateescu CreditAttribution: amateescu for Pfizer, Inc. commentedBig +1. That's what we discussed in Dublin and I was a bit surprised to see it as a module in the initial patch.
Comment #90
Fabianx CreditAttribution: Fabianx as a volunteer commented+1 to workflow being part of core instead of being a module
Comment #91
tacituseu CreditAttribution: tacituseu commentedThere 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 ?
Comment #92
alexpottThanks 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.
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.
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 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.
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.
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.
Comment #93
Fabianx CreditAttribution: Fabianx as a volunteer commented+1 to moving the parts into an experimental workflow_ui module and moving the rest centralized into core
Comment #94
Sam152 CreditAttribution: Sam152 as a volunteer and at PreviousNext commentedThank 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?
Comment #95
bojanz CreditAttribution: bojanz at Centarro commentedGreat progress since my review in #48.
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)
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:
Not the moderation state ID.
the machine name for the state.
Missing first line of docs.
Wrong class name in constructor docblock.
Comment #96
alexpottRerolled on top of #2824912: The moderation_state field incorrectly reports being read-only and #2787881: Moderating a non-translatable entity type throws exception. Going to begin work on the recent reviews now...
Comment #97
alexpottMoving 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.
Comment #99
alexpottSo in order to do this we need to do an update :(
This moving to core is becoming a bad idea for me.
Comment #101
timmillwoodWhy is the update such a bad thing? because we're enforcing the installation of an entity marked @internal?
Comment #102
alexpottJust 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.
Comment #103
timmillwoodOne 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.
Comment #104
catchSo 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:
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.
Comment #105
tacituseu CreditAttribution: tacituseu commentedJust 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:
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).
Comment #106
catchSo 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.
Comment #107
alexpottAfter 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.
Comment #108
alexpottDiscussed two more issues with @bojanz:
Patch attached improves test coverage.
Comment #109
alexpottComment #110
alexpottPatch attached prevent transitions from duplicating a transition made possible by another already existing transition.
Comment #111
alexpottAnd here's transition weights plus a way of setting them in the UI.
Comment #115
tacituseu CreditAttribution: tacituseu commentedJust a nitpick:
'#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.
Comment #116
Sam152 CreditAttribution: Sam152 as a volunteer and at PreviousNext commentedThis 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.
Can be made DRY with perhaps a validateLowercaseMachineName method?
Perhaps some protected method to get the max weight out of a certain property? Could be used in both places.
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?
This is injected, no need for \Drupal
This might need a comment, don't understand this. If there are multiple workflow types the access result becomes neutral?
Maybe we should have directional constants?
Seems like a good as time as any to bikeshed this. "Basic Editorial Workflow", "Editorial Workflow"?
Reading this makes the approach to the decoupling aspect of the patch make a lot of sense to me.
Do we need an interface that extends StateInterface to house these?
Should probably be type => value
Should probably be type => value
Should probably be type => value
Need a follow up issue link.
Docblock incorrect
I assume this needs a @todo to make it only format the moderation string fields.
Does this class have an isApplicable as well or need one?
@todo for easy grep? Also lets add a follow up.
Maybe this is implied knowledge if you get this far?
heh, good thinking
Lets create an issue.
type => value?
Comment #117
alexpottI (or someone else) needs to create all the other followups listed in the issue summary.
Comment #118
alexpottComment #119
Sam152 CreditAttribution: Sam152 as a volunteer and at PreviousNext commentedComment #120
Sam152 CreditAttribution: Sam152 as a volunteer and at PreviousNext commentedFollow up issues created.
Comment #121
timmillwoodEvery 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.
Comment #123
alexpottThere was a conflict in core/modules/system/system.post_update.php because a recent issue added one.
Comment #125
tstoecklerRead 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.
So everywhere else you are very strict about validating input, so maybe we can validate $direction as well?
This one is interesting. I can see why we do this, but I can also see two reasons against it:
Can also be discussed in a follow-up but I thought would be worthwhile to bring up.
Super minor, but we should be using
$this->id()
or$this->id
consistently.Triple space after the array typehint.
Why not extend State?
We should add an entity-level constraint that the moderation state is a valid workfow state of the referenced workflow.
Couldn't this use $this->moderationInfo->getWorkflowForEntity() instead of passing in the workflow?
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.
I think this could use some code comments why the logic is different in the two cases.
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.
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.
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.
This comment seems to be falsely copied from the edit form.
Let's use double quotes for these strings.
I guess this should sort $configuration['states'] to minimize config diffs.
wouldn't it be nicer to call the parent and unset $actions['delete']?
Same as above: I guess we should sort this?
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.
This should not be here.
Comment #126
alexpottThanks for the review @tstoeckler - good to know that this is very similar to what you came up with!
Comment #127
tstoecklerYay, 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.
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.
Comment #128
xjmSo, 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 incore/lib
, along the lines of the questions I asked in the comments linked above?Comment #129
xjmComment #130
alexpottHere are the questions and my thoughts of them:
My thoughts:
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.
Comment #131
alexpottComment #132
alexpottComment #133
catchRight 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.
Comment #134
alexpottCreated #2831714: Remove configuration entities from entity schema repository to discuss the entity repository issue.
Comment #135
alexpottHere'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.
Comment #136
Sam152 CreditAttribution: Sam152 as a volunteer and at PreviousNext commentedStarted on a CR here: [#2833359]
Should this be RTBC based on the concerns in #128 being addressed in #130?
Comment #137
timmillwoodYes, 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
Comment #138
Sam152 CreditAttribution: Sam152 as a volunteer and at PreviousNext commentedI 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:
Lets double check we're satisfying that criteria and then lets get this in. We're experimental after all. :)
Comment #139
xjm@alexpott and I discussed this issue at length yesterday and the committers also discussed it as a group afterward.
Thanks everyone!
Comment #140
yoroy CreditAttribution: yoroy at Roy Scholten commentedQuestion 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
Comment #141
Gábor HojtsyNeeds works for the fatal in #140.
Comment #142
alexpottThis 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.
Comment #143
alexpott@yoroy / # #140 - there is no desire to share states across workflows. Thanks for the bug report!
Comment #144
alexpottHere's a start - renaming workflow_ui to workflows - still need to move everything out of core/lib
Comment #146
alexpottMoved all the code... now on to try and address the bug found in #140
Comment #148
alexpottThe update function is no longer necessary.
Comment #149
Fabianx CreditAttribution: Fabianx as a volunteer commentedI 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.
Comment #150
Fabianx CreditAttribution: Fabianx as a volunteer commentedComment #151
alexpott@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.
Comment #152
alexpottSo 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.
Comment #153
alexpottHere's a patch to address #140.
Comment #154
Sam152 CreditAttribution: Sam152 as a volunteer and at PreviousNext commentedOnly 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.
Comment #155
alexpottThis got assigned to me in #139 - that's been answered by #142
Comment #156
yoroy CreditAttribution: yoroy at Roy Scholten commentedI can't find how to create my own workflow anymore because the button to create a new one seems to have disappeared :)
Comment #157
Sam152 CreditAttribution: Sam152 as a volunteer and at PreviousNext commentedThat is working for me and also has test coverage, so it might be an issue with your environment?
Comment #158
yoroy CreditAttribution: yoroy at Roy Scholten commentedMaybe. I compared both patches in separate simplytest installs.
Comment #159
yoroy CreditAttribution: yoroy at Roy Scholten commentedOk, tested #153 again in simplytest and the button is there.
Comment #160
jojototh CreditAttribution: jojototh at Pfizer, Inc. commentedHere 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
Comment #161
Sam152 CreditAttribution: Sam152 as a volunteer and at PreviousNext commentedIn #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.
Comment #162
alexpottEdited the CR https://www.drupal.org/node/2833359 and updated the IS for the latest changes.
Comment #163
alexpottComment #164
catchOK. 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!
Comment #166
jibranPublished the CR.
Comment #167
tacituseu CreditAttribution: tacituseu commentedAdding missing follow-up from #48.2, #50.2 #2835545: Provide a Workflow FieldType that references a workflow entity and tracks the current state of an entity
Comment #168
xjmRelease manager signoff is implied by @catch's commit. :) (I myself did not review this.)
Comment #169
samuel.mortensonWas 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.
Comment #170
TR CreditAttribution: TR commentedWhere is the documentation describing how all this is supposed to work?
Comment #171
alexpott@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.
Comment #173
scookie CreditAttribution: scookie at Workday, Inc. commentedI 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.
Comment #174
cilefen CreditAttribution: cilefen as a volunteer commented@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?
Comment #175
Gábor Hojtsy@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.
Comment #176
tstoecklerSo 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:
Comment #177
nlisgo CreditAttribution: nlisgo commentedI'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.
Comment #178
nlisgo CreditAttribution: nlisgo commentedIf it is possible to avoid the trouble I had by introducing an update hook we should do it.
Comment #179
Sam152 CreditAttribution: Sam152 as a volunteer and at PreviousNext commented@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.
Comment #180
nlisgo CreditAttribution: nlisgo commented@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.
Comment #181
Sam152 CreditAttribution: Sam152 as a volunteer and at PreviousNext commentedI 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.
Comment #182
yoroy CreditAttribution: yoroy at Roy Scholten commentedProbably best as a new issue :)