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.

CommentFileSizeAuthor
#71 Edit_Editorial_workflow_workflow___Drupal_8_x.png218.51 KBplach
#58 2896722-58.patch62.96 KBSam152
#58 interdiff.txt16.61 KBSam152
#56 2896722-56.patch62.45 KBSam152
#56 interdiff.txt4.47 KBSam152
#52 2896722-52.patch62.74 KBSam152
#52 interdiff.txt6.15 KBSam152
#50 2896722-50.patch62.65 KBSam152
#48 interdiff.txt775 bytesSam152
#48 2896722-48.patch65.37 KBSam152
#46 2896722-46.patch65.16 KBSam152
#46 interdiff.txt24.95 KBSam152
#45 2896722-45.patch63.68 KBSam152
#45 interdiff.txt1.16 KBSam152
#43 2896722-43.patch63.67 KBSam152
#41 2896722-41.patch62.84 KBSam152
#41 interdiff.txt3.5 KBSam152
#39 2896722-39.patch62.58 KBSam152
#39 interdiff.txt4.72 KBSam152
#34 2896722-34.patch59.64 KBSam152
#34 interdiff.txt2.92 KBSam152
#32 2896722-31.patch59.22 KBSam152
#32 interdiff.txt5.03 KBSam152
#29 2896722-29.patch57.19 KBSam152
#11 interdiff.txt3.8 KBSam152
#11 2896722-11.patch11.24 KBSam152
#4 2896722-4.patch7.44 KBSam152
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Sam152 created an issue. See original summary.

Sam152’s picture

Issue tags: +Workflow Initiative
Sam152’s picture

Component: content_moderation.module » workflows.module
Issue tags: -workflows.module
Sam152’s picture

This applies on top of the blocker.

Sam152’s picture

Status: Active » Needs review

NR so issue statues correctly reflect progress.

Status: Needs review » Needs work

The last submitted patch, 4: 2896722-4.patch, failed testing. View results

tacituseu’s picture

Title: Introduce submitStateConfigurationForm/submitTransitionConfigurationForm for encapsulation of workflow type plugins configuration » [PP-1] Introduce submitStateConfigurationForm/submitTransitionConfigurationForm for encapsulation of workflow type plugins configuration
Wim Leers’s picture

Can we document why we want this in the IS?

Wim Leers’s picture

larowlan’s picture

Title: [PP-1] Introduce submitStateConfigurationForm/submitTransitionConfigurationForm for encapsulation of workflow type plugins configuration » Introduce submitStateConfigurationForm/submitTransitionConfigurationForm for encapsulation of workflow type plugins configuration
Status: Needs work » Active
Sam152’s picture

Status: Active » Needs review
FileSize
11.24 KB
3.8 KB

Missed a spot.

Sam152’s picture

Issue summary: View changes
Sam152’s picture

Wim Leers’s picture

Issue summary: View changes
Issue tags: -Needs issue summary update

IS was updated in #12. I just improved formatting a bit more.

Wim Leers’s picture

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

Emphasis mine.

+++ b/core/modules/workflows/src/WorkflowTypeInterface.php
@@ -108,6 +109,18 @@ public function getInitialState(WorkflowInterface $workflow);
+  public function submitStateConfigurationForm(array &$form, SubformStateInterface $form_state, StateInterface $state);

