Select entity type / bundle in workflow settings

Attached a new patch which is doing following;
1) Injected the EntityTypeManager & moderationInfo service
2) Used the #markup for entitytype labels instead of #checkboxes

// Naveen

As discussed, here is an approach that adds a tab to prevent clutter on the form. Tests can follow consensus.

+++ b/core/modules/content_moderation/src/Plugin/WorkflowType/ContentModeration.php
@@ -156,11 +207,61 @@ public function defaultConfiguration() {
+      '#title' => $this->t('Select types to use this workflow'),

its better to keep it shorter. "Select Entity types" ?

+++ b/core/modules/workflows/src/WorkflowTypeInterface.php
@@ -117,4 +117,24 @@ public function buildStateConfigurationForm(FormStateInterface $form_state, Work
+  /**
+   * Returns a form array to append to the Workflow entity edit form for
+   * workflow type specific settings.
+   *
+   * @param \Drupal\workflows\WorkflowInterface $workflow
+   *
+   * @return array
+   *
+   * @see \Drupal\workflows\Form\WorkflowTransitionEditForm::form()
+   */
+  public function editForm(WorkflowInterface $workflow);
+
+  /**
+   * Save workflow type specific settings.
+   *
+   * @param \Drupal\workflows\WorkflowInterface $workflow
+   * @param array $form
+   * @param \Drupal\Core\Form\FormStateInterface $form_state
+   */
+  public function editFormSave(WorkflowInterface &$workflow, array $form, FormStateInterface $form_state);

We don't need these extra functions now. We can move its functionality to WorkflowTypeSettings class.

@timmilliwood,
I really like a separate tab for settings b/c it's preventing form from cluttering. Could we get the @jojototh on #6 respective to UI/UX ?
// Naveen

I don’t think tabs would simplify it, it could event make it less intuitive to find the type settings.

But if people think the form would get too long, here are some suggestions:

• when creating workflow it could be a multi step form: 1/ configure states & transitions; 2/ configure types
• when editing - I would show on one page, but types settings could perhaps be collapsed by default
• alternatively (less preferred) it could be tabs once created, but the “Settings” tab label should be something easy to understand like “Select types”, not just "Settings"

At least for me its the bundle/entity type is never an afterthought. Instead I already know, I need to create a workflow for articles, which looks like this, and I know for videos, this workflow could be slightly simpler. Given that, separating this into two pages seems to make things harder. Maybe there are different kind of usecases.

Personally I like tabs, it feels to me like the Drupal pattern for "configure more stuff related to this primary thing you're looking at" with the states/transitions representing the meaty part of the interface and which entities they apply to as sort of supporting configuration. But that's just my 2c, will defer to any consensus to the contrary. I would be down for any of the three suggestions in #10, all sound reasonable.

A few extra things which might help decide:

1. There are likely to be a few more settings that will appear where this form appears as part of other issues in the queue.
2. Other types of workflows for other purposes will be able to define similar forms for additional configurations if required.

Also, just to be clear the suggestion of using tabs appeared in IRC. I'm more than happy to review and assist with moving forward patches based on #4, not stuck on it by any means.

In depth review of #4.

1. +++ b/core/modules/content_moderation/src/Plugin/WorkflowType/ContentModeration.php
   @@ -19,11 +23,58 @@
   +  public function __construct(array $configuration, $plugin_id, $plugin_definition, EntityTypeManager $entity_type_manager, ModerationInformationInterface $moderation_info) {
   

   EntityTypeManager should hint the interface.

2. +++ b/core/modules/content_moderation/src/Plugin/WorkflowType/ContentModeration.php
   @@ -19,11 +23,58 @@
   +   * The Moderation Information service.
   

   Moderation Information case should be lower.

3. +++ b/core/modules/content_moderation/src/Plugin/WorkflowType/ContentModeration.php
   @@ -19,11 +23,58 @@
   +  public function __construct(array $configuration, $plugin_id, $plugin_definition, EntityTypeManager $entity_type_manager, ModerationInformationInterface $moderation_info) {
   

   EntityTypeManager interface should be hinted.

4. +++ b/core/modules/content_moderation/src/Plugin/WorkflowType/ContentModeration.php
   @@ -156,11 +207,55 @@ public function defaultConfiguration() {
   +        $form['attachments'][$entity_type->id()] = [
   +          '#markup' => $entity_type->getLabel()->render(),
   +        ];
   +
   +        $bundles = $this->entityTypeManager->getStorage($entity_type->getBundleEntityType())->loadMultiple();
   +        foreach ($bundles as $bundle) {
   +          $form['attachments'][$entity_type->id()][$bundle->id()] = [
   +            '#type' => 'checkbox',
   +            '#title' => $bundle->label(),
   +            '#default_value' => $this->appliesToEntityTypeAndBundle($entity_type->id(), $bundle->id()),
   +          ];
   +        }
   

   Lets just combine these all into one #checkboxes element?

5. +++ b/core/modules/content_moderation/src/Plugin/WorkflowType/ContentModeration.php
   @@ -156,11 +207,55 @@ public function defaultConfiguration() {
   +  public function editFormSave(WorkflowInterface &$workflow, array $form, FormStateInterface $formState) {
   

   missing docblock.

6. +++ b/core/modules/content_moderation/src/Plugin/WorkflowType/ContentModeration.php
   @@ -156,11 +207,55 @@ public function defaultConfiguration() {
   +          $workflow->getTypePlugin()
   +            ->addEntityTypeAndBundle($entity_type_id, $bundle_id);
   ...
   +          $workflow->getTypePlugin()
   +            ->removeEntityTypeAndBundle($entity_type_id, $bundle_id);
   

   style thing, maybe getTypePlugin can be assigned to a variable and stop the chaining over two lines.

7. +++ b/core/modules/workflows/src/WorkflowTypeInterface.php
   @@ -117,4 +117,24 @@ public function buildStateConfigurationForm(FormStateInterface $form_state, Work
   +   * Returns a form array to append to the Workflow entity edit form for
   +   * workflow type specific settings.
   

   standards violation

8. +++ b/core/modules/workflows/src/WorkflowTypeInterface.php
   @@ -117,4 +117,24 @@ public function buildStateConfigurationForm(FormStateInterface $form_state, Work
   +   * @return array
   

   missing comment

9. +++ b/core/modules/workflows/src/WorkflowTypeInterface.php
   @@ -117,4 +117,24 @@ public function buildStateConfigurationForm(FormStateInterface $form_state, Work
   +   * @param \Drupal\workflows\WorkflowInterface $workflow
   +   * @param array $form
   +   * @param \Drupal\Core\Form\FormStateInterface $form_state
   

   missing comments

Looks like your last patch has some unwanted changes in it. Also just documenting the fact that we'll need a followup to restrict 1 workflow per bundle.

+++ b/core/modules/workflows/src/WorkflowTypeInterface.php
@@ -117,4 +117,29 @@ public function buildStateConfigurationForm(FormStateInterface $form_state, Work
diff --git a/sites/default/default.services.yml b/sites/default/default.services.yml

diff --git a/sites/default/default.services.yml b/sites/default/default.services.yml
old mode 100644
new mode 100755

One last sneaky one :)

Doing a detailed review of this, it looks like there is a pattern in core already for managing plugins that provide a configuration form. Attached is a patch which follows the existing pattern, specifically by:

• Introducing \Drupal\workflows\ConfigurableWorkflowTypeInterface which extends PluginFormInterface, WorkflowTypeInterface
• Introducing \Drupal\workflows\Plugin\ConfigurableWorkflowTypeBase which implements ConfigurableWorkflowTypeInterface
• Introducing SubformStateInterface to the form handling

I've uploaded the patch as well as just a workflows patch to make sure the test coverage for the form methods aren't implicit based on content_moderation existing and this is tested/validated and independent of CM.

The interface doesn't specify anything.

  /**
   * Form validation handler.
   *
   * @param array $form
   *   An associative array containing the structure of the plugin form as built
   *   by static::buildConfigurationForm().
   * @param \Drupal\Core\Form\FormStateInterface $form_state
   *   The current state of the form. Calling code should pass on a subform
   *   state created through
   *   \Drupal\Core\Form\SubformState::createForSubform().
   */
  public function validateConfigurationForm(array &$form, FormStateInterface $form_state);

  /**
   * Form submission handler.
   *
   * @param array $form
   *   An associative array containing the structure of the plugin form as built
   *   by static::buildConfigurationForm().
   * @param \Drupal\Core\Form\FormStateInterface $form_state
   *   The current state of the form. Calling code should pass on a subform
   *   state created through
   *   \Drupal\Core\Form\SubformState::createForSubform().
   */
  public function submitConfigurationForm(array &$form, FormStateInterface $form_state);

$key as "0" was falsey. tests++

1. +++ b/core/modules/content_moderation/src/Plugin/WorkflowType/ContentModeration.php
   @@ -156,11 +211,73 @@ public function defaultConfiguration() {
   +        if ($entity_type->getBundleEntityType()) {
   +          $bundles = $this->entityTypeManager->getStorage($entity_type->getBundleEntityType())
   +            ->loadMultiple();
   +          foreach ($bundles as $bundle) {
   +            if (!$this->moderationInfo->shouldModerateEntitiesOfBundle($entity_type, $bundle->id()) || $this->appliesToEntityTypeAndBundle($entity_type->id(), $bundle->id())) {
   +              $options[$bundle->id()] = $bundle->label();
   +              $defaults[$bundle->id()] = $this->appliesToEntityTypeAndBundle($entity_type->id(), $bundle->id());
   +            }
   +          }
   +        }
   +        else {
   +          if (!$this->moderationInfo->shouldModerateEntitiesOfBundle($entity_type, $entity_type->id()) || $this->appliesToEntityTypeAndBundle($entity_type->id(), $entity_type->id())) {
   +            $options[$entity_type->id()] = $entity_type->getLabel();
   +            $defaults[$entity_type->id()] = $this->appliesToEntityTypeAndBundle($entity_type->id(), $entity_type->id());
   +          }
   +        }
   

   There is no need to make a distinction between bundles that are stored in config entities and the ones that are not. We should simply use the entity_type.bundle.info service to get a list bundles for all entity types.

   Also, the second part of the condition hardcodes the bundle name to be the same as the entity type ID, but that's not necessarily the case.

   Even more, it doesn't account for bundles defined through hook_entity_bundle_info().

2. +++ b/core/modules/workflows/src/ConfigurableWorkflowTypeInterface.php
   @@ -0,0 +1,16 @@
   +interface ConfigurableWorkflowTypeInterface extends PluginFormInterface, WorkflowTypeInterface {
   
   +++ b/core/modules/workflows/src/WorkflowInterface.php
   @@ -281,7 +281,7 @@ public function deleteTransition($transition_id);
   -   * @return \Drupal\workflows\WorkflowTypeInterface
   +   * @return \Drupal\workflows\WorkflowTypeInterface|\Drupal\workflows\ConfigurableWorkflowTypeInterface
   

   Is there a use-case for a non-configurable workflow type plugin?

   Why don't we make WorkflowTypeInterface extend PluginFormInterface directly?

1. Good point, can fix.
2. I think we should actually go one step further and move all of the methods related to state and transition configuration forms to the new interface, but that's of course follow up territory. When configuring a plugin isn't essential, the most common pattern in core as far as I can see is to make it optional to implement. A workflow field, which has come up before, would probably store any configuration (if required) on the field instance for example.

Good clean up. UI with entity_test enabled, shows revisionable entity types right now.

The interface claims to handle this case for us:

 /**
   * @return array
   *   An array of bundle information where the outer array is keyed by the
   *   bundle name, or the entity type name if the entity does not have bundles.
   *   The inner arrays are associative arrays of bundle information, such as
   *   the label for the bundle.
   */
  public function getBundleInfo($entity_type);

FWIW I actually found it intuitive for my moderation workflow selection to be available on the content type operations. Does this patch also retain that functionality?

Thanks for the screenshots of the UI with Multiversion exposing more entity types, also. I think usability/design feedback would be good once this is ready for such review (either in this issue or possibly in a followup since it is experimental), because I find the list in the proposed UI a bit overwhelming.

This issue was created to implement feedback from the usability review in #2830584: Use modals for creating, updating, and deleting workflows, with a new DialogFormTrait.

Thanks @Sam152. Tagging for usability review then (to see if others also find it overwhelming or if there are improvements we could make), and adding the screenshot to the summary to help that review.

Still also would like to know #41; can we add whether or not that part of the UI is changed as explicit info in the summary?

Related to #38 / #39, @alexpott has also already filed #2850353: Test content_moderation with a non-bundleable content entity based on discussion in #2830581: Fix ContentModeration workflow type to calculate correct dependencies.

Adding the more realistic screenshot instead.

IMHO conceptually the closest thing to Workflow in core is a Vocabulary, yet there is no checkbox wall on its edit page, in my eyes it is anti-pattern and makes as much sense as pressing php devs to replace 'implements' and 'extends' keywords with 'implementedby' and 'extendedby' :)

TLDR: define relationships on the container not on contained.

I guess something to consider is that there are a few other issues blocked on having the plugin having a configuration form, which this patch provides. Maybe that should be split out from moving the bundle selection to the workflow edit form, as I think it's important implementors of the workflow type plugin have the option to use PluginFormInterface.

One thing which the issue summary doesn't really mention is, this allows us add moderation to entities without a config entity based bundles, currently that's only possible programatically.

Right now the whole UI is blocked on a $entity_type->getBundleEntityType() check. This patch at least puts more entity types on a level playing field, which is great IMO.

Wishful thinking, but based on this being the result of a usability review, maybe making this less overwhelming is follow-up territory? As mentioned in #48 PluginFormInterface is useful beyond this issue.

How is this list organised? Its not alphabetical or priority?

I would at least have content on top.

This is looking really good. Closest we've come to the original mockups and a really nifty way of selecting entities/bundles. Will leave the usability review tag for others to have a look at the functionality.

Feedback on the code as follows:

1. +++ b/core/modules/content_moderation/content_moderation.module
   @@ -251,3 +243,13 @@ function content_moderation_workflow_insert(WorkflowInterface $entity) {
   +  if (isset($entity_types['workflow'])) {
   

   Do we need this? We have a dependency on workflows, so it should always be there.

2. +++ b/core/modules/content_moderation/content_moderation.module
   @@ -251,3 +243,13 @@ function content_moderation_workflow_insert(WorkflowInterface $entity) {
   +    $entity_types['workflow']->setFormClass('edit-type', WorkflowTypeEditForm::class);
   
   +++ b/core/modules/content_moderation/content_moderation.routing.yml
   @@ -0,0 +1,7 @@
   +entity.workflow.edit_type_form:
   +  path: '/admin/config/workflow/workflows/manage/{workflow}/type/{entity_type}'
   +  defaults:
   +    _entity_form: 'workflow.edit-type'
   +    _title: 'Add / remove type'
   +  requirements:
   +    _entity_access: 'workflow.edit'
   

   Why bother trying to make this part of the entity form handlers? Can't we just make this a custom route/controller. I don't love shoe-horning this onto the entity type where we don't have to and when it's not a generic feature of the entity type in question, just a specific requirement for one of many possible plugin types. It sets the tone for how other modules are going to interact with workflows, and by using "edit-type", we encourage that same pattern.

   Why not an unrelated route ID called "content_moderation.entity_type_select_form"?

3. +++ b/core/modules/content_moderation/content_moderation.routing.yml
   @@ -0,0 +1,7 @@
   \ No newline at end of file
   

   Newline.

4. +++ b/core/modules/content_moderation/src/Form/WorkflowTypeEditForm.php
   @@ -0,0 +1,115 @@
   +class WorkflowTypeEditForm extends EntityForm {
   

   This is possibly a good candidate for a dedicated form kernel test.

5. +++ b/core/modules/content_moderation/src/Form/WorkflowTypeEditForm.php
   @@ -0,0 +1,115 @@
   +  /**
   +   * @var
   +   */
   +  protected $workflow;
   

   Missing hints

6. +++ b/core/modules/content_moderation/src/Form/WorkflowTypeEditForm.php
   @@ -0,0 +1,115 @@
   +    foreach (\Drupal::service('entity_type.bundle.info')->getBundleInfo($this->entityType->id()) as $bundle_id => $bundle) {
   

   Can inject these services.

7. +++ b/core/modules/content_moderation/src/Form/WorkflowTypeEditForm.php
   @@ -0,0 +1,115 @@
   +      if (!\Drupal::service('content_moderation.moderation_information')->shouldModerateEntitiesOfBundle($this->entityType, $bundle_id)
   +        || $this->workflow->getTypePlugin()->appliesToEntityTypeAndBundle($this->entityType->id(), $bundle_id)) {
   

   This is a bit hard to follow. Maybe some variables assigned to each condition would make it more readable.

8. +++ b/core/modules/content_moderation/src/Form/WorkflowTypeEditForm.php
   @@ -0,0 +1,115 @@
   +  public function submitForm(array &$form, FormStateInterface $form_state) {
   +    foreach ($form_state->getValue('bundles') as $bundle_id => $checked) {
   +      if ($checked) {
   +        $this->workflow->getTypePlugin()->addEntityTypeAndBundle($this->entityType->id(), $bundle_id);
   +      }
   +      else {
   +        $this->workflow->getTypePlugin()->removeEntityTypeAndBundle($this->entityType->id(), $bundle_id);
   +      }
   +    }
   +    $this->workflow->save();
   +  }
   

   It's kind of interesting that the workflow is updated as soon as you close the modal. My initial instinct and what I've seen of core elsewhere, when ajaxy stuff like this returns, you still have to click save on the parent form to persist it. Will see what the UX review says.

9. +++ b/core/modules/content_moderation/src/Form/WorkflowTypeEditForm.php
   @@ -0,0 +1,115 @@
   +   * @param array $form
   +   * @param \Drupal\Core\Form\FormStateInterface $form_state
   ...
   +   * @return \Drupal\Core\Ajax\AjaxResponse
   

   No comments on hints.

10. +++ b/core/modules/content_moderation/src/Plugin/WorkflowType/ContentModeration.php
    @@ -172,6 +206,9 @@ public function appliesToEntityTypeAndBundle($entity_type_id, $bundle_id) {
    +    if (!isset($this->configuration['entity_types'][$entity_type_id])) {
    +      return;
    +    }
    

    Maybe one of the unit tests can be extended to test this.

11. +++ b/core/modules/content_moderation/src/Plugin/WorkflowType/ContentModeration.php
    @@ -289,4 +326,74 @@ public function getConfiguration() {
    +              'url' => Url::fromRoute('entity.workflow.edit_type_form', ['workflow' => $form_state->getBuildInfo()['callback_object']->getEntity()->id(), 'entity_type' => $entity_type->id()]),
    

    Lets just pass the entity in as a parameter to the plugins in WorkflowEditForm.

12. +++ b/core/modules/content_moderation/src/Plugin/WorkflowType/ContentModeration.php
    @@ -289,4 +326,74 @@ public function getConfiguration() {
    +    if (isset($form['attachments']['node'])) {
    +      $form['attachments'] = ['node' => $form['attachments']['node']] + $form['attachments'];
    +    }
    

    What does this do? Comment maybe?

Here I followed the pattern @alexpott set with editing states and transitions. It also seemed a good way to keep this coupled to the workflow entity, rather than being an arbitrary route/controller.

Well, it associates the route to the entity type, which I don't think is correct in this case. It's only a feature of the one specific plugin, not something all workflows can make use of. Using the entity form handlers inside workflows for states and transitions makes sense because those are features of the entity type itself.

Aren't we going to face a similar issue no matter how the route is implemented?

Not really, the difference is that route wouldn't be associated to the workflow entity as a form handler.

Right now the content_moderation module is occupying the "edit-type" form handler. Where does that leave another plugin type who might want something similar? "edit-type" is taken, do they prefix their handler with the module or plugin name type or come up with some other keyword?

I feel like the expectation from entity API is that when you are dealing with an entities form handlers, they are routes based on the operations connected to that specific entity type, which isn't true in this case.

Slight adjustment to give a bit more context on the modal. The select all label now shows the name of the entity type.
Also changed the title to be Add / remove types so it matches the button that the user just clicked.

+++ b/core/modules/content_moderation/src/Form/WorkflowTypeEditForm.php
@@ -101,11 +101,12 @@ public function buildForm(array $form, FormStateInterface $form_state, $workflow
+

Apologies for the extra line... :S

Here is another scan over the code. Would be keen to see an updated usability review as we get closer on the code side.

1. +++ b/core/modules/content_moderation/content_moderation.module
   @@ -7,6 +7,7 @@
   +use Drupal\content_moderation\Form\WorkflowTypeEditForm;
   

   This isn't used anywhere in the file.

2. +++ b/core/modules/content_moderation/content_moderation.routing.yml
   @@ -0,0 +1,7 @@
   +    _title: 'Add / remove types'
   

   Can we come up with a more natural title for this? "Add or remove types"? I've not seen the patter of "This / that" text strings in core.

3. +++ b/core/modules/content_moderation/src/Form/WorkflowTypeEditForm.php
   @@ -0,0 +1,170 @@
   +  /**
   +   * The workflow entity object.
   +   *
   +   * @var \Drupal\workflows\WorkflowInterface
   +   */
   +  protected $workflow;
   ...
   +  /**
   +   * {@inheritdoc}
   +   */
   +  public function buildForm(array $form, FormStateInterface $form_state, $workflow = NULL, $entity_type = NULL) {
   +    $this->workflow = $this->entityTypeManager->getStorage('workflow')->load($workflow);
   

   If we hint WorkflowInterface for the $workflow param, we don't have to load it. Routing will sort it out for us.

4. +++ b/core/modules/content_moderation/src/Form/WorkflowTypeEditForm.php
   @@ -0,0 +1,170 @@
   +      $moderation_enabled = $this->moderationInformation->shouldModerateEntitiesOfBundle($this->entityType, $bundle_id);
   +      $workflow_moderation_enabled = $this->workflow->getTypePlugin()->appliesToEntityTypeAndBundle($this->entityType->id(), $bundle_id);
   +      if (!$moderation_enabled || $workflow_moderation_enabled) {
   

   Do these two methods actually check the same thing? I don't really understand the difference between them at a glance.

   edit: Ah, looks like one is workflow specific and one is global. Confusing!

5. +++ b/core/modules/content_moderation/src/Form/WorkflowTypeEditForm.php
   @@ -0,0 +1,170 @@
   +        'callback' => [$this, 'ajaxcallback'],
   ...
   +        'callback' => [$this, 'ajaxcallback'],
   ...
   +  /**
   +   * Ajax callback.
   +   *
   +   * @return \Drupal\Core\Ajax\AjaxResponse
   +   *   An ajax response object.
   +   */
   +  public function ajaxcallback() {
   +    $selected_bundles = [];
   +    foreach (\Drupal::service('entity_type.bundle.info')->getBundleInfo($this->entityType->id()) as $bundle_id => $bundle) {
   

   Should this be [static::class, ''] instead? The use of \Drupal kind of implies it was meant to be static, otherwise we should the injected bundle info service.

6. +++ b/core/modules/workflows/tests/modules/workflow_type_test/src/Plugin/WorkflowType/ComplexTestType.php
   @@ -82,10 +82,30 @@ public function buildTransitionConfigurationForm(FormStateInterface $form_state,
   -  public function onDependencyRemoval(array $dependencies) {
   -    // Always return TRUE to allow the logic in
   -    // \Drupal\workflows\Entity\Workflow::onDependencyRemoval() to be tested.
   -    return TRUE;
   

   Why was this removed?

7. +++ b/core/modules/content_moderation/src/Form/WorkflowTypeEditForm.php
   @@ -0,0 +1,170 @@
   +
   +
   

   nit, double space

8. +++ b/core/modules/content_moderation/src/Form/WorkflowTypeEditForm.php
   @@ -0,0 +1,170 @@
   +        'callback' => [$this, 'ajaxcallback'],
   ...
   +        'callback' => [$this, 'ajaxcallback'],
   ...
   +  public function ajaxcallback() {
   

   Should be ajaxCallback, but maybe we can describe what it's doing. ajaxUpdateSelectedBundlesText?

9. +++ b/core/modules/content_moderation/src/Form/WorkflowTypeEditForm.php
   @@ -0,0 +1,170 @@
   +   * Ajax callback.
   

   Lets put some better docs on this.

10. +++ b/core/modules/content_moderation/src/Plugin/WorkflowType/ContentModeration.php
    @@ -289,4 +326,70 @@ public function getConfiguration() {
    +              'url' => Url::fromRoute('entity.workflow.edit_type_form', ['workflow' => $form_state->getBuildInfo()['callback_object']->getEntity()->id(), 'entity_type' => $entity_type->id()]),
    

    My point was, this is a hugely indirect way of getting the workflow entity. Lets set it on the sub-form state or add it as a parameter to buildConfigurationForm method to make this more straight forward.

11. +++ b/core/modules/workflows/src/Form/WorkflowEditForm.php
    @@ -180,16 +182,44 @@ public function form(array $form, FormStateInterface $form_state) {
    +      $sub_form_state = SubformState::createForSubform($form['type_settings'], $form, $form_state);
    ...
    +      $sub_form_state = SubformState::createForSubform($form['type_settings'], $form, $form_state);
    ...
    +      $sub_form_state = SubformState::createForSubform($form['type_settings'], $form, $form_state);
    

    I think this was probably me, but looks like subform is being used as one word, this should probably be $subform_state.

12. +++ b/core/modules/content_moderation/src/Form/WorkflowTypeEditForm.php
    @@ -0,0 +1,170 @@
    +    $this->entityType = $this->entityTypeManager->getDefinition($entity_type);
    

    I'm guessing this fatals if we pass some gibberish into this param. Perhaps we can validate this in the routing system somehow.

Having a go at #67

Fixed #67.1

Fixed #67.2 (also on core/modules/content_moderation/src/Plugin/WorkflowType/ContentModeration.php)

Tried doing #67.3, but if we do that we get this error:
Error: Call to a member function getTypePlugin() on null in Drupal\content_moderation\Form\WorkflowTypeEditForm->buildForm()
If we don't set a default value of null on the $workflow parameter, then buildForm is not compatible with Drupal\Core\Form\FormInterface::buildForm. Same goes for $entity_type.

Fixed #67.7

Fixed #67.8 as far as naming it ajaxUpdateSelectedBundlesText instead... I'm no expert in naming things, but what if its functionality changes sometime from now? Rather fix that in the comment of the function by being more descriptive? (as mentioned in #67.9).

Fixed #67.11

RE: #67.12 See my response to #67.3 above.

So, left to do:

• Review my comments above on #67.3 and 12
• Review/answer #67.4
• Review/answer #67.5
• Review/answer #67.6
• Fix #67.9
• Fix #67.10

Still waiting for a usability review, but code looks great +1 from me.

1. +++ b/core/modules/content_moderation/src/Form/WorkflowTypeEditForm.php
   @@ -0,0 +1,181 @@
   +      // Check if moderation is enabled for this bundle on any workflow.
   +      $moderation_enabled = $this->moderationInformation->shouldModerateEntitiesOfBundle($this->entityType, $bundle_id);
   +      // Check if moderation is enabled for this bundle on this workflow.
   +      $workflow_moderation_enabled = $this->workflow->getTypePlugin()->appliesToEntityTypeAndBundle($this->entityType->id(), $bundle_id);
   +      // Only show bundles that are not enabled anywhere, or enabled on this
   +      // workflow.
   

   Makes way more sense with the comments. Maybe a follow up should be to tidy up these method names.

2. +++ b/core/modules/content_moderation/src/Plugin/WorkflowType/ContentModeration.php
   @@ -289,4 +325,70 @@ public function getConfiguration() {
   +  public function buildConfigurationForm(array $form, FormStateInterface $form_state, WorkflowInterface $workflow = null) {
   ...
   +              'url' => Url::fromRoute('workflow.update_entity_type_form', ['workflow' => $workflow->id(), 'entity_type_id' => $entity_type->id()]),
   

   This looks great now.

Is the screen still up to date for review?

Actually the screen on #65 now reads "Add or remove types"

On the workflow overview screen:

1. On the fieldset label: avoid using "entity" in UI texts. I would suggest something that's more descriptive of what you can configure here. Not too happy yet with this version that ends with a colon, but something like it?

2. "Items" is also not super nice but I think it would be good to be explicit in listing Custom block *types*, Content *types. Maybe it can stay "Types" afterall.

3. For the buttons: why not simply use "select" instead of add/remove?

4. The "loading" spinner is not shown in the right place

On the modal window

5. Again, trying to come up with a more descriptive title. Maybe it should repeat the "select" word. "Select the [entity label?] types for this workflow" ?

Oh, and: I tried to come up with other solutions for the general pattern of providing access to (potentially long) lists of entity types and didn't find anything better (that wouldn't be very custom), so lets go ahead with this table + dialog window approach.

Excellent points @yoroy, thanks!

Having a go at this...

Fixed #79.1, #79.2 and #79.3

About #79.5: For the title of the form page (modal) itself, we'll need to define a _title_callback since it'd need to be dynamic. Where should we put this? We've got no controller on content_moderation yet. In case we decide not to do this, I've changed it to say 'Select the types for this workflow'

About #79.4, the spinner not showing in the right place, this is happening because its div is being inserted inside the <li class="add dropbutton-action"> which holds the link for the button. I couldn't figure out how to fix it apart from adding custom CSS. Is this a UI bug in core? it should work out of the box in Seven & Bartik themes

Also fixed along the way:

• The Details render element only has two properties, #title and #open. Removed property #collapsible from ContentModeration::buildConfigurationForm
• The Table render element has no #title property, removed it from ContentModeration::buildConfigurationForm

RE: #80:

Oh, and: I tried to come up with other solutions for the general pattern of providing access to (potentially long) lists of entity types and didn't find anything better (that wouldn't be very custom), so lets go ahead with this table + dialog window approach.

I believe we have actually already solved this in the block layout page (see the Place block buttons on admin/structure/block). The height of the modal is managed dynamically by the JS if we remove the height from the data-dialog-options attribute, so I have done that in this patch.

Adding the title callback to \Drupal\content_moderation\Form\WorkflowTypeEditForm.

I just thought we could just add the workflow label to the title as well, so the user looses even less context. It also makes the non-modal version of this form more clear as to what you're doing.

Simplifying getTitle based on Tim's feedback on slack...

Nice! This is shaping up very well.

The patch looks generally good to me as well, here's a small review:

1. +++ b/core/modules/content_moderation/content_moderation.routing.yml
   @@ -0,0 +1,7 @@
   +workflow.update_entity_type_form:
   
   +++ b/core/modules/content_moderation/src/Form/WorkflowTypeEditForm.php
   @@ -0,0 +1,188 @@
   +  public function getFormId() {
   +    return 'workflow_type_edit_form';
   +  }
   

   Shouldn't these two machine names be more in sync with each other?

2. +++ b/core/modules/content_moderation/src/Form/WorkflowTypeEditForm.php
   @@ -0,0 +1,188 @@
   +    $options = [];
   +    $defaults =[];
   

   Missing a space on the second line, but OTOH, we could also put both on the same line, i.e. $options = $defaults = [];

3. +++ b/core/modules/content_moderation/src/Form/WorkflowTypeEditForm.php
   @@ -0,0 +1,188 @@
   +          'type' => $this->t('All @entity_type types', ['@entity_type' => $this->entityType->getLabel()]),
   

   We have EntityTypeInterface::getPluralLabel() and we can use it here.

4. +++ b/core/modules/content_moderation/src/Plugin/WorkflowType/ContentModeration.php
   @@ -289,4 +325,67 @@ public function getConfiguration() {
   +        'type' => [
   +            'label' => ['#markup' => $this->t('%bundle types', ['%bundle' => $entity_type->getLabel()])],
   

   Two extra spaces here for 'label' :)

5. +++ b/core/modules/workflows/src/ConfigurableWorkflowTypeInterface.php
   @@ -0,0 +1,16 @@
   +interface ConfigurableWorkflowTypeInterface extends PluginFormInterface, WorkflowTypeInterface {
   
   +++ b/core/modules/workflows/src/Form/WorkflowEditForm.php
   @@ -180,16 +182,44 @@ public function form(array $form, FormStateInterface $form_state) {
   +    $is_configurable = $workflow->getTypePlugin() instanceof ConfigurableWorkflowTypeInterface;
   

   This is a bit confusing because WorkflowTypeInterface already extends ConfigurablePluginInterface, so the workflow type is.. "extra" configurable? :)

6. +++ b/core/modules/workflows/src/Form/WorkflowEditForm.php
   @@ -180,16 +182,44 @@ public function form(array $form, FormStateInterface $form_state) {
   +    $form_state->set('is_configurable', $is_configurable);
   ...
   +    if ($form_state->get('is_configurable')) {
   ...
   +    if ($form_state->get('is_configurable')) {
   

   Can't we simply check for the interface in validateForm() and submitForm() instead of setting a form state value?

I think these are my final nitpicks!

1. I suggested it myself, but on testing the "Applies to:" seemed quite terse and maybe a little bit cryptic still.
2. Lets simplify the styling a bit, and show the "types" labels bold (strong)
3. And the selected types in italics (em)
4. If we show "none" when none are selected we can drop the "Selected:"

Is the followup for the wrong positition of the spinner in place?

Thanks @amateescu && @yoroy for the reviews! Made some more progress:

Re: #89.3
+++ b/core/modules/content_moderation/src/Form/WorkflowTypeEditForm.php
@@ -0,0 +1,188 @@
+ 'type' => $this->t('All @entity_type types', ['@entity_type' => $this->entityType->getLabel()]),
We have EntityTypeInterface::getPluralLabel() and we can use it here.

This actually doesn't make sense in this case, we want to show "ALL CONTENT TYPES", not "ALL CONTENT ITEMS TYPES" ;)

Done #89.1, #89.2, #89.4

Done #90.1, #90.2, #90.3, #90.4

Left to do: #89.5 & #89.6

we could rename ConfigurableWorkflowTypeInterface to WorkflowTypeWithFormInterface, but not sure what else we can do that'd make sense.

My suggestion is to not have the new interface at all and just make \Drupal\content_moderation\Plugin\WorkflowType\ContentModeration implement PluginFormInterface directly, and use that generic plugin form interface for the if statements in WorkflowEditForm.

@Manuel Garcia:

This actually doesn't make sense in this case, we want to show "ALL CONTENT TYPES", not "ALL CONTENT ITEMS TYPES" ;)

Then it looks to me that we actually want to use the plural label of the bundle entity type, no? "ALL CONTENT TYPES" makes sense for node, because its bundles are actually named "content types", but if we look at Taxonomy for instance: "All vocabulary types" doesn't make sense at all, it should be "All vocabularies".

There is some precedent in core for having a dedicated interface. See image effect system:

/**
 * Defines the interface for configurable image effects.
 *
 * @see \Drupal\image\Annotation\ImageEffect
 * @see \Drupal\image\ConfigurableImageEffectBase
 * @see \Drupal\image\ImageEffectInterface
 * @see \Drupal\image\ImageEffectBase
 * @see \Drupal\image\ImageEffectManager
 * @see plugin_api
 */
interface ConfigurableImageEffectInterface extends ImageEffectInterface, PluginFormInterface {
}

Personally it feels a bit less "magic" because the interface is a specific indication that the type of plugin you are implementing actually supports configuration. In addition to this it allows you to hint the configurable interface where appropriate. If you had a context that required a configurable plugin and a workflow type plugin, how would you hint it?

@Sam152,

Personally it feels a bit less "magic" because the interface is a specific indication that the type of plugin you are implementing actually supports configuration.

The confusing part that I was referring to in #89.5 is that the term 'configurable' is used for two different things now: one is that the plugin implements \Drupal\Component\Plugin\ConfigurablePluginInterface, so it has its three methods: getConfiguration(), setConfiguration() and defaultConfiguration(), but the new interface proposed here actually makes the plugin "UI configurable" (i.e. in a form), but still uses the generic 'Configurable' name.

If you had a context that required a configurable plugin and a workflow type plugin, how would you hint it?

See, that's the problem. As mentioned above, there are two different things that we're conflating into the 'configurable' term. I would argue that workflow types are already configurable (because they implement ConfigurablePluginInterface), so I hope this makes it a bit more clear why your question (and the new interface) can be seen as very ambiguous :)

Yeah, really good points re: the double up in vocabulary for the word "configurable". Makes you wonder why ConfigurableImageEffectInterface wasn't called ImageEffectFormInterface instead, so maybe we should do that and call ours WorkflowTypeFormInterface.

I would argue that workflow types are already configurable (because they implement ConfigurablePluginInterface)

My opinion would be that we should not directly implement this interface. Lets not assume future workflow plugins are going to need forms. We're aiming for workflows to potentially be a low level building block of any number of other subsystems after all.

Yep, point taken. Naming things is hard.

If we really want the new interface, then I agree that WorkflowTypeFormInterface would be a better name for it.

However, getting to the meat of your question (emphasis addition mine):

If you had a context that required a UI configurable plugin and a workflow type plugin, how would you hint it?

I would hint it with the existing WorkflowTypeInterface and do an instanceof check for PluginFormInterface where it's needed, like we also do in the latest patch.

The only other thing that would push me in the direction of an additional interface is some extra discoverability around the options you have available to you when implementing the plugin type. Otherwise, you're right PluginFormInterface can be detected in whatever circumstance requires it.

Addressing #98.

I think right now the expected DX is that if an interface has a impact on how a plugin behaves, the interface exists in the format of "$pluginType$extraBehaviorName". I can't find any examples of non-base plugin classes implementing PluginFormInterface directly, core or contrib.

Current patch does s/ConfigurableWorkflowTypeInterface/WorkflowTypeFormInterface/.

@timmillwood raised the point that we could essentially merge WorkflowTypeBase and WorkflowTypeFormBase. I think it makes sense to keep them separate, but would be open exploring this.

Based on testing the patch in #100:

Lets not use the word "entities" in user interface text, so "Content types" and "Custom block types"

There's an <em class="placeholder"></em> that makes the entity labels show in italics. I don't know what the placeholder class is for but I'd rather not have those italics there.

We should also fix the misplaced loading spinner pointed out in #79. Since we're in the business of stabilizing ideally in this issue and not a followup :)

@Sam152, my preference would also be to merge WorkflowTypeFormBase into WorkflowTypeBase but I won't fuss too much over it. Otherwise, the changes in #102 look great!

@yoroy, the word 'entities' appears in the latest iterations of the patch because we're now using the proper API for getting the plural label of an entity type, but #2702683: Add plural labels to entity types is not done yet so what is shown is the generic plural label. Once that issue get is, we will also have the proper labels here.

So the latest patch looks good, but putting to NW for fixing the spinner and the italics issue pointed out in #103.

Removing the <em></em> tags, and tackling the issue with the throbber.

About the throbber, I have done this by patching a css file in seven theme, which I'm not sure would be in scope for this issue - however it should also prevent this on any other similar buttons in seven. In any case there could be other better solutions so any feedback would be welcome :)

So with this patch it looks like this:

On point in this patch that I don't think has been mentioned before is the fact we're moving the ability to add moderation to an entity from the 'administer content moderation' permission to 'administer workflows'. If we wanted to maintain 'administer content moderation', the form in the plugin could have some access controls wrapped around it and the modal route could also require that permission.

For now, that was the only responsibility 'administer content moderation' had, so can it be removed?

+++ b/core/modules/content_moderation/content_moderation.routing.yml
@@ -0,0 +1,7 @@
+workflow.type_edit_form:

I think this route should also be called "content_moderation.workflow_type_edit_form".

I think #109 makes sense, here it is.

Apologies, I haven't been clear enough about which italics to remove. I meant the <em class="placeholder">Custom block</em>types bit.

Looking again this is quite comparable to how we show summaries in vertical tabs. So I would suggest no italics at all:

Sorry for the confusion.

No worries @yoroy thanks for being thorough!
Addressing #112. No italics now =)

Let's try again...

Reviewed the patch again and found just a minor thing, we have to do the plural entity label dance from #100 for the title of the dialog as well.

Here's a quick update for that and I think this is ready to go.

#116interdiff looks ok, good catch @amateescu

Simple reroll of #116, these files were moved:

• core/modules/content_moderation/src/Tests/ModerationStateBlockTest.php
• core/modules/content_moderation/src/Tests/ModerationStateNodeTest.php
• core/modules/content_moderation/src/Tests/ModerationStateNodeTypeTest.php

Just hit a problem rolling out content_moderation a new site on 8.3. Had to manually edit the workflow config to start moderating an entity type without a bundle.

+1 for RTBC.

To be clear: this is ready to go in from my perspective (which is user interface design and product management (provisional)) :)

+1 for RTBC Looking good from the design perspective!

Updating commit credit. I added the answer to #41 / #44 to the issue summary.

First of all, the expanded test coverage to cover more combinations of custom settings with assigned workflows looks good :) There are also some unrelated code style fixes, which I don't feel strong about, so IMHO they can stay.

When looking at the issue summary, the screenshot presented was not a very compelling UI improvement. However the patch does not actually use the list of checkboxes from the summary. So needs a summary update first.

This is also a sizable UI change, so a change notice to inform people on the new way would be important IMHO.

One minor code find:

+++ b/core/modules/content_moderation/src/Plugin/WorkflowType/ContentModeration.php
@@ -38,11 +42,38 @@ class ContentModeration extends WorkflowTypeBase implements ContainerFactoryPlug
+   * Constructs an ContentModeration object.

Should be "a".

Also a user interaction problem. When I started off selecting bundles in the modal, it highlighted changes yellow. Cool. So I knew which ones I enabled (which at the start equaled all the ones I had checked). Next when I went back and *disabled* stuff, it did not highlight as yellow. So apparently only ones that are *newly* checked are ever highlighted as yellow. Is this intentional? We usually use this affordance to signal which things changed in general from when the form was loaded.

Updated the change record to cover permissions and that the config storage did not change: https://www.drupal.org/node/2875643/revisions/view/10469280/10469298

Interdiff LGTM.

I don't think the yellow marking makes sense with this solution, it just serves to amplify the checkbox. All rows where the checkbox is checked are yellow and none of the rows where the checkbox is not checked are yellow. So when you load a form (even before making any changes), rows where the checkbox is checked are yellow. After making changes, rows that are at that point checked will be yellow. We definitely don't do this anywhere, we use this yellow affordance to highlight *changes* made when editing the form. I'd rather we not try to do this than to use the same affordance for something else entirely.

I don't think patching classy is going to work... and we can't guaranteee turning off this behaviour otherwise since people can use whatever theme they want for the admininstration pages.

In my opinion we should do this as a follow up, and patch the tableselect element to allow people to disable the hightlight behaviour properly.

Rerolled #136, conflict on core/modules/content_moderation/src/Plugin/WorkflowType/ContentModeration.php

I'll have a thought about my last comment and keep you posted. Perhaps we can already use the js_select option on our element and thats it.

We are not allowed to make changes to Classy. Removing the highlight color in Seven is fine. Could we also move adding the no-highlight class to Seven? Core and its modules should only ship with functional classes which I don't think this is.

OK indeed to disable the selected background behavior all we need to do is set the #js_select option to false when using the Tableselect element. So the follow up i was talking about on #137 is already done essentially. <3 Drupal

Attached patch does that and removes the need for the extra class and the patching of classy.

Note that with my patch on #140 we loose the select all checkbox, so that is not ideal either.

Patching just Seven theme would mean that if people used a different admin theme, then the no-highlight added on #138 would not be accounted for (unless the custom admin does account for it of course).

So I don't know which path we should take, as I see it we have 3 options:

1. Add the no-highlight css class and patch Seven. Add this to the CR and hope other admin themes pick up on it.
2. Disable the js_select option from the Tableselect element, but loose the select all checkbox.
3. Use js_select as is, where any selected row is highlighted.