Early Bird Registration for DrupalCon Portland 2024 is open! Register by 23:59 PST on 31 March 2024, to get $100 off your ticket.
It appears there's no way to edit the Panelizer content/layout/settings for non-current node revisions. I've done a little digging and added some menu callbacks, etc. It seems the key limitation here is that panelizer_panels_cache_get() doesn't differentiate based on revision. Am I barking up the wrong tree?
I'm continuing to look into this in hope of rolling a patch but perhaps someone with better architectural knowledge of panels/panelizer can weigh in on the best approach.
Comment | File | Size | Author |
---|---|---|---|
#64 | panelizer-n1798294-64.patch | 1.23 KB | DamienMcKenna |
#63 | panelizer-n1798294-63.patch | 892 bytes | michielnugter |
Comments
Comment #1
azinck CreditAttribution: azinck commentedI've updated panelizer_panels_cache_get() and have basic editing working but links for operations on my content panes (settings, disable, etc.) don't pass the vid in the ajax request. Looks like I'll have to dig deeper into the architecture here. I seem to recall that this works in the IPE so perhaps I can learn something from work done there...
Comment #2
DamienMcKennaPlease see #1402860: Panelizer is incompatible with moderation.
Comment #3
azinck CreditAttribution: azinck commentedDamien: I saw that issue but it seems focused on the problem of extra unwanted revisions when saving in Panelizer.
This issue is about being able to use the Panelizer interface to edit non-current node revisions. There seem to be a number of architectural issues preventing this currently. FWIW, (though I don't see how it pertains to this issue) I'm not using Revisioning or Workbench Moderation but rather State Machine.
If you still think there's sufficient overlap I can continue this discussion on #1402860: Panelizer is incompatible with moderation, but this seems like a distinct issue to me.
Comment #4
DamienMcKenna@azinck: I stand corrected.
Comment #5
azinck CreditAttribution: azinck commentedHere's a first-pass at a patch against 2.x-dev. Works for me, YMMV.
Comment #6
azinck CreditAttribution: azinck commentedComment #7
Silicon.Valet CreditAttribution: Silicon.Valet commentedPreliminary review of ui appears that this patch fixes issues with the revisioning module. Was able to go through a few iterations of the revision/moderation process and change layouts between.
May offer a more formal RTBC when I get a chance to review later tonight or tomorrow morning.
Comment #8
azinck CreditAttribution: azinck commentedJust a note: I found another revision-related "bug". Creating a new revision of a node does not always/automatically create a new panels display. A new panels display is only created if you made a change to the display at the time the new node revision is created. I think it makes more sense to always create a new display. I'll work on updating the patch...
Comment #9
DamienMcKennaThe patch in #5 does not resolve the problem for me, at least when using Revisionining.
Comment #10
DamienMcKennaAt the very least the patch needs to be fixed to use the Drupal coding standards.
Comment #11
DamienMcKennaThis is an updated version of my patch from #1402860: Panelizer is incompatible with moderation that now works correctly with all of the Panelizer subpage settings so that the core Revisions option is properly adhered to.
Comment #12
azinck CreditAttribution: azinck commentedI'll update it to match coding standards.
Damien: you said it does not "resolve the problem" for you. Specifically which problem were you testing? The patch in #5 allows me to edit non-current node revisions at node/%node/revisions/%/panelizer -- does that not work for you?
Comment #13
DamienMcKennaThe patch applies cleanly to both the 7.x-2.x and 7.x-3.x branches, but has only been tested with the 7.x-2.x branch.
Comment #14
DamienMcKennaApologies, I posted the patch to the wrong issue, my patch was for #1804156: Panelizer always creates a new revision.
Comment #15
DamienMcKennaUpdated azinck's patch to match coding standards and tidied up the path-related code in PanelizerEntity->page_layout().
Comment #16
DamienMcKennaThe patch should add a new Panelizer item to the secondary tabs, beside the revision-specific links for edit & view, at least it doesn't when using Revisioning.
Comment #17
azinck CreditAttribution: azinck commentedI didn't add those tabs because there's no good place to "hang" the revision-specific Panelizer tabs in core. We could check for the existence of specific modules (like Revisioning or State Machine) and adjust the menu as appropriate, but I felt it was more the role of those modules to add support for Panelizer (through hook_menu_alter) rather than the other way around.
Comment #18
DamienMcKennaFYI I created a related issue: #1820882: Make node revisions use the node_view display
Comment #19
DamienMcKennaRerolled for the 7.x-3.x branch.
Comment #20
DamienMcKennaThere is no entity_load_single() function in D7.
Comment #21
DamienMcKennaI'm now getting the error "Unsupported operand types in entity.inc on line 355" so will have to debug this further.
Comment #22
DamienMcKennaWIP, and it needs a patch added to Workbench Moderation.. I'll be working on this some more tomorrow.
Comment #23
DamienMcKennaThis patch fixes some of the problems with the last one, so you can at least now load all of the Panelizer pages for each revision. This requires the patch from #1868144: Allow the node history list to be customized.
Comment #24
DamienMcKennaDoing further testing and the patch in #23 doesn't work correctly, there's confusion over what the correct paths should be. I've worked up a quick plan for how the URL hierarchy should be (PNG and OmniOutliner formats), tomorrow I'll update the patch to match.
Comment #25
frakke CreditAttribution: frakke commentedArgh, wrong issue. Sorry
Comment #26
DamienMcKennaFixed at least part of the problem with the patch in #23, but it needs some further work.
Comment #27
DamienMcKennaWith the patch in #26 it is no longer possible to open the Settings dialog for panes, you get the following error instead:
Comment #28
DamienMcKennaI've identified the symptom - without this patch the Edit link's URL is as follows:
panels/ajax/editor/edit-pane/panelizer:default:node:page.page_manager:node:page:default2/10
With the patch the URL changes to:
panels/ajax/editor/edit-pane/panelizer:default:node:page.page_manager:node:page:default2:node/10
Now to find out why.
Comment #29
DamienMcKennaThis patch fixes the problem with opening any of the contextual menu items from the Content pane. One step closer.
FYI the problem was the $cache->display->cache_key line in panelizer_panels_cache_get(), when editing a default display object, rather than a per-entity display override, the $vid variable would end up being the entity type, e.g. 'node', so this was being appended to the cache_key. My fix for that was to do an additional is_numeric() check.
Comment #30
DamienMcKennaUpdated patch that has the local tasks as close to correct as I can make them.
Comment #31
DamienMcKennaFYI the patch in #30 doesn't require the CTools patch to display revisions properly as Workbench Moderation already handles that, however editing & saving revisions still isn't working quite right.
Comment #32
DamienMcKennaCorrect make_fake_tabs() for when Workbench Moderation is enabled and you're viewing a non-current revision.
Comment #33
kjl CreditAttribution: kjl commentedUsing patched workbench moderation and the patch in #32, after you save your panelizer changes, the page reloads and it appears to revert and you have lost the changes. What happens is that a new current revision is created with the changes you have made. It's only a UI thing that makes it appear that the save hasn't happened, because you're still on the old revision's page.
Comment #34
merlinofchaos CreditAttribution: merlinofchaos commentedI will reject a patch that creates new displays unnecessarily; displays can be heavy, and revisions can happen a lot. I am very specific about not exploding the display table. Imagine a site with 1 million panelized nodes and an average of 5 revisions per node and an average of 10 panes per panel. That's 50 million pane entries, and some large percentage of that (probably 90%) are pure duplication -- and these are not high averages.
Comment #35
DamienMcKennaWorking on a reroll.
@merlinofchaos: The goal is not to create new displays unnecessarily.
Comment #36
DamienMcKennaRerolled.
Comment #37
DamienMcKennaThe node/:node/revisions/:revision/panelizer/page_manager/:task pages aren't loading correctly..
Comment #38
recidive CreditAttribution: recidive commentedJust a re-roll of #36.
Could you provide steps for testing your patch?
Comment #39
recidive CreditAttribution: recidive commentedI'm getting those notices several times when saving a display for a not published revision (via node/[nid]/revisions) after applying this patch:
Panelizer still saves the latest revision but discards the changes done in the display.
Also when editing the display for a not published revision and clicking the "+" button for adding content to some region, it breaks with the following javascript alert and the throbber keeps spinning forever:
Comment #40
DamienMcKennaTentatively adding this to the todo list for the next release.
Comment #41
DamienMcKennaComment #42
DamienMcKennaRerolled.
Comment #43
DamienMcKennaRerolled, plus removed some code cleanup that wasn't strictly necessary right now.
Comment #44
SocialNicheGuru CreditAttribution: SocialNicheGuru commentedthe patch introduces this error
Notice: Undefined offset: 3 in menu_translate() (line 777 of drupal7/includes/menu.inc).
Notice: Undefined offset: 3 in menu_translate() (line 783 of drupal7/includes/menu.inc).
Comment #45
SocialNicheGuru CreditAttribution: SocialNicheGuru commentedComment #46
michielnugter CreditAttribution: michielnugter commentedI attempted to create a patch for the notices. A lot of the revision menu callbacks are seen as relevant tabs while they can't be processed properly because the required param VID is missing. I changed the % for the vid to use a load function which returns FALSE if it's not available. This prevent's any further processing and prevents the notice. I attached the changed patch.
I tested the patch itself and it seems to work nicely.
Comment #47
michielnugter CreditAttribution: michielnugter commentedComment #48
michielnugter CreditAttribution: michielnugter commentedI found a very nasty bug in the patch, it appended the vid to the cache key but it didn't check if it was already there. Because of this on each page load within the editor another :vid was added causing the cache key to change. Panes and changes were added to an incorrect cache and therefore never saved.
The new attached patch fixes this issue by checking if the vid isn't there already. I used a substr to check, couldn't think of any better solution than this..
Comment #49
partdigital CreditAttribution: partdigital commentedI've tested this patch and it works. It fixed an issue where it wasn't displaying the most recent pane changes until the cache was cleared. I needed this patch on the latest release so I've rerolled this for the 3.1 release. Everyone else should use the patch in #48 for the dev release.
Comment #51
DamienMcKennaChanging the status back to 'needs review'.
@michielnugter, @partdigital: Thank you both for your work on this, I hope to have some time to test it soon.
Comment #52
michielnugter CreditAttribution: michielnugter commentedI found another small bug. I got some sql errors:
Duplicate entry 'node-287-324-page_manager' for key 'PRIMARY': INSERT INTO {panelizer_entity}
Traced it back to PanelizerEntityDefault.class.php line 1312.
What happened was that the entity_id and the revision_id were empty, causing this piece of code to reset the $update array. The weird fact is though that $entity_id is not empty.
Further along the entity_id is set and because the $update is now empty an insert is attempted resulting in a duplicate record warning.
I added a !empty($entity_id) condition to fix the problem, which it does for me.
Comment #53
DamienMcKennaThis does indeed make it so that Panelizer displays for revisions can be accessed & edited, so we're getting close.
The remaining problem is that there is no Panelizer tab when you view a revision, or more specifically there are no tabs at all. I don't think this is Panelizer's fault.
Comment #54
mglamanCorrect - tabs won't render because all MENU_LOCAL_TASK are under node/%node/*, and get lost after node/%node/revisions/%/*. Haven't gotten a chance to test patch myself, but figure I'd chime in on that :)
Comment #55
DamienMcKennaThis version contains an improved _workbench_moderation_node_history_view_alter(), which now verifies permissions before adding the links, thus removing a minor security problem before it happens.
Comment #56
DamienMcKennaAnd... COMMITTED! Thank you all for the help finishing this one!
Comment #57
michielnugter CreditAttribution: michielnugter commentedSorry, my latest patch actually introduced another bug.
2 cases to clarify the problems I had and introduced:
1. A content type is panelized with a default AND a default for a certain display mode. Both are exported in features and therefore in code.
When saving a node with the default panel it passes the following if (twice for each display mode):
Resetting the $update and creating a row in panelizer_entity.
The second save action on the entity passes this if twice again. The first going well but the second (the display mode panelizer default) resulting in another db_insert which failes because of a duplicate record.
With the patch applied:
2. Another content type is panelized with a default. The default is exported in features and therefore in code.
The default pane works and there are no problems when saving. Howover, when overriding the default with a custom layout/custom content it ignores the changes and doesn't save the override.
Two questions come in mind:
1. Why save a record for the panels settings at all if using the default?
2. Is adding a $panelizer->display_is_modified == TRUE instead of !empy($panelizer->entity_id) the solution. (this does fix both situations for me).
Comment #58
DamienMcKennaA record is saved in {panelizer_entity} so that revisions can be tracked correctly.
I'm unable to reproduce any problems with entity record saving when using this patch, are you sure you're using the latest codebase and you didn't just apply the patch to an older release of the code?
Comment #59
michielnugter CreditAttribution: michielnugter commentedI'm very sure, I pulled the latest from the dev branch and patched it again to be sure.
There is no problem as long as there is only 1 display mode for a content type but it fails as soon as there are more. I have a default and a search_result display for a node and on the search_result display a duplicate record warning is issued.
Comment #60
DamienMcKennaThe reason I asked is that the line you quoted above (comment 57) doesn't exist in the codebase. Can you please confirm what line you're seeing the error on? Thanks.
Comment #61
michielnugter CreditAttribution: michielnugter commentedI quoted the line before the patch from this issue was applied.
The relevant bit from the patch:
I hope this clears it up. Let me know if it doesn't.
Comment #62
michielnugter CreditAttribution: michielnugter commentedI researched some more because I really need this fixed for a project. The additional patch in #52 definitly breaks more than it fixes. Please don't use this one and revert the line:
back to:
The problem is in the fact that the form_state doesn't contain all the panlizer settings of each display mode for the node. This causes an incomplete $entity->panelizer array to be passed to hook_entity_update(). I'm still searching for a fix for this, I'll post again if I find it.
Comment #63
michielnugter CreditAttribution: michielnugter commentedI finally found the problem. I first tested in my existing project and after that I installed a clean Drupal and tested there. Unfortunatly I messed up both environments causing non-used records in the panelizer_defaults table. While the settings didn't specify an override for certain display modes anymore the database still contained these records.
On inserting a new node it works correctly, it only adds records in the panelizer_entity table for display modes that are enabled. However, on updating the node it adds a record for each available default in the table panelizer_defaults regardless of if they are used. Saving again will fail because of the conflicting settings and reality in the tables.
I reproduced it by setting an override for a display mode, creating a default layout, exporting it to features and unsetting the override. It will now fail.
There might be in issue to resolve in the above allthough I have no idea on how to fix it.
I attached a patch to revert my change from #52 as it causes serious bugs with panels losing the layout settings on a new revision, it's very important to apply the patch.
Comment #64
DamienMcKennaOut of interest, does this resolve the problem too?
Comment #65
DamienMcKennaI've committed the patch in #64, please give the latest -dev release a try and let me know if the problem persists.
Comment #67
DamienMcKennaFYI there's a bug in this that is being dealt with separately: #2408301: Displays not saving correctly