We are using Fieldable Panels Panes, Panelizer and Workbench Moderation in tandem to build a site with a moderation workflow in which a draft of the entire page is created and then reviewed before being published.

It works well, except when a fieldable panels pane is edited on the draft version of the page, the edits automatically appear on the published version of the page too. This is because the panelized page references the entity ID of the fieldable panels pane and therefore always uses the most recent revision.

I propose allowing the panelized page to refer to the revision ID of the fieldable panels pane instead. That way the draft revision of the node and the published version of the node will refer to different revisions of the FPP when the FPP is edited, and everything magically works.

Comments

David_Rothstein’s picture

Status:Active» Needs work
StatusFileSize
new1.07 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch fpp-revisions-1986334-1.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

So, the Fieldable Panels Panes module almost supports this already. Here's a patch demonstrating a small tweak that makes it actually support it.

This patch is fine for our use case, but would need to be developed further in order to be committed to the module. Instead of forcing the revision ID to be stored in all cases like I'm (hack-ily) doing in the attached patch, it would presumably need to be some kind of option.

David_Rothstein’s picture

Another issue (credit goes to grndlvl for thinking of this) is that this model kind of breaks down for reusable FPPs... There, the goal (even for our use case) actually sort of is that if you edit the FPP in one place, your edits take effect everywhere at once.

merlinofchaos’s picture

You have just discovered the use-case for ERS, which works on fieldable panel panes where workbench moderation does not.

It's...vaguely sort of kind of possible that you could use ERS only for fieldable panel panes while still utilizing workbench moderation for your nodes, though I suspect it will be extremely clunky.

grndlvl’s picture

StatusFileSize
new2.08 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch fpp-revisions-1986334-4.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

I am not sure how this follows ERS's use-case, while, ERS handles the publishing of revisions at specific times it still separates the pane from the Panelized entity.

We have Panelizer working with workbench with the hack/patch from http://drupal.org/node/1402860#comment-7344842 and we def. found out the hard way that these modules do not play together nicely, however, I think this particular use-case could still be valid because it is not dependent on workbench, but rather revisionable entities in general. Especially when you wish to tie an FPP to a specific revision so that when a user edits the FPP on a draft Panelized node via the IPE then the live FPP should not be changed, but yet lives as a separate revision that is specific for the current revision of the Panelized node.

To open up additional dialog I have attached a patch that accomplish this, albeit pretty hackishly.

I also wonder if another approach to this would be to extend the entity field panels content type to allow on the fly editing that respects the revision of the node.

Thanks,

Jonathan

David_Rothstein’s picture

Right, my understanding is that ERS would be for a situation where you want each pane to have its own independent publishing workflow. But in our case the panes are closely related so the goal is to have them reviewed and published all at once.

In other words, if the page starts off with 4 panes:

A - B - C - D

The idea is that an editor can add, delete, rearrange, and edit them while working on a draft of the page, such that they wind up (for example) like this:

B - A - D (edited) - E

And then that whole set of changes is pushed live at once.

It does work very nicely with this patch as well as the other one Jonathan mentioned, although both are still a bit hackish.

Since all the changes in the patch here are in a single submit handler, it's possible the right way to do this doesn't involve patching the FPP module at all; another module could swap out the submit handler instead and just deal with it that way. We do require a couple other UI changes for this to behave sensibly (for example, hiding the FPP revision checkbox and forcing it to always be checked) and a separate module could take care of that all at once.

MiroslavBanov’s picture

There is a similar patch that allows use of vuuid here:
https://drupal.org/comment/8428099#comment-8428099

It may be worth looking at it and possibly combining the two patches here.

azinck’s picture

Issue summary:View changes

I agree that the logic in #4 is sound. The real challenge here is a UX one. I'd suggest that one way to communicate the difference between reusable and non-reusable panes would be to prohibit the editing of reusable panes in the Panels interface and instead force the user to go to the Fieldable Panels Panes UI to edit those panes.

I'm also thinking we shouldn't allow panes to move between being reusable and non-reusable panes. Once created they should remain as either reusable or non-reusable. If it's necessary to switch from one to the other then the pane should be duplicated. Otherwise you could get into some really confusing UX situations.

azinck’s picture

Also: I'd love to hear more from merlin about how he sees ERS as addressing this problem. I've given ERS a try and cannot see how it addresses this problem but I may just be dense or there may be another way of approaching this problem that I'm blind to.

EclipseGc’s picture

