Workflow states don't get deleted when their workflow is deleted.

  // Delete the state. -- We don't actually delete, just deactivate.
  // This is a matter up for some debate, to delete or not to delete, since this
  // causes name conflicts for states. In the meantime, we just stick with what we know.
  // db_delete('workflow_states')->condition('sid', $sid)->execute();
  db_update('workflow_states')->fields(array('status' => 0,))->condition('sid', $sid, '=')->execute();

So what's the debate on deleting them? Why wouldn't we delete them when we're done with the workflow they're attached to? It's not like they'll ever get reassigned to a new workflow. Are they still necessary somewhere?

Comments

Ogredude’s picture

Ogredude commented
Assigned: Ogredude » Unassigned
Bastlynn’s picture

Bastlynn commented

This is how Workflow has worked up until now, hence my noting it as something to come back to but leaving it until I can get enough information to confirm that deletion won't completely bork something else for the user base.

So - if anyone has any reason why these states should not be deleted... speak now, or forever hold... you get the idea. ;)

Bastlynn’s picture

Bastlynn commented

This is a issue raised previously in #370347: workflow_states not actually deleted when removing a workflow - in which it sounds like this was an intentional choice: http://drupal.org/node/370347#comment-2666884

Problem is, I'm not sure why it was an intentional choice. Perhaps to make sure that the node_history table results don't suddenly become unreadable as a long term record just because the state is disabled? At that point, would it not be better to allow for a toggle enable/disable, and delete only after a warning about 'this will much up your node history display if you delete it'.

Bastlynn’s picture

Bastlynn commented

Dupe #1396014: Deleted workflow states still exists in rules

apmsooner’s picture

apmsooner commented

How about deleting them but providing a confirmation message that tells the user that deletion will also delete node history etc...? (not that i even think that is completely necessary but to explain it i guess).
I think thats the simplest way about this without having to setup another admin interface to assign alternate states to replace deleted ones. My opinion is if the user wants to delete the state, the node history for that state isn't critical to retain. If it was, then the user could simply rename the existing workflow state. My vote is for deleting them.

jvandyk’s picture

jvandyk commented

Perhaps to make sure that the node_history table results don't suddenly become unreadable as a long term record just because the state is disabled?

This is it. When using Workflow in an audit situation you don't want to destroy the state history of a node just because the workflow is being modified. The history log indicates that the node was in a certain state in the past, even if that state is no longer active in the workflow.

Bastlynn is exactly right: the correct thing to do here is offer a warning if the state about to be deleted exists in node state history, and in such a case confirm that the admin wants to go ahead and orphan the history entries.

See workflow_tab_page() in workflow_pages.inc for the code that will be affected. That function builds a list of deleted states based on the status being 0.

Bastlynn’s picture

Bastlynn commented

Another dupe #1412588: States are not properly deleted

At this point it's clear we have two sets of expected behavior going on here. I know keeping the record properly is how the code is intended (so we could close with working as intended) but this comes up often enough I think it would be a good idea to find a resolution between the two requirements. I'm going to think on this a bit and see what I can come up with.

jramby’s picture

jramby commented

Hi all,

I think that deleting the state is not really necessary even if we want to reuse the same statename for another workflow. This is just disable/enabling it. So, this should be a features to be added. Then we should be able to assign this state to the same or another workflow (like fields for a content type).

If we want to leave history in the database, we just disable the state.

The reason why I think this way is that, I deleted accidentally my state and I can't recreate it to rescue my error. And even creating and defining state acces of a new workflow using the same statename returns some validation errors. [Author should be able to pass a state from (creation) to at least one other state.] (or something like that... I'm in a fr version).

Thank you all, Jonathan.

Encarte’s picture

Encarte commented
Category: bug » feature

Like jvandyk said, keeping the record makes sense. Like Bastlynn said, it would be a good idea to find a resolution between the two requirements, since people often raise this question. I think jramby found the solution. Instead of deleting the state (which actually doesn't happen), the state should became disabled.

I can think of two different features to implement here:
- Changing user interface «Delete» expression to «Disable» expression and let the state be seen in a grey collor or something and with no links
- Introducing a new feature to re-enable the state, which doesn't exist today

I think this would be a more best practice solution than what we have today, which is somewhat misleading.

gnucifer’s picture

gnucifer commented

Also, states should be deleted (for real) when workflow is deleted, this does not currently seem to be the case. (It's quite frustrating when using rules, since all states (also "deleted") are displayed, including those in deleted workflows.)

Encarte’s picture

Encarte commented

@gnucifer the problem with the rules module is a different one: rules shouldn't use disabled workflow states. I think you should open a new issue (a bug report actually).

nancydru’s picture

nancydru
she/her/hers
Primary language English
Location Boston
commented
Status: Active » Postponed (maintainer needs more info)

As John indicates in #6, there is value in maintaining a proper audit trail. To that end, I would rather that workflows themselves never actually get deleted. That would require retrofitting a status flag there as well.

As to this request, there are numerous ways a state name may be displayed even if the workflow is no longer active. In those cases, deleting the state means a potentially broken display.

For those who are concerned about reusing a state, that is a non-issue. The same state name can exist in multiple workflows with no problem, except possibly to the admins.

John and Sarah have more experience with this module than I, so I will leave this to them.

nancydru’s picture

nancydru
she/her/hers
Primary language English
Location Boston
commented
Status: Postponed (maintainer needs more info) » Fixed

I have just committed a new Workflow_Cleanup module. I suggest due caution in using it.

nancydru’s picture

nancydru
she/her/hers
Primary language English
Location Boston
commented
Status: Fixed » Closed (fixed)

Included in 7.x-1.1-rc1

nancydru’s picture

nancydru
she/her/hers
Primary language English
Location Boston
commented
Issue summary: View changes

Edit post