Summary updated 13 Dec.,2012

Current patches

Adds UUIDs to CTools: http://drupal.org/files/ctools-uuids_for_exported_objects-1277908-90_0.p...
Replaces pids in Panels with UUIDs: http://drupal.org/files/panels-uuids_for_exported_objects-1277908-100.patch

Caching of Panels that were Feature Exported on multiple pages is broken

Export of Panels leverages CTools export code. The generated pid's (new-1, new-2, etc) currently start over with every page, causing collisions with other pages.

Proposed resolution

Replace pid's with UUIDs.

Remaining tasks

1. Decide where UUID code should exist. Require the UUID module? copy/paste code from uuid.inc?

2. Update functions for the following modules that programmatically go through and load all known objects and re-save them to generate uuids:
panels_mini
panels_node
panelizer
page_manager

as per sdboyer #81:

in this vein, i would also like to see update functions for panels_mini, panels_node, panelizer, and page_manager that programatically go through and load all known objects and re-save them to ensure that uuids are generated at install-time. i strongly dislike having to carry around logic in the core codebase that is designed to accommodate multiple possible data states. panels code is messy, but that doesn't make it OK to make it worse.

jweowu comments in #92:

If we are saving all displays and panes from the panels_display and panels_pane tables, does that not cover the other modules listed? Those are the two tables to which a uuid column has been added, and I had thought that all the displays and panes for those other modules mentioned were present in these two tables?

Remaining question: Does saving the panels in the patched update code cover the modules listed, namely panels_mini, panels_node, panelizer and page_manager?

3. Prevent UUID's from being duplicated (e.g. by cloning or importing a display), while still ensuring that they remain unchanged in other circumstances.

as per sdboyer #81:

yes, this is a *crucial* question in all this. uuids are supposed to...well, uniquely identify the same object. it's right there in the name. in the case of cloning, it is pretty clear that the goal is to produce a copy, which means that the uuids should change as the identifier linkage between the cloned object and its source should be broken. with imports, however, it is less clear. i haven't entirely thought this through, but my gut says that we should add a checkbox for either retaining or regenerating uuids on import to all of the import forms, then have the behavior default to regeneration.

Remaining questions: Does cloning need to be solved with this patch? Does importing need to be solved with this patch? Would sdboyer's suggestion of offering a "regenerate UUIDs" checkbox on import (while always regenerating UUIDs on cloning) solve the problem

User interface changes

none

API changes

Adds a UUID generation and validation to the CTools API.

Original report by ASupinski:

Caching of Panels that were Feature Exported on multiple pages is broken

Essentially the Features has implemented the "export" of Panels by using the ctools export, in case you are not familiar with Features module, it is basically using a in-code version of entities that might otherwise be stored in the database. The generated Pid (new-1, new-2, etc) currently start over with every page, this causes duplicate pids when these meant to be temporary pids are used in caching and every page has at least a "new-1" pid. This basically caused the first panels cached of a certain pid/layout to be displayed on all pages with that pid/layout combination.

Proposed resolution

In the patch I am about to submit I am suggesting that we make the temp id increment throughout an export rather than on a per-page basis. This is not perfect since multiple features could include panels and the conflict could then happen again but on my current site we only use one feature. The ultimate "fix" would either be in always using some for of UUID for the temporary Pid.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

ASupinski’s picture

Promised Patch

Letharion’s picture

Assigned: Unassigned » merlinofchaos
Status: Active » Needs review
merlinofchaos’s picture

Ew globals!

Let's not introduce new globals. That's what drupal_static() is for.

Letharion’s picture

Assigned: merlinofchaos » Unassigned
Status: Needs review » Needs work
ASupinski’s picture

Status: Needs work » Needs review
FileSize
783 bytes

Modified patch, thanks, I had not heard of drupal_static yet.

Letharion’s picture

Assigned: Unassigned » merlinofchaos
merlinofchaos’s picture

Status: Needs review » Fixed

Committed;

for some reason the patch didn't apply, it claimed it was corrupted even though it looked okay to me visually. I applied it by hand.

jweowu’s picture

Cross-linking to #1369246: Features incorrectly reporting panels as overridden. Pid counting changed.

drush features-list now incorrectly shows features as Overridden because the pids no longer match.

merlinofchaos’s picture

Since CTools exportables has a flag that expressly says what the status of the exportable is, features is in error for not using that.

Letharion’s picture

Assigned: merlinofchaos » Unassigned

Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for 2 weeks with no activity.

jweowu’s picture

Status: Closed (fixed) » Needs review
FileSize
8.33 KB

Here's a first pass at using Universally Unique Identifiers in place of sequential numbers for exported panel panes (as suggested by ASupinski initially).

As well as resolving the stated problem of conflicting pids when there are multiple features, this also resolves a major problem with version control for exported panels (which was actually exacerbated by the current solution for this issue).

With the current release of Panels, adding or removing a single pane would cascade changes throughout the remainder of the file, and merging changes by different developers would invariably result in conflicts which were tedious to resolve.

By using persistent UUIDs in the export, the diffs are reduced to only the parts of the panel that actually changed.

I referred to http://drupal.org/project/uuid for a reliable implementation of a UUID generator and validator.

jweowu’s picture

Trivial re-roll so that the new functions do not fall under the "deprecated" comment.

das-peter’s picture

I really like this uuid approach! It would solve some headache I've with #1179034: Translatable panel titles: Implement i18n_strings
Currently I can only deal with displays / panels that are stored in the DB - uuids would really change the game :)

das-peter’s picture

I've extended the patch from jweowu and added uuids to the displays as well.
First smoke-tests look good.

das-peter’s picture

