Problem/Motivation

In #2849827: Move workflow "settings" setters and getters to WorkflowTypeInterface many methods were deprecated as we moved a lot of functionality from the Workflow config entity to the @WorkflowType plugin. These methods can now be removed.

Proposed resolution

Delete the methods from the Workflow config entity type and the WorkflowInterface plugin interface, then update all uses of the method to call the @WorkflowType plugin instead.

The deprecated methods are:

  • addState($state_id, $label)
  • hasState($state_id)
  • getStates($state_ids = NULL)
  • getState($state_id)
  • setStateLabel($state_id, $label)
  • setStateWeight($state_id, $weight)
  • deleteState($state_id)
  • getInitialState()
  • addTransition($id, $label, array $from_state_ids, $to_state_id)
  • getTransition($transition_id)
  • hasTransition($transition_id)
  • getTransitions(array $transition_ids = NULL)
  • getTransitionsForState($state_id, $direction = 'from')
  • getTransitionFromStateToState($from_state_id, $to_state_id)
  • hasTransitionFromStateToState($from_state_id, $to_state_id)
  • setTransitionLabel($transition_id, $label)
  • setTransitionWeight($transition_id, $weight)
  • setTransitionFromStates($transition_id, array $from_state_ids)
  • deleteTransition($transition_id)

Remaining tasks

None.

User interface changes

None.

API changes

Deprecated methods listed above are removed. Since the Workflows module is still experimental, and about to be marked beta-level stability, at which point its APIs will be frozen, this is the perfect (and last) time where we can do this. If we don't do this now, we'll be required to support this the entire Drupal 8 cycle.

Data model changes

None.

CommentFileSizeAuthor
#36 2893778-36.patch100.61 KBSam152
#36 interdiff.txt2.06 KBSam152
#21 2893778-21.patch98.96 KBSam152
#9 2893778-9-combined.patch170.66 KBtimmillwood
#9 2893778-9.patch99.2 KBtimmillwood
#4 2893778-4-combined.patch171.02 KBtimmillwood
#4 2893778-4.patch99.54 KBtimmillwood
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

timmillwood created an issue. See original summary.

plach’s picture

plach
he/him
Primary language Italian
Location Venezia
CreditAttribution: plach for Acquia commented
Status: Active » Postponed
Issue tags: +WI critical

Needs to happen before 8.4.0

plach’s picture

plach
he/him
Primary language Italian
Location Venezia
CreditAttribution: plach for Acquia commented
Priority: Normal » Major
timmillwood’s picture

timmillwood
Primary language English
Location 🏴󠁧󠁢󠁷󠁬󠁳󠁿 Wales, UK
CreditAttribution: timmillwood at Appnovation for Pfizer, Inc. commented
Status: Postponed » Needs review
FileSize
2893778-4.patch99.54 KB
2893778-4-combined.patch171.02 KB

Would've just uploaded to #2849827: Move workflow "settings" setters and getters to WorkflowTypeInterface but it's not working and didn't want to derail. So here's a patch (which is just an interdiff from the latest patch in the parent issue) and a combined patch of the two.

It is failing on \Drupal\Tests\content_moderation\Kernel\ContentModerationStateTest::testWorkflowDependencies. When you delete a bundle which is assigned to a workflow, the whole workflow disappears, not sure why yet.

timmillwood’s picture

timmillwood
Primary language English
Location 🏴󠁧󠁢󠁷󠁬󠁳󠁿 Wales, UK
CreditAttribution: timmillwood at Appnovation for Pfizer, Inc. commented

It seems to be something in the preDelete() method called when deleting the bundle which causes the workflow to disappear.

timmillwood’s picture

timmillwood
Primary language English
Location 🏴󠁧󠁢󠁷󠁬󠁳󠁿 Wales, UK
CreditAttribution: timmillwood at Appnovation for Pfizer, Inc. commented

ok, it's \Drupal\Core\Config\Entity\ConfigEntityBase::preDelete doing it, why is this doing it now, but not before?

