Features import / revert broken in 2.5+

This approach looks promising! I'm currently experimenting with changing transition permissions between dev -> staging. But I bumped into a problem: On the development site, where the feature was originally created, using drush to update the feature containing the workflow definition (drush -y fu example_workflow) would actually revert the feature instead of update it. I had to update the feature using the browser.

But otherwise it works beautifully! Using machine name to export workflow states to features is the future!

I found a small bug in the patch, on a fresh install adding a brand new workflow yields:

Warning: reset() expects parameter 1 to be array, null given in Workflow->unpackStates() (line 222 of /Library/WebServer/Documents/drupal7/sites/all/modules/workflow/includes/Entity/Workflow.php).

It looks like we need an isset() check, I'll reroll the patch.

Rerolled with the isset added.

Hmm, now I'm seeing a more important bug: If I have two feature modules with an exported workflow in each, when I enable the second module, I end up with two copies of all its transitions! I'm not sure what's going on, I'll look into it.

Hmm. So this is all happening after the normal load from defaults. Instead our workflow gets saved a second time during the cache flush, here's the call stack where the problem occurs:

drush_module_enable()
drupal_flush_all_caches()
module_invoke_all('flush_caches')
entity_flush_caches()
entity_defaults_rebuild()
entity_save()
Workflow->save()

When we get to the "Delete all existing transitions" step, the old workflow is fetched from the entity cache, and for some reason the cached workflow has no transitions. So we fail to delete the old transitions, but then go ahead and recreate them all.

And I can't for the life of me figure out why the wrong transitions are in the cache. Ideas, anyone?

For now, I'm using a little sandbox project that I put together: https://www.drupal.org/sandbox/evolvingweb-vasi/2493673

It basically just takes over export of workflows, and does similar things to GuyPaddock's patch.

@vasi,
in Workflow::save(), try replacing $this->states by public function getStates($all, $reset = TRUE)
This will bypass buffers.

The patch works brilliantly. However, I have some workflow states with long names which will make workflow transition names too long, because the column is only 32 bytes long.

I've added a patch that changes workflows machine names to varchar 64 (which is now 255) and workflow transition machine names to varchar 255 (which is now 32). This way we can be sure that the workflows names aren't too long when concatenated and '_to_' added.

@bartvig, please open a new issue with your patch. Thanks.

Well, the patch is related to patch #10, and my patch was wrong as it should have included patch #10.

I've included the full patch and the interdiff.

Ok, I've fixed up GuyPaddock's patch a bit. It basically uses the same export code, but completely changes the way revert (import) works. Highlights:

Some changes:

• It now works with multiple features!
• We sort the export, so we get a better features-diff
• We use the same code for features-revert and reversion using the UI
• Editing a state/transition now marks the workflow itself as "overridden" in the UI
• Both old- and new-style features continue to work.

I've tested a variety of situations, but I'd love to see more testing. Thanks so much to GuyPaddock for the export code, that helped a ton.

We are also having this problem trying to use features for version control on our dev team.

I attempted to use #19 patch to our workflow-7.x-2.5 install. I was able to apply the patch and create a feature, but upon rebuilding/importing that feature, received the following error:

Reverting feature module 'our_feature' dependencies...
Fatal error: Call to a member function getName() on a non-object in /build/sites/all/modules/workflow/includes/Entity/Workflow.php on line 226

I tried to apply patch 18 using the dev version of workflow, but that threw errors in using the workflow (not in features).

I finally got a combination of workflow-7.x-2.5 + patch #18 to work for us. I was able to apply the patch, export and the rebuild/import the feature AND keep workflow functioning at every step.

@hwasem, can you post the resulting patch?

Hi johnv,

I used patch #18 as is, I just applied it to the recommended workflow version 2.5 instead of the dev branch. If there is anything else you need in way of a patch, I'm not sure how to produce that. I'm happy to do so, I'll need some guidance on what you need.

Thanks,
Heidi

Patch #19 works well here.

In fact, if it works for me and author, maybe this would be better status.

@hwasem please test with dev branch.

Seems to work here for transitions.

In our case however the exported workflow field is broken when I revert, because of wid changing. Apparently you can work around this by updating the wid' => 1,' line of your export code in hook_field_default_field_bases. Not a very practical workaround, but it can save you should it ever happen in production.

Not sure if I should post this comment on #2152435: [EXPORTING] Export of Workflow Field using machine_name, not $wid, I agree that we should fix it on this side of the code rather, in order not to break existing sites.

Using workflow field, after a feature revert, even if you fix the bad wid on the field definition, the sid on workflow_states table change, so all the data on existing nodes (and their revisions), gets corrupted, since it is refering to workflow states sid's that no longer exist. This translates into:

Fatal error: Call to a member function showWidget() on a non-object in workflow/workflow_field/workflowfield.formatter.inc on line 55

This is obviously a deal breaker for workflow field and features right now, you end up with a broken site on your first feature revert.

