Allow non-destructive reverting of Flags in Features [#968450]

Currently, the Features reversion code for Flag simply deletes the Flag. Since the data is tied to the Flag ID, this causes a loss of the link between the existing data (that wasn't deleted) and the Flag once it's recreated with its new Flag ID. The attached patch adds a rebuild() method to the flag_flag class that performs a non-descructive rebuild of the flag by deleting only the flags and flag_types rows, getting the default version of the flag, assigning the existing fid to that flag, and re-saving it. It also changes the features.inc to use the rebuild() method instead of delete().

I have not yet tested this, but I expect to do this tomorrow. I just wanted to get this in here.

Comment	File	Size	Author
#8	969372_flag_revert.patch	2.34 KB	mooffie
#6	969372_flag_revert.patch	1.27 KB	mooffie
#5	969372_flag_revert.patch	1.01 KB	hefox
#2	flag_features_revert-nondestructive.patch	1014 bytes	mgriego
	flag_features_revert_nodestruct.patch	1.36 KB	mgriego

Comments

Comment #1

mgriego commented 11 November 2010 at 16:11

Just realized I flubbed part of the patch. There's actually no real reason to delete the flag from the database before re-saving it, and doing so is actually problematic. Reworking the patch and testing it now.

Comment #2

mgriego commented 11 November 2010 at 16:43

Status	File	Size
new	flag_features_revert-nondestructive.patch	1014 bytes

OK, here's the revised patch. Obviously, I was a bit tired when I rolled the original patch last night. After reviewing and testing, it's now much simpler and works better.

Comment #3

mgriego commented 23 December 2010 at 17:25

Reporting back that we've been using this patch on a production site since the last comment (Nov 11), and it's been working great. We've been able to revert features without the loss of Flag data.

Comment #4

mooffie commented 26 December 2010 at 01:38

We have a duplicate issue: #969372: flag_features_revert doesn't really do what I'd except it to do

(I had the impression it was you who started it, perhaps because it was reported almost on the same day, so I didn't bother to notify "you" earlier.)

(I like that other issue's patch better because it doesn't have a loop.)

Comment #5

hefox commented 26 December 2010 at 05:25

Status	File	Size
new	969372_flag_revert.patch	1.01 KB

Bah, how did I miss this issue? Here's the patch from that issue (marking that one as duplicate)

Comment #6

mooffie commented 26 December 2010 at 10:28

Status	File	Size
new	969372_flag_revert.patch	1.27 KB

Here's hefox's patch with minor changes:

- Removed "&& $this->name".
- Changed drupal_clone() to clone() to be compatible with D7. Flag 2.x is php5 only. I don't think clone is needed but it's logical.
- Reset flag_get_flags()'s cache.
- Documentation.

One potential problem:

We're doing "flag_get_default_flags(TRUE)" to load disabled flags as well. But then we have the problem described here, of writing to database a structure of an older API we aren't compatible with.

Perhaps we should define an is_compatible() method that tells us if the flag is compatible with the current Flag version, and save() and export it only if it is.

Comment #7

mgriego commented 26 December 2010 at 16:04

Oy. Looking back, I'm not sure why I did that loop over all the flags. *sigh* Oh well. Looks pretty much the same except for that loop.

With a potential is_compatible() method, what would happen if someone had a Flag that was overridden, and the in-code version was from an older API version, but the database version was current?

Comment #8

mooffie commented 27 December 2010 at 16:09

Status	File	Size
new	969372_flag_revert.patch	2.34 KB

($flag->is_compatible() was committed.)

Here's an updated patch.

what would happen if someone had a Flag that was overridden, and the in-code version was from an older API version

See tha patch: The revert operation won't proceed and the user will see an error message explaining this to him.

[...] if someone had a Flag that was overridden, and the in-code version was from an older API version, but the database version was current?

(BTW, the "database version" is always current: it was created when your Flag module was old, and, as with all the other database flags, the "upgrade paths" in the .install file keep it up-to-date.)