Problem/Motivation

In #2830584: Review the workflow UI it was suggested to select in the workflow entity form which entity types / bundles the workflow should work with.

Proposed resolution

Add entity type checkboxes to the workflow form.

Remaining tasks

User interface changes

New section is added to the UI:

API changes

Data model changes

Files: 
CommentFileSizeAuthor
#116 interdiff.txt1.26 KBamateescu
#116 2843083-116.patch49.98 KBamateescu
#113 interdiff.txt1.03 KBManuel Garcia
#113 2843083-113.patch49.6 KBManuel Garcia
#112 no-italics.png25.87 KByoroy
#112 italics.png101.01 KByoroy
#110 interdiff.txt1.52 KBManuel Garcia
#110 2843083-110.patch49.6 KBManuel Garcia
#108 interdiff.txt3.19 KBSam152
#108 2843083-108.patch49.56 KBSam152
#106 Screenshot from 2017-04-20 16-21-13.png19.04 KBManuel Garcia
#106 interdiff.txt1.49 KBManuel Garcia
#106 2843083-106.patch47.61 KBManuel Garcia
#102 2843083-102.patch47.16 KBSam152
#102 interdiff.txt7.07 KBSam152
#100 Screenshot from 2017-04-12 11-14-14.png18.77 KBManuel Garcia
#100 interdiff.txt1.14 KBManuel Garcia
#100 2843083-100.patch47.33 KBManuel Garcia
#92 2843083-92.patch47.02 KBtimmillwood
#92 interdiff-2843083-92.patch1.97 KBtimmillwood
#91 Screenshot from 2017-04-11 13-57-47.png11.35 KBManuel Garcia
#90 applies-to.png56.69 KByoroy
#85 Screenshot from 2017-04-10 11-50-41.png20.56 KBManuel Garcia
#84 Screenshot from 2017-04-10 11-41-18.png18.89 KBManuel Garcia
#79 select-entity-types-3.png35.68 KByoroy
#79 select-entity-types-2.png21.3 KByoroy
#79 Select-entity-types.png27.81 KByoroy
#65 add-remove-types.png20.72 KBManuel Garcia
#58 Screenshot from 2017-03-17 15-44-25.png70.18 KBtimmillwood
#55 interdiff-2843083-55.txt5.55 KBtimmillwood
#22 Screenshot from 2017-02-01 15-19-42.png87.59 KBtimmillwood

Comments

timmillwood created an issue. See original summary.

timmillwood’s picture

Status: Active » Needs review
FileSize
6.81 KB
102.46 KB

In this patch we add an editForm() and editFormSave() method to the workflow plugin.

This could just been done with hook_form_alter() and hook_entity_presave(), not really sure what would be better.

Status: Needs review » Needs work

The last submitted patch, 2: 2843083-2.patch, failed testing.

naveenvalecha’s picture

Status: Needs work » Needs review
FileSize
9.42 KB
4.18 KB

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

naveenvalecha’s picture

FileSize
133.42 KB
Sam152’s picture

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

naveenvalecha’s picture

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

timmillwood’s picture

@naveenvalecha with the wording "Select types to use this workflow" I was trying to prevent us from using the word "entity" anywhere, because Drupal doesn't do this anywhere.

I still like the original mock up of this all being one form. Also still wonder if this would be better just as a form alter.

naveenvalecha’s picture

@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

jojototh’s picture

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"
dawehner’s picture

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.

Sam152’s picture

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.
Sam152’s picture

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.

Sam152’s picture

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

Sam152’s picture

Assigned: Unassigned » Sam152
Sam152’s picture

Sam152’s picture

Assigned: Sam152 » Unassigned
Issue tags: +Needs tests

Version: 8.3.x-dev » 8.4.x-dev

Drupal 8.3.0-alpha1 will be released the week of January 30, 2017, which means new developments and disruptive changes should now be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

timmillwood’s picture

Status: Needs review » Needs work

Marking "Needs work" for the tests.