@jweowu: I just thought about this code: if (empty($pane->uuid) || !panels_uuid_is_valid($pane->uuid)) { in the export.
Shouldn't we resave the display / pane if a uuid was created / changed? Because atm. a re-export would lead to changed uuid, right?

jweowu’s picture

(merged two replies together)

jweowu’s picture

> atm. a re-export would lead to changed uuid, right?

For panes in the database which do not yet have uuids, that's correct.

uuids are only made permanent when the panel is saved (either via the web interface, or an import like a features-update from an exported file) and, aside from introducing the new column to the panels_pane table, nothing in my patch writes to the database. I didn't want to make any significant changes to the existing behaviour, and frankly I don't know enough about ctools and panels to feel comfortable that there wouldn't be any unwanted side-effects to adding extra panel saves.

Therefore if you do have panes which do not yet have uuids, every export of that panel (while it is in that state) will generate a new uuid for each of those panes. As soon as the panel is saved or updated from that export, the uuids will be made permanent.

I believe the only time I've really noticed this was when doing repeated "drush features-diff" for a feature containing a panel pane that did not already have a uuid.

I didn't see this as a notable issue, as I only anticipated it being a transitional concern. If this is a problem, maybe saving all the panels to the database in an update hook would be an acceptable approach?

das-peter’s picture

Thanks for the detailed reply. That's indeed the behavior I had in mind.

I've now fixed (I introduced an error) and extended the update function. It fetches now all displays without an uuid from the db and resaves them. This will also trigger a save of the related panes.

jweowu’s picture

That all sounds good to me. I'll try to review & test your changes sometime soon, but I'm pretty short on time at present, so it might well take a few days. (Hopefully your i18n_strings project will get some more eyes on this, though!)

davidwhthomas’s picture

Hey, this idea of using UUID works really well.

It ensures Panels is always using unique ids.

Note, it doesn't depend on the uuid module, but rather uses the same functions to generate and validate the UUIDs for Panels ( via the well documented function panels_uuid_generate() {} ) so the patch doesn't add any other dependencies.

+1

lee20’s picture

Patch in #19 worked well for me.

jweowu’s picture

I've reviewed the code changes in #19 (over #13), and I updated to it earlier in the month without any issues, and it's all looking good to me. FWIW I've not encountered any issues running these patches, and the trouble-free VCS merges have definitely made my life a lot easier :)

das-peter: Obviously additional input from others would be good, but between us we could probably consider this RTBC, if you don't have any other concerns?

das-peter’s picture

jweowu: We use it in a heavy feature based project and it works like a charm. I also know from other people using this to be able to use i18n_panels. From my perspective this seems RTBC too.

Only thing I lately came across was that panels doesn't define a key / key name in the schema addition export and thus sometimes notices occur.
Without any further analysis I decided to add this to the install file:

@@ -79,6 +95,8 @@ function panels_schema_3() {
       'export callback' => 'panels_export_display',
       'can disable' => FALSE,
       'identifier' => 'display',
+      'key' => 'uuid',
+      'key name' => 'UUID',
     ),
     'fields' => array(
       'did' => array(
@@ -136,6 +154,8 @@ function panels_schema_3() {
       'can disable' => FALSE,
       'identifier' => 'pane',
       'bulk export' => FALSE,
+      'key' => 'uuid',
+      'key name' => 'UUID',
     ),
     'fields' => array(
       'pid' => array(

Notices are gone and no side-effects occurred so far.

merlinofchaos’s picture

So we need an updated patch which includes #24?

jweowu’s picture

I don't believe I've encountered those notices. In which circumstances are they issued?

das-peter’s picture

As far as I remember the notices occurred when I was reverting panels.
According to ctools/help/export.html:

key
This is the primary key of the exportable object and should be a string as names are more portable across systems. It is possible to use numbers here, but be aware that export collisions are very likely. Defaults to 'name'.

Thus the uuid seems to fit perfectly. However, I'm not sure if key / key name really are required to defined "exportables".
If these values are required the current definition is wrong, if not the handling shouldn't throw notices if this values aren't present.
Whatever applies, something has to be patched :)

samhassell’s picture

Here's a patch combining #19 & #24.

Working great for me!

jweowu’s picture

We'll need to make additional changes to the docstrings as well. I edited them for the original pane uuid patch, but we need to ensure they encompass the new display uuids too.

chrisschaub’s picture

Any chance #28 can get committed soon?

mikeker’s picture

@cschaub: This issues needs to be marked as RTBC before it'll get committed (and etiquette says that those that made the patch cannot mark it as RTBC) . So, if the patch in #28 worked for you, make a note saying that you've done some degree of testing on it or reviewed the code and if you feel it is ready to be committed, change the status to RTBC. If you've only been able to do limited testing (eg: "it works for the one site I was having this issue on") then let us know the details so that others can try and test different scenarios. Once consensus is reached and the issue is RTBC'ed then it's got a good chance of being committed.

Having said all that... Attached is an initial port to D6. The D7 code all looks good -- I'd mark this as RTBC except for the fact that I haven't actually tested it in practice as I don't have D7 panels project active at the moment.

My particular use case involves rearranging panel variants and how they are saved to Features. More testing coming soon...

mstrelan’s picture

I would say that #28 works very well. I tested various combinations of editing panels (adding / removing panes) and recreating and reverting features. The only issue that I ran in to was that I had to revert existing panels before I could update my feature, otherwise I would continuously regenerate new uuids and my feature would constantly be overridden. I wonder if the update script could force creation of a UUID for existing panes, I believe that would do the trick.

jweowu’s picture

mstrelan: Are you saying the panels_update_7302() added with this patch is not sufficient?

mstrelan’s picture

I applied the patch in #28 and ran update.php. My existing page manager page that was in already feature did not get uuids applied to it until I edited it and saved it. Before I had done this features kept saying my feature was overridden, even after recreating the features. After editing, saving and recreating the "overridden" warning went away.

merlinofchaos’s picture

I don't think it will be possible to assign UUIDs to panels that exist only in code until they are re-exported.

jweowu’s picture

mstrelan: Are you saying that you had a panel without uuids that was in code only, features was consequently listing it as overridden, and when you recreated the feature (without first editing and saving it into the database) you still ended up with code without uuids?

I'm not in a dev environment at the moment, but I didn't think that would happen?

If it is the case, should we consider saving code-only panels into the database as well? The features would still be labelled as overridden until recreated, but it should ensure that recreating them was successful.

Or is forcibly saving code-only panels to the database an undesirable thing to do?

merlinofchaos’s picture

It could be problematic since code-only Panels could live in a number of different applications. Page Manager and Panelizer at the very least.

mstrelan’s picture

I had two features, one had a page manager panel and one had a panelizer panel. I first focussed on the page manager panel, and I'm going from memory here, but each of its panes were in the database, with a NULL value for uuid. I may have overridden the defaults before applying this patch, I'm not too sure.

My feature showed up overridden so I recreated it. It showed up overridden again, with a different set of uuids. After I edited the panes and resaved the panel the panes had uuids in the database. I then exported my feature once more and it no longer showed up as overridden. I then tried editing the panes to make it show up as overridden, and this worked perfectly fine. I then exported it and it showed up as not overridden, so again working perfectly fine.

I then tried to reproduce the scenario with Panelizer panels. I can't remember the exact steps I had to take to get them to export/import correctly but it definitely involved reverting them via the Panelizer interface.

jantoine’s picture

The patch from #28 works flawlessly. In my testing I have two servers, dev and prod. I applied the patch on dev, recreated the feature, applied the patch to prod, uploaded the recreated feature and successfully reverted prod.

acbramley’s picture

I've tested patch #28 and have had some problems:
I have 1 feature with 2 variants of node_view in it, these were in code prior to patching. I patched panels, edited both variants and saved (so they were in the database), reexported the feature (I tried this both through drush and the web interface), only to find that my feature was still overridden. I've tried repeating the edit, save, export steps but I just get regenerated uuids for all panes and the feature is still overridden after export.

mstrelan’s picture

Status: Needs review » Needs work

#40 sounds very similar to my experience.

davidwhthomas’s picture

@acbramley #40 & mstrelan #41

Make sure you flush all caches after applying the panels uuid patch.

Also, if the panel display or pane already has a uuid set, it won't get a new one.

We have a similar node_view panel with several variants that export fine with this patch, using the same export method you mentioned.

I hope that helps,

regs,

DT

Letharion’s picture

The patch in #28 has a problem in the install, it skips schema version 4. A single character in difference, but an important one.

jweowu’s picture

Thanks, Letharion. That one was my fault, I see :/

I circumvented this sort of copy/paste error in Views by making the schema update functions call $schema = views_schema(__FUNCTION__); and letting the code figure it out. Maybe that approach would be useful here as well? (although not in this issue, obviously).

acbramley’s picture

Thank you Letharion, using your path completely fixed my problem :) reexporting my panels after applying the new patch added in $pane->locks to my panes and made features no longer overriden!

jweowu’s picture

Ah, that sounds promising.

mstrelan, are you able to confirm that?

mstrelan’s picture

As much as I want to confirm it ... no. I managed to revert my database and code to a state where I was constantly getting overridden features and the uuid column definitely didn't exist. I then applied the patch from #43 and my panels_display table now had the uuid column with all NULL values. I recreated my feature and it showed up as overridden. It was not until I manually edited my panels page that my panes got uuids, and then after exporting it was no longer overridden.

Anyway, I don't think that should block this from getting in, because it is now possible to fix constantly overridden features. I just wonder why it is that my panes and displays in code did not get uuids when update.php ran.

jweowu’s picture

That's a shame. It would certainly be good to sort this out if possible, before a merge. I've been pretty flat-out on other things of late, so if anyone else is willing and able to investigate, please do. Otherwise I'll look into it myself once other things have calmed down a bit.

Letharion’s picture

The patch I posted in #43 should _not change any functionality_ compared to #31. 31 just had a minor mistake that under certain conditions would break the "lock" update hook. Unless you were executing both the older 7302 update hook at the same time as the the one from this patch, the patch makes no difference what so ever.

acbramley’s picture

Sorry, it's happening to me again. Every export regenerates all uuids and even after export shows an overriden feature until reverting it.

mstrelan’s picture

@acbramley - any chance we can get a dump of your database as well as indication of which patch you've got applied? I am happy to do some more debugging but it takes me a while to revert my database back to that state.

acbramley’s picture

@mtstrelan I'm using patch #43 I can't give you a copy of my db sorry as it's a client project. That patch is applied to Panels 7.x-3.2, I'm using the node_view page manager handler with a handful of variants for different content types. Nothing crazy custom in the panels either, just views and node fields.

I had the panel variants in features prior to patching I then:
1) Patched panels
2) Cleared cache and updbd
3) Edited and saved all variants
4) Reexported the feature (tried both through web interface and using drush)

