Architectural review of the Workflows module (documentation cleanups)

I think a "constant motivation" module in core would be awesome, but I assume we're looking at "Content Moderation" here.

Full review of the Workflows module below (Content Moderation to follow, but we'd first need Workflows to be stable, so reviewed that first). Note that I've already addressed all nitpicky parts of my feedback. Parts that need to still be addressed by Workflows maintainers are marked with "👻"! IOW: chase the ghost, then make sure it's gone! #lamehumor

Overall, I think it looks like it's in a good place. I have only two areas of concern:

1. the access checking lacks test coverage, seems brittle, and has some very atypical design choices (see below)
2. overall architecture documentation, which would explain some of the design choices that at first seem a bit strange (see below)

I think it's very doable to address these concerns in a span of days, not weeks or months. :) I think especially the first would be a reason for not marking it stable just yet. The second I think doesn't need need to be a blocker per se, but has the risk of not happening once it's marked stable.

workflows.schema.yml

1.     type_settings:
         type: workflow.type_settings.[%parent.type]
         label: 'Custom settings for workflow type'
   

   Nit: this label is useless, because it'll inherit the label from the resolved type's config schema. See for example how editor.schema.yml does something similar, and it inherits the label from ckeditor.schema.yml. Verify this with config_inspector.
   Fixed.

2. 👻
   

   workflows.state:
     type: mapping
     mapping:
   …
       weight:
         type: integer
         label: 'Weight'
   
   workflows.transition:
     type: mapping
     mapping:
   …
       weight:
         type: integer
         label: 'Weight'
   

   These mappings both have a weight, but neither specify orderby to ensure consistent config export order. See #2361539: Config export key order is not predictable for sequences, add orderby property to config schema plus CR.

\Drupal\workflows\Annotation\WorkflowType

Annotation docs look perfect! In line with all other standardized/elaborate annotation class docs. Except there's one small bug and one small omission. Plus some properties docs nits. Fixed in attached patch.

\Drupal\workflows\Entity\Workflow

1. Hypernit: missint trailing commas in some parts of the annotation. Fixed.
2. Some docs nits: fixed.
3. ::preSave() validates basic assumptions and throws a custom exception: 👍

\Drupal\workflows\WorkflowTypeInterface

👻 It's weird that this lives at core/modules/workflows/src/, yet the base class lives at core/modules/workflows/src/Plugin/. Why not have them live in the same directory? (Same for WorkflowTypeManager.)

1. 👻::checkWorkflowAccess() makes me suspicious — why do we need additional access checking methods? We'll need docs for that. (But at least it is returning an AccessResultInterface object, so at least it's not losing cacheability metadata, good!)
2. 👻 ::getInitialState(WorkflowInterface $workflow) looks a bit suspicious: why do we even need a method for this? I figured the reason is:
   

      * This means that the same workflow type can offer workflow-specific initial
      * states.
   

   … but then I noticed that \Drupal\content_moderation\Plugin\WorkflowType\ContentModeration::getInitialState(WorkflowInterface $workflow, $entity = NULL) — note that second parameter there! It's made optional, but really, it's kind of required for it to work correctly. Why not make it part of the interface then? That makes a ton more sense than the explanation I put forth.

   Also: why a method? This could easily have been stored in a Workflow config entity: different initial states for new an existing entities. Also, if it's an existing entity, won't it already have a state assigned? Then what is the meaning of an "initial" state?

3.    * @return array[]
      *   The required states.
   …
      */
     public function getRequiredStates();
   

   Nit: That doesn't seem right. Should be string[] AFAICT. Fixed.

4.    * @return \Drupal\workflows\WorkflowTypeInterface
      *   The workflow type plugin.
   …
      */
     public function addState($state_id, $label);
   

   Nit: Should be @return $this. Fixed.

5. 👻 The many transition & state methods: hasTransition(), getTransitions(), getTransitionFromStateToState(), setTransitionLabel(), setTransitionWeight(), and so on and so on … this is a code smell. It seems that we should have separate "state" and "transition" objects. However … I think this was consciously designed this way. It'd be great to document this architectural rationale for future maintainers as well as for future @WorkflowType plugin implementors.
   Precedent for this exists in the class docblock of \Drupal\big_pipe\Render\BigPipe — where the overall architecture is explained.
   In this case, you'd want the WorkflowType class doc to explain the architecture of WorkflowType plugins, its relation to Workflow entities, and to State(Interface) and Transition(Interface).

