Problem/Motivation

since experimental modules go into cofd with minimum expectationsAnd do not always meet final quality requirements or the core gates, It is a good idea to take a holistic look at their architecture before marking them stable.In this caseThe workflow and content motivation codeShould probably get an overall architectural review before we sign off, development on these issues has been extremely active in the past few weeks,And the architecture has shifted and improved a lot, so it is especially valuable to get a final review and mistakes.

Proposed resolution

Review the module code as needed and file issues for anything in particular that seems off or needs work.

Remaining tasks

TBD

User interface changes

TBD

API changes

TBD

Data model changes

TBD

Stable-blocking Follow ups

#2900320: Remove workflow type checkWorkflowAcess() & "view content moderation" permission
#2900046: $workflow param is no longer required in getInitialState 🎉
#2900634: Mark WorkflowDeleteAccessCheck @internal until a UI pattern for access-restricted states has been established
#2900700: Add orderby to content_moderation schema sequences

Follow ups

#2896726: Expand the entity access model for workflow states and transitions.
#2900047: Add a link to install content_moderation when no workflow types are present
#2897148: Remove @internal from workflowHasData/workflowStateHasData and use those methods for access control and configuration validation.
#2898913: Review a11y concerns in ContentModerationConfigureForm

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

xjm created an issue. See original summary.

timmillwood’s picture

Title: Architectural review of the workflows and constant motivation modules » Architectural review of the Workflows and Content Moderation modules

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

Wim Leers’s picture

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?
timmillwood’s picture

Status: Needs review » Needs work

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.

Sam152’s picture

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.

Sam152’s picture

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?

timmillwood’s picture

Status: Needs work » Reviewed & tested by the community
FileSize
629 bytes
10.3 KB

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.

timmillwood’s picture

Issue summary: View changes

Adding a list of follow ups to the issue summary.

Wim Leers’s picture

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.

tim.plunkett’s picture

Title: Architectural review of the Workflows and Content Moderation modules » Architectural review of the Workflows module
Issue tags: -Needs followup, -Needs title update
Related issues: +#2900421: Architectural review of the Content Moderation module
Wim Leers’s picture

  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.

Wim Leers’s picture

#10: thanks!

tim.plunkett’s picture

Sam152’s picture

Issue summary: View changes

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.

Sam152’s picture

Issue summary: View changes
timmillwood’s picture

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

Wim Leers’s picture

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.

Wim Leers’s picture

timmillwood’s picture

Wim Leers’s picture

Wim Leers’s picture

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

Sam152’s picture

Wim Leers’s picture

#22: Good call.

Wim Leers’s picture

timmillwood’s picture

Status: Needs work » Reviewed & tested by the community

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

timmillwood’s picture

Issue tags: +Workflow Initiative
larowlan’s picture

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

Sam152’s picture

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

tim.plunkett’s picture

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.

xjm’s picture

Interfaces for value objects are bad.

Agreed FWIW. @Sam152 (or others), do you think the interface needs to exist at all? Is there ever a case to provide a custom implementation? Scanning through, it seems to provide convenience wrappers to methods for the workflow as well as the value, label, and weight, and the workflow the state is attached to.

xjm’s picture

I could imagine that there could be usecases for providing custom logic for whether a state could transition to another state beyond the logic of a give workflow, so maybe the docs are just not right. Or would that always be coupled to the workflow?

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 7: 2899553-7.patch, failed testing. View results

Sam152’s picture

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.

Sam152’s picture

Status: Needs work » Needs review
timmillwood’s picture

Status: Needs review » Reviewed & tested by the community

I think this can go back to RTBC.

catch’s picture

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?

timmillwood’s picture

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

catch’s picture

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.

xjm’s picture

A custom implementation of the state object already exist in the form of \Drupal\content_moderation\ContentModerationState

But it's not actually a custom implementation; it's just adding methods for whether it's published or the default revision that it was just constructed with, and there's still no logic. ContentModerationState could subclass Workflow\State (don't know if it should) rather than being an implementation of an interface.

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.

I think we could explicitly make them internal, with documentation and a @todo, and discuss how they should be deprecated, changed, or removed in a dedicated issue.

Edit: Fixed class/interface names.

timmillwood’s picture

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.

catch’s picture

#40 looks great.

Sam152’s picture

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.

Wim Leers’s picture

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

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 42: 2899553-42.patch, failed testing. View results

tacituseu’s picture

Status: Needs work » Reviewed & tested by the community

Unrelated failure.

  • larowlan committed fe7caba on 8.5.x
    Issue #2899553 by timmillwood, Sam152, Wim Leers: Architectural review...
larowlan’s picture

Status: Reviewed & tested by the community » Fixed

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

  • larowlan committed 9cdd6bf on 8.4.x
    Issue #2899553 by timmillwood, Sam152, Wim Leers: Architectural review...
xjm’s picture

Title: Architectural review of the Workflows module » Architectural review of the Workflows module (documentation cleanups)
Category: Bug report » Task

Thanks all!

Note that the issue title nonwithstanding, this patch is mostly documentation cleanups. (The actual architectural improvements were split off into other issues.)

Wim Leers’s picture

#49++ — you're absolutely right.

Status: Fixed » Closed (fixed)

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