Reverting a feature doesn't revert DS layouts

I can confirm this is the case, after upgrading to the dev release to fix the variables table problem.

Specifically, any ds based on a ds custom build mode wont export, and build modes declared in code export but do not rebuild.

Actually, it seems inconsistent. Ive had some Teasers which rebuild and some which dont, for example.

Ive almost tracked this one down...

Normally, a display entry in the ds_settings table has quasi-compound key including a module showing which module originated it (e.g. nd, cd, vd). When a feature is reverted, it is written to the ds_settings table with the feature module instead, creating a duplicate. So something like:

record 1: nd, story, teaser, array...

becomes

record 1: nd, story, teaser, array...
record 2: my_feature, story, teaser, array...

When retrieving a display, the calling module (nd in this case), requests the settings so it can build the display and passes itself, the type and the build-mode to select the display. This will always select the first, unexported display. As long as you keep making changes to the nd display and exporting them, you'll never notice that that the two are not different, because the records stay in sync.

The problem comes when you make changes to a feature on one site then try to revert a feature on another (e.g. to push ds changes from development to production). In this case, record 1 and record 2 are out of sync, and reverting the display will only reset record 2. Record 1, which ds is retrieving, will always be different.

There's a couple of ways to solve this, and Im still thinking through what might be most effective. While Im working on a patch, if anyone has any ideas please pitch in.

It could well do. The way ds implemented Features was a little bit awkward, because it was trying to save an entry in the variable table, and because node displays are, in a way, a secondary namespace. The new ds_settings table implemented in #1168130: Backport D7 data storage techniques for display settings does not have a primary key, making it - at least conceptually - somewhat incompatible with Features.

The approach I'm working on is to add a machine name and numeric ID to the ds_settings table, then rework the Features implementation around that. I haven't decided whether to go the whole hog and use ctools exportables, or just adapt the existing methods. I will probably try it without ctools first and see if that is workable, however ctools would give us some extra UI tools around managing displays (look at the way Context manages its context list, for example), which may be useful, and would be mostly "free".

The slightly complicated part is ensuring that any patch not only updgrades cleanly from 6.x-1.x, but also fixes any 6.x-2.x installations as well. Working through it should reveal whether #1300478: New DS data storage causes loss of ND layouts during update. is related or not...

I seem to have the same problem in 7.x-1.3. My problems also seem similar to the other issue mentioned in #5.

I didn't mean to change the original issue's version. Should I create another issue for 7.x-1.3?

No need to create a new issue. If an issue spans multiple versions, normally it gets fixed in 7.x first, and then gets backported to 6.x. To keep track of issues that need porting you can add the 'Needs backport to X' tag.

However seeing that progress has been made here in the 6.x version it is maybe better to do it the other way round :)

Also, I'm not responsible for features update/revert/insert - whatever action there is - that's all handles through Ctools' features integration - or the other way around - so this is only applicable to the D6 branch.

I take it that means the 7.x branch uses ctools already?

xtfer, yup, the D7 branch was completely rewritten with that in mind.

I spent a considerable amount of time refactoring to use a machine_name and a lightweight Class, and normalise its usage across display suite, and I'll hang on to this for another patch as I think its an improvement... After doing about 10 hours on it, however, I suddenly realised that the revert issue could be fixed with the addition of 3 characters.

ds_features_revert was passing the module name (i.e. the feature name) to the ds_settings table, not the ds provider modules name. This patch corrects that and also adds an additional SQL DELETE to catch those spurious entries created by previous (failed) revert attempts.

This patch is against 6.x-2.x, but should also work against 6.x-1.x.

Oh my - what typos can do heh. Jstoller could you confirm this one ? If so, xtfer can commit this :)

There are some big changes in 2.x-dev to try and complete multigroup, but THIS patch (which also applies to 1.x) ONLY does the following:

When a Feature is reverted...

1. Delete any existing displays which are registered for the display handler (e.g. "nd")
2. Delete any existing displays which are registered incorrectly with the feature name
3. Write the display definition from the Feature correctly

Because the displays were mixed up by the error relating to module names, the act of reverting may well change your layouts, though in theory they should be correct after the update.

Committed to 6.x-2.x & 6.x-1.x

Finding that patch in #13 is failing due to whitespace errors. Resubmitting...

This was fixed in both 6.x-1.x and 6.x-2.x over a year ago...