\Drupal\workflows\WorkflowInterface

   * @return \Drupal\workflows\WorkflowTypeInterface|\Drupal\workflows\WorkflowTypeFormInterface
   *   The workflow type plugin.

is no longer accurate since #2896722: Leverage PluginWithFormsInterface to encapsulate @WorkflowType schema and clean up state/transition form methods.. Fixed.

\Drupal\workflows\WorkflowListBuilder

1. 👻 Nit: \Drupal\workflows\WorkflowListBuilder::render() has a string explaining you must install a module to add workflow types,
   and Content Moderation is the prime example. The only thing missing here, is to make that a clickable link that points to /admin/modules#content-moderation to make it easier to install that module.
2. Kudos for this class, it's surprisingly elegant! 👏

\Drupal\workflows\WorkflowAccessControlHandler

1. Access controller for the Moderation State entity. is outdated. Fixed.
2. 👻 if (strpos($operation, 'delete-state') === 0) { made me 😲 — a quick grep later it became more clear. This definitely needs docs.
3. 👻Apparently this can call our previous friend WorkflowTypeInterface::checkWorkflowAccess(). The default implementation (\Drupal\workflows\Plugin\WorkflowTypeBase::checkWorkflowAccess()) returns AccessResult::neutral(). Apparently it's for "non-admin access", and whatever is in WorkflowAccessControlHandler's logic is for "admin access"? This is very very different from all other entity types and definitely merits detailed docs. I again recommend a class docblock to explain the architecture,
   just like I said above, but this time only covering workflows' access control.
4. 👻 Frighteningly, there seems to be zero test coverage for WorkflowAccessControlHandlerTest! This is an absolute must-have. (Yes, many things in core don't have it yet. But that has led to security vulnerabilities, and slower iteration later on. I promise you you this is worth it, even if only for the peace of mind!)
5. 👻 checkCreateAccess() does something interesting: it uses the config:core.extension cache tag. But if that's the cache tag we use here, then we should also specify it in \Drupal\workflows\WorkflowTypeManager::__construct()'s call to setCacheBackend(). And we probably want test coverage for it, to ensure it behaves as expected.

workflows.module

Language nits: fixed.

workflows.info.yml

Language nits: fixed.

State(Interface) + Transition(Interface)

1. These are pure value objects — great! 🎉
2. 👻 However, AFAICT \Drupal\workflows\State::labelCallback() can be removed?

WorkflowDeleteAccessCheck

👻 AFAICT this is dead code? Can we just remove it? Does that mean we can also simplify the "strpos delete-state" thing in WorkflowAccessControlHandler?

Thanks for this review @Wim Leers, we'll get these addressed ASAP because we need Workflows and Content Moderation stable in 8.4.0.

Not sure I'll be able to get to them until Monday, so if anyone wants to take a look at some or all, please do.

Thanks for the review. Here are a few access related issues I've already been thinking about:

#2897148: Remove @internal from workflowHasData/workflowStateHasData and use those methods for access control and configuration validation.
#2896726: Expand the entity access model for workflow states and transitions.

I think the issues marked as needing attention will be a mixture of clarifications/better docs and genuine issues to work through. How should we go about addressing them, an issue for each one? Perhaps we should triage them ahead of time and put all the documentation fixes in a single issue.

workflows.schema.yml.2

We don't sort the states by weight during storage, looks like it's sorted by ID. This means the order of the saved configuration is based on the ID, alphabetically. Perhaps this is inconsistent with how core deals with this broadly?

WorkflowTypeInterface.0

Happy to move these.

WorkflowTypeInterface.1

My understand is, this allows plugins to play a role in the access of the workflow and associated states. It's an API for the implementer more than the caller.

WorkflowTypeInterface.2

Since the restructure, $workflow shouldn't be there now that plugins control the state schema (opened #2900046: $workflow param is no longer required in getInitialState for that). Content moderation passes $entity because it uses the publish status of the content to see what state should be first. This is unique to that plugin type and is too specific and presumptuous for the interface imo.

Re: why a method? The default implementation could easily add this to the plugin configuration and return that, it just returns the first one instead. Content moderation would do neither, so our main reference implementation doesn't require it.

WorkflowTypeInterface.5

Sounds like it could do with some focused discussion.

WorkflowListBuilder.1

Opened #2900047: Add a link to install content_moderation when no workflow types are present.

WorkflowAccessControlHandler

Maybe we can cover all of these broadly in #2896726: Expand the entity access model for workflow states and transitions..

State(Interface) + Transition(Interface).2

This looks like it's pure candy so we can do: array_map([State::class, 'labelCallback'], $states).

WorkflowDeleteAccessCheck

This is currently being used to control access to the state delete form, @see entity.workflow.delete_state_form.

Regarding the patch in #3:

+++ b/core/modules/workflows/src/WorkflowTypeInterface.php
@@ -80,6 +80,9 @@ public function workflowStateHasData(WorkflowInterface $workflow, StateInterface
+   * This means that the same workflow type can offer workflow-specific initial
+   * states.

I don't think this makes sense really, the $workflow doesn't offer much context anymore, so this comment should be removed. Beyond that, should we RTBC this issue to get those nits in?

workflows.schema.yml.2 - The orderby should go in content_moderation.schema.yml as that's where the states and transitions sequence is stored.

WorkflowTypeInterface.1 - I've created a follow up #2900320: Remove workflow type checkWorkflowAcess() & "view content moderation" permission.
WorkflowTypeInterface.2 - +1 to @Sam152, the follow up looks great, the explanation is perfect.
WorkflowTypeInterface.5 - Again, I agree with @Sam152. I'm not sure we can, or should change how this works for 8.4, but would be a good discussion to have.

+++ b/core/modules/workflows/src/WorkflowTypeInterface.php
@@ -80,6 +80,9 @@ public function workflowStateHasData(WorkflowInterface $workflow, StateInterface
+   * This means that the same workflow type can offer workflow-specific initial
+   * states.

Removing this as @Sam152 suggests, and RTBCing, we can resolve in #2900046: $workflow param is no longer required in getInitialState.

Adding a list of follow ups to the issue summary.

Either we need to change the scope and therefore update the title to not include Content Moderation, and create a follow-up for it. Or we can't yet RTBC this.

@tim.plunkett was going to review the Content Moderation module's architecture.

Opened #2900421: Architectural review of the Content Moderation module

1. If we're going to move the access-related stuff to #2896726: Expand the entity access model for workflow states and transitions., then please move relevant information from this issue to that issue. I think that may also need to become WI Critical?
2. Similarly, #2897148: Remove @internal from workflowHasData/workflowStateHasData and use those methods for access control and configuration validation. is making API changes, which means this may also need to become WI Critical?
3. Same for #2900046: $workflow param is no longer required in getInitialState (which #6 created in response to my review).
4. Same for #2900320: Remove workflow type checkWorkflowAcess() & "view content moderation" permission (which #7 created in response to my review).
5. #2900047: Add a link to install content_moderation when no workflow types are present is a simple fix, no actual architectural problem :)
6. #6+#7 RE: workflows.schema.yml.2 — I don't see why the orderby belongs in content_moderation.schema.yml, because weight lives in workflows.schema.yml?
7. #6 + #7 RE: WorkflowTypeInterface.5 needing focused discussion — well this is meant to be an architectural review issue, why not do it here? Otherwise, can we have an issue?

NW solely for the feedback/concerns in this comment, not for #9. I don't think this can be RTBC until all follow-up issues except #2900047 (which is trivial) are properly triaged, so we are certain that the architectural concerns that were raised are actually going to be addressed, and are able to block a stable release if necessary.

#10: thanks!

Going to try organise the follow-ups in to those which are stable blocking vs not, just so we can organise what needs to be focused on over the next week.

@Wim Leers - I've been looking at orderby and can't seem to get it working. It seemed to make sense to add orderby as part of #2886567: Adding a workflow state or transition with an integer ID results in unrecoverable fatal errors and remove the custom sorting we're doing. I added orderby to content_moderation.schema.yml, because that's where the sequence is added, in workflow.schema.yml we just add the schema for state and transition items. We want to order by the weight field so I added orderby: weight although looking at the patch in #2361539: Config export key order is not predictable for sequences, add orderby property to config schema it only adds ordering by key or value in \Drupal\Core\Config\StorableConfigBase::castValue. Digging a little deeper I found that \Drupal\Core\Config\StorableConfigBase::castValue isn't even called for Workflows.

So either orderby is broken, or just doesn't work in this use case.

So either orderby is broken, or just doesn't work in this use case.

Config system maintainer @alexpott wrote this at #2361539-78: Config export key order is not predictable for sequences, add orderby property to config schema :

@Wim Leers hmmm this is a good question. Actually in this instance the best practice is to have a weight key and sort by something that does not change. This means that if things are re-ordered when you review the config change the only thing you have to review is the weight key changes. However maybe we need to add "in the order added" type of "sort" so at least the current behaviour of cdn could be documented and we could document that this is bad practice and recommend ways of fixing it.

Soo… apparently you're not meant to sort by weight, you're meant to sort by key in this case, so that you see something like this in your diff:

    bar:
      something: 'ljlk43j5l3k45j'
-     weight: 4
+     weight: 3
    foo:
      something: 'asdfasdf'
-     weight: 3
+     weight: 4

(Note how it's sorted by key, so by bar and foo, and then we see diff lines just for the changing weight.) So what I've been insinuating is wrong, but adding the orderby: key line is right — AFAICT.

#2897148: Remove @internal from workflowHasData/workflowStateHasData and use those methods for access control and configuration validation. can only not be stable-blocking (as it currently is listed in the IS), if #2900634: Mark WorkflowDeleteAccessCheck @internal until a UI pattern for access-restricted states has been established lands, because that allows it to happen later.

Added follow up for orderby: key: #2900700: Add orderby to content_moderation schema sequences

Thanks Tim!

I just RTBC'd #2900700: Add orderby to content_moderation schema sequences. #2900634: Mark WorkflowDeleteAccessCheck @internal until a UI pattern for access-restricted states has been established already was RTBC yesterday. I RTBC'd #2900320: Remove workflow type checkWorkflowAcess() & "view content moderation" permission earlier today. #2898913: Review a11y concerns in ContentModerationConfigureForm was RTBC'd earlier today by the accessibility maintainer.

That means all stable blockers are RTBC except #2900046: $workflow param is no longer required in getInitialState, which is also moving forward quickly. Hurray!

Once the 5 stable blockers mentioned in #20 are in, I'd like to re-review this, i.e. confirm that the points raised in #3 are addressed. But I'm certain we'll be very close :)

Moving #2898913: Review a11y concerns in ContentModerationConfigureForm down.

#22: Good call.

#2900046: $workflow param is no longer required in getInitialState landed!

All "WI Critical" issues are now committed or RTBC, so I think we can make this one RTBC too.

My review, only mentioning new items - all minor.

All in all, workflows feels modern and clean, especially in reference to other modules that have been around for more than 10 years. Test coverage is thorough.

1. \Drupal\workflows\Plugin\WorkflowTypeBase::getTransitionsForState if 'from' and 'to' are the only valid directions, should we add an assert() to verify. I think in a previous review I felt that 'from' and 'to' should be constants too.
2. \Drupal\workflows\WorkflowInterface::getTypePlugin has a return type referencing Drupal\workflows\WorkflowTypeFormInterface which I think we've since removed.
3. In \Drupal\workflows\Form\WorkflowAddForm::form
   

    $workflow_types = array_map(function ($plugin_definition) {
         return $plugin_definition['label'];
       }, $this->workflowTypePluginManager->getDefinitions());

   could just use array_column

4.  *     "form" = {
    *       "add" = "Drupal\workflows\Form\WorkflowAddForm",
    *       "edit" = "Drupal\workflows\Form\WorkflowEditForm",
    *       "delete" = "Drupal\workflows\Form\WorkflowDeleteForm",
    *       "add-state" = "Drupal\workflows\Form\WorkflowStateAddForm",
    *       "edit-state" = "Drupal\workflows\Form\WorkflowStateEditForm",
    *       "delete-state" = "Drupal\workflows\Form\WorkflowStateDeleteForm",
    *       "add-transition" = "Drupal\workflows\Form\WorkflowTransitionAddForm",
    *       "edit-transition" = "Drupal\workflows\Form\WorkflowTransitionEditForm",
    *       "delete-transition" = "Drupal\workflows\Form\WorkflowTransitionDeleteForm",
    *     },

   - the way in which this indents is aesthetically pleasing, bonus points

Thanks for the review @larowlan.

1. #2896724: Create constants for transition directions. is still on my radar. I've been focusing on WI criticals but will have a good pass over the queue soon.
2. #2899859: Remove incorrect documentation hint for "WorkflowTypeFormInterface". I noticed this, but Wim also fixed this in his nits patch.
3. I've opened #2902018: Use array_column instead of array_map where possible in the Workflows module.
4. Never noticed this, but you're right :)

Something I noticed when reviewing CM:
Any time we've had an interface on a value object, it has come back to haunt us later.

Interfaces for value objects are bad.

Good: Route, Config
Bad: FormState

\Drupal\workflows\StateInterface is problematic.
A true value object should NOT have an identifier.
So either it's not a value object, or it needs to change.

Reroll of #7.

A custom implementation of the state object already exist in the form of \Drupal\content_moderation\ContentModerationState. Up until recently (#2896721: Remove the concept of decorated states/transitions from worfklow type plugins. ), the API encouraged providing your own implementation of these objects.

If we continue to use this decorator pattern, the interface definitely seems useful. If we think an alternative can provide similar utility and other semantic benefits, I'm interested in exploring it. Otherwise, I'm happy to also work towards clearer docs.

My initial impression is, it's fine in the current form but I'm not aware of what issues are being referenced in #29 and if those are relevant here.

I think this can go back to RTBC.

Could we keep the interface internal - i.e. discourage uses of it outside content_moderation, as an interim step to removing it if that's what we decide?

@catch - I assume you men StateInterface and TransitionInterface, they're already internal.

StateInterface is internal, but here's the @internal docs at the moment:

 * @internal
 *   The workflow system is currently experimental and should only be leveraged
 *   by experimental modules and development releases of contributed modules.

What I mean is changing the docs so the @internal-ness isn't tied to the experimental-ness of the module.

Added a follow up to discuss the interfaces #2902309: Decide if State and Transition value objects need interfaces. Adding as a @todos as @catch suggested.

#40 looks great.

Combining #33 which was RTBC with #40 and RTBCing that part of it.

The patch is now the nits fixed by @Wim Leers and the @internal docs added by @timmillwood.

Would be great to see these go in.

#27:

All in all, workflows feels modern and clean, especially in reference to other modules that have been around for more than 10 years. Test coverage is thorough.

+1, definitely with the stable-blocking follow-ups in that this issue identified.

Good catches! I love the bonus points for the aesthetic pleasingness :D

#28: commented on #2896724, it's super close.

#29: right, because value objects can't really have alternative implementations, because … they're value objects. Good insight. I have one counter example though: AccessResultInterface, with the AccessResultAllowed, AccessResultNeutral and AccessResultForbidden implementations, as well as AccessResult. If there can be different kinds of the same type of value object, it seems like it is useful. \Drupal\Core\Url doesn't have an interface, because URLs are always URLs. I don't yet know how to extract a general rule from this though.

#36 + #38: agreed with @catch's question to mark StateInterface and TransitionInterface unconditionally @internal, that allows us to still add it as an official/public API in the future, but doesn't force us to commit to that now.

#39:

Then there's the added confusion to me that ContentModerationStateInterface is an empty interface that does not have any relationship to any of the above.

Yes, wholly agreed!

#42++ — looks like an RTBC to me :)

Major thanks to the Workflow Initiative Team for acting so quickly on my feedback. I hope you feel the module is now better for it, and (especially) more maintainable now.

Unrelated failure.

Committed as fe7caba and pushed to 8.5.x.
Cherry-picked as 9cdd6bf and pushed to 8.4.x.

#49++ — you're absolutely right.