We are having a problem when creating a new node revision, editing its panelizer instance affects other revisions displays. That happens because the instance created for the new revision points to the same display as the previous revision, making it don't fully work with revision based content staging solutions (CSI).

Comments

Status:Active» Needs review
StatusFileSize
new1.11 KB
PASSED: [[SimpleTest]]: [MySQL] 0 pass(es).
[ View ]

This one line patch make it clone the display when customizing a page for a revision that still don't have its own display, instead of creating view modes pointing to the previous revision display.

Status:Needs review» Needs work

No, the whole point is to NOT create clones of unmodified display so that you don't waste a lot of data. Panels can get very large, and you can really bloat the panes table unnecessarily with this patch. I've rejected this patch in the past and I'll reject it again.

It sounds like the problem is on the other side: Panel displays are getting modified without new revisions being created. That is where the error is, and that is where the error should be fixed.

StatusFileSize
new558 bytes
new316 bytes

I was trying to fix this for a modified display only, so this would only clone the display when saving the modifications in panelizer, and also would make changing the layout affects only the revision that's being edited. I believe this is how most people want this to behave.

I tested my patch without creating new revisions, i.e. creating a node, customizing its display then editing the node without creating a new revision and I could see the other record is created with a cloned display (attached is a csv with the records in panelizer_entity page for that node). I see what you were talking about now. What's interesting is that without my patch the display don't get cloned but records for all view modes are created (see the second csv, with my patch only a record was created when editing the node), could you explain how this system works?

We need to add some tests for some scenarios to test and fix the panelizer/entities integration that's currently broken. I'm putting together a list of what works and what doesn't so we can start from there, will share this soon.

If you can elaborate on your insight for us to fix this issue, please go ahead and I'll try to implement this immediately.

Thanks.

Also, we will need to create new revisions for fieldable panel panes and possible other panes of other types too, or clone them if they don't support revision. Any advises in this direction would be welcome.

Sorry, I'm not entirely sure how to explain.

Because a display and panes can end up taking up a lot of space, I am specifically only creating a new display when the display_is_modified flag is set, which means that the display was modified. That means that several revisions can point to the same display.

Each combination of entity id, revision id and view mode create a unique record in the panelizer_entity table, but they can (and often do) point to the same display.

When that is in use, it is important that any changes to a display do create a new revision. If that is failing, then the failure is there; changes should not be made to a display without creating a new revision unless it is the only reference to that display ID. In theory we could test that by querying the panelizer_entity database and looking to see if revisions other than the current revision id point to that did if we are saving and a new revision is not being created and the display is marked as modified.

@merlinofchaos do you know if it's feasible to use DrupalWebTestCase::drupalPostAJAX() for writing test cases for Panelizer page customizations with IPE?

I'm afraid I'm not familiar with the test system's AJAX capabilities.

This patch from Workbench Moderation queue (#1402860: Panelizer is incompatible with moderation) seems to implement some workarounds for panelizer and revisions and may provide some insights on how to fix this issue.

Status:Needs work» Needs review
StatusFileSize
new2.08 KB

This one seems better. I've copied the query from the patch in that workbench moderate issue.

This one seems better. I've copied the query from the patch in that workbench moderate issue.

#10 looks very reasonable. Would love a little feedback from testers.

Just for the record, I tested the patch, and apparently it behaves exactly as without the patch for when you're not creating revisions. And clones the display only when you're editing a display for a revision that shares the display with other revisions.

I used the patch in #9, and by and large it works. However, I ran into a special edge-case that required a little bit more for that patch to work for me.

In my case, we're using Panelizer and Workbench Moderation. Some content types are revisioned and/or moderated, and some aren't. The case I ran into was with a content type that is not using revisions, and a particular node that had a modified Panelizer layout. So, after editing the node (but not the modified layout), I would get a PDO exception:
General error: 1364 Field 'did' doesn't have a default value: INSERT INTO {panelizer_entity} (entity_type, entity_id, revision_id, name, view_mode) VALUES (:db_insert_placeholder_0, :db_insert_placeholder_1, :db_insert_placeholder_2, :db_insert_placeholder_3, :db_insert_placeholder_4); Array ( [:db_insert_placeholder_0] => node [:db_insert_placeholder_1] => 185721 [:db_insert_placeholder_2] => 284801 [:db_insert_placeholder_3] => node:site_page:default [:db_insert_placeholder_4] => page_manager ) in drupal_write_record()

I modified the patch in #9 to handle this case. It seems a little strange to have to carry over the panelizer->did and set the update array, but works in all my test cases (with and without node revisions, with and without modified Panelizer layouts, etc). I'd appreciate some feedback on it.

I haven't tested this, but in a cursory review, I noticed a typo in a comment in the patch from #13. Attached fixes.

Issue summary:View changes
StatusFileSize
new2.74 KB
PASSED: [[SimpleTest]]: [MySQL] 0 pass(es).
[ View ]

Same as seanr, just fixing a comment :)

Assigned:recidive» Unassigned

Status:Needs review» Needs work

I agree that a full display record should not be created for each revision. Instead, the {panelizer_entity} record for the last revision should be cloned. I'm working on some other issues, will look back to this shortly.

Component:Code» Revisions

Status:Needs work» Needs review
StatusFileSize
new610 bytes
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch migrate_d2d-n2166101-7_0.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Rerolled.

Status:Needs review» Needs work

The last submitted patch, 21: migrate_d2d-n2166101-7.patch, failed testing.

StatusFileSize
new2.29 KB
PASSED: [[SimpleTest]]: [MySQL] 0 pass(es).
[ View ]

Doh! Wrong file.

Status:Needs work» Needs review

Status:Needs review» Fixed

It seems like I inadvertently committed this as part of #2006288: Panelizer defaults for unused view modes. Oops. Thanks for your work in fixing the bug.

Status:Fixed» Closed (fixed)

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