Problem/Motivation

Follow up to #2844594: Default workflow states and transitions.

Currently we have a workflow config entity type and WorkflowType plugin. The schema of the workflow entity is such that each plugin may define the schema of the settings stored for each entity:

workflows.workflow.*:
  type: config_entity
  label: 'Workflow'
  mapping:
    ...
    settings:
      type: workflow_settings.[%parent.type]
      label: 'Settings for workflow type'

These settings hold the states and transitions the workflow will have, as well as any other bits of config the specific workflow type will need to store.

Right now, there are several setters and getters on the workflow entity which assume the structure in the settings. This is incorrect because this structure isn't enforced anywhere and shouldn't be relied on.

Proposed resolution

Instead of the workflow entity changing the workflow settings, all methods required to set/get workflow concepts like states and transitions and additional associated settings should be encapsulated in WorkflowTypeInterface. That way the schema of the states and transitions is internal to the plugin and exposed only via an interface. The end goal being $this->settings is never used in the workflow entity.

Remaining tasks

User interface changes

None.

API changes

API changes to the way states and transitions are created and accessed.

Data model changes

Minimal data model changes, as the schema change has been done in #2844594: Default workflow states and transitions.

Comments

Sam152 created an issue. See original summary.

Sam152’s picture

Sam152’s picture

Initial crack at this, with a BC layer left in, as suggested by @alexpott. While doing this, it felt like a few things fell into place and there were some simplifications that might point to this being a better schema. Namely:

  • I don't think we need the decorator methods any more, the plugins control what states are returned already.
  • The initialise workflow method wasn't required for content_moderation anymore, defaultConfiguration .
  • The config is more DRY, with there only being a single tree for each state (vs one on type_settings and one on settings).

I've implemented a test plugin which hardcodes 4 states and stores configuration for transitions. Simple example, but hopefully allows for more integration with broader concepts.

Status: Needs review » Needs work

The last submitted patch, 3: 2849827-rearrange-workflow-settings-3.patch, failed testing.

Sam152’s picture

Status: Needs work » Needs review
FileSize
531 bytes
99.41 KB

Status: Needs review » Needs work

The last submitted patch, 5: 2849827-rearrange-workflow-settings-4.patch, failed testing.

Sam152’s picture

Status: Needs work » Needs review
FileSize
1.48 KB
99.49 KB

Status: Needs review » Needs work

The last submitted patch, 7: 2849827-rearrange-workflow-settings-7.patch, failed testing.

Sam152’s picture

Status: Needs work » Needs review
FileSize
1.36 KB
99.79 KB
timmillwood’s picture

I know this is an experimental module, and we don't do upgrade paths, but I wonder if it might be a nice idea to in this case?

Otherwise I think this makes sense.

chiranjeeb2410’s picture

Status: Needs review » Reviewed & tested by the community

@Sam152,

Patch applies cleanly, changes are good to go.
Updating to RTBC.

timmillwood’s picture

Status: Reviewed & tested by the community » Needs review

We know the test applies cleanly, test bot told us that.

Back to needs review based on my previous comment.

Sam152’s picture

I think it should go into the unofficial 8.3 => 8.4 upgrade patch + issue, once that's open (likely once 8.4 is released?) for a few reasons:

  • I think it might send the wrong message that upgrade paths and BC are actually going to be maintained, when generally that isn't a consideration for most issues.
  • It might not be the final schema, meaning the upgrade path would also be mandatory for any future schema breaks of the module due to the nature of upgrade path tests.
  • It'll slow down momentum when we start drawing these kind of lines in issues for experimental modules.

FWIW, I'll be writing and completing a full CM + workflows upgrade path for 8.3 to 8.4 at some stage, given I'm maintaining projects in production that have successfully used the 8.2 => 8.3 path.

Sam152’s picture

Reroll. I think it would be good to get this in before workflows hits stable, although it has been a while since it was written, so I might have a review of this myself before harassing anyone else to look at it :)