And started getting the issue of regenerated uuids on every export, plus an overridden feature straight after the export. Let me know if there's any other information that might be helpful.

jweowu’s picture

FWIW, my dev project is presently running Panels 7.x-3.0 with the patch from #19, and we're not experiencing these problems.

If people want to throw database dumps around, I would suggest setting up a new minimal-case test site to experiment on, both for speedier restores, and to exclude any real data.

merlinofchaos’s picture

This is a patch I'm sort of keeping my eye on, but not really looking closely at it as long as it's in needs work. I'm noticing that status doesn't seem to agree with the last couple of comments.

mstrelan’s picture

Assigned: Unassigned » mstrelan

#52 and #47 suggest that this needs work. I am going to do some debugging today. I don't believe we have exact steps to reproduce the issue from a fresh database but I'm hoping to work that out today.

mstrelan’s picture

Status: Needs work » Needs review
FileSize
10.63 KB

Ok I'm happy now with this patch. I am yet to figure out how to reproduce this problem with a fresh database, but I have an existing database with the issue as does acbramley.

The only problem with the previous patches are that panels_save_display($display); in the update script would not actually save the changes to the database, for what reasons I'm not sure. I put some debugging statements in panels_update_7202() and they showed that uuids were generated, but after the update I checked the database and the columns were NULL. I then ran the second half of panels_update_7202() from #43 using devel module execute php function and my panels were given uuids and saved in the database. It would appear that splitting the update function in to 2 functions resovles this problem.

@acbramley - please test this updated patch. After running update.php you simply have to recreate your features, or run drush fu-all, and they should no longer appear overridden.

mstrelan’s picture

Sorry, that last patch doesn't return anything from the second update. Try this one instead.

acbramley’s picture

Really unsure how to test this on my existing database now, tried downloading panels-7.x-3.2 and applying patch #57 to it then reexporting my panels but I still have the same issue. Maybe I should be uninstalling panels completely first, install unpatched panels, patch and updb? I have a feeling that since I've already applied some db updates some things might be going wrong. I will test more thoroughly when I have time.

acbramley’s picture

Really unsure how to test this on my existing database now, tried downloading panels-7.x-3.2 and applying patch #57 to it then reexporting my panels but I still have the same issue. Maybe I should be uninstalling panels completely first, install unpatched panels, patch and updb? I have a feeling that since I've already applied some db updates some things might be going wrong. I will test more thoroughly when I have time.

mstrelan’s picture

tried downloading panels-7.x-3.2 and applying patch #57 to it then reexporting my panels but I still have the same issue.

Did you run update.php after patching and before re-exporting?

Also after running update.php please check your database in the panels_pane table and see if the uuid column is all NULL. It should be populated with uuids.

scottshipman’s picture

Title: Introduce UUIDs onto panes & displays for better exportability & features compatibility » Caching of Panels that were Feature Exported on multiple pages is broken

subscribing.

Patch #57 works for me in my development environment. Haven't tested in my other environments.

dimitriseng’s picture

Does anybody know if this is planned to be commited to Panels soon? This is a prerequisite for getting #1179034: Translatable panel titles: Implement i18n_strings working. I could try using the patch here but it includes database updates and would not like to do this unless this is going to be definitely commited. Many thanks.

jweowu’s picture

It appears to be causing problems for some users (but not others) as per the previous comments, so I think it's unlikely to be committed in its current form (unless that problem is tracked down to some other cause).

As with anything, test it against a copy of your database. If you don't experience issues regenerating uuids, then everything else seems solid.

lotyrin’s picture

Status: Needs review » Needs work

For what it's worth, I also got the issue, on one environment out of 4 tested. (Predictably, it was production.) Simply re-running the update hook solved the problem.

I haven't been able to reproduce the issue in a instrumented environment, but something is certainly amiss.

samhassell’s picture

I just had the same issue as acbramley in #40. The UUIDs were regenerating on every update and my features were listed as overridden when I was using 'drush fu myfeature'.

Not sure why, but when I ran 'drush fua' it seems to have fixed the issue.

The site is using panels everywhere, which may be related?

jweowu’s picture

It looks as if cloning a variant clones its uuids as well, which seems like a bad thing.

jweowu’s picture

I'd set an incorrect uuid length in the schema from the outset (I believe I was unnecessarily accounting for a "new-" prefix), and for whatever reason this was not resulting in padded uuids on my system, and so I never spotted the error.

Naturally it is doing so on other systems, which meant that the uuid validation regexp wasn't matching; hence all the uuid regenerations being reported.

Here's a re-roll of #57 with a fixed schema, and a new update hook to sort out those column lengths for people who have applied previous versions of the patch.

For existing databases with uuids, my understanding is that reducing the column length from 40 to 36 chars will retain the 36 chars we actually want, but I would suggest that you back up your database first to be safe.

I looked briefly at the cloning issue, but I'm not addressing that here, as I realised it was more involved than I initially thought. There are a few ways that a pane or an entire display can be cloned or imported, and I'm not sure if there's a single place at which we can eliminate duplicates? I didn't have time to look into this properly.

wiifm’s picture

Patch #67 has improved the exportability of panel panes *a lot*. Now we do not regenerate UUID's when exporting panels

acbramley’s picture

Woohoo!! Updated to this patch #67, added a pane and reexported and I got no regenerated uuids. Nice work @jweowu

jweowu’s picture

I'm glad that seems to have done the trick.

I've found that the reason that problem didn't crop up for some of us is that MySQL's default treatment of char columns is broken. They added an option to fix its behaviour in 5.1.20, but that option remains switched off by default.

http://dev.mysql.com/doc/refman/5.1/en/server-sql-mode.html#sqlmode_pad_...

http://bugs.mysql.com/bug.php?id=24424

