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.

CommentFileSizeAuthor
#63 2849827-workflow-setters-63.patch104.81 KBSam152
#63 interdiff.txt838 bytesSam152
#57 2849827-workflow-setters-57.patch104.82 KBSam152
#57 interdiff.txt1.42 KBSam152
#53 2849827-workflow-setters-53.patch104.68 KBSam152
#53 interdiff.txt11.97 KBSam152
#50 2849827-workflow-setters.patch104.44 KBlarowlan
#50 interdiff-f83355.txt3.69 KBlarowlan
#48 2849827-47.patch103.63 KBtimmillwood
#47 interdiff-2849827-47.txt7.13 KBtimmillwood
#47 interdiff-2849827-47.txt7.13 KBtimmillwood
#44 2849827-44.patch103.63 KBtimmillwood
#44 interdiff-2849827-44.txt1.78 KBtimmillwood
#36 2849827-36.patch101.85 KBtimmillwood
#36 interdiff-2849827-36.txt973 bytestimmillwood
#28 2849827-28.patch101.26 KBtimmillwood
#28 interdiff-2849827-28.txt615 bytestimmillwood
#27 interdiff-2849827-27.txt627 bytestimmillwood
#27 2849827-27.patch101.29 KBtimmillwood
#17 2849827-17.patch101.28 KBtimmillwood
#17 interdiff-2849827-17.txt3.61 KBtimmillwood
#14 2849827-rearrange-workflow-settings-14.patch98.95 KBSam152
#9 2849827-rearrange-workflow-settings-9.patch99.79 KBSam152
#9 interdiff.txt1.36 KBSam152
#7 2849827-rearrange-workflow-settings-7.patch99.49 KBSam152
#7 interdiff.txt1.48 KBSam152
#5 2849827-rearrange-workflow-settings-4.patch99.41 KBSam152
#5 interdiff.txt531 bytesSam152
#3 2849827-rearrange-workflow-settings-3.patch99.39 KBSam152
Members fund testing for the Drupal project. Drupal Association Learn more

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

timmillwood’s picture

Status: Needs review » Reviewed & tested by the community

With @Sam152's "+1 RTBC", and my agreement, I think we're good to go.

Status: Reviewed & tested by the community » Needs work

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

timmillwood’s picture

Status: Needs work » Reviewed & tested by the community

Unrelated test fail, so back to RTBC

Sam152’s picture

I know this is experimental, but if we could not backport this to 8.3.x, that would be great. Will save a lot of headaches with the upgrade path, it would allow us to write upgrade paths for just the minor releases, not the patch releases.

timmillwood’s picture

I agree @Sam152, this should go into 8.4.x only!

Status: Reviewed & tested by the community » Needs work

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

timmillwood’s picture

Status: Needs work » Needs review
FileSize
973 bytes
101.85 KB

Fixing failing test.

plach’s picture

Priority: Normal » Major
Issue tags: +WI critical

This is marked as MUST-HAVE in the roadmap.

plach’s picture

Status: Needs review » Reviewed & tested by the community

I didn't review the whole patch but the interdiff makes sense, so moving back to RTBC.

plach’s picture

+++ b/core/modules/workflows/src/WorkflowInterface.php
@@ -237,6 +277,8 @@ public function setTransitionLabel($transition_id, $label);
+   * @deprecated in Drupal 8.4.x and will be removed before 8.4.0.

If this is the case I guess we need to create a follow-up and add it to the roadmap as a MUST-HAVE.

timmillwood’s picture

Note: I moved this issue up to must have because of it being a BC break and not having an upgrade path.

Adding follow up #2893778: Remove deprecated Workflows methods

Status: Reviewed & tested by the community » Needs work

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

timmillwood’s picture

@plach maybe we update to "will remove before 8.5.0" not to make too much work for ourselves?

Sam152’s picture

I believe @alexpott originally suggested removing them in a follow up to keep the focus on the change in architecture vs all of the places these methods are used. I think the conversion will actually be quite trivial, if not a simple find/replace, but given this is experimental and we aren't really supporting BC in the first place, the comments themselves are somewhat misleading, implying there is any kind of BC to begin with? Maybe they should just be removed entirely?

Will have a look at the new fails.

timmillwood’s picture

Status: Needs work » Needs review
FileSize
1.78 KB
103.63 KB

Here's an update fixing the failing tests in #36.

