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.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

azinck’s picture

I'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...

DamienMcKenna’s picture

Status: Active » Closed (duplicate)
azinck’s picture

Status: Closed (duplicate) » Active

Damien: 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.

DamienMcKenna’s picture

@azinck: I stand corrected.

azinck’s picture

Here's a first-pass at a patch against 2.x-dev. Works for me, YMMV.

azinck’s picture

Status: Active » Needs review
Silicon.Valet’s picture

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

azinck’s picture

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

DamienMcKenna’s picture

The patch in #5 does not resolve the problem for me, at least when using Revisionining.

DamienMcKenna’s picture

Status: Needs review » Needs work

At the very least the patch needs to be fixed to use the Drupal coding standards.

DamienMcKenna’s picture

Status: Needs work » Needs review
FileSize
884 bytes

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

azinck’s picture

I'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?

DamienMcKenna’s picture

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

DamienMcKenna’s picture

Apologies, I posted the patch to the wrong issue, my patch was for #1804156: Panelizer always creates a new revision.

DamienMcKenna’s picture

FileSize
8.22 KB

Updated azinck's patch to match coding standards and tidied up the path-related code in PanelizerEntity->page_layout().

DamienMcKenna’s picture

Status: Needs review » Needs work

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

azinck’s picture

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

DamienMcKenna’s picture

DamienMcKenna’s picture

Version: 7.x-2.x-dev » 7.x-3.x-dev
Status: Needs work » Needs review
FileSize
8.66 KB

Rerolled for the 7.x-3.x branch.

DamienMcKenna’s picture

There is no entity_load_single() function in D7.

DamienMcKenna’s picture

Status: Needs review » Needs work

I'm now getting the error "Unsupported operand types in entity.inc on line 355" so will have to debug this further.

DamienMcKenna’s picture

FileSize
9.48 KB

WIP, and it needs a patch added to Workbench Moderation.. I'll be working on this some more tomorrow.

DamienMcKenna’s picture

Status: Needs work » Needs review
FileSize
9.82 KB

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

DamienMcKenna’s picture

Status: Needs review » Needs work
FileSize
1.95 KB
48.91 KB

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

frakke’s picture

FileSize
1.92 KB

Argh, wrong issue. Sorry

DamienMcKenna’s picture

FileSize
9.78 KB

Fixed at least part of the problem with the patch in #23, but it needs some further work.

DamienMcKenna’s picture

With the patch in #26 it is no longer possible to open the Settings dialog for panes, you get the following error instead:

An AJAX HTTP error occurred.
HTTP Result Code: 200
Debugging information follows.
Path: /panels/ajax/editor/edit-pane/panelizer%3Adefault%3Anode%3Apage.page_manager%3Anode%3Apage%3Adefault2%3Anode/10
StatusText: OK
ResponseText: [{"command":"settings","settings":{"basePath":"\/","pathPrefix":"","ajaxPageState":{"theme":"seven","theme_token":"2ltDBsyWbN0xIfDeh15GkHYbOzP5C_XB76Tn0VF2HTE"}},"merge":true},{"command":"modal_display","title":"Error","output":"Invalid pane id."}][{"command":"settings","settings":{"basePath":"\/","pathPrefix":"","ajaxPageState":{"theme":"seven","theme_token":"2ltDBsyWbN0xIfDeh15GkHYbOzP5C_XB76Tn0VF2HTE"}},"merge":true},{"command":"modal_dismiss"}]
DamienMcKenna’s picture

I'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.

DamienMcKenna’s picture

Status: Needs work » Needs review
FileSize
10.12 KB

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

DamienMcKenna’s picture

FileSize
14.24 KB

Updated patch that has the local tasks as close to correct as I can make them.

DamienMcKenna’s picture

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

DamienMcKenna’s picture

FileSize
14.38 KB

Correct make_fake_tabs() for when Workbench Moderation is enabled and you're viewing a non-current revision.

kjl’s picture

editing & saving revisions still isn't working quite right

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

merlinofchaos’s picture

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

DamienMcKenna’s picture