samhassell’s picture

Can we RTBC this now?

Simmol’s picture

I tested the patch on one of our Projects and it is working fine.
It fix the described problem and don't break anything else.

So i think is ready.

lotyrin’s picture

Status: Needs work » Needs review

This should at least be CNR if not RTBC.

jweowu’s picture

I think the final question is: how do we prevent uuids from being duplicated (e.g. by cloning or importing a display), while still ensuring that they remain unchanged in other circumstances?

The problem for which this issue was originally raised was that the repeating sequence of "new-1", "new-2", etc... for the exported pids was causing issues with caching. ASupinski wrote that "This basically caused the first panels cached of a certain pid/layout to be displayed on all pages with that pid/layout combination."

The current uuid solution certainly deals with that issue if we ignore the possibility of cloning (as well as doing a better job in the case of multiple features), and it handily resolves the version control issues of the old system, but if cloned uuids are possible then presumably the original caching problem can still occur?

That needs to be verified, and appropriate solutions established.

Is anyone able to enumerate the different ways in which panes and displays could be cloned?

sylus’s picture

Is it possible to create a new issue for jweowu concerns to do with cloning and caching and still move this patch forward. I have tested this patch for i18n_panels as well and works like a charm. I'd rather get this feature in and deal with the specific clone + caching bug in a separate issue as it seems far more wide stretching and could potentially derail i18n_panels for months while that specific regression is trying to be fixed.

Of course I can be shot down on this thought but definitely want to move the yard stick further on this as i18n panels is a required for me and definitely want it to be promoted to full project status or be integrated back in main.

wiifm’s picture

Also keen to see this patch committed, it is a huge leap forward from stock panels, and has been tested to work on both MySQL and PostgreSQL. +1

acbramley’s picture

+1 to get this committed!

das-peter’s picture

@jweowu: While cloning has definitely to be supported I'm not sure if importing the same export (uuids) twice should be considered as an "user error" (wrong way of cloning).
However, I know the following cloning locations - and I think we should be able adjust them as done in the code below:
panels.module:

  function clone_pane($pid) {
    $pane = clone $this->content[$pid];
    $pane->uuid = panels_uuid_generate();
    return $pane;
  }

panels\plugins\task_handlers\panel_context.inc

/**
 * When a handler is cloned, we have to clone the display.
 */
function panels_panel_context_clone(&$handler) {
  $old_display = panels_panel_context_get_display($handler);
  $code = panels_export_display($old_display);
  eval($code);
  foreach (array('display', 'did', 'css_cache', 'temp_layout') as $item) {
    if (isset($handler->conf[$item])) {
      unset($handler->conf[$item]);
    }
  }
  $display = new StdClass;
  $display->uuid = panels_uuid_generate();
  $display->did = 'new-' . $display->uuid;
  $handler->conf['display'] = $display;
}

If you're okay with that approach I'll update the patch.

jweowu’s picture

Thanks, das-peter. I do have a nagging feeling that something expects that plain 'new' value (but I'm not certain of it). Let's give it a whirl and see what happens, and if there aren't any issues then it's a good improvement at the very least!

edit: Yes, try grepping the panels code for 'new'. You would need to be certain it was safe to make that particular change, but you also might not have to do it??

das-peter’s picture

Re-rolled patch with the suggested changes including the ones mentioned by jweowu in #79.

Let's hope we can get this in soon :)

sdboyer’s picture

Title: Caching of Panels that were Feature Exported on multiple pages is broken » Introduce UUIDs onto panes & displays for better exportability & features compatibility
Status: Needs review » Needs work

really glad to see people working on this very important problem. retitled appropriately. some comments:

+++ b/panels.install
@@ -375,3 +395,77 @@ function panels_update_7301() {
+function panels_update_7304() {

While i understand a number of people might be running this patch locally, there is no real reason to have an update function that fixes another update function which has not yet been committed. and actually, from looking at this, 7305 seems completely superfluous as it's simply reusing that which is defined in panels_schema_5(), and therefore will have no additional effect beyond 7303.

in this vein, i would also like to see update functions for panels_mini, panels_node, panelizer, and page_manager that programatically go through and load all known objects and re-save them to ensure that uuids are generated at install-time. i strongly dislike having to carry around logic in the core codebase that is designed to accommodate multiple possible data states. panels code is messy, but that doesn't make it OK to make it worse.

so yes, that'll mean associated patches for panelizer and ctools.

+++ b/panels.module
@@ -1731,6 +1740,63 @@ function _panels_builder_filter($layout) {
+function panels_uuid_generate() {

i think i'd rather have this in ctools. who knows who else might need simple, standard uuid generation but doesn't want a full dependency on the uuid module.

I think the final question is: how do we prevent uuids from being duplicated (e.g. by cloning or importing a display), while still ensuring that they remain unchanged in other circumstances.

yes, this is a *crucial* question in all this. uuids are supposed to...well, uniquely identify the same object. it's right there in the name. in the case of cloning, it is pretty clear that the goal is to produce a copy, which means that the uuids should change as the identifier linkage between the cloned object and its source should be broken. with imports, however, it is less clear. i haven't entirely thought this through, but my gut says that we should add a checkbox for either retaining or regenerating uuids on import to all of the import forms, then have the behavior default to regeneration.

also, with such discussions such as #1557842: Allow Feature module exports to add Panes to Existing Panels, it becomes more complicated because we may want to have pane uuids stay the same while the display uuid changes. we don't have to solve that problem in this issue for it to be committed, but it is something people invested in this issue should be aware of.

all of these are comments based on not having installed & locally tested the patch, but i plan to do so presently. so, know that this is on my immediate radar, and i would really like to commit it, but it is a foundational component of panels, so it's not going in until it's really, really right.

jweowu’s picture

Thanks for getting involved, sdboyer.

Update 7304 isn't as superfluous as it looks (for users who had installed a prior version), as I modified the uuid column definitions in schema_update_5 at the same time.

You're quite right about adding multiple update hooks of course, and I expected it would all be consolidated into a single update before being merged, but I thought that in the short term it would make things easier for people testing the patch if it automatically fixed that bug for them (see comments 67 and 70), as it had been a bit of a show-stopper on non-MySQL databases.

mstrelan’s picture

It would seem to me that the issues I was experiencing should be fixed by #1647894: Features with page manager components are perpetually overridden in the Features module.

Darren Shelley’s picture

I can confirm that #1647894: Features with page manager components are perpetually overridden also solves the issue for me and has been committed in to the dev branch of features.
Thankyou mstrelan for taking the time to continue to research this problem and for cross-linking the issue queues.
I am currently using features-7.x-1.x-dev on several sites and have yet to experience a red herring override as I did with panels in the previous release of features.

A heads up to people upgrading to the newest features, you will need to feature update (drush fu featurename) in instances where you have for example a panel and mini-panel as the existing export will be incorrect. Upgrading features alone will not be sufficient.

sylus’s picture

Would it be possible to get an explicit point a point walkthrough of what is still missing from this patch? Obviously this issue has been held up for a long time and until it is resolved Panels i18n will not get committed (the translatable panel titles is done and is only waiting on this issue).

Additional hooks: #1179034-41: Translatable panel titles: Implement i18n_strings
UUID support: #1277908-19: Introduce UUIDs onto panes & displays for better exportability & features compatibility

Ideally if we could focus on the bare minimum of work that is needed and resolve any nice to have features in future patches. While I agree there is definite value in getting this done right and the correct way. There is also value in logicially breaking down the steps and moving the yard stick where possible.

Am I correct in that these are the issues holding this up?

1) Fix the update functions before merge as one is a bit superfluous
2) Update functions for the following modules that programmatically go through and load all known objects and re-save them to generate uuids:

  • panels_mini
  • panels_node
  • panelizer
  • page_manager

3) Move function panels_uuid_generate() { to ctools
4) Answer the question: How do we prevent uuids from being duplicated (e.g. by cloning or importing a display), while still ensuring that they remain unchanged in other circumstances.

jweowu’s picture

sylus: Yes, I think that list is accurate.

1, 2, & 3 could probably be tackled at any time.

Regarding 4, I was hoping that sdboyer would have some follow-up comments to #81 after trying out the patch, and could provide some recommendations on how to proceed wrt the issue of duplicate uuids.

berdyshev’s picture

Any update on this? What we should do to resolve this issue, since there is another issue (with i18n Panels integration) which is waiting the fix of this issue.

dimitriseng’s picture

Hi, I am also interested in the i18n Panels integration but that depends on this issue. As per #85, I also agree that this should be done in a proper way, but it would be great if we could split this and get the bare minimum working first so that the i18n Panels issue can progress. Many thanks.

davidwhthomas’s picture

I'm using this Panels UUID patch and working fine, fwiw.

jweowu’s picture

I've implemented items 1 & 3 from the list in comment 85.

This means we now have a ctools patch (for the UUID functions), and a panels patch (with the remainder of the code changes). We will also end up needing a patch for the panelizer module, for item 2 on that list.

As per item 1, I've reduced the update hooks to a single panels_update_7302(). This means that if you are already running an earlier version of this patch, you should revert your schema_version for panels back to 7302 in the system table of your database, to ensure that future updates to panels will run correctly.

Leaving as "needs work" while the other items are outstanding, but please feel free to test this version out and confirm that I didn't break anything. Remaining items are:

1) Done.
2) Update functions for the following modules that programmatically go through and load all known objects and re-save them to generate uuids:

  • panels_mini
  • panels_node
  • panelizer
  • page_manager

