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).

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

recidive’s picture

Status: Active » Needs review
FileSize
1.11 KB

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.

merlinofchaos’s picture

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.

recidive’s picture

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.

recidive’s picture

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.

merlinofchaos’s picture

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.

recidive’s picture

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

merlinofchaos’s picture

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

recidive’s picture

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.

recidive’s picture

Status: Needs work » Needs review
FileSize
2.08 KB

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

recidive’s picture

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

merlinofchaos’s picture

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

recidive’s picture

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.

tobby’s picture

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.

seanr’s picture

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

DamienMcKenna’s picture

DamienMcKenna’s picture

DamienMcKenna’s picture

Assigned: recidive » Unassigned
DamienMcKenna’s picture

DamienMcKenna’s picture

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.

DamienMcKenna’s picture

Component: Code » Revisions
DamienMcKenna’s picture

Status: Needs work » Needs review
FileSize
610 bytes

Rerolled.

Status: Needs review » Needs work

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

DamienMcKenna’s picture

FileSize
2.29 KB

Doh! Wrong file.

DamienMcKenna’s picture

Status: Needs work » Needs review
DamienMcKenna’s picture

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.

grndlvl’s picture

When using the patch from #1402860-47: Panelizer is incompatible with moderation to get this working with workbench moderation I ended up making one tiny change where we are currently checking if the did w/ empty() to is_null()

// Make sure we keep the same did as the original if the layout wasn't
// changed.
if (is_null($panelizer->did) && !empty($entity->original->panelizer[$view_mode]->did)) {

In the case that the display is the default then it could be "0" and this will fasly recoginize that it should be loading the "original" or the draft version and ovewrite the did for the published version with the draft versions did. @see scenario from #1402860-52: Panelizer is incompatible with moderation

Since I am using 7.x-3.1 I currently only had time to roll a patch using #15 from this thread since it still applied to the current release. Since this ticket is closed I can roll a new ticket later, but it seems relevant that this should at least be listed here hidden for now.

grndlvl’s picture

FileSize
2.75 KB

heh wrong patch.. here is the right one this time

DamienMcKenna’s picture

Thanks Jonathan :)