Problem/Motivation
Currently the following forms break encapsulation by assuming the structure of the schema of a @WorkflowType
plugin:
WorkflowTransitionAddForm
WorkflowTransitionEditForm
WorkflowStateAddForm
WorkflowStateEditForm
The offending code looks something like:
$workflow->set('type_settings', $configuration);
The 'type_settings'
property on the Workflow
entity is a plugin collection and thus the structure of it depends on the schema the plugin chooses to define. By not giving @WorkflowType
plugins a chance to react to the submission of their state/transition forms (just like \Drupal\Core\Plugin\PluginFormInterface
allows you to), we're breaking encapsulation by blindly setting those form values into 'type_settings'
.
We should only interact with @WorkflowType
settings through WorkflowTypeInterface
otherwise we're assuming details of the storage of states/transitions, which is a private implementation detail.
Proposed resolution
Implement PluginWithFormsInterface and provide forms for the global, state and transition forms.
Remaining tasks
Agree and fix.
User interface changes
None.
API changes
Since the Workflows module is still experimental, and about to be marked beta-level stability, at which point its APIs will be frozen, this is the perfect (and last) time where we can do this. If we don't do this now, we'll be required to support this the entire Drupal 8 cycle.
Data model changes
None.
Comment | File | Size | Author |
---|---|---|---|
#71 | Edit_Editorial_workflow_workflow___Drupal_8_x.png | 218.51 KB | plach |
#58 | 2896722-58.patch | 62.96 KB | Sam152 |
#58 | interdiff.txt | 16.61 KB | Sam152 |
#56 | 2896722-56.patch | 62.45 KB | Sam152 |
#56 | interdiff.txt | 4.47 KB | Sam152 |
Comments
Comment #2
Sam152 CreditAttribution: Sam152 as a volunteer and at PreviousNext commentedComment #3
Sam152 CreditAttribution: Sam152 as a volunteer and at PreviousNext commentedComment #4
Sam152 CreditAttribution: Sam152 as a volunteer and at PreviousNext commentedThis applies on top of the blocker.
Comment #5
Sam152 CreditAttribution: Sam152 as a volunteer and at PreviousNext commentedNR so issue statues correctly reflect progress.
Comment #7
tacituseu CreditAttribution: tacituseu commentedComment #8
Wim LeersCan we document why we want this in the IS?
Comment #9
Wim LeersComment #10
larowlanComment #11
Sam152 CreditAttribution: Sam152 as a volunteer and at PreviousNext commentedMissed a spot.
Comment #12
Sam152 CreditAttribution: Sam152 as a volunteer and at PreviousNext commentedComment #13
Sam152 CreditAttribution: Sam152 as a volunteer and at PreviousNext commentedUpdated IS and added a related issue.
Comment #14
Wim LeersIS was updated in #12. I just improved formatting a bit more.
Comment #15
Wim LeersEmphasis mine.
The devil's advocate question is:
AFAICT the answer is
Questions:
\Drupal\Core\Plugin\PluginWithFormsInterface
(which is fairly new, it was added almost exactly a year ago, in #2763157: Allow plugins to provide multiple forms). Then you could changeto
Comment #16
Wim LeersComment #17
Wim LeersWhichever direction this ends up taking, a change record will be necessary.
For now, updated the IS to reflect the solution the current patch is proposing.
Comment #18
timmillwoodUltimately this is needed because we can't have Workflows module assuming anything about the type_settings, however I have one concern.
In each of the form classes we're moving the construction of the entity object from copyFormValuesToEntity() to save(). Wouldn't this cause validation issues?
Comment #19
Sam152 CreditAttribution: Sam152 as a volunteer and at PreviousNext commentedRe #15:
WorkflowTypeFormInterface is a base class that does extend PluginFormInterface. Implementations can extend WorkflowTypeFormBase or WorkflowTypeBase depending on their requirements. See the related issue about moving the state/transition form methods to WorkflowTypeFormInterface.
@WorkflowType
plugins can provide. A global one, one for individual states and one for individual transitions. The global workflow form is the one that PluginFormInterface assists with.#18, this might become apparent looking at point 2 above.
Comment #20
timmillwoodCouldn't we call submitStateConfigurationForm() and submitTransitionConfigurationForm() within copyFormValuesToEntity.
Or add copyStateToEntity() or copyTransitionToEntity()?
Comment #21
Wim Leers#19: Oh, I hadn't even found
WorkflowTypeFormInterface
! I see it doesinterface WorkflowTypeFormInterface extends PluginFormInterface, WorkflowTypeInterface {}
, which makes sense. Andclass ContentModeration extends WorkflowTypeFormBase {}
, which also makes sense. But that means that forContentModeration
, there will not be two, but three submit handlers, so it's even more complex than I thought!workflows.api.php
file. It's also not documented in either\Drupal\workflows\Annotation\WorkflowType
or\Drupal\workflows\WorkflowTypeInterface
. Even just adding this one sentence that you provided here would be super helpful:SubformStateInterface
! You might want to talk to @tim.plunkett about this, he worked on this, and is a Form API maintainer, so he'll be able to best explain this :)Comment #22
Wim LeersOh, also — by adding these 2 methods to
WorkflowTypeInterface
, you're effectively making the state & transitions forms required, but the global one is optional. Is this intentional?Comment #23
Sam152 CreditAttribution: Sam152 as a volunteer and at PreviousNext commentedThe related issue I linked aims to resolve this. #2877926: Move buildStateConfigurationForm/buildTransitionConfigurationForm to WorkflowTypeFormInterface..
Comment #24
Sam152 CreditAttribution: Sam152 as a volunteer and at PreviousNext commentedComment #25
Sam152 CreditAttribution: Sam152 as a volunteer and at PreviousNext commentedMarking this as major. I think we need a plan to address this in a BC way or make this a WI Critical. Discussed this here.
Comment #26
Sam152 CreditAttribution: Sam152 as a volunteer and at PreviousNext commentedComment #27
Sam152 CreditAttribution: Sam152 as a volunteer and at PreviousNext commentedClosing #2877926: Move buildStateConfigurationForm/buildTransitionConfigurationForm to WorkflowTypeFormInterface., as doing this will make that issue defunct.
Comment #28
Sam152 CreditAttribution: Sam152 as a volunteer and at PreviousNext commentedHaving a look at this.
Comment #29
Sam152 CreditAttribution: Sam152 as a volunteer and at PreviousNext commentedProgress patch to see what tests fail. Still need a pass over all of the docs as well as a sanity check on the names of everything.
Comment #31
Wim LeersWould be good to
@see
the 3 base classes here too.Do we also need an
else
case? Or are these entirely optional?(Would be good to have that documented.)
👍 — but would be great to get @tim.plunkett's thoughts on this.
👍
👍
Perhaps an
@see \Drupal\workflow_type_test\Plugin\WorkflowType\ComplexTestType
here?❤️
Comment #32
Sam152 CreditAttribution: Sam152 as a volunteer and at PreviousNext commentedTest fixes.
Comment #33
Sam152 CreditAttribution: Sam152 as a volunteer and at PreviousNext commentedThanks for the review in #31, didn't see it before posting #32. Will take a look.
Comment #34
Sam152 CreditAttribution: Sam152 as a volunteer and at PreviousNext commentedFeedback from #31. Would be great to @tim.plunkett's eyes across this too.
Comment #35
Sam152 CreditAttribution: Sam152 as a volunteer and at PreviousNext commentedComment #37
Wim LeersYou added these, but I think you also want this for the
content_moderation
@WorkflowType
plugin.So
content_moderation
does not have atransition
form, right?P.S.: pinged @tim.plunkett :)
Comment #39
Sam152 CreditAttribution: Sam152 as a volunteer and at PreviousNext commentedLast few test fixes, and #37.1.
Re: #37.2, yep, no transition form for content moderation.
Comment #40
tim.plunkettThe third parameters of submitConfigurationForm seem wrong to me. Are those really required? I'm not sure, but in the case of passing $workflow ($this->entity), it should be retrievable as
$form_state->getFormObject()->getEntity()
...Comment #41
Sam152 CreditAttribution: Sam152 as a volunteer and at PreviousNext commentedRe: #40, from the
$form
and$form_state
context passed to the state and transition forms, there is no way to know what state/transition object you are building the form for, or which state/transition you should associate the submitted values with during save. The workflow was passed to be consistent with this approach.One alternative might be to set the state and transition into
$form_state
instead, and perhaps provide protected methods on the base forms forgetState()
,getTransition()
,getWorkflow()
which derive those values from the$form_state
and$form_state->getFormObject()->getEntity()
as you suggest.I'm okay with both approaches, maybe @Wim Leers has some thoughts.
I also had some consistency issues in the patch with using
SubformState
as well and when that extra param was being passed (there are a lot of methods involved). Documented each of them in the table below with the clean up attached.Comment #42
larowlanThis sounds like a good approach
If configure, state and transition have special meaning - should we make them constants. Nothing worse than loosing an hour to a typo.
We add this three times - case for am abstract base class that sits above EntityForm?
is this correct?
Comment #43
Sam152 CreditAttribution: Sam152 as a volunteer and at PreviousNext commentedReroll, will have a squiz at the feedback. Thanks @larowlan.
Comment #45
Sam152 CreditAttribution: Sam152 as a volunteer and at PreviousNext commentedMissed a spot.
Comment #46
Sam152 CreditAttribution: Sam152 as a volunteer and at PreviousNext commentedHere is an approach using form state to pass the state and transition.
1. Added constants. We can use constants in the annotations too, but it looks super verbose. I couldn't find any examples of other plugin types using these either (searched with " \* (.*)::[A-Z]").
2. Had a look at this, but it didn't seem right adding a base class just to inject a service. Open to be convinced though.
3. Good catch, fixed.
Comment #47
Wim LeershasFormClass()
calls. But we're still doing$form_state→get('some_string')
. Why not use the constants there too?Perhaps now also
@see
the constants?In #46.2, you say that you don't think a base class is a good idea. An alternative could be a trait.
#46 will also need a review from Tim Plunkett for #40.
Comment #48
Sam152 CreditAttribution: Sam152 as a volunteer and at PreviousNext commented1. Okay cool.
2. Any ideas on where we'd put these? These seem kind of implementation specific, it doesn't feel like they belong on an interface.
3. Added.
4. Do we really any abstraction to inject a single service? Adding an abstraction so we can use an abstraction with less lines of code just seems overly meta to me.
Comment #50
Sam152 CreditAttribution: Sam152 as a volunteer and at PreviousNext commentedRerolled.
Comment #51
Sam152 CreditAttribution: Sam152 as a volunteer and at PreviousNext commentedComment #52
Sam152 CreditAttribution: Sam152 as a volunteer and at PreviousNext commentedFixing CS issues.
Comment #54
larowlanNone of our traits in core container a constructor, let's keep it that way cause the DX of extending and calling the parent is naff with a trait.
Let's open a follow up to discuss if its worth adding a base class, can be done later.
Perhaps move the existing three constants off WorkflowTypeInterface onto their respective interfaces?
or could name them PLUGIN_FORM_KEY so its more explicit.
Will ping timplunkett to chase that re-review
Comment #55
tim.plunkettIt's not common to provide an empty build or submit method. No reason to subclass if you don't have form elements to return or submit.
Same here. The corresponding submit method in this class isn't empty, so the build shouldn't be either.
And again
No need to empty out $form here
Same
The rest of the PWFI changes are great.
I have no opinion on constants vs magic strings in this case.
I agree that a trait is harmful here, and don't think a base class would be much help. Boilerplate is boilerplate.
Comment #56
Sam152 CreditAttribution: Sam152 as a volunteer and at PreviousNext commentedAddressing feedback in #55.
Comment #58
Sam152 CreditAttribution: Sam152 as a volunteer and at PreviousNext commentedMoved the constants to their respective interfaces. I think the global 'configure' one still belongs on WorkflowTypeInterface, because it's the form for the type plugin, not much to do with the entity itself.
I tried simply using the same constant for the key used in
$form_state
but the way it's currently named, it doesn't work. I can't think of a name that'd neatly sum up both these concepts, so my preference for now is to leave them as strings.Comment #60
tacituseu CreditAttribution: tacituseu commentedBoth (#56 and #58) failures are segfaults in garbage collector.
Comment #61
Sam152 CreditAttribution: Sam152 as a volunteer and at PreviousNext commentedComment #62
timmillwoodI've taken a look at the code and manually tested, looks good to me.
Not sure I'm qualified to RTBC, but +1 from me.
Comment #63
tim.plunkettNot used to seeing markup concatenated like this anymore, could this instead use
'#type' => 'inline_template'
?I see that it's copied from another place, and not changed in this issue. Can someone open a follow-up to fix this?
Still needs a change record, but marking RTBC for the patch itself.
Comment #64
Sam152 CreditAttribution: Sam152 as a volunteer and at PreviousNext commentedAdded follow-up #2896722: Leverage PluginWithFormsInterface to encapsulate @WorkflowType schema and clean up state/transition form methods..
Since the CR is live, I'll add this as soon as the issue goes in:
Comment #65
tim.plunkettYou linked this issue instead, I believe you meant #2898886: ContentModerationConfigureForm should use inline list in place of concatenated markup. Thanks for opening it!
Comment #66
Sam152 CreditAttribution: Sam152 as a volunteer and at PreviousNext commentedBleh, good catch. Thanks.
Comment #68
larowlanAgain I realise this is just being moved from elsewhere, but this needs a follow-up for a11y, there is no context to the link for a screen reader. Needs to the entity type and the workflow labels into a hidden span.
I love that this solved the issue without needing to use instanceof
Committed as f7f742e and pushed to 8.5.x.
Cherry-picked as 65b9f1f and pushed to 8.4.x
@Sam152 can you make those change record changes?
See you in #2843494: WI: Workflows module roadmap
Comment #70
Sam152 CreditAttribution: Sam152 as a volunteer and at PreviousNext commentedUpdated the change record and opened #2898913: Review a11y concerns in ContentModerationConfigureForm.
Comment #71
plachAfter running updates and rebuilding caches, I'm still getting some issues that seem related to this:
Comment #72
Sam152 CreditAttribution: Sam152 as a volunteer and at PreviousNext commentedHave you tried reinstalling? That looks like what you'd get if you installed before #2849827: Move workflow "settings" setters and getters to WorkflowTypeInterface and then did a pull.
Comment #73
larowlanWorking fine on HEAD for me @plach - see https://d2ghq.ply.st/ (23hrs to go).
Did you reinstall?
@Sam152 pointed me towards #2896630: Unofficial content_moderation 8.3.7 to 8.4.0 upgrade path for an upgrade path if you don't have the option of reinstall.
Comment #74
plachI was just testing in my local dev env, I forgot we don't support an upgrade path yet for CM, sorry for the noise :)
Comment #75
Wim LeersPer #70.
Comment #76
Wim LeersUpdated CR to clarify impact: https://www.drupal.org/node/2897706/revisions/view/10586340/10586346.