So for workflow field and features to work, all I can think of is that we should NOT be changing the wid or the sid, (if that's a possibility). Or perhaps rework workflow field to store machine names of workflows and workflow states, instead of wid and sid.

@mansspams you say it works for you, are you using workflow field?

@Manuel Garcia,

using the dev version of workflow and applying patch #19, I'm able to properly export and enable/import a feature that has a workflow, workflow_access settings and includes a workflow_field base and its instances.

Before this patch I encountered the problem you (and this issue) described, the SID's were constantly changing. Interestingly, this was because my local environment where the workflow was developed has db auto incrementing set to 1 (so each new SID is incremented by one), but on the Acquia environment this was deployed to, the DB has auto incrementing set to 5 (each SID will be 5 higher than the previous). This means a) the feature will never reach a default state, and b) each time it is reverted the SID's will get recreated a step higher.

This posed a particular problem because workflow_access correctly stores the the workflow and state by machine name in the feature, so it never looks like there's a problem, and no overrides are shown. However in the database table, the access settings are stored by SID. When the workflow is reverted, the SID's move forward in the primary table and suddenly the access permissions tables foreign keys don't line up with their primary key complements.

To your questions:
After applying the patch in #19, the export now has machine names for the states, and correctly imports them, and does not rebuild the SID each time. This fixes all of my problems, and workflow_access remains happy afterwards.

I checked the workflow_field base that I exported, and noted this:

...
    'module' => 'workflowfield',
    'settings' => array(
      'allowed_values_function' => 'workflowfield_allowed_values',
      'allowed_values_string' => '1 | (creation)
2 | Draft
3 | Review
4 | Approval
5 | Published
6 | Deleted',
      'entity_translation_sync' => FALSE,
      'history' => array(
        'history_tab_show' => 1,
        'roles' => array(
          -1 => 0,
          1 => 0,
        ),
      ),
      'watchdog_log' => 1,
      'wid' => 1,
      'widget' => array(
        'comment' => 0,
...

What is interesting to me is that the "allowed values string" contains a string representation of the settings from my local instance where I created the workflow. Those key values are totally different on the destination environment, but the feature somehow thinks this is just fine. Presumably that's because of the "allowed_values_function" that is being used, which must supersede the string.

The other thing I noticed is what you are specifically concerned about and cited the other issue about: The WID value is the numeric WID of the workflow. This is definitely a problem, and really just tells me that I got lucky that this workflow I enabled/imported is the first and only workflow, and therefore this field lines up with the desired destination workflow. Had I declared a workflow in a different feature that was enabled before my current one, these fields would reference the wrong workflow entirely.

That is definitely a problem, but maybe not one for this issue, which is tackling the problem of SID's rotating on each revert. I would say that fully functional features support in the workflow module will certainly require both of these issues to go in before things are 100% ready, but each one can probably be separately handled.

With that being said, I'm going to +1 the "RTBC tag" for this patch, and ask/beg the maintainers to commit it!

Thanks a lot @netw3rker for your in depth explanation!

Let's get this one in then, and focus next on #2152435: [EXPORTING] Export of Workflow Field using machine_name, not $wid

This is committed. Thanks guys!

Great news @johnv thanks!

Should we set this to 'needs work' or 'active' again?

I understand that exporting/importing now works as long as ths id's are equal in both systems.
Then, still to be done:
- export workflow with machine_name
- #2152435: [EXPORTING] Export of Workflow Field using machine_name, not $wid
- separate exports of workflow and workflow_field (they were combined the last year, leading to several comments here and elsewhere.)

The exporting of workflow and Workflow Field definition is now decoupled. See #2375261-7: [EXPORTING] Workflow exported to new feature even if it already exists in an other feature

I believe that this remains a significant issue, as both Workflow Field instances and Workflow State view filters refer to specific SIDs, and these references are broken whenever the Workflow component of a feature is reverted.

The solution from comment #19 cannot be applied to the current code base.

I confirm this is still an issue. I wasn't able to deploy workflow config with Features - the feature is always overriden and the workflow doesn't get created on the target site.

Same here with version 7.x-2.8

Could we use the "export" functionality provided by this module and use that to be included thanks to a custom module? Would it be possible at all? If so, how?
Just thinking as a workaround as for now this module is not suitable for production...

EDIT: we actually cannot, as the "Import" functionality is broken too AFAICT

#19 re-rolled for 7.x-2.8

Just to comment that patch #41 solved my problem with exporting/importing workflow data to/from a feature.

Thanks @bleedev.

Rerolling against the latest 7.x-2.x

This also fixes a bug introduced by #41 where an install of a feature containing an exported workflow gives the following error:

PHP Fatal error:  Call to a member function getName() on a non-object in /vagrant/docroot/sites/all/modules/contrib/workflow/includes/Entity/Workflow.php on line 163

Hi klaasvw,

Thank for the patch. Some simple issue there in PHP 5.4 version array declaration is changed.
So kindly check my patch.

@harivenu_zyxware .. array() is still needed, because workflow supports Drupal and Drupal supports PHP 5.3 and PHP 5.3 doesn't support Short syntax for arrays :( .. array() is still valid in even PHP 7.0

Because of that, patch #43 is the better than #44, see also interdiff

Used patch #43 on a prototype site and it worked perfectly with Workflow (dev) and Workflow Access. Noticed only that using the module 'role_export' could break the feature, but not a show stopper.

Didn't check the code.

Quiet a big patch! Thanks all. Patch #43 was committed.