Status:Needs work» Needs review
StatusFileSize
new2.41 KB
PASSED: [[SimpleTest]]: [MySQL] 0 pass(es).
[ View ]

Rerolled against head.

Eclipse

EclipseGc’s picture

StatusFileSize
new3.31 KB
PASSED: [[SimpleTest]]: [MySQL] 0 pass(es).
[ View ]

I'm actively using this on a site right now and it's performing pretty well, however it has a couple of issues.

1.) Edit->Save a reusable FPP will result in it being removed from the interface.
2.) Cached view of the FPP after save in IPE.

Logic to fix 1.) resulted in a cached FPP because passing the revision id is what causes entity class load() method to bypass cache, so I had to introduce a new type of subtype to support this which is "current:$fpid". There is corresponding logic in _fieldable_panels_panes_load_entity() that selects the max vid of the FPP and passes that along with the load to get the FPP that was just saved. This is definitely still NR but I think we're getting closer to something that's got a fair bit of testing on it and which actually works!

Eclipse

EclipseGc’s picture

StatusFileSize
new1.44 KB

Interdiff for those interested

azinck’s picture

I should add that we've been using #4 in production on a very large site for about 10 months now with no problems. We didn't run into the problems described in #10 because we're not using the IPE and we've done a number of alterations to prevent editing reusable FPPs in the Page Manager interface. So for our self-limited use-case it's been working extremely well.

EclipseGc’s picture

In the spirit of full disclosure, I suggested a similar set of limitations in the system I'm working in, but ultimately the changes required to support the actual feature were pretty minimal. I'm very pleased with the basic approach of this patch.

Ramasubbu Drupal’s picture

Hi to everyone,

If we use the "Workbench" module then the node must enable with "Revision". But if we use the "Workflow" module then no need to use the "Revision" option. So the panelized fieldable panel pane will display with any changes and workflow changes on the node.

No need any patches. I have done this flow in my project. Its working fine with workflow and panelizing.

The workflow module is available for Drupal 7 also. Click here to download the workflow module.

ruloweb’s picture

This patch breaks fieldable_panels_panes export using features, because entity_id is containing vid instead of uuid when revision is enabled.

Thanks!

MiroslavBanov’s picture

@ruloweb
If anyone is interested, I have a patch here that fixes pane load by vuuid:
https://www.drupal.org/node/2101255#comment-8428099

ruloweb’s picture

StatusFileSize
new3.1 KB
PASSED: [[SimpleTest]]: [MySQL] 0 pass(es).
[ View ]
new1.46 KB

Thanks @MiroslavBanov !

I created a patch merging all.

Anyway in order to export fpp using uuid and uuid_features we need also this patch #2499099: FPP vuuid are needed in order to support revisioning

Status:Needs review» Needs work

The last submitted patch, 17: fpp-revisions_and_uuid-1986334-17.patch, failed testing.

vilepickle’s picture

StatusFileSize
new5.14 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch fieldable_panels_panes-fix_uuid_fpp_revisions-1986334.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

On our site, the #17 patch isn't working with uuid installed. Not sure if it's specifically uuid causing the problem, but the root cause is in PanelsPaneController.class.php's save function.

This is what I changed the line to that checks if it should unset vid or not:

