Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
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).
Comment | File | Size | Author |
---|---|---|---|
#23 | panelizer-n2053721-23.patch | 2.29 KB | DamienMcKenna |
#3 | panelizer-did-no-patch.csv_.txt | 558 bytes | recidive |
#1 | panelizer-clone-display-node-revisions-2053721-1.patch | 1.11 KB | recidive |
Comments
Comment #1
recidive CreditAttribution: recidive commentedThis 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.
Comment #2
merlinofchaos CreditAttribution: merlinofchaos commentedNo, 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.
Comment #3
recidive CreditAttribution: recidive commentedI 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.
Comment #4
recidive CreditAttribution: recidive commentedAlso, 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.
Comment #5
merlinofchaos CreditAttribution: merlinofchaos commentedSorry, 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.
Comment #6
recidive CreditAttribution: recidive commented@merlinofchaos do you know if it's feasible to use DrupalWebTestCase::drupalPostAJAX() for writing test cases for Panelizer page customizations with IPE?
Comment #7
merlinofchaos CreditAttribution: merlinofchaos commentedI'm afraid I'm not familiar with the test system's AJAX capabilities.
Comment #8
recidive CreditAttribution: recidive commentedThis 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.
Comment #9
recidive CreditAttribution: recidive commentedThis one seems better. I've copied the query from the patch in that workbench moderate issue.
Comment #10
recidive CreditAttribution: recidive commentedThis one seems better. I've copied the query from the patch in that workbench moderate issue.
Comment #11
merlinofchaos CreditAttribution: merlinofchaos commented#10 looks very reasonable. Would love a little feedback from testers.
Comment #12
recidive CreditAttribution: recidive commentedJust 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.
Comment #13
tobby CreditAttribution: tobby commentedI 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.
Comment #14
seanrI haven't tested this, but in a cursory review, I noticed a typo in a comment in the patch from #13. Attached fixes.
Comment #15
DamienMcKennaSame as seanr, just fixing a comment :)
Comment #16
DamienMcKennaComment #17
DamienMcKennaComment #18
DamienMcKennaComment #19
DamienMcKennaI 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.
Comment #20
DamienMcKennaComment #21
DamienMcKennaRerolled.
Comment #23
DamienMcKennaDoh! Wrong file.
Comment #24
DamienMcKennaComment #25
DamienMcKennaIt 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.
Comment #27
grndlvl CreditAttribution: grndlvl commentedWhen 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()
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.
Comment #28
grndlvl CreditAttribution: grndlvl commentedheh wrong patch.. here is the right one this time
Comment #29
DamienMcKennaThanks Jonathan :)