Status: Needs review » Needs work

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

Sam152’s picture

Sam152 CreditAttribution: Sam152 as a volunteer and at PreviousNext commented

I'm glad these were two different issues, reviewing another 100kb of code as part of the original issue would have been a huge headache.

Awesome work on this, confident we can fix the fails and get this in if the other issue is committed based on the progress already made.

timmillwood’s picture

timmillwood
Primary language English
Location 🏴󠁧󠁢󠁷󠁬󠁳󠁿 Wales, UK
CreditAttribution: timmillwood at Appnovation for Pfizer, Inc. commented
Status: Needs work » Needs review
FileSize
2893778-9.patch99.2 KB
2893778-9-combined.patch170.66 KB

I'd removed onDependencyRemoval from Workflow entity, when I shouldn't have.

Thanks @Sam152 for the pointers via IRC.

Sam152’s picture

Sam152 CreditAttribution: Sam152 as a volunteer and at PreviousNext commented
Issue tags: +workflows.module
Sam152’s picture

Sam152 CreditAttribution: Sam152 as a volunteer and at PreviousNext commented
Component: content_moderation.module » workflows.module
Issue tags: -workflows.module
Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers at Acquia commented

Is this actually reviewable & RTBC'able, or blocked on another issue going in first?

Sam152’s picture

Sam152 CreditAttribution: Sam152 as a volunteer and at PreviousNext commented

It's currently blocked on another issue.

tacituseu’s picture

tacituseu CreditAttribution: tacituseu commented
Title: Removing deprecated Content Moderation methods » [PP-1] Removing deprecated Content Moderation methods
plach’s picture

plach
he/him
Primary language Italian
Location Venezia
CreditAttribution: plach for Acquia commented
Status: Needs review » Reviewed & tested by the community

Postponed on https://www.drupal.org/node/2849827

plach’s picture

plach
he/him
Primary language Italian
Location Venezia
CreditAttribution: plach for Acquia commented
Status: Reviewed & tested by the community » Postponed

Err, I mean this

Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers at Acquia commented
Related issues: +#2849827: Move workflow "settings" setters and getters to WorkflowTypeInterface
Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers at Acquia commented
Issue tags: +Needs issue summary update

It'd be great to have an overview of what we're actually removing in the IS. If not for passing reviewers, then definitely for the committer who's undoubtedly going to say "WOW 100K PATCH!" :)

timmillwood’s picture

timmillwood
Primary language English
Location 🏴󠁧󠁢󠁷󠁬󠁳󠁿 Wales, UK
CreditAttribution: timmillwood at Appnovation for Pfizer, Inc. commented
Issue summary: View changes
Issue tags: -Needs issue summary update
larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
CreditAttribution: larowlan at PreviousNext commented
Title: [PP-1] Removing deprecated Content Moderation methods » Removing deprecated Content Moderation methods
Status: Postponed » Active
Sam152’s picture

Sam152 CreditAttribution: Sam152 as a volunteer and at PreviousNext commented
Status: Active » Needs review
FileSize
2893778-21.patch98.96 KB

#9 no longer applies.

timmillwood’s picture

timmillwood
Primary language English
Location 🏴󠁧󠁢󠁷󠁬󠁳󠁿 Wales, UK
CreditAttribution: timmillwood at Appnovation for Pfizer, Inc. commented
Status: Needs review » Reviewed & tested by the community

Looks like we're good to go!

Sam152’s picture

Sam152 CreditAttribution: Sam152 as a volunteer and at PreviousNext commented

Just re-reviewed this as well. +1 RTBC.

Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers at Acquia commented
Issue summary: View changes
Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers at Acquia commented
Title: Removing deprecated Content Moderation methods » Remove deprecated Workflows methods
Issue summary: View changes

Explained why we want to do this now. Associated the https://www.drupal.org/node/2897706 change record with this issue too.

Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers at Acquia commented