@@ -128,6 +141,18 @@ public function buildStateConfigurationForm(FormStateInterface $form_state, Work
+  public function submitTransitionConfigurationForm(array &$form, SubformStateInterface $form_state, TransitionInterface $transition);

The devil's advocate question is: Why can't WorkflowTypeInterface extend PluginFormInterface then?

AFAICT the answer is Because every @WorkflowType plugin supports two kinds of forms: one for state, one for configuration.

Questions:

  1. Is my understanding correct — is that the reason why this is not reusing the existing interface, and is instead adding two methods?
  2. Why is there no need for validation callbacks, only for submit callbacks?
  3. Why not use \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 change
     * @WorkflowType(
     *   id = "content_moderation",
     *   label = @Translation("Content moderation"),
     *   required_states = {
     *     "draft",
     *     "published",
     *   },
     * )
    

    to

     * @WorkflowType(
     *   id = "content_moderation",
     *   label = @Translation("Content moderation"),
     *   required_states = {
     *     "draft",
     *     "published",
     *   },
     *   forms = {
     *     "state" = "FQCN",
     *     "transition" = "FQCN",
     *   },
     * )
    
Wim Leers’s picture

Wim Leers’s picture

Issue summary: View changes
Issue tags: +Needs change record

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

timmillwood’s picture

Ultimately this is needed because we can't have Workflows module assuming anything about the type_settings, however I have one concern.

+++ b/core/modules/workflows/src/Form/WorkflowTransitionAddForm.php
@@ -5,6 +5,7 @@
@@ -105,11 +106,6 @@ protected function copyFormValuesToEntity(EntityInterface $entity, array $form,

@@ -136,6 +132,11 @@ public function validateForm(array &$form, FormStateInterface $form_state) {
   public function save(array $form, FormStateInterface $form_state) {

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?

Sam152’s picture

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

  1. There are three kinds of forms @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.
  2. For consistency sake, we should probably also add the validation methods. Form api has some neat ways of getting around this, in the form #element_validate, but I agree it's weird to not have the same set of methods to implement for each form.
  3. This looks pretty interesting. The only part that isn't a perfect fit is, the current methods provide a smaller part of a larger form. We use SubFormInterface to satisfy the build/validate/submit methods. Not sure if this would provide a better DX, but worth looking into.

#18, this might become apparent looking at point 2 above.

timmillwood’s picture

Couldn't we call submitStateConfigurationForm() and submitTransitionConfigurationForm() within copyFormValuesToEntity.
Or add copyStateToEntity() or copyTransitionToEntity()?

Wim Leers’s picture

#19: Oh, I hadn't even found WorkflowTypeFormInterface! I see it does interface WorkflowTypeFormInterface extends PluginFormInterface, WorkflowTypeInterface {}, which makes sense. And class ContentModeration extends WorkflowTypeFormBase {}, which also makes sense. But that means that for ContentModeration, there will not be two, but three submit handlers, so it's even more complex than I thought!

  1. Ahh! This is extremely illuminating :) Is that documented somewhere? I don't see a 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:

    There are three kinds of forms @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.

  2. 👍
  3. You can still use 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 :)
Wim Leers’s picture

Oh, 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?

Sam152’s picture

Sam152’s picture

Priority: Normal » Major
Sam152’s picture

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

Sam152’s picture

Title: Introduce submitStateConfigurationForm/submitTransitionConfigurationForm for encapsulation of workflow type plugins configuration » Leverage PluginWithFormsInterface to encapsulate @WorkflowType schema and clean up state/transition form methods.
Issue summary: View changes
Sam152’s picture

Sam152’s picture

Assigned: Unassigned » Sam152

Having a look at this.

Sam152’s picture

Assigned: Sam152 » Unassigned
FileSize
57.19 KB

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

Status: Needs review » Needs work

The last submitted patch, 29: 2896722-29.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

Wim Leers’s picture

  1. +++ b/core/modules/workflows/src/Annotation/WorkflowType.php
    @@ -50,4 +50,17 @@ class WorkflowType extends Plugin {
    +   * Forms which will be used for the workflow UI are 'configure', 'state' and
    +   * 'transition'.
    +   *
    +   * @see \Drupal\Core\Plugin\PluginWithFormsInterface
    +   * @see \Drupal\Core\Plugin\PluginFormInterface
    

    Would be good to @see the 3 base classes here too.

  2. +++ b/core/modules/workflows/src/Form/WorkflowStateAddForm.php
    @@ -86,12 +120,23 @@ protected function copyFormValuesToEntity(EntityInterface $entity, array $form,
    +    if ($workflow_type->hasFormClass('state')) {
    
    @@ -101,6 +146,15 @@ protected function copyFormValuesToEntity(EntityInterface $entity, array $form,
    +    if ($workflow_type->hasFormClass('state')) {
    
    +++ b/core/modules/workflows/src/Form/WorkflowTransitionAddForm.php
    @@ -128,6 +161,13 @@ public function validateForm(array &$form, FormStateInterface $form_state) {
    +    if ($workflow_type->hasFormClass('transition')) {
    
    @@ -136,6 +176,15 @@ public function validateForm(array &$form, FormStateInterface $form_state) {
    +    if ($workflow_type->hasFormClass('transition')) {
    

    Do we also need an else case? Or are these entirely optional?

    (Would be good to have that documented.)

  3. +++ b/core/modules/workflows/src/Plugin/WorkflowTypeConfigureFormBase.php
    @@ -0,0 +1,51 @@
    +abstract class WorkflowTypeConfigureFormBase implements PluginFormInterface, PluginAwareInterface {
    
    index 942c2d6..0000000
    --- a/core/modules/workflows/src/Plugin/WorkflowTypeFormBase.php
    
    +++ b/core/modules/workflows/src/Plugin/WorkflowTypeStateFormBase.php
    @@ -0,0 +1,55 @@
    +abstract class WorkflowTypeStateFormBase implements PluginFormInterface, PluginAwareInterface {
    
    +++ b/core/modules/workflows/src/Plugin/WorkflowTypeTransitionFormBase.php
    @@ -0,0 +1,55 @@
    +abstract class WorkflowTypeTransitionFormBase implements PluginFormInterface, PluginAwareInterface {
    

    👍 — but would be great to get @tim.plunkett's thoughts on this.

  4. +++ /dev/null
    @@ -1,16 +0,0 @@
    -interface WorkflowTypeFormInterface extends PluginFormInterface, WorkflowTypeInterface {
    

    👍

  5. +++ b/core/modules/workflows/src/WorkflowTypeInterface.php
    @@ -15,7 +15,7 @@
    -interface WorkflowTypeInterface extends PluginInspectionInterface, DerivativeInspectionInterface, ConfigurablePluginInterface {
    +interface WorkflowTypeInterface extends PluginWithFormsInterface, DerivativeInspectionInterface, ConfigurablePluginInterface {
    
    @@ -121,45 +121,6 @@ public function decorateTransition(TransitionInterface $transition);
    -  public function buildStateConfigurationForm(FormStateInterface $form_state, WorkflowInterface $workflow, StateInterface $state = NULL);
    ...
    -  public function buildTransitionConfigurationForm(FormStateInterface $form_state, WorkflowInterface $workflow, TransitionInterface $transition = NULL);
    

    👍

  6. +++ b/core/modules/workflows/tests/modules/workflow_type_test/src/Form/ComplexTestTypeConfigureForm.php
    @@ -0,0 +1,37 @@
    +class ComplexTestTypeConfigureForm extends WorkflowTypeConfigureFormBase {
    
    +++ b/core/modules/workflows/tests/modules/workflow_type_test/src/Form/ComplexTestTypeStateForm.php
    @@ -0,0 +1,29 @@
    +class ComplexTestTypeStateForm extends WorkflowTypeStateFormBase {
    
    +++ b/core/modules/workflows/tests/modules/workflow_type_test/src/Form/ComplexTestTypeTransitionForm.php
    @@ -0,0 +1,29 @@
    +class ComplexTestTypeTransitionForm extends WorkflowTypeTransitionFormBase {
    

    Perhaps an @see \Drupal\workflow_type_test\Plugin\WorkflowType\ComplexTestType here?

  7. +++ b/core/modules/workflows/tests/modules/workflow_type_test/src/Plugin/WorkflowType/ComplexTestType.php
    @@ -17,9 +15,14 @@
    + *   forms = {
    + *     configure = "\Drupal\workflow_test_type\Form\ComplexTestTypeConfigureForm",
    + *     state = "\Drupal\workflow_test_type\Form\ComplexTestTypeStateForm",
    + *     transition = "\Drupal\workflow_test_type\Form\ComplexTestTypeTransitionForm",
    + *   }
    

    ❤️

Sam152’s picture

Status: Needs work » Needs review
FileSize
5.03 KB
59.22 KB

Test fixes.

Sam152’s picture

Thanks for the review in #31, didn't see it before posting #32. Will take a look.

Sam152’s picture

Feedback from #31. Would be great to @tim.plunkett's eyes across this too.

Sam152’s picture

Issue tags: +WI critical

The last submitted patch, 32: 2896722-31.patch, failed testing. View results

Wim Leers’s picture

  1. +++ b/core/modules/workflows/tests/modules/workflow_type_test/src/Form/ComplexTestTypeConfigureForm.php
    @@ -8,6 +8,8 @@
    + * @see \Drupal\workflow_type_test\Plugin\WorkflowType\ComplexTestType
    
    +++ b/core/modules/workflows/tests/modules/workflow_type_test/src/Form/ComplexTestTypeStateForm.php
    @@ -8,6 +8,8 @@
    + * @see \Drupal\workflow_type_test\Plugin\WorkflowType\ComplexTestType
    
    +++ b/core/modules/workflows/tests/modules/workflow_type_test/src/Form/ComplexTestTypeTransitionForm.php
    @@ -8,6 +8,8 @@
    + * @see \Drupal\workflow_type_test\Plugin\WorkflowType\ComplexTestType
    

    You added these, but I think you also want this for the content_moderation @WorkflowType plugin.

  2. +++ b/core/modules/content_moderation/src/Plugin/WorkflowType/ContentModeration.php
    @@ -29,9 +27,13 @@
    + *   forms = {
    + *     "configure" = "\Drupal\content_moderation\Form\ContentModerationConfigureForm",
    + *     "state" = "\Drupal\content_moderation\Form\ContentModerationStateForm"
    + *   },
    

    So content_moderation does not have a transition form, right?

P.S.: pinged @tim.plunkett :)

Status: Needs review » Needs work

The last submitted patch, 34: 2896722-34.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

Sam152’s picture

Status: Needs work » Needs review
FileSize
4.72 KB
62.58 KB

Last few test fixes, and #37.1.

Re: #37.2, yep, no transition form for content moderation.

tim.plunkett’s picture

+++ b/core/modules/workflows/src/Form/WorkflowEditForm.php
@@ -213,12 +247,16 @@ public function validateForm(array &$form, FormStateInterface $form_state) {
+        ->submitConfigurationForm($form['type_settings'], $subform_state, $workflow);

+++ b/core/modules/workflows/src/Form/WorkflowStateAddForm.php
@@ -101,6 +146,15 @@ protected function copyFormValuesToEntity(EntityInterface $entity, array $form,
+        ->submitConfigurationForm($form['type_settings'], $subform_state, $workflow_type->getState($form_state->getValue('id')));

+++ b/core/modules/workflows/src/Form/WorkflowStateEditForm.php
@@ -138,6 +185,15 @@ protected function copyFormValuesToEntity(EntityInterface $entity, array $form,
+        ->submitConfigurationForm($form['type_settings'], $subform_state, $workflow_type->getState($this->stateId));

+++ b/core/modules/workflows/src/Form/WorkflowTransitionAddForm.php
@@ -136,6 +176,15 @@ public function validateForm(array &$form, FormStateInterface $form_state) {
+        ->submitConfigurationForm($form['type_settings'], $subform_state, $workflow_type->getTransition($form_state->getValue('id')));

+++ b/core/modules/workflows/src/Form/WorkflowTransitionEditForm.php
@@ -141,6 +182,16 @@ protected function copyFormValuesToEntity(EntityInterface $entity, array $form,
+        ->submitConfigurationForm($form['type_settings'], $subform_state, $transition);

+++ b/core/modules/workflows/src/Plugin/WorkflowTypeConfigureFormBase.php
@@ -0,0 +1,51 @@
+  public function submitConfigurationForm(array &$form, FormStateInterface $form_state, WorkflowInterface $workflow = NULL) {

+++ b/core/modules/workflows/src/Plugin/WorkflowTypeStateFormBase.php
@@ -0,0 +1,55 @@
+  public function submitConfigurationForm(array &$form, FormStateInterface $form_state, StateInterface $state = NULL) {

+++ b/core/modules/workflows/src/Plugin/WorkflowTypeTransitionFormBase.php
@@ -0,0 +1,55 @@
+  public function submitConfigurationForm(array &$form, FormStateInterface $form_state, TransitionInterface $transition = NULL) {

+++ b/core/modules/workflows/tests/modules/workflow_type_test/src/Form/ComplexTestTypeConfigureForm.php
@@ -0,0 +1,39 @@
+  public function submitConfigurationForm(array &$form, FormStateInterface $form_state, WorkflowInterface $workflow = NULL) {

The 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()...

Sam152’s picture

Re: #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 for getState(), 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.

Form Class Method SubformState Extra param
WorkflowStateAddForm build X (doesn't exist yet)
validate X (may not exist, if not valid)
submit
WorkflowStateEditForm build
validate
submit
WorkflowTransitionAddForm build X
validate X
submit
WorkflowTransitionEditForm build
validate
submit
WorkflowAddForm N/A N/A N/A
WorkflowEditForm build
validate
submit
larowlan’s picture

One alternative might be to set the state and transition into $form_state instead, and perhaps provide protected methods on the base forms for getState(), getTransition(), getWorkflow() which derive those values from the $form_state and $form_state->getFormObject()->getEntity() as you suggest.

This sounds like a good approach

  1. +++ b/core/modules/workflows/src/Form/WorkflowEditForm.php
    @@ -184,12 +212,14 @@ public function form(array $form, FormStateInterface $form_state) {
    +    if ($workflow_type->hasFormClass('configure')) {
    ...
    +        ->createInstance($workflow_type, 'configure')
    
    @@ -201,9 +231,13 @@ public function form(array $form, FormStateInterface $form_state) {
    +    if ($workflow_type->hasFormClass('configure')) {
    ...
    +        ->createInstance($workflow_type, 'configure')
    
    @@ -213,12 +247,16 @@ public function validateForm(array &$form, FormStateInterface $form_state) {
    +    if ($workflow_type->hasFormClass('configure')) {
    ...
    +        ->createInstance($workflow_type, 'configure')
    
    +++ b/core/modules/workflows/src/Form/WorkflowStateAddForm.php
    @@ -42,11 +73,15 @@ public function form(array $form, FormStateInterface $form_state) {
    +    if ($workflow_type->hasFormClass('state')) {
    ...
    +        ->createInstance($workflow_type, 'state')
    
    @@ -86,12 +121,23 @@ protected function copyFormValuesToEntity(EntityInterface $entity, array $form,
    +    if ($workflow_type->hasFormClass('state')) {
    ...
    +        ->createInstance($workflow_type, 'state')
    
    @@ -101,6 +147,16 @@ protected function copyFormValuesToEntity(EntityInterface $entity, array $form,
    +    if ($workflow_type->hasFormClass('state')) {
    ...
    +        ->createInstance($workflow_type, 'state')
    
    +++ b/core/modules/workflows/src/Form/WorkflowStateEditForm.php
    @@ -62,10 +93,15 @@ public function form(array $form, FormStateInterface $form_state) {
    +    if ($workflow_type->hasFormClass('state')) {
    ...
    +        ->createInstance($workflow_type, 'state')
    
    @@ -125,10 +161,22 @@ protected function copyFormValuesToEntity(EntityInterface $entity, array $form,
    +    if ($workflow_type->hasFormClass('state')) {
    ...
    +        ->createInstance($workflow_type, 'state')
    
    @@ -138,6 +186,15 @@ protected function copyFormValuesToEntity(EntityInterface $entity, array $form,
    +    if ($workflow_type->hasFormClass('state')) {
    ...
    +        ->createInstance($workflow_type, 'state')
    
    +++ b/core/modules/workflows/src/Form/WorkflowTransitionAddForm.php
    @@ -62,10 +93,15 @@ public function form(array $form, FormStateInterface $form_state) {
    +    if ($workflow_type->hasFormClass('transition')) {
    ...
    +        ->createInstance($workflow_type, 'transition')
    
    @@ -128,6 +161,13 @@ public function validateForm(array &$form, FormStateInterface $form_state) {
    +    if ($workflow_type->hasFormClass('transition')) {
    ...
    +        ->createInstance($workflow_type, 'transition')
    
    @@ -136,6 +176,16 @@ public function validateForm(array &$form, FormStateInterface $form_state) {
    +    if ($workflow_type->hasFormClass('transition')) {
    ...
    +        ->createInstance($workflow_type, 'transition')
    

    If configure, state and transition have special meaning - should we make them constants. Nothing worse than loosing an hour to a typo.

  2. +++ b/core/modules/workflows/src/Form/WorkflowTransitionEditForm.php
    @@ -21,6 +24,32 @@ class WorkflowTransitionEditForm extends EntityForm {
    +   * The plugin form factory.
    +   *
    +   * @var \Drupal\Core\Plugin\PluginFormFactoryInterface
    +   */
    +  protected $pluginFormFactory;
    +
    +  /**
    +   * Creates an instance of WorkflowStateEditForm.
    +   *
    +   * @param \Drupal\Core\Plugin\PluginFormFactoryInterface $pluginFormFactory
    +   *   The plugin form factory.
    +   */
    +  public function __construct(PluginFormFactoryInterface $pluginFormFactory) {
    +    $this->pluginFormFactory = $pluginFormFactory;
    +  }
    +
    +  /**
    +   * {@inheritdoc}
    +   */
    +  public static function create(ContainerInterface $container) {
    +    return new static(
    +      $container->get('plugin_form.factory')
    +    );
    +  }
    

    We add this three times - case for am abstract base class that sits above EntityForm?

  3. +++ b/core/modules/workflows/src/Plugin/WorkflowTypeConfigureFormBase.php
    @@ -0,0 +1,51 @@
    + * The content moderation WorkflowType configuration form.
    

    is this correct?

Sam152’s picture

Reroll, will have a squiz at the feedback. Thanks @larowlan.

Status: Needs review » Needs work

The last submitted patch, 43: 2896722-43.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

Sam152’s picture

Status: Needs work » Needs review
FileSize
1.16 KB
63.68 KB

Missed a spot.

Sam152’s picture

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

Wim Leers’s picture

  1. Agreed that using the constants in the annotations is perhaps too verbose.
  2. #46 is now using the constants for hasFormClass() calls. But we're still doing $form_state→get('some_string'). Why not use the constants there too?
  3. +++ b/core/modules/workflows/src/Annotation/WorkflowType.php
    @@ -50,4 +50,20 @@ class WorkflowType extends Plugin {
    +   * @see \Drupal\Core\Plugin\PluginWithFormsInterface
    +   * @see \Drupal\Core\Plugin\PluginFormInterface
    +   * @see \Drupal\workflows\Plugin\WorkflowTypeConfigureFormBase
    +   * @see \Drupal\workflows\Plugin\WorkflowTypeStateFormBase
    +   * @see \Drupal\workflows\Plugin\WorkflowTypeTransitionFormBase
    

    Perhaps now also @see the constants?

  4. +++ b/core/modules/workflows/src/Form/WorkflowEditForm.php
    @@ -17,6 +19,32 @@
       /**
    +   * The plugin form factory.
    +   *
    +   * @var \Drupal\Core\Plugin\PluginFormFactoryInterface
    +   */
    +  protected $pluginFormFactory;
    +
    +  /**
    +   * Creates an instance of WorkflowStateEditForm.
    +   *
    +   * @param \Drupal\Core\Plugin\PluginFormFactoryInterface $pluginFormFactory
    +   *   The plugin form factory.
    +   */
    +  public function __construct(PluginFormFactoryInterface $pluginFormFactory) {
    +    $this->pluginFormFactory = $pluginFormFactory;
    +  }
    +
    +  /**
    +   * {@inheritdoc}
    +   */
    +  public static function create(ContainerInterface $container) {
    +    return new static(
    +      $container->get('plugin_form.factory')
    +    );
    +  }
    +
    

    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.

Sam152’s picture

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

Status: Needs review » Needs work

The last submitted patch, 48: 2896722-48.patch, failed testing. View results

Sam152’s picture

Rerolled.

Sam152’s picture

Status: Needs work » Needs review
Sam152’s picture

Fixing CS issues.

Status: Needs review » Needs work

The last submitted patch, 52: 2896722-52.patch, failed testing. View results

larowlan’s picture

In #46.2, you say that you don't think a base class is a good idea. An alternative could be a trait.

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

Any ideas on where we'd put these? These seem kind of implementation specific, it doesn't feel like they belong on an interface.

Perhaps move the existing three constants off WorkflowTypeInterface onto their respective interfaces?

TransitionInterface::PLUGIN_FORM
StateInterface::PLUGIN_FORM
WorkflowInterface::PLUGIN_FORM

or could name them PLUGIN_FORM_KEY so its more explicit.

Will ping timplunkett to chase that re-review

tim.plunkett’s picture

  1. +++ b/core/modules/workflows/src/Plugin/WorkflowTypeConfigureFormBase.php
    @@ -0,0 +1,50 @@
    +  /**
    +   * {@inheritdoc}
    +   */
    +  public function buildConfigurationForm(array $form, FormStateInterface $form_state) {
    +  }
    +
    +  /**
    +   * {@inheritdoc}
    +   */
    +  public function validateConfigurationForm(array &$form, FormStateInterface $form_state) {
    +  }
    +
    +  /**
    +   * {@inheritdoc}
    +   */
    +  public function submitConfigurationForm(array &$form, FormStateInterface $form_state) {
    +  }
    

    It'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.

  2. +++ b/core/modules/workflows/src/Plugin/WorkflowTypeStateFormBase.php
    @@ -0,0 +1,55 @@
    +  /**
    +   * {@inheritdoc}
    +   */
    +  public function buildConfigurationForm(array $form, FormStateInterface $form_state) {
    +  }
    

    Same here. The corresponding submit method in this class isn't empty, so the build shouldn't be either.

  3. +++ b/core/modules/workflows/src/Plugin/WorkflowTypeTransitionFormBase.php
    @@ -0,0 +1,55 @@
    +  /**
    +   * {@inheritdoc}
    +   */
    +  public function buildConfigurationForm(array $form, FormStateInterface $form_state) {
    +  }
    

    And again

  4. +++ b/core/modules/workflows/tests/modules/workflow_type_test/src/Form/ComplexTestTypeStateForm.php
    @@ -0,0 +1,31 @@
    +  public function buildConfigurationForm(array $form, FormStateInterface $form_state) {
    ...
    +    $form = [];
    

    No need to empty out $form here

  5. +++ b/core/modules/workflows/tests/modules/workflow_type_test/src/Form/ComplexTestTypeTransitionForm.php
    @@ -0,0 +1,31 @@
    +  public function buildConfigurationForm(array $form, FormStateInterface $form_state) {
    ...
    +    $form = [];
    

    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.

Sam152’s picture

Status: Needs work » Needs review
FileSize
4.47 KB
62.45 KB

Addressing feedback in #55.

Status: Needs review » Needs work

The last submitted patch, 56: 2896722-56.patch, failed testing. View results

Sam152’s picture

Status: Needs work » Needs review
FileSize
16.61 KB
62.96 KB

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

diff --git a/core/modules/content_moderation/src/Form/ContentModerationStateForm.php b/core/modules/content_moderation/src/Form/ContentModerationStateForm.php
index 3ba5f65..57a4f4a 100644
--- a/core/modules/content_moderation/src/Form/ContentModerationStateForm.php
+++ b/core/modules/content_moderation/src/Form/ContentModerationStateForm.php
@@ -18,7 +18,7 @@ class ContentModerationStateForm extends WorkflowTypeStateFormBase {
    */
   public function buildConfigurationForm(array $form, FormStateInterface $form_state, StateInterface $state = NULL) {
     /** @var \Drupal\content_moderation\ContentModerationState $state */
-    $state = $form_state->get('state');
+    $state = $form_state->get(StateInterface::PLUGIN_FORM_KEY);
     $is_required_state = isset($state) ? in_array($state->id(), $this->workflowType->getRequiredStates(), TRUE) : FALSE;
 
     $form = [];
diff --git a/core/modules/workflows/src/Form/WorkflowStateEditForm.php b/core/modules/workflows/src/Form/WorkflowStateEditForm.php
index a04998d..4fdf8b7 100644
--- a/core/modules/workflows/src/Form/WorkflowStateEditForm.php
+++ b/core/modules/workflows/src/Form/WorkflowStateEditForm.php
@@ -99,7 +99,7 @@ public function form(array $form, FormStateInterface $form_state) {
         '#tree' => TRUE,
       ];
       $subform_state = SubformState::createForSubform($form['type_settings'], $form, $form_state);
-      $subform_state->set('state', $state);
+      $subform_state->set(StateInterface::PLUGIN_FORM_KEY, $state);
       $form['type_settings'] += $this->pluginFormFactory
         ->createInstance($workflow_type, StateInterface::PLUGIN_FORM_KEY)
         ->buildConfigurationForm($form['type_settings'], $subform_state);
@@ -176,7 +176,7 @@ public function validateForm(array &$form, FormStateInterface $form_state) {
 
     if ($workflow_type->hasFormClass(StateInterface::PLUGIN_FORM_KEY)) {
       $subform_state = SubformState::createForSubform($form['type_settings'], $form, $form_state);
-      $subform_state->set('state', $workflow_type->getState($this->stateId));
+      $subform_state->set(StateInterface::PLUGIN_FORM_KEY, $workflow_type->getState($this->stateId));
       $this->pluginFormFactory
         ->createInstance($workflow_type, StateInterface::PLUGIN_FORM_KEY)
         ->validateConfigurationForm($form['type_settings'], $subform_state);
@@ -193,7 +193,7 @@ public function save(array $form, FormStateInterface $form_state) {
 
     if ($workflow_type->hasFormClass(StateInterface::PLUGIN_FORM_KEY)) {
       $subform_state = SubformState::createForSubform($form['type_settings'], $form, $form_state);
-      $subform_state->set('state', $workflow_type->getState($this->stateId));
+      $subform_state->set(StateInterface::PLUGIN_FORM_KEY, $workflow_type->getState($this->stateId));
       $this->pluginFormFactory
         ->createInstance($workflow_type, StateInterface::PLUGIN_FORM_KEY)
         ->submitConfigurationForm($form['type_settings'], $subform_state);
diff --git a/core/modules/workflows/src/Plugin/WorkflowTypeStateFormBase.php b/core/modules/workflows/src/Plugin/WorkflowTypeStateFormBase.php
index 274ddd6..8d93922 100644
--- a/core/modules/workflows/src/Plugin/WorkflowTypeStateFormBase.php
+++ b/core/modules/workflows/src/Plugin/WorkflowTypeStateFormBase.php
@@ -7,6 +7,7 @@
 use Drupal\Core\Form\FormStateInterface;
 use Drupal\Core\Plugin\PluginFormInterface;
 use Drupal\Core\StringTranslation\StringTranslationTrait;
+use Drupal\workflows\StateInterface;
 
 /**
  * A base class for workflow type state forms.
@@ -40,7 +41,7 @@ public function validateConfigurationForm(array &$form, FormStateInterface $form
    */
   public function submitConfigurationForm(array &$form, FormStateInterface $form_state) {
     $values = $form_state->getValues();
-    $state = $form_state->get('state');
+    $state = $form_state->get(StateInterface::PLUGIN_FORM_KEY);
     $configuration = $this->workflowType->getConfiguration();
     $configuration['states'][$state->id()] = $values + $configuration['states'][$state->id()];
     $this->workflowType->setConfiguration($configuration);

Status: Needs review » Needs work

The last submitted patch, 58: 2896722-58.patch, failed testing. View results

tacituseu’s picture

Both (#56 and #58) failures are segfaults in garbage collector.

Sam152’s picture

Status: Needs work » Needs review
timmillwood’s picture

I'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.

tim.plunkett’s picture

Status: Needs review » Reviewed & tested by the community
  1. +++ b/core/modules/content_moderation/src/Form/ContentModerationConfigureForm.php
    @@ -0,0 +1,136 @@
    +          'label' => ['#markup' => '<strong>' . $this->t('@bundle types', ['@bundle' => $entity_type->getLabel()]) . '</strong>'],
    ...
    +            '#prefix' => '<br/><span id="selected-' . $entity_type->id() . '">',
    ...
    +            '#suffix' => '</span>',
    

    Not used to seeing markup concatenated like this anymore, could this instead use '#type' => 'inline_template'?

  2. +++ b/core/modules/content_moderation/src/Plugin/WorkflowType/ContentModeration.php
    @@ -375,68 +347,4 @@ public function getInitialState(WorkflowInterface $workflow, $entity = NULL) {
    -          'label' => ['#markup' => '<strong>' . $this->t('@bundle types', ['@bundle' => $entity_type->getLabel()]) . '</strong>'],
    -          'selected' => [
    -            '#prefix' => '<br/><span id="selected-' . $entity_type->id() . '">',
    -            '#markup' => !empty($selected_bundles) ? implode(', ', $selected_bundles) : $this->t('none'),
    -            '#suffix' => '</span>',
    

    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.

Sam152’s picture

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

Plugin forms are now defined in individual form classes referenced in the @WorkflowType annotation. These forms replace the methods that were previously implemented directly on the plugin type.

The workflows UI currently makes use of three forms; "configure", "state" and "transition". The new form classes should implement PluginFormInterface and may extend WorkflowTypeConfigureFormBase,
WorkflowTypeStateFormBase or WorkflowTypeTransitionFormBase respectively.

tim.plunkett’s picture

You linked this issue instead, I believe you meant #2898886: ContentModerationConfigureForm should use inline list in place of concatenated markup. Thanks for opening it!

Sam152’s picture

Bleh, good catch. Thanks.

  • larowlan committed f7f742e on 8.5.x
    Issue #2896722 by Sam152, Wim Leers, tim.plunkett, larowlan, timmillwood...
larowlan’s picture

Status: Reviewed & tested by the community » Fixed
  1. +++ b/core/modules/content_moderation/src/Form/ContentModerationConfigureForm.php
    @@ -0,0 +1,136 @@
    +              'title' => $this->t('Select'),
    

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

  2. +++ b/core/modules/workflows/src/Form/WorkflowTransitionAddForm.php
    @@ -128,6 +161,13 @@ public function validateForm(array &$form, FormStateInterface $form_state) {
    +    if ($workflow_type->hasFormClass(TransitionInterface::PLUGIN_FORM_KEY)) {
    
    @@ -136,6 +176,17 @@ public function validateForm(array &$form, FormStateInterface $form_state) {
    +    if ($workflow_type->hasFormClass(TransitionInterface::PLUGIN_FORM_KEY)) {
    

    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

  • larowlan committed 65b9f1f on 8.4.x
    Issue #2896722 by Sam152, Wim Leers, tim.plunkett, larowlan, timmillwood...
Sam152’s picture

plach’s picture

After running updates and rebuilding caches, I'm still getting some issues that seem related to this:

Sam152’s picture

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

larowlan’s picture

Working fine on HEAD for me @plach - see https://d2ghq.ply.st/ (23hrs to go).

Only local images are allowed.

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.

plach’s picture

I was just testing in my local dev env, I forgot we don't support an upgrade path yet for CM, sorry for the noise :)

Wim Leers’s picture

Issue tags: -Needs change record

Per #70.

Wim Leers’s picture

Status: Fixed » Closed (fixed)

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