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.
Comments
Comment #1
ASupinski CreditAttribution: ASupinski commentedPromised Patch
Comment #2
Letharion CreditAttribution: Letharion commentedComment #3
merlinofchaos CreditAttribution: merlinofchaos commentedEw globals!
Let's not introduce new globals. That's what drupal_static() is for.
Comment #4
Letharion CreditAttribution: Letharion commentedComment #5
ASupinski CreditAttribution: ASupinski commentedModified patch, thanks, I had not heard of drupal_static yet.
Comment #6
Letharion CreditAttribution: Letharion commentedComment #7
merlinofchaos CreditAttribution: merlinofchaos commentedCommitted;
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.
Comment #8
jweowu CreditAttribution: jweowu commentedCross-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.
Comment #9
merlinofchaos CreditAttribution: merlinofchaos commentedSince CTools exportables has a flag that expressly says what the status of the exportable is, features is in error for not using that.
Comment #10
Letharion CreditAttribution: Letharion commentedComment #12
jweowu CreditAttribution: jweowu commentedHere'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.
Comment #13
jweowu CreditAttribution: jweowu commentedTrivial re-roll so that the new functions do not fall under the "deprecated" comment.
Comment #14
das-peter CreditAttribution: das-peter commentedI 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 :)
Comment #15
das-peter CreditAttribution: das-peter commentedI've extended the patch from jweowu and added uuids to the displays as well.
First smoke-tests look good.
Comment #16
das-peter CreditAttribution: das-peter commented@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?
Comment #17
jweowu CreditAttribution: jweowu commented(merged two replies together)
Comment #18
jweowu CreditAttribution: jweowu commented> 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?
Comment #19
das-peter CreditAttribution: das-peter commentedThanks 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.
Comment #20
jweowu CreditAttribution: jweowu commentedThat 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!)
Comment #21
davidwhthomas CreditAttribution: davidwhthomas commentedHey, 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
Comment #22
lee20 CreditAttribution: lee20 commentedPatch in #19 worked well for me.
Comment #23
jweowu CreditAttribution: jweowu commentedI'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?
Comment #24
das-peter CreditAttribution: das-peter commentedjweowu: 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 additionexport
and thus sometimes notices occur.Without any further analysis I decided to add this to the install file:
Notices are gone and no side-effects occurred so far.
Comment #25
merlinofchaos CreditAttribution: merlinofchaos commentedSo we need an updated patch which includes #24?
Comment #26
jweowu CreditAttribution: jweowu commentedI don't believe I've encountered those notices. In which circumstances are they issued?
Comment #27
das-peter CreditAttribution: das-peter commentedAs far as I remember the notices occurred when I was reverting panels.
According to
ctools/help/export.html
: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 :)
Comment #28
samhassell CreditAttribution: samhassell commentedHere's a patch combining #19 & #24.
Working great for me!
Comment #29
jweowu CreditAttribution: jweowu commentedWe'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.
Comment #30
chrisschaub CreditAttribution: chrisschaub commentedAny chance #28 can get committed soon?
Comment #31
mikeker CreditAttribution: mikeker commented@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...
Comment #32
mstrelan CreditAttribution: mstrelan commentedI 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.
Comment #33
jweowu CreditAttribution: jweowu commentedmstrelan: Are you saying the panels_update_7302() added with this patch is not sufficient?
Comment #34
mstrelan CreditAttribution: mstrelan commentedI 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.
Comment #35
merlinofchaos CreditAttribution: merlinofchaos commentedI don't think it will be possible to assign UUIDs to panels that exist only in code until they are re-exported.
Comment #36
jweowu CreditAttribution: jweowu commentedmstrelan: 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?
Comment #37
merlinofchaos CreditAttribution: merlinofchaos commentedIt could be problematic since code-only Panels could live in a number of different applications. Page Manager and Panelizer at the very least.
Comment #38
mstrelan CreditAttribution: mstrelan commentedI 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.
Comment #39
jantoine CreditAttribution: jantoine commentedThe 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.
Comment #40
acbramley CreditAttribution: acbramley commentedI'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.
Comment #41
mstrelan CreditAttribution: mstrelan commented#40 sounds very similar to my experience.
Comment #42
davidwhthomas CreditAttribution: davidwhthomas commented@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
Comment #43
Letharion CreditAttribution: Letharion commentedThe patch in #28 has a problem in the install, it skips schema version 4. A single character in difference, but an important one.
Comment #44
jweowu CreditAttribution: jweowu commentedThanks, 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).Comment #45
acbramley CreditAttribution: acbramley commentedThank 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!
Comment #46
jweowu CreditAttribution: jweowu commentedAh, that sounds promising.
mstrelan, are you able to confirm that?
Comment #47
mstrelan CreditAttribution: mstrelan commentedAs 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.
Comment #48
jweowu CreditAttribution: jweowu commentedThat'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.
Comment #49
Letharion CreditAttribution: Letharion commentedThe 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.
Comment #50
acbramley CreditAttribution: acbramley commentedSorry, it's happening to me again. Every export regenerates all uuids and even after export shows an overriden feature until reverting it.
Comment #51
mstrelan CreditAttribution: mstrelan commented@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.
Comment #52
acbramley CreditAttribution: acbramley commented@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.
Comment #53
jweowu CreditAttribution: jweowu commentedFWIW, 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.
Comment #54
merlinofchaos CreditAttribution: merlinofchaos commentedThis 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.
Comment #55
mstrelan CreditAttribution: mstrelan commented#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.
Comment #56
mstrelan CreditAttribution: mstrelan commentedOk 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.
Comment #57
mstrelan CreditAttribution: mstrelan commentedSorry, that last patch doesn't return anything from the second update. Try this one instead.
Comment #58
acbramley CreditAttribution: acbramley commentedReally 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.
Comment #59
acbramley CreditAttribution: acbramley commentedReally 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.
Comment #60
mstrelan CreditAttribution: mstrelan commentedDid 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.
Comment #61
scottshipman CreditAttribution: scottshipman commentedsubscribing.
Patch #57 works for me in my development environment. Haven't tested in my other environments.
Comment #62
dimitriseng CreditAttribution: dimitriseng commentedDoes 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.
Comment #63
jweowu CreditAttribution: jweowu commentedIt 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.
Comment #64
lotyrin CreditAttribution: lotyrin commentedFor 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.
Comment #65
samhassell CreditAttribution: samhassell commentedI 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?
Comment #66
jweowu CreditAttribution: jweowu commentedIt looks as if cloning a variant clones its uuids as well, which seems like a bad thing.
Comment #67
jweowu CreditAttribution: jweowu commentedI'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.
Comment #68
wiifmPatch #67 has improved the exportability of panel panes *a lot*. Now we do not regenerate UUID's when exporting panels
Comment #69
acbramley CreditAttribution: acbramley commentedWoohoo!! Updated to this patch #67, added a pane and reexported and I got no regenerated uuids. Nice work @jweowu
Comment #70
jweowu CreditAttribution: jweowu commentedI'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
Comment #71
samhassell CreditAttribution: samhassell commentedCan we RTBC this now?
Comment #72
Simmol CreditAttribution: Simmol commentedI 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.
Comment #73
lotyrin CreditAttribution: lotyrin commentedThis should at least be CNR if not RTBC.
Comment #74
jweowu CreditAttribution: jweowu commentedI 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?
Comment #75
sylus CreditAttribution: sylus commentedIs 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.
Comment #76
wiifmAlso 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
Comment #77
acbramley CreditAttribution: acbramley commented+1 to get this committed!
Comment #78
das-peter CreditAttribution: das-peter commented@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:
panels\plugins\task_handlers\panel_context.inc
If you're okay with that approach I'll update the patch.
Comment #79
jweowu CreditAttribution: jweowu commentedThanks, 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??Comment #80
das-peter CreditAttribution: das-peter commentedRe-rolled patch with the suggested changes including the ones mentioned by jweowu in #79.
Let's hope we can get this in soon :)
Comment #81
sdboyer CreditAttribution: sdboyer commentedreally glad to see people working on this very important problem. retitled appropriately. some comments:
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.
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.
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.
Comment #82
jweowu CreditAttribution: jweowu commentedThanks 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.
Comment #83
mstrelan CreditAttribution: mstrelan commentedIt 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.
Comment #84
Darren Shelley CreditAttribution: Darren Shelley commentedI 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.
Comment #85
sylus CreditAttribution: sylus commentedWould 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:
3) Move
function panels_uuid_generate() {
to ctools4) 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.
Comment #86
jweowu CreditAttribution: jweowu commentedsylus: 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.
Comment #87
berdyshev CreditAttribution: berdyshev commentedAny 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.
Comment #88
dimitriseng CreditAttribution: dimitriseng commentedHi, 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.
Comment #89
davidwhthomas CreditAttribution: davidwhthomas commentedI'm using this Panels UUID patch and working fine, fwiw.
Comment #90
jweowu CreditAttribution: jweowu commentedI'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:
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.
Comment #91
jweowu CreditAttribution: jweowu commentedFixed my broken panels update hook.
Comment #92
jweowu CreditAttribution: jweowu commentedActually, do we need additional update functionality?
In comment #81, sdboyer wrote:
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.
Comment #93
jweowu CreditAttribution: jweowu commentedAdded-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
Comment #94
mikeker CreditAttribution: mikeker commentedBased on the last two comments... Thanks for your work on this @jweowu.
Comment #95
jweowu CreditAttribution: jweowu commentedmikeker: 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.
Comment #96
jweowu CreditAttribution: jweowu commentedOops. I had intended to leave that status alone...
Comment #97
berdyshev CreditAttribution: berdyshev commented@jweowu, thanks for your work.
Use single quotes instead of double ones.
Comment #98
jweowu CreditAttribution: jweowu commentedThanks 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.
Comment #99
lotyrin CreditAttribution: lotyrin commentedYes, 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.
Comment #100
Kristen PolI have replaced the double quotes to single quotes in the patch to move this along.
Comment #101
SocialNicheGuru CreditAttribution: SocialNicheGuru commentedi 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
Comment #102
jweowu CreditAttribution: jweowu commentedNow 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
Comment #103
lotyrin CreditAttribution: lotyrin commentedComment #104
sylus CreditAttribution: sylus commentedThe 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. ^_^
Comment #105
jweowu CreditAttribution: jweowu commentedNo, 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.
Comment #105.0
mikeker CreditAttribution: mikeker commentedUpdated issue summary.
Comment #106
jweowu CreditAttribution: jweowu commentedSetting this back to CNW, as obviously it's confusing otherwise.
Comment #107
mikeker CreditAttribution: mikeker commentedI 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.
Comment #108
jweowu CreditAttribution: jweowu commentedWhile we wait, it would be good to get some discussion happening on this one:
In comment #81, sdboyer wrote:
And I responded in comment #92:
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?
Comment #109
mikeker CreditAttribution: mikeker commentedjweowu, thank you for moving the conversation forward.
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.
Comment #110
jweowu CreditAttribution: jweowu commentedRe: 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.
Comment #111
lotyrin CreditAttribution: lotyrin commentedConfirm 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.
Comment #112
merlinofchaos CreditAttribution: merlinofchaos commentedFirst, 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.
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.
Comment #112.0
jweowu CreditAttribution: jweowu commentedCleaned up formatting
Comment #113
jweowu CreditAttribution: jweowu commentedI 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.
Comment #114
merlinofchaos CreditAttribution: merlinofchaos commentedWe 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. :/
Comment #115
jweowu CreditAttribution: jweowu commentedI'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...)
Comment #116
merlinofchaos CreditAttribution: merlinofchaos commented$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).
Comment #117
OnkelTem CreditAttribution: OnkelTem commentedCTools 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
Comment #118
OnkelTem CreditAttribution: OnkelTem commentedRe-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.
Comment #119
logaritmisk CreditAttribution: logaritmisk commentedPatch 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.
Comment #120
SocialNicheGuru CreditAttribution: SocialNicheGuru commentedin 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.
Comment #121
dixon_@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.
Comment #122
manuelBS CreditAttribution: manuelBS commented#118 works for me, too. Thanks for that work!
Comment #123
geek-merlinAs of #1179034-82: Translatable panel titles: Implement i18n_strings and #1277908-119: Introduce UUIDs onto panes & displays for better exportability & features compatibility both patches are rtbc:
* patch in #1179034-80: Translatable panel titles: Implement i18n_strings
* patch in #1277908-118: Introduce UUIDs onto panes & displays for better exportability & features compatibility
Comment #124
saltednutWhy 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. :)
Comment #125
acbramley CreditAttribution: acbramley commentedNice! Maybe we could maintain this make snippet in the summary?
Comment #125.0
acbramley CreditAttribution: acbramley commentedRestored some of ASupinski's original text, as the discussion of the caching problem had been lost in the more recent edits.
Comment #126
japerryProgress! 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!
Comment #127
das-peter CreditAttribution: das-peter commented@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.
Comment #128
japerryIf 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.
Comment #129
mikeker CreditAttribution: mikeker commentedI don't see anything in the recent comments that would keep this from remaining RTBC...
Comment #130
japerryThe 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.
Comment #131
mikeker CreditAttribution: mikeker commentedThanks 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:
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!
Comment #132
jweowu CreditAttribution: jweowu commentedIt 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).
Comment #133
jweowu CreditAttribution: jweowu commentedI'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
Comment #134
japerryHere 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.
Comment #135
SocialNicheGuru CreditAttribution: SocialNicheGuru commentedThis is the ctools issue with patch. https://drupal.org/node/2155825
I'm posting it because it just makes it easier to find.
Comment #136
jweowu CreditAttribution: jweowu commentedOr 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 :)
Comment #137
japerryBetter yet, this code is committed!
Any further bugs should be opened up in specific issues.
http://drupalcode.org/project/panels.git/commit/bab02c0
Comment #138
das-peter CreditAttribution: das-peter commentedThis is awesome!!!
Comment #139
jweowu CreditAttribution: jweowu commentedHoly cow :)
I guess there will be some spin-off issues, but this is certainly a good way to get people looking at them :)
Comment #140
das-peter CreditAttribution: das-peter commentedI'll be happy to help dealing with any upcoming spin-off issues ;)
Comment #141
davidwhthomas CreditAttribution: davidwhthomas commentedWoohoo! (that is all) :)
Comment #142
pfrenssenThanks very much, this is great news!!
Comment #143
Anonymous (not verified) CreditAttribution: Anonymous commentedUpdate fails with:
Fatal error: Call to undefined function ctools_uuid_generate()
Comment #144
jweowu CreditAttribution: jweowu commentedmorningtime: you need to update ctools.
(hmm. I guess we could throw an update exception if that function doesn't exist.)
Comment #145
tim.plunketthook_update_dependencies() FTW
Comment #146
jweowu CreditAttribution: jweowu commentedNo, 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.
Comment #147
jweowu CreditAttribution: jweowu commentedComment #148
jweowu CreditAttribution: jweowu commentedWe 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)
Comment #149
saltednut#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.
Comment #150
jweowu CreditAttribution: jweowu commentedTrue. #2187433: UUID update necessitates an explicit dependency on the updated Ctools
Comment #151
jweowu CreditAttribution: jweowu commentedAdding 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.)
Comment #153
jweowu CreditAttribution: jweowu commentedComment #155
jweowu CreditAttribution: jweowu commentedComment #156
hargobindPerhaps 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.
Comment #157
jweowu CreditAttribution: jweowu commented