This issue is not making big API changes. The big changes were made in #2849827: Move workflow "settings" setters and getters to WorkflowTypeInterface. This issue is merely following through on the changes that issue introduced, so that the Workflows module doesn't need to maintain support for lots of deprecated methods — method that were deprecated before the API was marked stable (which will happen when Workflows reaches "beta" level stability per https://www.drupal.org/core/experimental#beta — it is currently https://www.drupal.org/core/experimental#alpha).

This makes total sense.

99% of changes in this patch are trivial (the hard work was done in #2849827): most of it is just prefixing existing call chains with a getTypePlugin() call.

RTBC +1

Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers at Acquia commented
Parent issue: » #2843494: WI: Workflows module roadmap
timmillwood’s picture

timmillwood
Primary language English
Location 🏴󠁧󠁢󠁷󠁬󠁳󠁿 Wales, UK
CreditAttribution: timmillwood at Appnovation for Pfizer, Inc. commented

Thanks for the clarifications @Wim Leers, this patch could've easily been part of the previous issue, but at a combined size of 170kb, it seemed best to split.

Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers at Acquia commented

Yep, it totally makes sense :)

I'm just trying to look at this issue from a committer's POV, trying to criticize it from that POV, but as you can see, I can't find anything to criticize. Because the scope of #2849827 and the scope of this issue is so well-managed! 👏

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 21: 2893778-21.patch, failed testing. View results

Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers at Acquia commented
Status: Needs work » Reviewed & tested by the community

Retesting, re-RTBC.

larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
CreditAttribution: larowlan at PreviousNext commented
Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs reroll

Fail is valid, I triggered a retest after #2830740: Allow workflow types to lock certain changes to workflows once things are in use went in

larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
CreditAttribution: larowlan at PreviousNext commented
Status: Needs work » Needs review
Issue tags: -Needs reroll

Fail was in HEAD, so may come back green, reverted the issue that caused the fail in HEAD

Status: Needs review » Needs work

The last submitted patch, 21: 2893778-21.patch, failed testing. View results

larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
CreditAttribution: larowlan at PreviousNext commented

Yep, needs reroll after #2830740: Allow workflow types to lock certain changes to workflows once things are in use

Sam152’s picture

Sam152 CreditAttribution: Sam152 as a volunteer and at PreviousNext commented
Status: Needs work » Needs review
FileSize
interdiff.txt2.06 KB
2893778-36.patch100.61 KB
Sam152’s picture

Sam152 CreditAttribution: Sam152 as a volunteer and at PreviousNext commented
Status: Needs review » Reviewed & tested by the community

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 36: 2893778-36.patch, failed testing. View results

timmillwood’s picture

timmillwood
Primary language English
Location 🏴󠁧󠁢󠁷󠁬󠁳󠁿 Wales, UK
CreditAttribution: timmillwood at Appnovation for Pfizer, Inc. commented
Status: Needs work » Reviewed & tested by the community

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 36: 2893778-36.patch, failed testing. View results

Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers at Acquia commented
Status: Needs work » Reviewed & tested by the community
larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
CreditAttribution: larowlan at PreviousNext commented

Adding WimLeers to issue credits for significant issue summary updates, change record association etc

  • larowlan committed a16b9c7 on 8.5.x 
    Issue #2893778 by timmillwood, Sam152, Wim Leers: Remove deprecated...

  • larowlan committed 031d03b on 8.4.x 
    Issue #2893778 by timmillwood, Sam152, Wim Leers: Remove deprecated...
larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
CreditAttribution: larowlan at PreviousNext commented
Status: Reviewed & tested by the community » Fixed

Committed as a16b9c7 and pushed to 8.5.x.

Cherry-picked as 031d03b and pushed to 8.4.x.

Triggering a re-test on #2896721: Remove the concept of decorated states/transitions from worfklow type plugins. and #2896725: Entirely remove the concept of WorkflowType(Interface|Base)::initializeWorkflow()

Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers at Acquia commented

Clarified impact of this issue in the CR at https://www.drupal.org/node/2897706/revisions/view/10586181/10586340.

Status: Fixed » Closed (fixed)

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