Move workflow "settings" setters and getters to WorkflowTypeInterface

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.

@Sam152,

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

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

Back to needs review based on my previous comment.

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?

Updating to fix the issue of being unable to delete transitions, and providing better test coverage for that.

Unrelated fail, DrupalCI ran out of disk space.

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

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

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

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

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

Unrelated test fail, so back to RTBC

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

Fixing failing test.

This is marked as MUST-HAVE in the roadmap.

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

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

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

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

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

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.

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.

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.

oops, wrong file.

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.

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

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.

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

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.

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

Back to RTBC

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

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.