Modules that use panels_save_display() to save a display will occasionally run into issues whereby the UUID values of the display (panels_display table) and the attached panes (panels_pane table) are not reset, so the new records might have the same UUID values as the old records.

A specific example is Panelizer. When a display is saved in Panelizer it is passed to panels_save_display() with an empty $display->did value.

According to the comments on panels_save_display() this should be enough to save a new record instead of updating the previous record. However, when a display is passed from Panelizer via panels_save_display() it doesn't set/reset the UUID values if the did is empty but the UUID values already exist.

The solution should be to reset the display and pane UUID values any time the did value is blank (or set to "new-[blah]").

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

DamienMcKenna created an issue. See original summary.

DamienMcKenna’s picture

Status: Active » Needs review
FileSize
1.73 KB

This resets the UUID values if the did is empty, i.e. when $update is empty.

DamienMcKenna’s picture

The patch above resolves the two referenced Panelizer issues.

DamienMcKenna’s picture

Bumping this, it wasn't added to 3.6 but needs to be in 3.7.

dsnopek’s picture

Moving to 3.8 so we can get 3.7 out quicker!

dan.munn’s picture

I was testing this patch and starting to realise that this has potential to have an unforeseen impact on other modules that might need to specify the UUID of displays that are being created for a new display. An example of this would be using deploy and panelier_deploy, in this usage scenario they could be broadcasting the information over services for save on another system. In this particular use case it makes sense for the saving process to try and give a steer as to the UUID rather than guaranteeing it is new on first save, as otherwise we will end up with a whole bunch of orphaned displays where UUID will be mis-matched between source and target.

So I propose an alternative, if there is a mechanism of determining uniqueness of the UUID and using that as the focal point of UUID regeneration instead of an updated check, in this scenario a new UUID would be generated only if it exists at time of save against an alternative display id, I think that would still satisfy DamienMcKenna's proposal without preventing deploy providing an idea of UUID based on the source system.

Alternative patch is attached

DamienMcKenna’s picture

@dan.munn: Hrm... interesting point. At the very least shouldn't it also support exported displays, rather than just ones that are in the database?

dan.munn’s picture

@DamienMcKenna although displays and their panes aren't defined via a single pipe-line either are they? e.g. page manager, panelizer etc, they all define displays / panes nested within their respective default hooks when exported. Any thoughts?

badjava’s picture

badjava’s picture

joseph.olstad’s picture

retriggered tests

joseph.olstad’s picture

Status: Needs review » Reviewed & tested by the community

Great patch, thanks!

joelpittet’s picture

Status: Reviewed & tested by the community » Needs review

Moving back to needs review, I'm nervous about the answer needed for #7 and #8 before I commit this.