Sam152’s picture

  1. +++ b/core/modules/content_moderation/src/Plugin/WorkflowType/ContentModeration.php
    @@ -95,13 +95,8 @@ public static function create(ContainerInterface $container, array $configuratio
       public function initializeWorkflow(WorkflowInterface $workflow) {
    -    $workflow
    -      ->addState('draft', $this->t('Draft'))
    -      ->setStateWeight('draft', -5)
    -      ->addState('published', $this->t('Published'))
    -      ->setStateWeight('published', 0)
    -      ->addTransition('create_new_draft', $this->t('Create New Draft'), ['draft', 'published'], 'draft')
    -      ->addTransition('publish', $this->t('Publish'), ['draft', 'published'], 'published');
    +    // @todo, do we need initializeWorkflow now that defaultConfiguration can
    +    // initialise these.
         return $workflow;
       }
    

    I think getting rid of this would be a big win for this patch. We don't use this pattern anywhere else, but defaultConfiguraiton is pretty ubiquitous.

  2. +++ b/core/modules/content_moderation/tests/src/Kernel/ContentModerationPermissionsTest.php
    @@ -55,37 +55,13 @@ public function permissionsTestCases() {
    -          'transitions' => [
    -            'publish' => [
    -              'label' => 'Publish',
    -              'from' => ['draft'],
    -              'to' => 'published',
    -              'weight' => 0,
    -            ],
    -            'unpublish' => [
    -              'label' => 'Unpublish',
    -              'from' => ['published'],
    -              'to' => 'draft',
    -              'weight' => 0,
    -            ],
    -          ],
    -          'states' => [
    -            'draft' => [
    -              'label' => 'Draft',
    -              'weight' => -5,
    -            ],
    -            'published' => [
    -              'label' => 'Published',
    -              'weight' => 0,
    -            ],
    -          ],
    

    I think these have been removed because previously you wouldn't have the transitions enforced by initializeWorkflow unless you actually called it. Now that the initialisation is being taken care of by the defaultConfiguration, you can't get into a state that doesn't have them merged in. Hence moving this test to the default states.

  3. +++ b/core/modules/workflows/tests/src/Kernel/PredefinedWorkflowTypeTest.php
    @@ -0,0 +1,52 @@
    +    // No states configuration is stored for this workflow.
    +    $configuration = $workflow->getTypePlugin()->getConfiguration();
    +    $this->assertFalse(isset($configuration['states']));
    

    Probably worth adding some asserts that the workflow has states here.

  4. +++ b/core/modules/workflows/tests/modules/workflow_type_test/config/schema/workflow_type_test.schema.yml
    @@ -15,6 +19,26 @@ workflow.type_settings.workflow_type_required_state_test:
    +# @todo, inline this straight into "workflow.type_settings.workflow_type_complex_test"
    +# after https://www.drupal.org/node/2871746 is resolved.
    +workflows.state.complex_test_state:
    +  type: workflows.state
    +  mapping:
    +    extra:
    +      type: string
    +      label: 'Extra information'
    

    This is possibly just "works as designed" and the @todo should be removed.

timmillwood’s picture

Status: Needs review » Needs work

When manually testing I found this patch prevents transitions from being deleted. This also shows we don't have test coverage for deleting transitions.

+++ b/core/modules/content_moderation/src/Plugin/WorkflowType/ContentModeration.php
@@ -95,13 +95,8 @@ public static function create(ContainerInterface $container, array $configuratio
   public function initializeWorkflow(WorkflowInterface $workflow) {

Shall we just remove this?

timmillwood’s picture

Status: Needs review » Needs work

The last submitted patch, 17: 2849827-17.patch, failed testing. View results

timmillwood’s picture

Status: Needs work » Needs review

Unrelated fail, DrupalCI ran out of disk space.

Sam152’s picture

I think the deep merge is what was enforcing the required states to exist, so I don't think this can be changed without something else to enforce them.

+++ b/core/modules/content_moderation/src/Plugin/WorkflowType/ContentModeration.php
@@ -94,20 +94,6 @@ public static function create(ContainerInterface $container, array $configuratio
-  public function initializeWorkflow(WorkflowInterface $workflow) {
-    $workflow
-      ->addState('draft', $this->t('Draft'))
-      ->setStateWeight('draft', -5)
-      ->addState('published', $this->t('Published'))
-      ->setStateWeight('published', 0)
-      ->addTransition('create_new_draft', $this->t('Create New Draft'), ['draft', 'published'], 'draft')
-      ->addTransition('publish', $this->t('Publish'), ['draft', 'published'], 'published');
-    return $workflow;
-  }

I think this implies the behavior was already in HEAD that create_new_draft and publish are always there, we just see them in the UI now. Maybe the correct behavior is for these to stick around?

timmillwood’s picture

The issue doesn't exist in HEAD, it is currently possible to delete all transitions.

Access control is preventing states from being deleted (with and without the patch).

Status: Needs review » Needs work

The last submitted patch, 17: 2849827-17.patch, failed testing. View results

timmillwood’s picture

Status: Needs work » Needs review
Sam152’s picture

Right, so initializeWorkflow is called only when saving a new workflow, not something that is called to enforce those states/transitions exist, so the shallow merge approach will probably work for our purposes, my mistake. The only possible issue is settings that are nested that aren't transitions or states would possibly not behave like the default configuration in other plugins. Worth thinking about this.

Sam152’s picture

I think this is getting there.

+++ b/core/modules/workflows/src/Plugin/WorkflowTypeBase.php
@@ -108,10 +96,7 @@ public function getConfiguration() {
-    $this->configuration = NestedArray::mergeDeep(
-      $this->defaultConfiguration(),
-      $configuration
-    );
+    $this->configuration = array_merge($this->defaultConfiguration(), $configuration);

I wonder if we could somehow only shallow merge on certain keys. At the very least I think this needs quite a detailed explanation of why we have to do this.

Other than that, I'm happy with it.

timmillwood’s picture

@Sam152 - yes, this is a tricky one. The end goal here is to use the default configuration only for new workflows, but in \Drupal\workflows\Plugin\WorkflowTypeBase::setConfiguration we have no context about the workflow, only the workflow plugin. We might be able to go as far as $this->configuration = empty($configuration) ? $this->defaultConfiguration() : $configuration;.

timmillwood’s picture

FileSize
101.29 KB
627 bytes

Here's a simple option, as suggested in #26.

timmillwood’s picture

FileSize
615 bytes
101.26 KB

After looking at ImageEffectBase, FilterBase, VariantBase, ConditionPluginBase which @Sam152 pointed me to, this looks like a better solution.

Sam152’s picture

+1 RTBC