Allow a particular revision of a fieldable panels pane to be placed in a panelized page

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.

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

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.

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.

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.

Rerolled against head.

Eclipse

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

Interdiff for those interested

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.

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.

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.

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

Thanks!

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

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

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

@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
Please open a separate ticket for that.

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

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

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.

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.

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

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

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

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

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

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.

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.

@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

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.

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.

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

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.

@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! :-)

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?

Patch re-roll to resolve offsets with fieldable_panels_panes v7.x-1.8.
Just to reiterate @danieltome's question - it would be good to know what's needed to get this into the module.

Just to mention it, I am seriously considering closing this as a "works by design" and recommending people use Paragraph Panes instead. Has anyone considered that module for doing per-display revision-aware changes?

That's a very promising module @DamienMcKenna. Thanks for the pointer. I agree that it would solve these problems well. FPP would remain to serve use-cases where you'd want to re-use panes.

@azinck: That's exactly what I'm thinking.

I am a bit torn on #53. While I have seen a lot of blood, sweat and tears in this issue and others regarding this I don't want our collective pride to cloud judgement. Maybe this issue is a bridge too far or maybe its not.

At the same time everyone is really mostly focused on D8 and there hasn't been much review going on here. @mairi posts a new patch and @DamienMcKenna's response is kind of short and dismissive of the entire issue. Which I mean, maybe doesn't bother her, but I personally would hate to work on a patch so hard to have the maintainer completely ignore it and not even answer the question at hand, instead sort of kicking rocks around and telling everyone we're wrong for trying to work this way...

So for distros that use these patches (I know Lightning 7.x uses it, not sure who else besides that and DF) it would be a pretty sad day if this was completely abandoned. I don't have the resources to completely re-architect the 7.x branches of those distros. And there's no way with Panopoly and Lightning both promoting FPP that we could somehow transform everyone over to users of this new module recommended. People are using FPP in these distros and if they need moderation for panelized pages that include FPP, they are in trouble.

We're holding on to this, hoping we can finally get it working for the sake of the slim number of users that have Lightning 7.x in production. They are definitely affected by this and won't have the luxury of complete re-architecture either.

I hear you Brantwynn. I think this patch, as-is, solves the basic technical problem, but that there's a major UX problem with the notion of reusable panes and how that interacts with revisioning such that I really think this solution is only a half-measure as it stands. On the major site where we do use this patch we've had to write a decent bit of custom code to make our site behave as I outlined in #12. And I don't know that our approach could be suitably generalized to serve all use-cases.

I've not checked out Lightning. Does it solve the UX problem?

brantwynn: You are correct, I was definitely overly dismissive in my comment.

I am very appreciative of all of the work that went into this patch, so a huge thanks to mairi, EclipseGc, heddn, ruloweb, MiroslavBanov, David_Rothstein, vilepickle and grndlvl! I was too hasty with my response and I sincerely apologize for that.

I will test this patch out (probably next week), and given it's already in use in the wild so much (distros? ok, I didn't think about that) will likely commit it to avoid causing problems for these sites.

The situation we're in is analogous to what happened with EntityReferences and revision tracking - while some of us were working on shoehorning revisions into EntityReferences (#1837650: Allow referencing a specific revision ID) someone else came up with the Paragraphs module, which is almost exactly what most of us wanted in the first place (though I'd have preferred it to be more of a generic entityreference module that could work with all entities).

What I'll probably do is commit this and add documentation to say this works but explain that most sites will probably want what they get from Paragraph Panes instead because of the other benefits.

And again, I am sorry for how I handled my initial response.

@DamienMcKenna Please don't worry about your initial response - I completely appreciate Paragraph Panes is a better alternative & can totally understand your initial suggestion. Unfortunately, as @brantwynn suggests, we're not in a position to re-architect what we have, which was embarked upon when Paragraph Panes wasn't around. If this patch isn't accepted, we will at least for a while have to continue re-rolling & re-applying it on each release of FPP. Having the patch committed & docs updated to point to Paragraph Panes would be a great result for me personally since I'm usually the one doing the re-rolling & testing :-) Thanks and fingers crossed for a positive outcome! The patch has been in use on our production site for over a year now & working well, although we don't have re-usable panes enabled.

@DamienMcKenna: I should apologize, as my post yesterday should have been much nicer. Sorry about that. Thank you for the extremely well thought out response on this issue. It is a sticky one for distros but you are right 100% about the problem this causes re: multiple asynchronous entities. We already see this issue with blocks and panelizer in D8 too, but maybe that is to a lesser extent.