Status: Needs review » Needs work

Working on a reroll.

@merlinofchaos: The goal is not to create new displays unnecessarily.

DamienMcKenna’s picture

FileSize
14.17 KB

Rerolled.

DamienMcKenna’s picture

The node/:node/revisions/:revision/panelizer/page_manager/:task pages aren't loading correctly..

recidive’s picture

FileSize
14.19 KB

Just a re-roll of #36.

Could you provide steps for testing your patch?

recidive’s picture

I'm getting those notices several times when saving a display for a not published revision (via node/[nid]/revisions) after applying this patch:

Notice: Undefined offset: 3 in _menu_translate() (line 777 of /Users/henrique/Sites/panelizer/includes/menu.inc).
Notice: Undefined offset: 3 in _menu_translate() (line 783 of /Users/henrique/Sites/panelizer/includes/menu.inc).

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:

An AJAX HTTP error occurred.
HTTP Result Code: 200
Debugging information follows.
Path: /panels/ajax/ipe/select-content/panelizer%3Anode%3A7%3Adefault/center
StatusText: OK
ResponseText: [{"command":"settings","settings":{"basePath":"\/","pathPrefix":"","ajaxPageState":{"theme":"bartik","theme_token":"4N3HuOBbpY6jt7nQdXK5gDWspo0LONp6seB5PRqMx7k"}},"merge":true},{"command":"modal_display","title":"Error","output":"Invalid input"}][{"command":"settings","settings":{"basePath":"\/","pathPrefix":"","ajaxPageState":{"theme":"bartik","theme_token":"4N3HuOBbpY6jt7nQdXK5gDWspo0LONp6seB5PRqMx7k"}},"merge":true},{"command":"modal_display","title":"Add content to ","output":"\u003Cdiv class=\u0022panels-add-content-modal\u0022\u003E\n  \u003Cdiv class=\u0022panels-section-column panels-section-column-categories\u0022\u003E\n    \u003Cdiv class=\u0022inside\u0022\u003E\n      \u003Cdiv class=\u0022panels-categories-box\u0022\u003E\n              \u003Ca href=\u0022\/panels\/ajax\/ipe\/select-content\/panelizer%3Anode%3A7%3Adefault\/center\/activity\u0022 class=\u0022use-ajax panels-modal-add-category\u0022 title=\u0022\u0022\u003EActivity\u003C\/a\u003E              \u003Ca href=\u0022\/panels\/ajax\/ipe\/select-content\/panelizer%3Anode%3A7%3Adefault\/center\/comment\u0022 class=\u0022use-ajax panels-modal-add-category\u0022 title=\u0022\u0022\u003EComment\u003C\/a\u003E              \u003Ca href=\u0022\/panels\/ajax\/ipe\/select-content\/panelizer%3Anode%3A7%3Adefault\/center\/form\u...nelizer\/sites\/all\/modules\/ctools\/plugins\/content_types\/node\/icon_node.png\u0022 alt=\u0022\u0022 \/\u003E\u003C\/a\u003E  \u003Cdiv\u003E\u003Ca href=\u0022\/panels\/ajax\/ipe\/add-pane\/panelizer%3Anode%3A7%3Adefault\/center\/node\/node\u0022 class=\u0022use-ajax panels-modal-add-config\u0022 title=\u0022Add a node from your site as content.\u0022\u003EExisting node\u003C\/a\u003E\u003C\/div\u003E\r\n\u003C\/div\u003E\r\n\u003Cdiv class=\u0022content-type-button clearfix\u0022\u003E\r\n  \u003Ca href=\u0022\/panels\/ajax\/ipe\/add-pane\/panelizer%3Anode%3A7%3Adefault\/center\/custom\/custom\u0022 class=\u0022use-ajax panels-modal-add-config\u0022 title=\u0022Create a completely custom piece of HTML content.\u0022\u003E\u003Cimg typeof=\u0022foaf:Image\u0022 src=\u0022http:\/\/panelizer\/sites\/all\/modules\/ctools\/images\/no-icon.png\u0022 alt=\u0022\u0022 \/\u003E\u003C\/a\u003E  \u003Cdiv\u003E\u003Ca href=\u0022\/panels\/ajax\/ipe\/add-pane\/panelizer%3Anode%3A7%3Adefault\/center\/custom\/custom\u0022 class=\u0022use-ajax panels-modal-add-config\u0022 title=\u0022Create a completely custom piece of HTML content.\u0022\u003ENew custom content\u003C\/a\u003E\u003C\/div\u003E\r\n\u003C\/div\u003E\r\n    \u003C\/div\u003E\n  \u003C\/div\u003E\n\n  \n      \u003Cdiv class=\u0022panels-categories-description\u0022\u003E\n      Content options are divided by category. Please select a category from the left to proceed.    \u003C\/div\u003E\n  \n  \u003C\/div\u003E\n"}]
DamienMcKenna’s picture