plach’s picture

I agree with @Sam152 let's just remove them in the follow-up, we wouldn't be able to do that after 8.4.0 is released. LEt's leave them as deprecated so if for any reason that doesn't get in at least we're covered.

plach’s picture

I'd say we need to update the deprecation messages to mention D9 instead of 8.4.0. Then we will try to remove everything in the follow-up.

timmillwood’s picture

FileSize
7.13 KB
7.13 KB

Stating we'll officially fix the deprecated methods by D9, so that we can stabilize Workflows module.

However, I am currently working on them, so hope to have a patch soon.

timmillwood’s picture

FileSize
103.63 KB

oops, wrong file.

plach’s picture

Status: Needs review » Reviewed & tested by the community

Ok, back to RTBC.

The follow-up to remove deprecated methods is #2893778: Remove deprecated Workflows methods, if the patch is not ready in time to be merged here.

larowlan’s picture

Started reviewing this, found some phpcs issues - fixing those while I continue with review

larowlan’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs change record

Looking good - we need a change record here

Additional observations

  1. +++ b/core/modules/workflows/src/Entity/Workflow.php
    @@ -148,399 +114,206 @@ public function preSave(EntityStorageInterface $storage) {
    +   * {@inheritDoc}
    ...
    +   * {@inheritDoc}
    

    nit: uppercase D

  2. +++ b/core/modules/workflows/src/Entity/Workflow.php
    @@ -148,399 +114,206 @@ public function preSave(EntityStorageInterface $storage) {
    +  public function getTypePlugin() {
    +    return $this->getPluginCollection()->get($this->type);
       }
    

    This is called a lot - is there merit in caching it in a protected property?

  3. +++ b/core/modules/workflows/src/Form/WorkflowStateAddForm.php
    @@ -88,7 +88,7 @@ protected function copyFormValuesToEntity(EntityInterface $entity, array $form,
    -        $configuration['states'][$values['id']] = $values['type_settings'][$entity->getTypePlugin()->getPluginId()];
    +        $configuration['states'][$values['id']] += $values['type_settings'][$entity->getTypePlugin()->getPluginId()];
    
    +++ b/core/modules/workflows/src/Form/WorkflowStateEditForm.php
    @@ -127,7 +127,7 @@ protected function copyFormValuesToEntity(EntityInterface $entity, array $form,
    -      $configuration['states'][$values['id']] = $values['type_settings'][$entity->getTypePlugin()->getPluginId()];
    +      $configuration['states'][$values['id']] += $values['type_settings'][$entity->getTypePlugin()->getPluginId()];
    
    +++ b/core/modules/workflows/src/Form/WorkflowTransitionAddForm.php
    @@ -107,7 +107,7 @@ protected function copyFormValuesToEntity(EntityInterface $entity, array $form,
    -      $configuration['transitions'][$values['id']] = $values['type_settings'][$entity->getTypePlugin()->getPluginId()];
    +      $configuration['transitions'][$values['id']] += $values['type_settings'][$entity->getTypePlugin()->getPluginId()];
    
    +++ b/core/modules/workflows/src/Form/WorkflowTransitionEditForm.php
    @@ -130,7 +130,7 @@ protected function copyFormValuesToEntity(EntityInterface $entity, array $form,
    -      $configuration['transitions'][$values['id']] = $values['type_settings'][$entity->getTypePlugin()->getPluginId()];
    +      $configuration['transitions'][$values['id']] += $values['type_settings'][$entity->getTypePlugin()->getPluginId()];
    

    Perhaps we should add some comments here indicating why we're appending now instead of replacing?

  4. +++ b/core/modules/workflows/src/Plugin/WorkflowTypeBase.php
    @@ -153,4 +138,338 @@ public function getInitialState(WorkflowInterface $workflow) {
    +  public function addState($state_id, $label) {
    ...
    +      throw new \InvalidArgumentException("The state '$state_id' already exists in workflow.");
    ...
    +      throw new \InvalidArgumentException("The state ID '$state_id' must contain only lowercase letters, numbers, and underscores");
    

    Missing @throws on the interface docblock

  5. +++ b/core/modules/workflows/src/Plugin/WorkflowTypeBase.php
    @@ -153,4 +138,338 @@ public function getInitialState(WorkflowInterface $workflow) {
    +    if (preg_match('/[^a-z0-9_]+/', $state_id)) {
    ...
    +    if (preg_match('/[^a-z0-9_]+/', $transition_id)) {
    

    Is there merit in making this a constant? (minor, personal preference)

  6. +++ b/core/modules/workflows/src/Plugin/WorkflowTypeBase.php
    @@ -153,4 +138,338 @@ public function getInitialState(WorkflowInterface $workflow) {
    +    $states = $this->getStates();
    +    return isset($states[$state_id]);
    

    We're instantiating an array of State objects and decorating them just to check if the state exists and then throwing those objects away. (::getStates calls ::getState which builds a State object and decorates it)

    I think isset($this->configuration['states'][$state_id]) is enough here.

  7. +++ b/core/modules/workflows/src/Plugin/WorkflowTypeBase.php
    @@ -153,4 +138,338 @@ public function getInitialState(WorkflowInterface $workflow) {
    +      // Sort states by weight and then label.
    +      $weights = $labels = [];
    +      foreach ($states as $id => $state) {
    +        $weights[$id] = $state->weight();
    +        $labels[$id] = $state->label();
    +      }
    +      array_multisort(
    +        $weights, SORT_NUMERIC, SORT_ASC,
    +        $labels, SORT_NATURAL, SORT_ASC
    +      );
    +      $states = array_replace($weights, $states);
    ...
    +      $weights = $labels = [];
    +      foreach ($transitions as $id => $transition) {
    +        $weights[$id] = $transition->weight();
    +        $labels[$id] = $transition->label();
    +      }
    +      array_multisort(
    +        $weights, SORT_NUMERIC, SORT_ASC,
    +        $labels, SORT_NATURAL, SORT_ASC
    +      );
    +      $transitions = array_replace($weights, $transitions);
    

    We've repeated this twice, perhaps merit in a utility protected sort method?

  8. +++ b/core/modules/workflows/src/Plugin/WorkflowTypeBase.php
    @@ -153,4 +138,338 @@ public function getInitialState(WorkflowInterface $workflow) {
    +    if (!isset($this->configuration['states'][$state_id])) {
    ...
    +    if (!isset($this->configuration['states'][$state_id])) {
    ...
    +    if (!isset($this->configuration['states'][$state_id])) {
    ...
    +    if (!isset($this->configuration['states'][$state_id])) {
    ...
    +    if (isset($this->configuration['transitions'][$transition_id])) {
    ...
    +    if (!isset($this->configuration['transitions'][$transition_id])) {
    ...
    +    if (isset($this->configuration['transitions'][$transition_id])) {
    ...
    +    if (isset($this->configuration['transitions'][$transition_id])) {
    ...
    +    if (!isset($this->configuration['transitions'][$transition_id])) {
    ...
    +    if (isset($this->configuration['transitions'][$transition_id])) {
    

    Could reuse ::hasState/::hasTransition

  9. +++ b/core/modules/workflows/src/Plugin/WorkflowTypeBase.php
    @@ -153,4 +138,338 @@ public function getInitialState(WorkflowInterface $workflow) {
    +    $state = new State(
    ...
    +    return $this->decorateState($state);
    

    Is it worth static caching these in an object property, e.g. $this->states - multiple calls to ::getStates or ::getState rebuilds a new object each time?

  10. +++ b/core/modules/workflows/src/Plugin/WorkflowTypeBase.php
    @@ -153,4 +138,338 @@ public function getInitialState(WorkflowInterface $workflow) {
    +  public function setStateWeight($state_id, $weight) {
    

    Should we be asserting that weight is numeric?

  11. +++ b/core/modules/workflows/src/Plugin/WorkflowTypeBase.php
    @@ -153,4 +138,338 @@ public function getInitialState(WorkflowInterface $workflow) {
    +      if (empty($transition['from']) || $transition['to'] === $state_id) {
    +        $this->deleteTransition($transition_id);
    +      }
    

    Feels like we should be doing the $transition['to'] === $state_id check first, and if it is, delete and call continue and avoid the costly array_search for no reason. In addition, would allow us to get rid of the ugo elseif.

  12. +++ b/core/modules/workflows/src/Plugin/WorkflowTypeBase.php
    @@ -153,4 +138,338 @@ public function getInitialState(WorkflowInterface $workflow) {
    +    unset($this->configuration['states'][$state_id]);
    

    If we go down the static cache of built State objects, would need to clean that up here too (see comment above)

  13. +++ b/core/modules/workflows/src/Plugin/WorkflowTypeBase.php
    @@ -153,4 +138,338 @@ public function getInitialState(WorkflowInterface $workflow) {
    +    $transition = new Transition(
    +      $this,
    +      $transition_id,
    +      $this->configuration['transitions'][$transition_id]['label'],
    +      $this->configuration['transitions'][$transition_id]['from'],
    +      $this->configuration['transitions'][$transition_id]['to'],
    +      $this->configuration['transitions'][$transition_id]['weight']
    +    );
    +    return $this->decorateTransition($transition);
    

    Same question re static cache property - we rebuild these each time but they're thrown away.

  14. +++ b/core/modules/workflows/src/Plugin/WorkflowTypeBase.php
    @@ -153,4 +138,338 @@ public function getInitialState(WorkflowInterface $workflow) {
    +    return isset($this->configuration['transitions'][$transition_id]);
    

    Support my suggestion for using same approach in ::hasState (see earlier)

  15. +++ b/core/modules/workflows/src/Plugin/WorkflowTypeBase.php
    @@ -153,4 +138,338 @@ public function getInitialState(WorkflowInterface $workflow) {
    +    if (empty($transition_id)) {
    ...
    +    return !empty($this->getTransitionIdFromStateToState($from_state_id, $to_state_id));
    

    !$transition_id would suffice here, no need for empty - also '0' is a valid transition_id, so empty is technically the wrong choice (see https://3v4l.org/2d2PR)

    Strictly speaking, we should be using === NULL, looking at the return from ::getTransitionIdFromStateToState

  16. +++ b/core/modules/workflows/src/Plugin/WorkflowTypeBase.php
    @@ -153,4 +138,338 @@ public function getInitialState(WorkflowInterface $workflow) {
    +  public function setTransitionLabel($transition_id, $label) {
    ...
    +  public function setTransitionWeight($transition_id, $weight) {
    ...
    +  public function deleteTransition($transition_id) {
    

    In all other methods we avoid the else by negating the check and throwing the Exception early. I think we should do the same here, its more readable without the else.

  17. +++ b/core/modules/workflows/src/Plugin/WorkflowTypeBase.php
    @@ -153,4 +138,338 @@ public function getInitialState(WorkflowInterface $workflow) {
    +        $transition = $this->getTransitionFromStateToState($from_state_id, $this->configuration['transitions'][$transition_id]['to']);
    +        if ($transition_id !== $transition->id()) {
    

    Do we need to instantiate a Transition object here - we're only using the ID so ::getTransitionIdFromStateToState should be better than ::getTransitionFromStateToState

  18. +++ b/core/modules/workflows/src/Plugin/WorkflowTypeBase.php
    @@ -153,4 +138,338 @@ public function getInitialState(WorkflowInterface $workflow) {
    +    ksort($this->configuration['transitions']);
    

    This feels like an unintended consequence of calling ::setTransitionFromStates, we're not changing the keys of $this->configuration['transitions'] here - I think this should go in ::addTransition instead, where we are adding a new transition and hence sorting would be required.

  19. +++ b/core/modules/workflows/src/WorkflowInterface.php
    @@ -23,6 +23,8 @@
    +   * @deprecated in Drupal 8.4.x and will be removed before 9.0.0.
    
    @@ -34,6 +36,8 @@ public function addState($state_id, $label);
    +   * @deprecated in Drupal 8.4.x and will be removed before 9.0.0.
    
    @@ -48,6 +52,8 @@ public function hasState($state_id);
    +   * @deprecated in Drupal 8.4.x and will be removed before 9.0.0.
    
    @@ -62,6 +68,8 @@ public function getStates($state_ids = NULL);
    +   * @deprecated in Drupal 8.4.x and will be removed before 9.0.0.
    
    @@ -75,6 +83,8 @@ public function getState($state_id);
    +   * @deprecated in Drupal 8.4.x and will be removed before 9.0.0.
    
    @@ -88,6 +98,8 @@ public function setStateLabel($state_id, $label);
    +   * @deprecated in Drupal 8.4.x and will be removed before 9.0.0.
    
    @@ -102,10 +114,22 @@ public function setStateWeight($state_id, $weight);
    +   * @deprecated in Drupal 8.4.x and will be removed before 9.0.0.
    ...
    +   * @deprecated in Drupal 8.4.x and will be removed before 9.0.0.
    
    @@ -122,6 +146,8 @@ public function deleteState($state_id);
    +   * @deprecated in Drupal 8.4.x and will be removed before 9.0.0.
    
    @@ -136,6 +162,8 @@ public function addTransition($id, $label, array $from_state_ids, $to_state_id);
    +   * @deprecated in Drupal 8.4.x and will be removed before 9.0.0.
    
    @@ -147,6 +175,8 @@ public function getTransition($transition_id);
    +   * @deprecated in Drupal 8.4.x and will be removed before 9.0.0.
    
    @@ -162,6 +192,8 @@ public function hasTransition($transition_id);
    +   * @deprecated in Drupal 8.4.x and will be removed before 9.0.0.
    
    @@ -176,6 +208,8 @@ public function getTransitions(array $transition_ids = NULL);
    +   * @deprecated in Drupal 8.4.x and will be removed before 9.0.0.
    
    @@ -192,6 +226,8 @@ public function getTransitionsForState($state_id, $direction = 'from');
    +   * @deprecated in Drupal 8.4.x and will be removed before 9.0.0.
    
    @@ -205,6 +241,8 @@ public function getTransitionFromStateToState($from_state_id, $to_state_id);
    +   * @deprecated in Drupal 8.4.x and will be removed before 9.0.0.
    
    @@ -221,6 +259,8 @@ public function hasTransitionFromStateToState($from_state_id, $to_state_id);
    +   * @deprecated in Drupal 8.4.x and will be removed before 9.0.0.
    
    @@ -237,6 +277,8 @@ public function setTransitionLabel($transition_id, $label);
    +   * @deprecated in Drupal 8.4.x and will be removed before 9.0.0.
    
    @@ -253,6 +295,8 @@ public function setTransitionWeight($transition_id, $weight);
    +   * @deprecated in Drupal 8.4.x and will be removed before 9.0.0.
    
    @@ -267,6 +311,8 @@ public function setTransitionFromStates($transition_id, array   $from_state_ids)
    +   * @deprecated in Drupal 8.4.x and will be removed before 9.0.0.
    

    These need to reference a change record I think

  20. +++ b/core/modules/workflows/src/WorkflowTypeInterface.php
    @@ -68,14 +68,6 @@ public function checkWorkflowAccess(WorkflowInterface $entity, $operation, Accou
    -  public function deleteState($state_id);
    
    @@ -86,14 +78,6 @@ public function deleteState($state_id);
    -  public function deleteTransition($transition_id);
    

    We retain many methods and defer them to the workflow type, marked as deprecated - but we remove these two - is there a reason for that inconsistency?

  21. +++ b/core/modules/workflows/src/WorkflowTypeInterface.php
    @@ -170,4 +154,261 @@ public function getRequiredStates();
    +   *   (optional) The direction of the transition. Defaults to 'from'. Possible
    +   *   values are: 'from' and 'to'.
    ...
    +  public function getTransitionsForState($state_id, $direction = 'from');
    

    If we're prescribing two possible values, really feels like they should be constants.

Sam152’s picture

Assigned: Unassigned » Sam152

Thanks for the review! I think I'll be able to have a look at these today.

Sam152’s picture

Status: Needs work » Needs review
Issue tags: +workflows.module
FileSize
11.97 KB
104.68 KB

Given this is already quite big, I've addressed what seemed to be obvious fixes or improvements, opened up follow-ups where I think those improvements/changes should have their own testing/validation and put a few question marks where I didn't quite grok something.

Based on these two follow ups alone, I think it reaffirms why this change is worth it, we're essentially removing the need for two additional patterns we don't really see anywhere else (dx++):

#2896725: Entirely remove the concept of WorkflowType(Interface|Base)::initializeWorkflow()
#2896721: Remove the concept of decorated states/transitions from worfklow type plugins.

1. Fixed.
2. The plugin collection is cached in ::getPluginCollection and the plugin instance is cached in \Drupal\Component\Plugin\LazyPluginCollection::get. A new cache probably adds more complexity than reward.
3. If type plugins are now responsible for defining how states and transitions are stored, this breaks encapsulation anyway. Added a @todo to fix. I think we'll need submitStateConfigurationForm alongside buildStateConfigurationForm. Possibly a follow-up? #2896722: Leverage PluginWithFormsInterface to encapsulate @WorkflowType schema and clean up state/transition form methods.
4. Fixed.
5. Makes sense to me.
6. WorkflowTypeBase provides the foundation for a plugin that stores states and transitions in configuration, for plugins where this isn't the case, this is worse DX because it's more to override. @see \Drupal\workflow_type_test\Plugin\WorkflowType\PredefinedStatesWorkflowTestType.
7. Great clean up.
8. Fixed.
9. I think a reasonable follow-up will be to remove the methods for decorating states. The change in architecture this patch represents makes the concept irrelevant, because the type plugin is now the source of the states/transitions, so there is no need for the entity to create and decorate them. #2896721: Remove the concept of decorated states/transitions from worfklow type plugins.
10. Given this was basically just moved, I think it's a good idea, but scope creep.
11. Good catch, will have to look at this in some more detail. @todo
12. See comment above.
13. Lets follow this up in #2896721: Remove the concept of decorated states/transitions from worfklow type plugins. .
14. ?
15. Good catch.
16. Agree, fixed.
17. Good catch, fixed.
18. Agree, fixed.
19. We haven't created any change records thus far, do we need to for experimental modules? FWIW, we're only using this message to split the effort up into two issues, these will be removed right after this gets in.
20. I think these were just moved. Originally deleteState was on both the workflow entity and the type plugin, with the type plugin method existing to "React to the removal of a state". No longer required after this patch.
21. #2896724: Create constants for transition directions. for scope.

Sam152’s picture

Assigned: Sam152 » Unassigned
Sam152’s picture

larowlan’s picture

14 was a comment in support of 6.

But your comment suggests we do it differently for states for a reason.

I would be inclined to do both the same way, the current hasState method needlessly instantiates a State object in my book. I think having to override is worth the performance improvement, when by and large most implementations will use configuration right?

Thanks for accommodating feedback

Sam152’s picture

Fair enough, for the workflow type we are actually using, this will be faster. Fixed and overriden in the test type.

timmillwood’s picture

Status: Needs review » Reviewed & tested by the community

Should we open a follow up for #51.10 and #51.11

Other than that, looks good. Some awesome follow ups.

This is currently the biggest blocker to Workflows being stable. It's quite a new "must have" and only added because of the big schema changes. This could be a done in a BC way, with a full upgrade path in 8.5.x, but would be great to get such a big clean up issue in before stable.

Back to RTBC.

Sam152’s picture

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

I've filed #2897134: Assert weights are numeric when settings state/transition weights. and #2897133: Address performance issues in \Drupal\workflows\Plugin\WorkflowTypeBase::deleteState. I think it makes sense to do these in the follow-ups given this issue didn't introduce either, only moved the code.

larowlan’s picture

Status: Reviewed & tested by the community » Needs review
+++ b/core/modules/workflows/src/Plugin/WorkflowTypeBase.php
@@ -296,10 +289,22 @@ public function getTransitions(array $transition_ids = NULL) {
+      foreach ($objects as $id => $transition) {
         $weights[$id] = $transition->weight();
         $labels[$id] = $transition->label();
       }

this should be $object now right?

Sam152’s picture

$objects is hinted as either an array of states or transition value objects:

+ * @param \Drupal\workflows\StateInterface[]|\Drupal\workflows\TransitionInterface[] $objects

Sam152’s picture

Edit: right you are, missed the point of #61.

timmillwood’s picture

Status: Needs review » Reviewed & tested by the community

Back to RTBC

larowlan’s picture

Status: Reviewed & tested by the community » Needs review

Manually tested this once more on a fresh install.
Works well, we need a change record still though.

Sam152’s picture

Status: Needs review » Reviewed & tested by the community
Issue tags: -Needs change record

Added change record.

larowlan’s picture

Status: Reviewed & tested by the community » Fixed

Committed 6772e2d and pushed to 8.4.x.

I think #2830740: Allow workflow types to lock certain changes to workflows once things are in use will need a re-roll now - will kick off a retest.

Published change record.

Un-postponed related issues.

Keep up the good work, getting closer to a beta now.

  • larowlan committed 6772e2d on 8.4.x
    Issue #2849827 by Sam152, timmillwood, larowlan: Move workflow "settings...

Status: Fixed » Closed (fixed)

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