My take-away from this whole thing is that there isn't much we can do for the UX issues @azinck mentions. Lightning in D7 and D8 attempt to subvert the problem by turning off the Panels IPE when the user is viewing the currently published node. I am not sure if that approach is sufficient for everyone but it seems like it will prevent some problems that could occur - though not all.

Thanks again for taking the time to look into this and a big ++ to everyone that has been keeping this patch alive!

Rerolled, and includes a tiny fix to some comments.

Is it possible that the logic for detecting whether to the vid or fpid in _fieldable_panels_panes_custom_content_type() was incorrect? Shouldn't it track the vid if the FPP is reusable and the fpid if it is not reusable? In my testing it was not tracking the vid for a reusable FPP.

There was one more occurrence of the same logic.

I wonder if maybe the extra revision tracking should be toggleable via a variable, with it set to "off" by default (Lightning could add an update script to turn it "on")?

Some updates to the comments.

re #64, thats totally fine. We can ship with a variable_set during hook_install and also provide an update hook to set the new variable for existing users.

Re #62:
For my use-cases I want to track the vid only for non-reusable panels and *not* track the vid for reusable panels.

Tracking the vid for non-reusable panels allows you to edit the panel while editing the node, and then have different FPP revisions embedded on different node revisions. When you edit an FPP on a node (or other entity) revision you don't tend to want those FPP changes to spread to all places that FPP is embedded; you want the edits to be specific to the revision of the parent entity on which you're doing the editing. In our use-case we've hard-coded the "create new revision" checkbox to always be true when editing non-reusable FPPs so that you can never accidentally step on the toes of some other parent entity revision where you've embedded the same FPP. Something like this is often the desired result:

NID      VID       FPP ID      FPP VID
5        20        34          103             
5        21        34          104

But reusable FPPs are rather a different story altogether. The whole point of those is to be able to make a change in one place and have it propogate to everywhere the FPP is embedded. So those should not be embedded as a specific revision, instead always showing the current revision of the FPP.

That said, communicating all of this nuance to the user is tricky and is why we made the modifications I mentioned in comment 12.

This needs a little more work to accommodate a global setting, which could be added to the new settings page which was just added via #1910934: Expose Fieldable Panels Panes as blocks.

I also think we need to add something to the README.txt to describe the workflow change, and we'll need a changenotice too. That said, I think we can get it into the next release :)

Per #67, what we want is a checkbox to "Force revision on non-reusable panes". While I agree and would use the setting, can't we make it a follow-up issue?

@MiroslavBanov: Yes, that can be a follow-up issue.

#70 and #71: I was just trying to address the question raised in #62. In #62 and 63 Damien flipped the logic to embed a specific revision only if the FPP is reusable. I am trying to argue that the old logic was correct and that Damien's new logic is wrong.

Hm, I agree - the flipped logic is not correct.

Should be changed. Let's change the !empty() to empty(), and commit to get this out of the way.

And then to follow-up (in the same issue or in different one) and figure out what to do with configuration. Maybe #64 and #67 can be controlled by the same variable? But need more people to chime in on it.

Ok, life happened and one of our clients requested this, so I'll be finishing it off tomorrow. :-)

This adds a new setting that controls the revisions functionality via a new variable ("fpp_revision_locking").

Note - I haven't tested it yet, I'll do that in the morning.

Updated the dev version of Lightning Features to include FPP (dev HEAD) w/ the new patch. Everything is working fine but I don't have automated tests working on our end yet to ensure no regressions. That said, we are working on that. I did not want to hold anyone up and Damien had asked me on IRC to look at this. All of my manual tests seem to be working fine.

Test 1. Lightning 7.x-1.x-dev (non-reusable panes)

1. New landing page (panelized content type)
2. Add a new FPP (Lightning FPP media widget)
3. FPP is showing an image
4. Moderate with Workbench Moderation to Published
5. Create new draft
6. Edit the FPP on the new draft, changing the image on the file field to a new upload
7. Save FPP, Save as custom in Panels IPE for the Panelized node
8. View Published version of node (see old image) - w00t!, as expected with proper FPP revision
9. View Draft version of node (see new image) - w00t!!, as expected with proper FPP revision

Test 2. Lightning 7.x-1.x-dev (reusable panes)

