This is a spin-of this features issue: #1369246: Features incorrectly reporting panels as overridden. Pid counting changed.

Posted by merlinofchaos on January 24, 2012 at 9:17pm
Panels could probably be convinced not to scrub the pid if it's new-* -- that might not be too difficult of a patch if someone wants to pop over to the panels queue and give it a shot.

The attached patch should do exactly that :)

Comments

fabsor’s picture

Couldn't we use is_numeric instead of doing the substr? Extremely minor detail, but I think it makes for a bit cleaner code.

I will try this out in more depth soon.

das-peter’s picture

I'm absolutely fine with is_numeric() - frankly speaking I was just to lazy to evaluate if pid's have only numeric values besides the "new-" construct. ;)

merlinofchaos’s picture

is_numeric is probably better; that guarantees it was not a database pid and doesn't need rewriting.

I wonder if there's a case where we could have some panes in the database and some not; if we do partial rewriting we may duplicate pids by error with this. I'm not sure there's any case where we can do that, though. If there is we need to be careful about accidental pid duplication.

das-peter’s picture

Switched to is_numeric()

fabsor’s picture

Status: Needs review » Needs work

I tested this with NodeStream, which uses Page manager, panels, panels everywhere and mini panels, and this is the result:

* We don't get any "false" overrides in features, and the panels that we have not touched doesn't change when you export your panels again.
* Changing one panel makes only the pids in that panel change upon export.

From an exportability point of view this works properly.

I'm afraid this will break the issue that was the source of the change of the pid counting, #1277908: Introduce UUIDs onto panes & displays for better exportability & features compatibility however. Since only the panel that changes will get new pids, they won't be unique. I have to stress that the solution in there won't work that well either though, since it just works across one feature. The panels cache plugin already has support for other methods, like using a cache key if it's available, and I think it would make sense to make sure that we can use the cache key at all times rather than ever relying on the pid. I'm setting this to needs work, because I think we need to discuss a strategy for moving forward on that before commiting this.

jweowu’s picture

For my current project, the greatest pain (by far) caused by this issue is dealing with merge conflicts for a panel export with several variants.

A single pane added or removed in any variant will alter the numbering for all other panes, and it confuses git like nothing else when it tries to merge two sets of changes to that same export.

I can think of two things which might really help.

One would be if there was a way for the export to optionally generate multiple files -- one for each variant. That way, even though most of the pids may change, the VCS can't get all that confused unless more than one person changes the same variant. The only way I'm aware of for doing this at the moment is to export each variant into a different feature, but I understand that raises the problem of duplicate pids again.

The second idea is actually mentioned as the preferred solution in #1277908: Introduce UUIDs onto panes & displays for better exportability & features compatibility: replace the sequential numbered pids with unchanging unique identifiers (or similar -- would a hash value based upon the pane's definition + location be sufficient/appropriate?)

That should eliminate the version-control problems, and actually solve the original issue completely (which the current approach does not necessarily do).

I don't know what is involved in implementing this, but it seems to me that this is where any efforts to solve the problem should be spent?

jweowu’s picture

I can see that there is not enough context held in the $display object passed to panels_export_display() to generate a unique hash value for a pane, as identical displays are possible.

Would it be simple enough to include a reference to the parent handler in the display object, in order to provide the extra data? (that would be enough to identify it uniquely, yes?)

I realise that with that approach, any change to the handler data would affect all pids, but that's not so different to what's happening now.

Or would integration with http://drupal.org/project/uuid be the way to go? (It would be nice to avoid external dependencies, though).

edit: My initial experiments with hashing haven't been successful. When generating the hash I ignored the lack of context, and dropped properties which included one of the generated hash values, but I guess there must be other potential differences between $display objects for the same display depending on who was calling panels_export_display(), which makes me wonder if hashing is a viable approach after all (but I really don't know my way around this code, so I'm happy to be corrected!).

Does anyone have any thoughts on the UUID approach? I suppose if the UUID API module looked promising, integration could be optional, with the current static counter used as a fall-back.

edit 2: so uuid.inc provides uuid_generate() which uses other generators if available, but falls back to its own _uuid_generate_php() otherwise (based on http://php.net/uniqid#65879 )

So I'm guessing the thing is to generate a new uuid for panes when they are first generated through the UI, or whenever a pane with no uuid is imported from code. Then ensure that uuid is exported as part of the pane definition, so that imported panes keep the same id.

jweowu’s picture

Right, I have some working code for the UUID approach. drush features-list, features-update, and features-diff are all behaving nicely.

The basic approach was to add a new 'uuid' column to the panels_pane table (and so not touch the internal 'pid' column at all), and then use the uuid value for the exported code.

I need to clean it up and do more testing before rolling a patch for review, but I'm hopeful it will do the trick.

jweowu’s picture

jweowu’s picture

Issue summary: View changes
Status: Needs work » Closed (duplicate)