3) Done.
4) Answer the question: How do we prevent uuids from being duplicated (e.g. by cloning or importing a display), while still ensuring that they remain unchanged in other circumstances.

jweowu’s picture

jweowu’s picture

Actually, do we need additional update functionality?

In comment #81, sdboyer wrote:

in this vein, i would also like to see update functions for panels_mini, panels_node, panelizer, and page_manager that programatically go through and load all known objects and re-save them to ensure that uuids are generated at install-time.

If we are saving all displays and panes from the panels_display and panels_pane tables, does that not cover the other modules listed? Those are the two tables to which a uuid column has been added, and I had thought that all the displays and panes for those other modules mentioned were present in these two tables?

(Or did you mean that we should also load displays which exist only in code, and save those to the database as well? If so, I'm worried that might hinder people more than it helps, so it might need some discussion...)

I do note that the current update hook assumes that any pane without a UUID must live in a display without a UUID. I'm not sure if that's a 100% valid assumption, or if there could be edge cases where this doesn't hold? It would be easy to additionally query the panels_pane table for the DIDs of all panes without a UUID, and combine that with the DIDs from the panels_display query before re-saving those displays.

jweowu’s picture

Added-paranoia update hook, as per last paragraph in previous comment.

I won't keep uploading the same ctools patch, but it's probably sensible to include a link to the other patch(es) whenever one of them is updated.

ctools-uuids_for_exported_objects-1277908-90_0.patch

mikeker’s picture

Status: Needs work » Needs review

Based on the last two comments... Thanks for your work on this @jweowu.

jweowu’s picture

Status: Needs review » Needs work

mikeker: It most definitely still needs work, as per #90.

However I do think we're on hold until we can get some guidance regarding the duplicate uuid issue (see #81), and exactly what the update hook needs to do (see #92), so Needs Review may be somewhat appropriate in that respect.

jweowu’s picture

Status: Needs work » Needs review

Oops. I had intended to leave that status alone...

berdyshev’s picture

Status: Needs review » Needs work

@jweowu, thanks for your work.

+++ b/panels.installundefined
@@ -375,3 +395,72 @@ function panels_update_7301() {
+    $msg[] = t("Added panels_display.uuid column.");
...
+    $msg[] = t("UUID column already present in the panels_display & panels_pane tables.");
...
+    $msg[] = t("Generated UUIDs for database-based panel displays and panes.");
...
+    $msg[] = t("No database-based panel displays or panes for which to generate UUIDs.");

Use single quotes instead of double ones.

jweowu’s picture

Status: Needs work » Needs review

Thanks BerdArt. I realise there's a tiny performance benefit to single-quoting, but is that actually a concern in a one-time update hook?

I'm going to set this back to "needs review". I don't mind making the change if the maintainers want that quoting style, but it'll be simple enough to roll it into the next revision.

In the meantime, getting answers to the outstanding questions seems more important, so until then I'd prefer not to change this to "needs work" unless something more significant is found wrong with it.

lotyrin’s picture

Status: Needs review » Needs work

Yes, please fix the quotes.

As little practical (performance) reason as there is to change them, there's a significant policy reason, and even less reason to leave them as they are.

Kristen Pol’s picture

Status: Needs work » Needs review
FileSize
8.43 KB

I have replaced the double quotes to single quotes in the patch to move this along.

SocialNicheGuru’s picture

i get this error when I apply the patch
PHP Fatal error: Call to undefined function ctools_uuid_generate() in panels/panels.module on line 661

jweowu’s picture

Now that we have multiple patches, it's important that whenever anyone updates one patch, they provide a link to the other(s) in the same post. Otherwise newcomers are going to get confused.

The current patches are:

ctools-uuids_for_exported_objects-1277908-90_0.patch
panels-uuids_for_exported_objects-1277908-100.patch

lotyrin’s picture

Status: Needs review » Reviewed & tested by the community
sylus’s picture

Status: Needs work » Reviewed & tested by the community

The patches do work. However I was still getting a "The display new has no uuid, please resave or re-export it."

So I think we are still missing a few things (or another issue?):

2) Update functions for the following modules that programmatically go through and load all known objects and re-save them to generate uuids:

panels_mini
panels_node
panelizer
page_manager

After manually re-saving a few of the panels everything was working at the least. ^_^

jweowu’s picture

Status: Reviewed & tested by the community » Needs review

No, this isn't RTBC (although it does seem that current functionality is pretty solid).

As per #90, there is still work to do in two areas. However the issue needs some review from people who know Panels well in order to ascertain exactly what needs to be done for the remaining pieces of work.

mikeker’s picture

Issue summary: View changes

Updated issue summary.

jweowu’s picture

Status: Reviewed & tested by the community » Needs work

Setting this back to CNW, as obviously it's confusing otherwise.

mikeker’s picture

I agree with your earlier designation of CNR -- otherwise it won't get the attention of the powers-that-be, which we need before proceeding.

I've updated the issue summary (please feel free to add/edit/etc) in hopes of making this issue easier to digest.

jweowu’s picture

Title: Caching of Panels that were Feature Exported on multiple pages is broken » Introduce UUIDs onto panes & displays for better exportability & features compatibility

While we wait, it would be good to get some discussion happening on this one:

In comment #81, sdboyer wrote:

in this vein, i would also like to see update functions for panels_mini, panels_node, panelizer, and page_manager that programatically go through and load all known objects and re-save them to ensure that uuids are generated at install-time. i strongly dislike having to carry around logic in the core codebase that is designed to accommodate multiple possible data states. panels code is messy, but that doesn't make it OK to make it worse.

And I responded in comment #92:

If we are saving all displays and panes from the panels_display and panels_pane tables, does that not cover the other modules listed? Those are the two tables to which a uuid column has been added, and I had thought that all the displays and panes for those other modules mentioned were present in these two tables?

(Or did you mean that we should also load displays which exist only in code, and save those to the database as well? If so, I'm worried that might hinder people more than it helps, so it might need some discussion...)

I have since concluded that the latter most likely was what sdboyer intended, given the stated intention of ensuring that no displays or panes were left without a uuid; but while I do agree that it would be ideal, my main concern is whether that is actually a practical goal?

Specifically, if we save code-only displays (with added UUIDs) to the database, I suspect it would be fairly straightforward for a site to revert to the code-only (and UUID-less) versions of any such panels.

For instance, consider a site using the Features module and a contrib feature which provides a panel display in code. If our update hook saves that display (with added UUIDs) to the database, that feature is now over-ridden. If the site admin then does a feature-revert, they lose the UUIDs.

Consequently, I'm not sure that we can safely assume that all panel displays will definitely have UUIDs after we run our update hooks (even if that is true immediately after the update hooks run).

If we concede that this isn't a safe assumption to make, and that we do need to allow for UUID-less displays & panes, then I believe the current update hook is sufficient.

If OTOH we decide that we must find a way to enforce UUIDs, then I'm guessing we need a way to prevent a database display from being reverted back to its code-based default. I don't know whether any mechanism exists for achieving this?

The actual usage of the UUIDs may well factor into this discussion. My own purpose in adding them was solely to avoid some horrendous version-control issues when dealing with exported panel code (and in that context the UUIDs are entirely optional); but clearly the UUIDs have proved useful for #1179034: Translatable panel titles: Implement i18n_strings and perhaps other issues as well, and I don't know whether or not these other uses will be relying upon the presence of UUIDs everywhere?

mikeker’s picture

jweowu, thank you for moving the conversation forward.

The actual usage of the UUIDs may well factor into this discussion.

For what it's worth, my use case for UUID's regarded rearranging panes within a panel didn't export well. I'll have to look into my notes to see what the specifics were (I haven't had to deal with it for months since I backported the patch to 6.x). I'm currently on a Panels-based project with multiple devs so I'm sure I'll benefit from the headache of trying to merge changes to exported code.

Re: Features-based exports: once UUID's are added, I believe any features code with a Panels export will be listed as permanently overwritten. The code will have pids while the database has UUIDs, if they revert to code there will still be a mismatch between code and DB. The only option will be for them to update the feature code to include UUIDs.

While that sounds like a pain, it is to be expected that they may need to update features with exported panels if they upgrade Panels code. Hopefully some upgrade documentation (or a search-engine friendly issue queue entry) can alleviate some of the headaches.

Based on that, I believe the update code you have added is sufficient -- Panels in the DB are updated, those in code are not and require an update to the code. As far as I know, the case where a non-UUID panel in code is running alongside UUID-based panels in the database has not been tested. If someone has had a chance to verify that situation works, please chime in.

jweowu’s picture

Re: your last paragraph (and when considering this issue in isolation) it's certainly the case that a non-UUID panel can work alongside one with UUIDs with no ill effects.

The UUIDs actually have no effect on the existing functioning of Panels (avoiding side-effects was important, as I didn't know much about the Panels code base). They get added to the database, used for importing/exporting, and loaded as object properties; but otherwise they'll be ignored, meaning it doesn't matter if they exist or not for a given display or pane -- unless other people write code which requires them; so it falls back under the "do (or will) we need to enforce them?" heading.

lotyrin’s picture

Confirm that in-code state which may not have UUIDs does not interfere with my interest in this patch, which is for localization of (otherwise language-neutral) strings of panels_node/panelizer content which is created and managed in the database.

merlinofchaos’s picture

First, I'm sorry I haven't been able to look at this. It's a complicated issue and I've not really been able to take the time to delve into it. However, I think I have enough to move this issue forward now.

Question: If there is no UUID associated with an in-code panel pane, is there a way to generate one that will always be the same for that pane? For example, with md5 I would've used md5(serialize($pane)) and that would reliably generate the same ID. We could then do that as part of the load process to ensure a UUID always exists; either stored or generated upon demand. Code to do it would be relatively easy to accomplish, I believe.

In looking at the code, the proposed ctools_uuid_generate() function doesn't take a seed, however, so it doesn't look like that is possible with the current uuid. However, a quick google search shows a perl script that MIGHT do the trick: http://www.perlmonks.org/?node_id=645873

I'm not sure that CTools should take on a uuid function. uuid.module has one, for example, and it's superior to what's being put into CTools.

if (module_exists('uuid')) { 
  $uuid = uuid_generate(); 
} 
else { 
  $uuid = panels_uuid_fallback_generate(); 
}

However, if we use the above method we'll need our own anyway. Maybe/maybe not that should be in CTools. If it *is* it should go into includes/uuid.inc and use a ctools_include('uuid') to acquire it.

jweowu’s picture

Issue summary: View changes

Cleaned up formatting

jweowu’s picture

I initially looked at hashing the pane objects to generate the IDs, but if memory serves I quickly ran into the problem that the properties of a given pane object can vary in different situations (in ways which are not important to the rest of the functionality, but which prevent them from reliably being hashed to produce the same value).

I guess with code-based panes that's not a problem? I can see the potential benefit, although my gut reaction is that this could complicate the issue quite a bit.

In addition, if we used hashing and it was possible for separate panes with identical configurations to hash to the same value, I presume we could then run into the caching issue over which ASupinski originally raised this issue.

The current implementation has simplicity on its side (and also seems to be working pretty well), so I'm biased towards it for those reasons; but if there's a way to improve things then I'm all for it. I can't remember exactly what happened when I was initially playing with the hashing idea, so I might have been approaching it all wrong.

That aside, I think the biggest issue is still how to definitively deal with duplicate UUIDs resulting from cloning panes or displays. This problem exists regardless of how the UUIDs are generated, and if we could sort it out then it would be a huge step forward.

das-peter dealt with some of this in #78/#80, and I think sdboyer's comments on this in #81 are the closest thing we have to a plan for moving forward on the problem, but we really could use more help in confirming a solution and working out the exact implementation details.

merlinofchaos’s picture

We could potentially just hash the combination of type, subtype and configuration; those won't vary too much. Though I suppose we'll get a lot of exact duplicates of each other, and that won't help so much either. :/

jweowu’s picture

I'm not sure if any of the following is a good idea, but here are some thoughts that came to mind on the notion of repeatable IDs for code-based panels.

Firstly, what's the easiest way to generate a unique identifier for each code-based display & pane? (I'm not actually sure how we identify them as code-based, but I'll assume that's easy?)

$handler->name seems like it's unique; is that actually the case? If so, then maybe that combined with a counter (i.e. (name)-(n) for the nth pane in the display) would suffice as a unique identifier.

If we don't actually care about 'proper' UUIDs for code-based displays, then we could probably use that directly as the pid-(ID) in the exported code.

If we do care, maybe we could use that unique identifier to seed the random number generator? The bit I'm (very) unsure of is whether we could do this without compromising the randomness of the other UUIDs, and if there isn't a safe way to negate that concern then I definitely wouldn't want to do it.

A unique ID can be hashed to an integer (whether via md5 + base_convert, or some other hashing algorithm), which could be passed to mt_srand() when exporting a code-based display, which would result in a repeatable UUID sequence for the code-based display and its panes.

However, if we're seeding to fix the sequence for code-based displays, we would presumably need to call mt_srand() with no seed value before exporting non-code displays to re-randomise the sequence, and it seems to me that in seeding the generator, our (otherwise exceeding random) UUIDs all become entirely dependent on that one seed value, which might conceivably throw up a duplicate?

If we're not seeding, then we don't care about the chance of a duplicate, because the UUIDs are built from eight separate random numbers -- so making the chances of a collision (and indeed an entire sequence of collisions) dependent on a single random number would be an incredibly bad idea.

(Or does that risk effectively already exist with the initial seeding that PHP performs? :/ Ugh; I'm going to stop thinking about it for now...)

merlinofchaos’s picture

$handler probably doesn't exist down at the display level. Panel displays can be embedded in a lot of things unfortunately, and it can be difficult. Sometimes there is a $display->owner_id -- and we can use that to help with seeding.

In an ideal world, these uuids would become permanent, but I don't think that's a critical factor. Once written to the database, they get their permanent uuid. So I think giving them an obviously placeholder uuid might actually be just fine, as long as it's repeatable so it can be identified. It does mean, however, that we run the risk that any in-code display without proper UUIDs, when translated, might lose its translations when written to the DB. However, I think that is probably an acceptable consequence of the limitations we are working with.

And in that case, it means we could probably use some kind of md5 for the UUID. And even more, that means we could probably even put up a message in any UI that relies on the uuids and let the user know they need to be re-exported. We would then need to make certain that the export process will ensure UUIDs are assigned (in case they export directly from code and it never hits the database. It can happen).

OnkelTem’s picture

CTools patch mentioned in #102 doesn't apply on latest Ctools dev. Please re-roll.
Panels patch mentioned in #102 doesn't apply on latest Panels dev with patch from: #1179034-41: Translatable panel titles: Implement i18n_strings

OnkelTem’s picture

Re-rolled patch from #90 agianst the latest CTools.

The panels-uuids from #100 is merged with panels-add-hooks from #1179034-41: Translatable panel titles: Implement i18n_strings and published here: http://drupal.org/node/1179034#comment-7216342 The reason for this: these two panels patches don't apply since they patch same lines of code.

logaritmisk’s picture

Patch from #118 + http://drupal.org/node/1179034#comment-7216342 worked like a charm with ctools-1.3 and panels-3.3. Nice work!

Tested this on a quite features heavy site, and there was no problems updating or reverting existing features.

SocialNicheGuru’s picture

in features there is a way to have a base field definition and an instance of that field.

This way many features can implement the same field by defining a field instance without creating a conflict.

will this patch allow the same for panels?

Use case:

I create a content pane as part of a core feature

I want to then use that content pane in other features. I would just need to define the core panel feature in the .info files of the others. That way I don't have to recreate a similar content_pane multiple times.

dixon_’s picture

Assigned: mstrelan » Unassigned

@SocialNicheGuru No, this patch won't solve that for you. That's another feature request ;-)

But as @logaritmisk said, the patch from #118 + #1179034-80: Translatable panel titles: Implement i18n_strings works great and solves the problem for me.

However, I'm not sure the patch goes inline with @merlinofchaos's ideas in #116. So I'm not sure how we should move forward here.

manuelBS’s picture

#118 works for me, too. Thanks for that work!

saltednut’s picture

Issue tags: +demo_framework

Why is the ctools patch from #118 in this issue? This should have been moved into a ctools issue.

The directions in #123 are kind of confusing.

A [META] issue would be extremely helpful here though I guess the i18n_panels page does provide those instructions.

Here is a makefile snippet if anyone needs this working right now and doesn't feel like reading two queues w/ a combined 200+ comments. :)

projects[ctools][version] = "1.x-dev"
projects[ctools][type] = "module"
projects[ctools][subdir] = "contrib"
projects[ctools][download][type] = "git"
projects[ctools][download][revision] = "e81da7a"
projects[ctools][download][branch] = "7.x-1.x"
; Introduce UUIDs onto panes & displays for
; better exportability & features compatibility
; (ctools patch from panels queue)
; https://drupal.org/node/1277908#comment-7216356
projects[ctools][patch][1277908] = "https://drupal.org/files/ctools-uuids_for_exported_objects-1277908-118.patch"

projects[i18n_panels][version] = "1.x-dev"
projects[i18n_panels][type] = "module"
projects[i18n_panels][subdir] = "contrib"
projects[i18n_panels][download][type] = "git"
projects[i18n_panels][download][url] = "http://git.drupal.org/sandbox/daspeter/1444130.git"
projects[i18n_panels][download][revision] = "37a796b"
projects[i18n_panels][download][branch] = "7.x-1.x"

projects[panels][version] = "3.x-dev"
projects[panels][type] = "module"
projects[panels][subdir] = "contrib"
projects[panels][download][type] = "git"
projects[panels][download][revision] = "2bb470e"
projects[panels][download][branch] = "7.x-3.x"
; Translatable panel titles: Implement i18n_strings
; https://drupal.org/node/1179034#comment-7216342
; Introduce UUIDs onto panes & displays for better
; exportability & features compatibility
; https://drupal.org/node/1277908#comment-6771122
projects[panels][patch][1179034_1277908] = "https://drupal.org/files/panels-1179034-41_____panels-uuids-127790-100__-80.patch"
acbramley’s picture

Nice! Maybe we could maintain this make snippet in the summary?

acbramley’s picture

Issue summary: View changes

Restored some of ASupinski's original text, as the discussion of the caching problem had been lost in the more recent edits.

japerry’s picture

Issue summary: View changes
Status: Reviewed & tested by the community » Needs review
Issue tags: +commonslove, +commons 7.x-3.6 radar

Progress! I'm currently waiting on action from a ctools maintainer to commit the ctools patch here:

https://drupal.org/node/2155825

I've merged in the i18n_panels sandbox to be a submodule for panels, as well as the patches. They are now in a new panels branch called 7.x-3.x-i18n

Please review this branch, and pending the ctools patch, we might be able to get this into panels!

das-peter’s picture

@japerry That are great news!!
I'll try the branch asap.
About the i18n_panels submodule, if that doesn't "fit" into panels itself we could add it to the i18n module.
For now I kept it just as sandbox because it was not really usable out of the box. But as soon as the required patches are committed I could talk to the other i18n maintainers and if there are no objections commit the module to i18n.

japerry’s picture

If the i18n folk are willing to put it in their module, I'm fine with that. I'm personally a fan of i18n support being a part of the panels module, but either way, I don't mind.

mikeker’s picture

Status: Needs review » Reviewed & tested by the community

I don't see anything in the recent comments that would keep this from remaining RTBC...

japerry’s picture

Issue summary: View changes
Status: Reviewed & tested by the community » Needs review

The remaining work that needs to be done is deciding/reviewing where UUIDs will lay. Its becoming increasingly needed that we put a dependency on the UUID module. The UUID patch to ctools is no good, its a half-assed hack to get some type of UUID identification in, and has been marked as buggy. #2155825: Add UUID generation functionality to CTools

I don't feel comfortable about basically adding the whole uuid.inc file into ctools or panels, it just feels janky. That inevitably means that panels will need to rely on UUID in order for it to work OR we will have to support both pids and UUIDs (which, as sdboyer has said -- that just makes a mess even worse).

Once UUIDs is figured out, I think we can get this to a committed state.

mikeker’s picture

Thanks for the clarification, all fair points. Though, in defense of the original UUID implementation, it was the same as used in the UUID module at the time -- the hope was to avoid an additional dependency for Panels. Good to know that #1596350: _uuid_generate_php() does not create valid UUID v4 UUIDs. fixed the incorrect implementation in UUID.

At this point, we have these options as I see it:

  1. Reroll the CTools part of the patch with the new implementation used by the UUID module
  2. Create a dependency on the UUID module in Panels
  3. Ignore sdboyer's advice and include a lot of if (module_exists('uuid')) crap throughout Panels.

Clearly, I don't like #3.

#2 seems like the right way to go (don't duplicate code, keep the nitty-gritty of UUID generation with those that know the most about it), though it makes for an upgrade headache on the next release of Panels.

#1 is not terrible. Yes, we are duplicating UUID generation and validation code between contrib modules. But we avoid the update headache by not changing dependencies.

It would be lovely to get some input on this from the maintainers... Sam? Earl? If anyone following this issues sees them in IRC, please give them a nudge in this direction!

jweowu’s picture

It sounds like it will be painless to change to the correct UUID format -- https://drupal.org/comment/6067746#comment-6067746 states that the PECL uuid_is_valid() function accepts both versions, and the custom validation function does as well, so regardless of how this winds up being implemented, it would seem that there's no harm in making the change.

Introducing the uuid module as a dependency is certainly a call for the maintainers to make. As mikeker points out, the initial aim was avoid introducing any such dependency -- at the time it seemed to me that a panels patch which introduced a dependency on another contrib module was extremely unlikely to be accepted; however it's since become clear that we definitely want this functionality committed, so the dependency may be more palatable at this point.

I don't mind too much either way (although I also haven't checked uuid module to see how lightweight it is in this scenario, and my gut feeling is that we should be fine without it).

jweowu’s picture

I'll also say that it's not too surprising that the original source implementation got the position of the version wrong. I reviewed the relevant parts of the RFC in order to follow this up, and there are inconsistencies in that document, such that it's not actually clear where the version bits should be!

Some searching turned up the following discussion, which confirms that the change made in the uuid module is correct: http://www.pwg.org/archives/ipp/2013/017477.html

japerry’s picture

Here is a panels patch that applies to the latest code thats been committed to panels.

As for comments in #131:

I strongly disagree that panels should be relying on UUID module. UUID does much more than just generate UUIDs, it places uuids on entities, nodes, etc. Many sites do not need this functionality, and thus we shouldn't include it.

To copy and paste the uuid.inc file is not really a problem. I'm following up this issue with a patch in the ctools queue that will add a uuid.inc file to ctools, do a check to see if uuid or pecl uuid_generate is already there, and if not execute our own code. Its a very small snippet, and it will make sure ctools is consistant with at least the current version of uuid.

I've unlisted the other patches. We really don't want to be using that ctools patch that generates invalid UUID v4 ids.

SocialNicheGuru’s picture

This is the ctools issue with patch. https://drupal.org/node/2155825

I'm posting it because it just makes it easier to find.

jweowu’s picture

Or if you write it as #2155825: Add UUID generation functionality to CTools we can observe the status from afar! Which is now "Fixed", as it's been committed :)

japerry’s picture

Status: Needs review » Fixed

Better yet, this code is committed!

Any further bugs should be opened up in specific issues.

http://drupalcode.org/project/panels.git/commit/bab02c0

das-peter’s picture

This is awesome!!!

jweowu’s picture

Holy cow :)

I guess there will be some spin-off issues, but this is certainly a good way to get people looking at them :)