1. New landing page (panelized content type)
2. Add a new FPP (Lightning FPP media widget)
3. Check the "Make this reusable" checkbox
4. FPP is showing an image
5. Moderate with Workbench Moderation to Published
6. Create new draft
7. Edit the FPP on the new draft, changing the image on the file field to a new upload
8. Save FPP, Save as custom in Panels IPE for the Panelized node
9. View Published version of node (see new image) - w00t, its reusable
10. View Draft version of node (see new image) - w00t, it persisted

4 w00ts all around, which by my count seems to cover both re-usable and particular revisions of a FPP to be placed in a panelized page.

This setting fpp_revision_locking is explained as:
Should pane changes create new revisions?

1. legacy - Should pane changes create new revisions?
2. lock - Lock non-resuable FPPs.
3. lock_all - Lock non-resuable and reusable FPPs.

But what it actually does is control if panes content should target pane revision id, or the pane entity id. The term "locking" is not very informative by itself.

The checkbox "Create new revision" still controls if a new revision is created.

Trying a better description of the setting.

@MiroslavBanov: Did you try testing the functionality itself?

I have been using early versions of this patch for a while, in order to fix this issue #2101255: Updating existing FPP-pane with media field renders wrong in preview. I use it on a panel page that is not revision-enabled, so most of the issues resolved here only affect me in unlikely corner-cases. I did test this on a clean install, and validated some assumptions, but I can't be as detailed as brantwynn.

I agree with the changes. I am just unsure about the naming of the configuration, because it is hard to explain the implications. I did change it and I still don't like it. Feel free to reject the changes in #79.

We now have some automated testing via Lightning that shows this patch working. Video attached (sorry a little blurry, but you should get the gist).

Shows use Test 1 from #77

This is as close to RTBC I can get, but I will suggest we await @azinck for confirmation that his use cases are covered.

Thanks @brantwynn. I've been using an early version of this patch for a long time. I'll test the newer version this afternoon and report back.

I don't want to open a huge can of worms, but I'd like to suggest that, if someone has chosen to "lock" the pane, that perhaps we should also by default create a new revision of the FPP for them any time they edit it. I'm not clear on all the ways people are using this functionality so maybe I'm off-base but, to me, this seems a better default.

I just tested #79 and it is working correctly for me. I agree with @MiroslavBanov that the language on the settings page in #75 is very confusing. The language in #79 is better; not perfect, but at least better.

I'll mark #79 as RTBC. I still would love to see UX improvements (per my comment in #83) but recognize that this functionality is being used in a wide variety of ways and #79 is a definite step in the right direction.

Thanks to Damien and Miroslav for your contributions.

@azinck: Your idea of auto-creating a new revision is a pretty good one, maybe it's worth spinning off another issue to cover that?

There are no functional changes here from #79. This is just a re-roll of the patch with a couple of typo corrections in the admin interface.

I've taken out #2695499: Improve UX when working with FPP revisions as a follow-up to try to deal with some of the UX problems coming from this patch.

@azinck: Thanks.

Committed! Thank you all for the work and patience on this. We'll be following up with additional issues to improve the UX, and working on documentation to make it clear what happens, but this is a huge step forwards for sites that need it.

So for some reason, our automated tests are now failing since updating to the latest commit.
-- https://travis-ci.org/acquia/lightning-features#L1148

My manual tests (same as test case 1 from #77) are now failing too.

The point of failure is the actual retention of the revision -- e.g. I edit the FPP in Draft mode and the FPP is updated on both the Published revision and the Draft, both when the 'make this reusable' checkbox is checked and when its not checked.

Will try to get to the bottom of this since literally nothing has changed in the patches based on the interdiff.

The last patch we had applied working was from #75 applied to revision a067f9b

Ok, ignore #93 -- I am realizing now, in my earlier tests, something must have been off - not sure what, but we need to explicitly set fpp_revision_locking to 'lock' in order for this to work as expected. I went through the patch from #75 we had been using line by line and found that variable. Not sure why the tests managed to pass before given the variable should always have been set to 'legacy' by default. I think we're good. False alarm!
-- https://travis-ci.org/acquia/lightning-features/builds/119307183#L1148

Will try to get to the bottom of this since literally nothing has changed in the patches based on the interdiff.

For what it's worth, I just double-checked the patch re-uploaded in #86 against the original in #75. I'm not seeing anything changed that's not represented in the interdiff.

FYI please follow #2695499: Improve UX when working with FPP revisions while we refine this functionality.

I think this commit introduced a problem with Entity Translation.

See #2820177: Revision ID breaks translation workflow.