if (!$entity->is_new && $entity->vid) {

It was:

if (!$entity->is_new && !empty($entity->revision) && $entity->vid) {

$entity->revision was blank on our site, I'm not sure if it's because we have uuid or not but our FPP's are definitely revisioning-capable.

This patch should include everything in #17 but with my change. It also has some additional checking for the uuid module later on that could be looked over a little bit more.

EclipseGc’s picture

  1. +++ b/includes/PanelsPaneController.class.php
    @@ -120,7 +120,7 @@ class PanelsPaneController extends DrupalDefaultEntityController {
    -    if (!$entity->is_new && !empty($entity->revision) && $entity->vid) {
    +    if (!$entity->is_new && $entity->vid) {

    Why did this specifically need changing?

  2. +++ b/includes/PanelsPaneController.class.php
    @@ -183,7 +183,12 @@ class PanelsPaneController extends DrupalDefaultEntityController {
    +      ¶

    Extra spaces.

  3. +++ b/plugins/export_ui/fieldable_panels_pane.class.php
    @@ -74,6 +74,7 @@ class fieldable_panels_pane extends ctools_export_ui {
    +    drupal_static_reset();

Is there a specific static you're trying to reset here instead of just all of them?

All in all, this seems pretty sensible to me. It's unfortunate all the module_exists() special casing we've had to do for features like this, but it is what it is.

Eclipse

vilepickle’s picture

Ah, had some spacing from debug statements probably.

I think the static was from the patch originally? I thought that line was odd too, I did not really recall seeing it in the initial patch.

As for #1, I think that line was what was preventing our site from creating the revision since it was always empty for some reason on the entity. I was looking in PHPMyAdmin while testing and when removing it creates 2 revisions properly with uuid's set. With just #17 applied, it kept editing only the FPP that existed. Maybe there's some other setting on our site preventing FPP revisions? I don't see the revision UI element when editing FPP's on our IPE popup but can confirm it's set to 1 on the form element.

MiroslavBanov’s picture

@vilepickle
#1 $entity->revision comes from the UI element and is a flag for whether to create a new revision or not. I don't understand why you have the problem and can't reproduce it.
#3 The drupal_static_reset() is not part of the original patch.

Edit: I feel like you are experiencing a separate issue, aside from this one, but maybe I'm wrong.

EclipseGc’s picture

@vilepickle

I agree with MiroslavBanov here. The $entity->revision is just a boolean statement for the entity system about whether to create a new revision or not. If that is NOT set to true, you have another problem (which is likely part of the greater issue).

The drupal static is not part of the original patch. It's good practice to provide interdiffs between patches which would also reveal these sorts of issues.

Eclipse

vilepickle’s picture

So I agree that my patch is not for a generic use-case looking at it closer. We have no UI element for revisions on either /admin/structure/fieldable-panels-panes/*/add or the IPE popup. Is there some option for turning them on or off on FPP's? I'm logged in as an administrator and revisions are enabled on content types.

MiroslavBanov’s picture

@vilepickle
Please open a separate ticket for that.

I think we should re-post and review patch #17, because it focused on this issue.

vilepickle’s picture

Yeah, sorry about that. We had a feature with a custom form alter from when our site was built removing those elements from the form. Go figure.

heddn’s picture

Issue tags:+Needs reroll
MiroslavBanov’s picture

Status:Needs work» Needs review

#17 still needs review. Or maybe it can be re-uploaded without change, just to be the last patch.

The last submitted patch, 4: fpp-revisions-1986334-4.patch, failed testing.

The last submitted patch, 1: fpp-revisions-1986334-1.patch, failed testing.

Status:Needs review» Needs work

The last submitted patch, 20: fieldable_panels_panes-fix_uuid_fpp_revisions-1986334.patch, failed testing.

heddn’s picture

Status:Needs work» Needs review
Issue tags:-Needs reroll
StatusFileSize
new5.35 KB
PASSED: [[SimpleTest]]: [MySQL] 0 pass(es).
[ View ]

With the changes in _fieldable_panels_panes_load_entity(), I had to move a hunk around to fieldable_panels_panes_load_from_subtype_force. No interdiff as this is essentially a re-roll.

heddn’s picture

Should this block commit? I'm seeing that a new revision for the FPP gets created on *every* save. Nothing in here checks if the FPP is identical to the previous revision. I'd prefer some type of comparison so I don't clutter up field revision and fpp_revision tables. It was brought to my attention that is actually the case, even before this patch. So I should probably open a follow-up to address.

To reproduce:

  • Open a panel
  • Make no changes
  • Save
  • A new revision is created, always.
heddn’s picture

heddn’s picture

Ignore #34. This is due to a form alter that hides and checks the create new revision checkbox.

DamienMcKenna’s picture

@heddn: Which patch did you reroll from - #17 or a different one?

@EclipseGc: Could you please confirm that patch #33 still works for you? Thanks.

heddn’s picture

re #37: This was a re-roll of #17.

MiroslavBanov’s picture

Status:Needs review» Needs work
StatusFileSize
new3.1 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch fpp-revisions_and_uuid-1986334-17_0.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

@heddn
It's quite clearly not a re-roll of #17. It is a re-roll of #20. #20 introduces three changes, all of them questioned in #21, further discussed #22 through #27, and still without justification. #33 is a straight re-roll of #20 and includes all the changes from #20. I am re-uploading #17 without any modification.

DamienMcKenna’s picture

Status:Needs work» Needs review

Status:Needs review» Needs work

The last submitted patch, 39: fpp-revisions_and_uuid-1986334-17.patch, failed testing.

heddn’s picture

Status:Needs work» Needs review
StatusFileSize
new3.47 KB
PASSED: [[SimpleTest]]: [MySQL] 0 pass(es).
[ View ]

re #39, good catch. I had intended to grab #17, but apparently I didn't. This should fix that, and the re-roll too.

brantwynn’s picture

Has anyone tested this latest patch with UUID module turned on? I am finding that it functions as expected up until I turn on UUID. At that point I get the dreaded Placeholder for empty or inaccessible "Deleted/removed entity pane" message in place of the FPP inserted.

brantwynn’s picture

Ok I read the whole issue and found that the patch from #33 to be a stop gap for us. Guess we should address the issues brought up in #17 either in a followup, or an issue for UUID or as part of the next patch if we can determine there's something we can do on the FPP side of things and thats the correct thing to do.

MiroslavBanov’s picture

@brantwynn,
If I have to guess, there is one legitimate change in #20, which you can try:

@@ -183,7 +183,12 @@ class PanelsPaneController extends DrupalDefaultEntityController {
     $entity->uid = $uid;
     // Update the existing revision if specified.
     if (!empty($entity->vid)) {
-      drupal_write_record('fieldable_panels_panes_revision', $entity, 'vid');
+      if(module_exists('uuid')) {
+        drupal_write_record('fieldable_panels_panes_revision', $entity, 'vuuid');
+      } else {
+        drupal_write_record('fieldable_panels_panes_revision', $entity, 'vid');
+      }
+     
     }
     else {
       // Otherwise insert a new revision. This will automatically update $entity
mairi’s picture

StatusFileSize
new5.32 KB
PASSED: [[SimpleTest]]: [MySQL] 0 pass(es).
[ View ]
new3.77 KB

I am uploading an updated version of the patch at #42, along with an interdiff to show what has been changed. This patch resolves issues with FPP revisions for us - using fieldable_panels_panes 7.x-1.7 with uuid. We also have organic groups & workbench_moderation in play, but they don't seem to be a factor in this particular scenario.

Changes from the previous patch #42 are as follows:

  1. set $object instead of $content in modified function fieldable_panels_panes_load_from_subtype_force() as it is $object that is being returned, not $content;
  2. write using vuuid instead of vid when saving an FPP revision if uuid is installed;
  3. modify plugins/content_types/fieldable_panels_pane.inc to use vid or vuuid as appropriate when fpp revisions are in use (without this change in place, fieldable_panels_pane objects within the context of a panelizer revision always load the latest revision of a pane because it is using uuid rather than vid).

Interdiff attached to show changes between patches #42 and #46.

danieltome’s picture

Hi @mairi,

I applied the interdiff patch from 42-46 on a brand new lightning alpha install, and it is working well.
FPP Panels are showing correctly on draft and published versions.

There is one issue with media though.

Steps to reproduce:
1) Create new landing page panel and add a media panel and upload an image.
2) Publish page.
3) Create new draft, then edit media panel and change the image to a different one.

Issue:
Latest image is shown on draft and published nodes.

Expected result:
Draft node to show new image, published node to show the previous image.

brantwynn’s picture

@danieltome's issue sounds like it is related to the file entity that is attached to a fieldable panels pane field. It may be out of scope for this particular issue to solve keeping versions of file entities working with FPP revisions. If anyone else is seeing this or knows of a related issue, perhaps we can cross reference.

MiroslavBanov’s picture

I think the issue in #46 is integration of panelizer and workbench moderation. The patches help fixing uuid and FPP revisioning, but do not completely resolve original issue. Maybe need to split or do follow-up? Actually the other issues are probably for other modules to resolve.

mairi’s picture

@danieltome Sorry you're still having issues after that latest patch. We are not using media panels, or the lightning distribution so I'd need to get that locally to investigate, which I won't have time for this week. It does sound like a 'related but different' issue to me, but wouldn't want to dismiss it as out of scope for this patch without looking at it more closely. As @brantwynn suggests, perhaps we can cross reference if there are related issues.

I can say that we have had a lot of trouble getting fpp revisions & workbench_moderation to play nicely together, which seems to be a common experience! This patch and one of the ones from workbench_moderation (https://www.drupal.org/node/1402860) are working for us. I don't know if the latter would resolve your issue. I need to review our use of that patch actually as it's now ahead of where it was when we started using it. Ah, the joys of fpp/panelizer/workbench_moderation! :-)

danieltome’s picture

Thanks @mairi, I think @brantwynn is right, in regards to #48 and that it is related to file entities, and the way the replace image is done.
I can raise that in another issue. re: file_entity, fpp and workbench_moderation.

I'm happy this patch is moving forward and it is working well with all other panels.
What do we need to do to get this patch release into the module?