das-peter’s picture

I'll be happy to help dealing with any upcoming spin-off issues ;)

davidwhthomas’s picture

Woohoo! (that is all) :)

pfrenssen’s picture

Thanks very much, this is great news!!

Anonymous’s picture

Update fails with: Fatal error: Call to undefined function ctools_uuid_generate()

jweowu’s picture

morningtime: you need to update ctools.

(hmm. I guess we could throw an update exception if that function doesn't exist.)

tim.plunkett’s picture

hook_update_dependencies() FTW

jweowu’s picture

No, there's no ctools update to depend upon.

I'm also not certain whether h_u_d even throws an exception if a dependency is missing? I've never used it in that situation. An initial glance seems to suggest it gathers the information, but doesn't act upon it.

jweowu’s picture

jweowu’s picture

We could also set dependencies[] = ctools (>1.3) in panels.info, but I've no idea how that behaves for -dev versions; the documentation doesn't cover it. We might actually need a recognisable "-alpha" ctools release to reference, or some such?

Hmm. Of course the actual dev snapshots do have version strings of sorts.

The current ctools snapshot says "7.x-1.3+30-dev"

Is this valid and correct? dependencies[] = ctools (>1.3+30-dev)

saltednut’s picture

#147 looks like a followup issue that would prevent a release. Should not be posting patches in a fixed thread though. Needs to be a new issue.

jweowu’s picture

jweowu’s picture

Adding that as a "related issue" of this.

I think it would be a good idea if anyone adding a new UUID-related issue did likewise, so that everyone following this original issue will see the update, and can follow the new issue as well?

(I'm unlikely to ever notice new issues related to UUIDs if I have to manually look for them in the issue queue.)

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.

jweowu’s picture

  • Commit bab02c0 on 7.x-3.x, 8.x-3.x by japerry:
    Issue #1277908 by jweowu, das-peter, samhassell, Kristen Pol: Introduce...
  • Commit c87511e on 7.x-3.x, 7.x-3.x-i18n, 8.x-3.x by merlinofchaos:
    Issue #1277908 by ASupinski: Better pane counting on exports that will...
jweowu’s picture

hargobind’s picture

Perhaps this has been addressed in a hook_update_N(), but it was still an issue for me, so I'm posting my workaround here.

After enabling i18n_panels (Panels translate), I needed to refresh all strings in my panels. So I visited admin/config/regional/translate/i18n_string (Administration › Configuration › Regional and language › Translate interface).
For nearly every display, I was getting an error "The display [#] has no uuid, please resave or re-export it."

To make the update process quicker, here is a snippet of code that resaves all displays with missing UUIDs. Copy it into a file in your Drupal webroot and visit the page. It won't break anything since all it's doing is loading and saving each display. And it only searches for displays without UUIDs, so there is no harm in running it more than once.


// Bootstrap Drupal.
define('DRUPAL_ROOT', getcwd());
require_once DRUPAL_ROOT . '/includes/bootstrap.inc';
drupal_bootstrap(DRUPAL_BOOTSTRAP_FULL);

// Find all displays where UUID is null, then just load the display and immediately save it.
$ids = array();
$results = db_query("SELECT did FROM {panels_display} WHERE uuid IS NULL");
while ($did = $results->fetchField()) {
  $ids[] = $did;
  $panel = panels_load_display($did);
  panels_save_display($panel);
}

$count = count($ids);
print "<p>Updated $count panels displays</p><p>IDs: " . implode(', ', $ids) . "</p>";
die;