Tentatively adding this to the todo list for the next release.

DamienMcKenna’s picture

Component: Code » Revisions
DamienMcKenna’s picture

Status: Needs work » Needs review
FileSize
14.36 KB

Rerolled.

DamienMcKenna’s picture

SocialNicheGuru’s picture

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

SocialNicheGuru’s picture

Status: Needs review » Needs work
michielnugter’s picture

FileSize
11.6 KB

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

michielnugter’s picture

Status: Needs work » Needs review
michielnugter’s picture

FileSize
11.64 KB

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

partdigital’s picture

I'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.

Status: Needs review » Needs work

The last submitted patch, 49: panelizer-n1798294-v3.1-49.patch, failed testing.

DamienMcKenna’s picture

Status: Needs work » Needs review

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

michielnugter’s picture

FileSize
12.27 KB

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

DamienMcKenna’s picture

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

mglaman’s picture

Correct - 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 :)

DamienMcKenna’s picture

FileSize
12.53 KB

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

DamienMcKenna’s picture

Status: Needs review » Fixed

And... COMMITTED! Thank you all for the help finishing this one!

michielnugter’s picture

Status: Fixed » Needs work

Sorry, 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):

if (empty($panelizer->revision_id) || $panelizer->revision_id != $revision_id) {
 // ...
}

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

DamienMcKenna’s picture

Status: Needs work » Fixed

A 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?

michielnugter’s picture

I'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.

DamienMcKenna’s picture

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

michielnugter’s picture

I quoted the line before the patch from this issue was applied.

The relevant bit from the patch:

@@ -1297,7 +1354,7 @@ abstract class PanelizerEntityDefault implements PanelizerEntityInterface {
       if ($this->supports_revisions) {
         // If no revision value is assigned, or a new revision is being created,
         // create a new {panelizer_entity} record.
-        if (empty($panelizer->revision_id) || $panelizer->revision_id != $revision_id) {
+        if (!empty($panelizer->entity_id) && empty($panelizer->revision_id) || $panelizer->revision_id != $revision_id) {
           $panelizer->revision_id = $revision_id;
<code>

After this patch is applied the relevant line of code is now:

<code>
if (!empty($panelizer->entity_id) && empty($panelizer->revision_id) || $panelizer->revision_id != $revision_id) {

I hope this clears it up. Let me know if it doesn't.

michielnugter’s picture

Status: Fixed » Needs work

I 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:

if (!empty($panelizer->entity_id) && empty($panelizer->revision_id) || $panelizer->revision_id != $revision_id) {

back to:

if (empty($panelizer->revision_id) || $panelizer->revision_id != $revision_id) {

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.

michielnugter’s picture

Status: Needs work » Needs review
FileSize
892 bytes

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

DamienMcKenna’s picture

FileSize
1.23 KB

Out of interest, does this resolve the problem too?

DamienMcKenna’s picture

Status: Needs review » Fixed

I've committed the patch in #64, please give the latest -dev release a try and let me know if the problem persists.

Status: Fixed » Closed (fixed)

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

DamienMcKenna’s picture

FYI there's a bug in this that is being dealt with separately: #2408301: Displays not saving correctly