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
Comment | File | Size | Author |
---|---|---|---|
#42 | 2899553-42.patch | 10.54 KB | Sam152 |
#40 | 2899553-40.patch | 1.4 KB | timmillwood |
#40 | interdiff-2899553-40.txt | 1.4 KB | timmillwood |
Comments
Comment #2
timmillwoodI think a "constant motivation" module in core would be awesome, but I assume we're looking at "Content Moderation" here.
Comment #3
Wim LeersFull 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:
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
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 fromckeditor.schema.yml
. Verify this withconfig_inspector
.Fixed.
These mappings both have a
weight
, but neither specifyorderby
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
\Drupal\workflows\Entity\Workflow
::preSave()
validates basic assumptions and throws a custom exception: 👍\Drupal\workflows\WorkflowTypeInterface
core/modules/workflows/src/
, yet the base class lives atcore/modules/workflows/src/Plugin/
. Why not have them live in the same directory? (Same forWorkflowTypeManager
.)::checkWorkflowAccess()
makes me suspicious — why do we need additional access checking methods? We'll need docs for that. (But at least it is returning anAccessResultInterface
object, so at least it's not losing cacheability metadata, good!)::getInitialState(WorkflowInterface $workflow)
looks a bit suspicious: why do we even need a method for this? I figured the reason is:… 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?Nit: That doesn't seem right. Should be
string[]
AFAICT. Fixed.Nit: Should be
@return $this
. Fixed.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 ofWorkflowType
plugins, its relation toWorkflow
entities, and toState(Interface)
andTransition(Interface)
.\Drupal\workflows\WorkflowInterface
is no longer accurate since #2896722: Leverage PluginWithFormsInterface to encapsulate @WorkflowType schema and clean up state/transition form methods.. Fixed.
\Drupal\workflows\WorkflowListBuilder
\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.\Drupal\workflows\WorkflowAccessControlHandler
Access controller for the Moderation State entity.
is outdated. Fixed.if (strpos($operation, 'delete-state') === 0) {
made me 😲 — a quick grep later it became more clear. This definitely needs docs.WorkflowTypeInterface::checkWorkflowAccess()
. The default implementation (\Drupal\workflows\Plugin\WorkflowTypeBase::checkWorkflowAccess()
) returnsAccessResult::neutral()
. Apparently it's for "non-admin access", and whatever is inWorkflowAccessControlHandler
'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.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!)checkCreateAccess()
does something interesting: it uses theconfig: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 tosetCacheBackend()
. And we probably want test coverage for it, to ensure it behaves as expected.workflows.module
workflows.info.yml
State(Interface)
+Transition(Interface)
\Drupal\workflows\State::labelCallback()
can be removed?WorkflowDeleteAccessCheck
WorkflowAccessControlHandler
?Comment #4
timmillwoodThanks 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.
Comment #5
Sam152 CreditAttribution: Sam152 as a volunteer and at PreviousNext commentedThanks 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.
Comment #6
Sam152 CreditAttribution: Sam152 as a volunteer and at PreviousNext commentedworkflows.schema.yml
.2WorkflowTypeInterface
.0WorkflowTypeInterface
.1WorkflowTypeInterface
.2Re: 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
.5WorkflowListBuilder
.1WorkflowAccessControlHandler
State(Interface) + Transition(Interface)
.2array_map([State::class, 'labelCallback'], $states)
.WorkflowDeleteAccessCheck
entity.workflow.delete_state_form
.Regarding the patch in #3:
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?
Comment #7
timmillwoodworkflows.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.Removing this as @Sam152 suggests, and RTBCing, we can resolve in #2900046: $workflow param is no longer required in getInitialState.
Comment #8
timmillwoodAdding a list of follow ups to the issue summary.
Comment #9
Wim LeersEither 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.
Comment #10
tim.plunkettOpened #2900421: Architectural review of the Content Moderation module
Comment #11
Wim Leersworkflows.schema.yml
.2 — I don't see why theorderby
belongs incontent_moderation.schema.yml
, becauseweight
lives inworkflows.schema.yml
?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.
Comment #12
Wim Leers#10: thanks!
Comment #13
tim.plunkettComment #14
Sam152 CreditAttribution: Sam152 as a volunteer and at PreviousNext commentedGoing 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.
Comment #15
Sam152 CreditAttribution: Sam152 as a volunteer and at PreviousNext commentedComment #16
timmillwood@Wim Leers - I've been looking at
orderby
and can't seem to get it working. It seemed to make sense to addorderby
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 addedorderby
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 addedorderby: 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.Comment #17
Wim LeersConfig system maintainer @alexpott wrote this at #2361539-78: Config export key order is not predictable for sequences, add orderby property to config schema :
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:
(Note how it's sorted by key, so by
bar
andfoo
, and then we see diff lines just for the changingweight
.) So what I've been insinuating is wrong, but adding theorderby: key
line is right — AFAICT.Comment #18
Wim Leers#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.
Comment #19
timmillwoodAdded follow up for
orderby: key
: #2900700: Add orderby to content_moderation schema sequencesComment #20
Wim LeersThanks 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!
Comment #21
Wim LeersOnce 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 :)
Comment #22
Sam152 CreditAttribution: Sam152 as a volunteer and at PreviousNext commentedMoving #2898913: Review a11y concerns in ContentModerationConfigureForm down.
Comment #23
Wim Leers#22: Good call.
Comment #24
Wim Leers#2900046: $workflow param is no longer required in getInitialState landed!
Comment #25
timmillwoodAll "WI Critical" issues are now committed or RTBC, so I think we can make this one RTBC too.
Comment #26
timmillwoodComment #27
larowlanMy 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.
\Drupal\workflows\Plugin\WorkflowTypeBase::getTransitionsForState
if 'from' and 'to' are the only valid directions, should we add anassert()
to verify. I think in a previous review I felt that 'from' and 'to' should be constants too.\Drupal\workflows\WorkflowInterface::getTypePlugin
has a return type referencing Drupal\workflows\WorkflowTypeFormInterface which I think we've since removed.could just use
array_column
- the way in which this indents is aesthetically pleasing, bonus points
Comment #28
Sam152 CreditAttribution: Sam152 as a volunteer and at PreviousNext commentedThanks 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 :)
Comment #29
tim.plunkettSomething 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.
Comment #30
xjmAgreed 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.
Comment #31
xjmI 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?
Comment #33
Sam152 CreditAttribution: Sam152 as a volunteer and at PreviousNext commentedReroll 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.
Comment #34
Sam152 CreditAttribution: Sam152 as a volunteer and at PreviousNext commentedComment #35
timmillwoodI think this can go back to RTBC.
Comment #36
catchCould 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?
Comment #37
timmillwood@catch - I assume you men StateInterface and TransitionInterface, they're already internal.
Comment #38
catchStateInterface is internal, but here's the @internal docs at the moment:
What I mean is changing the docs so the @internal-ness isn't tied to the experimental-ness of the module.
Comment #39
xjmBut 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 subclassWorkflow\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.
Comment #40
timmillwoodAdded a follow up to discuss the interfaces #2902309: Decide if State and Transition value objects need interfaces. Adding as a @todos as @catch suggested.
Comment #41
catch#40 looks great.
Comment #42
Sam152 CreditAttribution: Sam152 as a volunteer and at PreviousNext commentedCombining #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.
Comment #43
Wim Leers#27:
+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 theAccessResultAllowed
,AccessResultNeutral
andAccessResultForbidden
implementations, as well asAccessResult
. 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
andTransitionInterface
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:
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.
Comment #45
tacituseu CreditAttribution: tacituseu commentedUnrelated failure.
Comment #47
larowlanCommitted as fe7caba and pushed to 8.5.x.
Cherry-picked as 9cdd6bf and pushed to 8.4.x.
Comment #49
xjmThanks all!
Note that the issue title nonwithstanding, this patch is mostly documentation cleanups. (The actual architectural improvements were split off into other issues.)
Comment #50
Wim Leers#49++ — you're absolutely right.