+++ b/core/modules/workflows/src/WorkflowTypeInterface.php
@@ -118,12 +118,13 @@ public function buildStateConfigurationForm(FormStateInterface $form_state, Work
+   *   THe workflow the form will be added to.

nit pick

+++ b/core/modules/workflows/src/Form/WorkflowEditForm.php
@@ -188,6 +190,7 @@ public function form(array $form, FormStateInterface $form_state) {
+    $workflow->getTypePlugin()->editFormSave($workflow, $form, $form_state);

I wonder if we need editFormSave(). If the form is constructed differently wouldn't it just save correctly?

timmillwood’s picture

Status: Needs work » Needs review
Issue tags: -Needs tests
FileSize
29.85 KB
15.36 KB

Getting pretty close.

timmillwood’s picture

We should support entity types without bundles too.

timmillwood’s picture

Here's a screenshot with Multiversion installed to show moderating many different entity types, with and without bundles.

Sam152’s picture

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.

timmillwood’s picture

Assigned: Unassigned » timmillwood
Status: Needs review » Needs work

urgh, looks like I diffed with the wrong branch of core. Will re-roll in a bit.

timmillwood’s picture

Fixing patch from #21, interdiff is still the same.

Sam152’s picture

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

Sam152’s picture

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.

Status: Needs review » Needs work

The last submitted patch, 27: 2843083-27-workflows-only.patch, failed testing.

Sam152’s picture

Status: Needs work » Needs review
timmillwood’s picture

  1. +++ b/core/modules/workflows/src/Plugin/ConfigurableWorkflowTypeBase.php
    @@ -0,0 +1,30 @@
    +  public function validateConfigurationForm(array &$form, FormStateInterface $form_state) {
    ...
    +  public function submitConfigurationForm(array &$form, FormStateInterface $form_state) {
    

    Should return empty arrays?

Sam152’s picture

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

timmillwood’s picture

ok, reread the patch, and it makes sense now.

Think this requires someone else to RTBC. @naveenvalecha? @dawehner?

The last submitted patch, 27: 2843083-27.patch, failed testing.

Sam152’s picture

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

amateescu’s picture

Status: Needs review » Needs work
  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?

Sam152’s picture

  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.
Sam152’s picture

Status: Needs work » Needs review
FileSize
5.01 KB
36.66 KB
125.62 KB

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

timmillwood’s picture

What if the entity type doesn't have a bundle? For example, what if \Drupal\file\Entity\File were to become revisionable?

Sam152’s picture

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

Just a little update to only show bundles that are not already in use by a workflow to enforce one workflow per bundle.

I can also confirm after installing contrib module Multiversion to make more entity types revisionable I got the following bundles:
-Custom block
-- Basic block
-Comment
-- Default comments
-File
-- File
-Workspace
-- Basic workspace
-Content
-- Article
-- Basic page
-Shortcut link
-- Default
-Taxonomy term
-- Tags
-Custom menu link
-- Custom menu link

xjm’s picture

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?

xjm’s picture

Issue tags: +Usability

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.

Sam152’s picture

This issue was created to implement feedback from the usability review in #2830584: Review the workflow UI.

xjm’s picture

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.

xjm’s picture

Issue summary: View changes

Adding the more realistic screenshot instead.

xjm’s picture

Issue summary: View changes
tacituseu’s picture

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.

Sam152’s picture

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.

Sam152’s picture

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.

timmillwood’s picture

I think we need this, currently Content Moderation supports any revisionable entity type, in core this is just Node and BlockContent, but in contrib there are so many. These may or may not have bundles, and even if they have bundles they might not have the UI expected by Content Moderation to add the "Manage moderation" tab and page. The Entity API is so flexible that we can't depend on anything for injecting our API into every entity type, therefore a unified UI is the only option.

The idea / design for this came from @jojototh who suggested some options in #10 for if the form was too overwhelming. Ideally we could cover these in a follow up.

If we drop the dependency on revisionable entity types then this will become more important as we will be able to moderate things like Files and Users, then a unified UI is paramount.

Bojhan’s picture

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

timmillwood’s picture

@Bojhan - There isn't any sorting in the latest patch, we could add whatever sorting you wish (within reason).

Bojhan’s picture

I would at least have content on top.

timmillwood’s picture

Version: 8.3.x-dev » 8.4.x-dev
FileSize
7.3 KB
37.64 KB

Rerolling for 8.4.x and forcing content to be on top.

timmillwood’s picture

FileSize
5.55 KB
38.98 KB

Quite a big update to try and implement the UI from the mock ups in https://marvelapp.com/1124911/screen/17578888.

Looks to mostly be working, needs testing and tidying up.
Open to suggestions on better implementation.

timmillwood’s picture

Here's a quick screencast to demo the patch from #55

Status: Needs review » Needs work

The last submitted patch, 55: 2843083-55.patch, failed testing.

timmillwood’s picture

Status: Needs work » Needs review
FileSize
70.18 KB
10.81 KB
43.63 KB

- Fixed the tests
- Added the routing and form which were missing from the last patch
- Switched the form from checkboxes to tableselect
- Made the modal bigger

Sam152’s picture

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?

timmillwood’s picture

FileSize
8.31 KB
47.06 KB

#59.1 - EntityTypeInfoTest wasn't passing, so added this in to fix the exception there, but this new patch updates the test to install workflows module.
#59.2 - 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.
#59.3 - Fixed
#59.4 - Not sure how best to tackle a kernel test against a form class? It's covered by multiple browser tests.
#59.5 - Fixed
#59.6 - Fixed
#59.7 - Fixed
#59.8 - Yes, I guess we need UX advise here, this was by far the easiest way to implement it and kinda made sense.
#59.9 - Fixed
#59.10 - Fixed
#59.11 - Not sure what you mean?
#59.12 - This was added to make node appear at the top of the list, not needed anymore, removed.

Sam152’s picture

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.

timmillwood’s picture

It's only a feature of the one specific plugin, not something all workflows can make use of.

I guess you're right here, for the content moderation workflow type this implementation is flawless, but is it really causing an issue to workflows which don't use that workflow type? Aren't we going to face a similar issue no matter how the route is implemented?

Sam152’s picture

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.

timmillwood’s picture

FileSize
5.95 KB
47 KB

Ok, it is now just a generic form. Also added a "Cancel" button to the modal which just closes it without saving.

Manuel Garcia’s picture

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.

Add or Remove entity types

Manuel Garcia’s picture

+++ 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

Sam152’s picture

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.

timmillwood’s picture

Status: Needs review » Needs work

Marking needs work for #67.

Manuel Garcia’s picture

Assigned: Unassigned » Manuel Garcia

Having a go at #67

Manuel Garcia’s picture

Assigned: Manuel Garcia » Unassigned
FileSize
5.64 KB
46.57 KB

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

Status: Needs work » Needs review
FileSize
7.33 KB
46.83 KB

#67.3 - Fixed
#67.4 - Added some comments to help improve understanding.
#67.5 - Using the injected bundle info, don't think it needs to be static.
#67.6 - Not sure, added it back.
#67.9 - Updated.
#67.10 - Fixed
#67.12 - Not sure it's worth implementing a new paramconverter for this, maybe as a follow up, but updated it to throw a 404 rather than a 500.

Status: Needs review » Needs work

The last submitted patch, 71: 2843083-71.patch, failed testing.

timmillwood’s picture

FileSize
1.53 KB
46.83 KB

Fixing the broken tests.

timmillwood’s picture

Status: Needs work » Needs review
Sam152’s picture

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.

Bojhan’s picture

Is the screen still up to date for review?

timmillwood’s picture

Yes, the screen shot in #65 is accurate.

Manuel Garcia’s picture

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

yoroy’s picture

Status: Needs review » Needs work
Issue tags: -Needs usability review
FileSize
27.81 KB
21.3 KB
35.68 KB

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" ?

yoroy’s picture

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.

Manuel Garcia’s picture

Assigned: Unassigned » Manuel Garcia

Excellent points @yoroy, thanks!

Having a go at this...

Manuel Garcia’s picture

Assigned: Manuel Garcia » Unassigned
Status: Needs work » Needs review
FileSize
46.75 KB
3.25 KB

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.

timmillwood’s picture

#79.5 could the title callback just be a method on the form class?
#79.4 Maybe we could fix this in Seven theme as a follow up?

Manuel Garcia’s picture

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

Manuel Garcia’s picture

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.

Manuel Garcia’s picture

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

yoroy’s picture

Nice! This is shaping up very well.

timmillwood’s picture

Status: Needs review » Reviewed & tested by the community

I think this looks good from a code point of view. So going to RTBC, @yoroy if you still have any UX concerns please switch back to "Needs Work".

amateescu’s picture

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?

yoroy’s picture

Status: Reviewed & tested by the community » Needs work
FileSize
56.69 KB

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?

Manuel Garcia’s picture

Status: Needs work » Needs review
FileSize
47.05 KB
4.18 KB
11.35 KB

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

timmillwood’s picture

This addresses #89.6 but not sure what to do with #89.5; we could rename ConfigurableWorkflowTypeInterface to WorkflowTypeWithFormInterface, but not sure what else we can do that'd make sense.

amateescu’s picture

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.

amateescu’s picture

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

Sam152’s picture

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?

amateescu’s picture

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

Sam152’s picture

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.

amateescu’s picture

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.

Sam152’s picture

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.

Manuel Garcia’s picture

Manuel Garcia’s picture

Sam152’s picture

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.

yoroy’s picture

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

Status: Needs review » Needs work

The last submitted patch, 102: 2843083-102.patch, failed testing.

amateescu’s picture

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

Manuel Garcia’s picture

Status: Needs work » Needs review
FileSize
47.61 KB
1.49 KB
19.04 KB

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:

Status: Needs review » Needs work

The last submitted patch, 106: 2843083-106.patch, failed testing.

Sam152’s picture

Status: Needs work » Needs review
FileSize
49.56 KB
3.19 KB

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?

Sam152’s picture

+++ 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".

Manuel Garcia’s picture

I think #109 makes sense, here it is.

timmillwood’s picture

Tempted to RTBC, but want another +1 from @yoroy and @amateescu.

yoroy’s picture

FileSize
101.01 KB
25.87 KB

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.

Manuel Garcia’s picture

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

Status: Needs review » Needs work

The last submitted patch, 113: 2843083-113.patch, failed testing.

Manuel Garcia’s picture

Status: Needs work » Needs review

Let's try again...

amateescu’s picture

Status: Needs review » Reviewed & tested by the community
FileSize
49.98 KB
1.26 KB

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.

Manuel Garcia’s picture

#116interdiff looks ok, good catch @amateescu

timmillwood’s picture

+1 to the RTBC.

jojototh’s picture