Panopoly Magic provides a "preview" functionality. This works pretty well with regular CTools content type plugins. To implement it with Fieldable Panel Panes, Panopoly hijacks the entity's revisions.

Whenever the preview is submitted it will save a new version of the FPP to generate a revision. As a "fix" it will make sure the entity keeps track of its proper revision key.

<?php
        fieldable_panels_panes_entity_edit_form_submit($form, $form_state);
        $preview_subtype = 'vid:' . $form_state['entity']->vid;
        $pane->subtype = 'fpid:' . $form_state['entity']->fpid;
      }

      // Properly adjust the VID if we are dealing with an existing object
      if (!empty($current_vid)) {
        db_query("UPDATE {fieldable_panels_panes} SET vid = :vid WHERE fpid = :fpid", array(':vid' => $current_vid, ':fpid' => $form_state['entity']->fpid));
      }
?>

This doesn't do any good for content moderation if you're trying to track entity updates to provide reverts. It also sucks for the database because the preview triggers on each field update - or sometimes even tabbing in and out of a window.

Trying a few different approaches, but running into kinks for fixing this. The biggest issue is ctools_content_render() requires just a type and subtype to load.

Here's how the preview gets built (the render array)

<?php
function panopoly_magic_form_post_render_preview($output, $form) {
  extract($form['#panopoly_magic_preview_info']);
  $content = (empty($preview_subtype)) 
    ? ctools_content_render($pane->type, $pane->subtype, $configuration, $keywords, $args, $context) : 
    ctools_content_render($pane->type, $preview_subtype, $configuration, $keywords, $args, $context);
?>

My first approach was to just say "oh, well if it's an FPP generate content type itself." Well I found my self copy+pasting that function and the render function from FPP content type in its entirety - not exactly a good thing.

Second approach was to provide a subtype render function for FPPs. If the subtype ID matches, it'd use Panopoly's own generator - by stashing the stubbed out FPP entity in the pane's configuration. This causes new FPPs to break (I don't get why yet.)

CommentFileSizeAuthor
#45 panopoly_core-fpp-revision-abuse-2398347-45.patch827 bytesdsnopek
#41 panopoly_test-fpp-revision-abuse-2398347-41.patch17.53 KBdsnopek
#39 panopoly_core-fpp-revision-abuse-2398347-39.patch749 bytesdsnopek
#37 panopoly_test-fpp-revision-abuse-2398347-37.patch17.47 KBdsnopek
#34 panopoly_test-fpp-revision-abuse-2398347-34.patch17.24 KBdsnopek
#33 panopoly_test-fpp-revision-abuse-2398347-33.patch15.65 KBdsnopek
#33 panopoly-fpp-revision-abuse-2398347-33.patch517 bytesdsnopek
#32 panopoly_test-fpp-revision-abuse-2398347-32.patch11.57 KBdsnopek
#31 panopoly_test-fpp-revision-abuse-2398347-31.patch11.24 KBdsnopek
#30 panopoly_test-fpp-revision-abuse-2398347-30.patch5.89 KBdsnopek
#30 interdiff.txt1.44 KBdsnopek
#30 panopoly_magic-fpp-revision-abuse-2398347-30.patch7.68 KBdsnopek
#29 panopoly_test-fpp-revision-abuse-2398347-29.patch4.19 KBdsnopek
#25 interdiff.txt1.34 KBdsnopek
#25 panopoly_magic-fpp-revision-abuse-2398347-25.patch6.48 KBdsnopek
#22 interdiff.txt1.41 KBdsnopek
#22 panopoly_magic-fpp-revision-abuse-22.patch6.43 KBdsnopek
#15 panopoly_core-fpp-revision-abuse-2398347-15.patch748 bytesdsnopek
#15 interdiff.txt942 bytesdsnopek
#15 panopoly_magic-fpp-revision-abuse-15.patch6 KBdsnopek
#14 interdiff.txt6.43 KBdsnopek
#14 panopoly_magic-fpp-revision-abuse-14.patch6.07 KBdsnopek
#13 interdiff.txt4.94 KBdsnopek
#13 panopoly_magic-fpp-revision-abuse-13.patch7.46 KBdsnopek
#8 panopoly_magic_abuses-2398347-8.patch5.92 KBmglaman
#7 panopoly_magic_abuses-2398347-7.patch9.51 KBmglaman
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

mglaman’s picture

Issue summary: View changes

Add code context to preview rendering.

mglaman’s picture

Also, if a user goes to create an FPP and cancels, the orphaned entity sits around in the database.

stevector’s picture

What's your ultimate goal here? Tying Panelizer changes to specific node revisions?

mglaman’s picture

Well, the end goal is still providing previews for Fieldable Panels Panes but not dumping cruft throughout the database.

As it currently stands

  • If I click "Add table", an FPP with the bundle of "table" is added to database regardless if I click Save or Cancel
  • Each time I modify the form and cause the preview to trigger a revision is saved (albeit the proper vid preserved.)

This can lead to some large tables. It also makes it impossible to say "I wish to revert fpid 123 to its previous vid" because who knows what it was (without tracking content pane saves to check the "real" vid)

stevector’s picture

How about a cron hook that deletes FPP vids that aren't used in any displays?

mglaman’s picture

Then you lose historical data that is useful. Also, there's also a need to fix this because what if you're listening on entity_insert, entity_update? You'll be spammed by preview without knowing what's truly an update or not.

mglaman’s picture

Status: Active » Needs review
FileSize
9.51 KB

Here is a patch to end the revision madness and improve performance! This introduces a new concept for providing previews with entity based panes (if anything ever rivals FPPs.) Panopoly Magic is now providing "Fieldable Panels Pane Preview". The magic of generating the preview hotswaps to the content type for generating the preview.

The changes are stubbed out as an entire entity and stashed in the pane's configuration. That way the content type preview render function can tap into that and render out the entity.

Patch was created against 1.13 tag initially.

mglaman’s picture

FileSize
5.92 KB

Whoops :) Forgot to make patch by diffing against the 1.13 tag instead of HEAD.

cboyden’s picture

I'd love to see something like this happen. See also the earlier issue #2312723: Panopoly Magic shouldn't generate so many *perminent* revisions for Fieldable Panel Panes in which @dsnopek outlines a different approach.

mglaman’s picture

After reviewing #2312723: Panopoly Magic shouldn't generate so many *perminent* revisions for Fieldable Panel Panes, I like the ideas proposed in my patch. The other issue provides a method for cleaning up revisions, but it also adds a lot of queries to be run, whereas this removes the need to write to the database until changes are saved.

Also, the other issue still saves entities. This causes entity_/update/save hooks to trigger. This causes overhead but also affects services which listen in on new/changed content.

My hope is that this suggested approach provides a different way for providing previews with entities without having to save them. Just like Page Manager supports other templating systems besides Panels, we could see another module beyond Fieldable Panels Panes which has a similar issue with previews.

dsnopek’s picture

I haven't tested this, but the general approach looks good. The only thing I worry about is declaring a CTools content type that we don't really want to allow users to use (ie. add to a Panel somewhere).

Would it be possible to implement this without declaring a new content type? Like, maybe you could mark it as using a "preview_callback" (rather than a "preview_type") which could generate the preview content without calling ctools_content_render()?

dsnopek’s picture

Assigned: Unassigned » dsnopek

I'm going to take a stab at finishing this!

dsnopek’s picture

Status: Needs review » Needs work
FileSize
7.46 KB
4.94 KB

Here is an updated version of this patch that works for me. It fixes the following issues:

  1. The patch from #8 didn't work for new FPPs, only existing ones
  2. Title (and other base property values) weren't sticking

Right now, it still has a bug that changes to 'link' and 'path' have no affect on the preview. I'm not sure exactly how those are implemented, but it's got to be in CTools or Panels somewhere because FPP has no explicit support for rendering them.

However, rather than fixing that, I think I'm going to persue switching to an approach that doesn't need to declare a content_type (per #11) which would mean that we wouldn't be able to depend on what CTools or Panels is doing for title support anyway.

dsnopek’s picture

FileSize
6.07 KB
6.43 KB

Here's a patch with the new approach from #11. However, I'm still having issues with changing the link/path when editing an existing FPP. It appears to be taking the values from the database, rather than using what we pass to it. :-/

dsnopek’s picture

Alright! I think I've got it. I needed to patch FPP to fix the title link/path issue (which was in FPP, I just didn't realize it at first, because it's in the theme layer).

Next, I need to try the Behat tests with these changes on Travis-CI!

EDIT: Here's the Travis-CI build:

https://travis-ci.org/dsnopek/panopoly/builds/48566961

mglaman’s picture

Amazing! I only read latest patch via phone, but looks like a much better approach than my content type hack. I like that it gives flexibility as well.

I can give it a test run tomorrow for my use case this required.

Thanks for finding a much simpler approach :)

dsnopek’s picture

@mglaman: Thanks for starting this patch! This overall approach (not saving the FPP at all) is better than my first attempt at #2312723: Panopoly Magic shouldn't generate so many *perminent* revisions for Fieldable Panel Panes. :-)

All the tests pass on Travis-CI! Looking forward to hearing back from @mglaman on his testing: if he doesn't find any problems, I think it's time to commit this.

dsnopek’s picture

Status: Needs review » Needs work

I messed up the FPP patch at #2415427: Avoid loading FPP from database in fieldable_panels_panes_preprocess_panels_pane(), so I need to update the panopoly_core patch for it. Setting back to "Needs work" to fix that - but this should still work enough to review!

mglaman’s picture

I'm putting my RTBC on the Panopoly Magic patch in #15. Fixes issue without hacking content type work around.

mglaman’s picture

I take back RTBC. Image and Spotlight are broken. This is because the uploads do not technically have a file ID yet and are passed 0. Only workaround would be to flip on media browser widget for these fields (probably.)

dsnopek’s picture

Thanks, @mglaman for testing!

I wonder what Drupal core does for node previews when image fields are involved? I hardly ever use node previews, but I vaguely remember it showing the actual images, so this might be fixable.

dsnopek’s picture

Status: Needs work » Needs review
FileSize
6.43 KB
1.41 KB

Looking at what node does for it's previews turned out to be the key! They had this magic incantation that fixes images:

_field_invoke_multiple('load', 'node', array($node->nid => $node))

I also fixed a PHP warning and decided it was a good idea to run the field validation callbacks (not just the submit callbacks) since there are some fields that do change their data during validation (of course, they shouldn't - but they do).

Anyway, here is the updated patch and interdiff!

@mglaman: Any chance you could take this one for a spin?

EDIT: Travis-CI link - https://travis-ci.org/dsnopek/panopoly/builds/48827793

mglaman’s picture

Status: Needs review » Reviewed & tested by the community

This fixes the images issues, and also fixes the constant saving of FPP entities. I'm giving two giant thumbs up

dsnopek’s picture

Woohoo, thanks!

dsnopek’s picture

Status: Reviewed & tested by the community » Needs work
FileSize
6.48 KB
1.34 KB

Blergh! I did some more testing myself and found some issues with this patch. :-/ Sorry, @mglaman, I jumped the gun on pinging you to test it.

I watched the revision tab on an FPP I was editing to make sure that new revisions weren't getting generated, and found that it wasn't generating new revisions during the preview, but it was saving a whole new FPP when you ultimately click "Save".

Attached is a new patch that fixes that, but now there is a new problem: After you click "Save", the overall page isn't updated with the new content (until you save the overall page with the IPE). I'm still trying to figure out what that's about.

mglaman’s picture

My day today is revolving around this exact bug. I'll see what I can get out of it.

mglaman’s picture

Assigned: dsnopek » mglaman

Going to take a stab at this for a few hours.

mglaman’s picture

Assigned: mglaman » Unassigned

Ok, after doing some investigation #25 is spot on. We've narrowed the issue down to FPP's which are marked as reusable content.

I did some debugging within \PanelsPaneController->view() and \PanelsPaneController->buildContent(). The $entity value being passed is stale. Inspecting the field values do not have updated data. So something isn't being cleared out or passed properly. I have a few meetings to run to, and can check into it later. But that's at least putting us a step closer.

dsnopek’s picture

Took a detour and started writing Behat tests for this functionality. So far, I've just got a test that shows the bug with reusable widgets. I want to write tests for all the things that we found broken along the way with the patch, so we can be sure not to break them again!

dsnopek’s picture

Alright! Here is a patch that fixes the new issue from #25 with reusable widgets (@mglaman: you were right! It did have to do with the entity getting static cached). Also, here is a new test patch that tests for the link issue that we fixed with the FPP patch in panopoly_core many comments ago.

So, this could use more review to find more new bugs if any exist!

But I'm also going to continue improving the tests to catch as many of the issues we found with this patch in the comments above, so that if we ever need to change this code again, we won't break all the use cases we just fixed. :-)

dsnopek’s picture

FileSize
11.24 KB

An experimental panopoly_test patch to try and make sure the images are valid. Due to the way I run Selenium, I can't get a full test of this, so I'm uploading here to run on Travis-CI and make sure it really works!

EDIT: The Travis-CI link: https://travis-ci.org/dsnopek/panopoly/builds/50545681

dsnopek’s picture

FileSize
11.57 KB

Hrm. Here's an attempt to get better error messages out that travis run.

EDIT: This ultimately passed - Huzzah! https://travis-ci.org/dsnopek/panopoly/builds/50549806

dsnopek’s picture

Status: Needs review » Needs work
FileSize
517 bytes
15.65 KB

A patch was need to the main Panopoly repo to get the image test to work, because the PHP webserver doesn't fully support clean URLs (so we need to disable them on Travis).

Also, I started (but didn't finish) a test to actually verify that only the number of expected revisions was created. It's in the updated test patch! To finish it I need to declare a new Behat step that looks like:

Given I view revision X of fieldable panels pane "admin name"

This will allow us to actually verify that each revision created matches what we think it should.

dsnopek’s picture

Status: Needs work » Needs review
FileSize
17.24 KB

Alright! This last test patch includes tests that should cover all of the issues that were discovered during development of this functionality. I'm going to run on Travis-CI in a moment.

EDIT: Here's the Travis-CI build: https://travis-ci.org/dsnopek/panopoly/builds/50959172

EDIT-2: That build was incomplete - here's a better one: https://travis-ci.org/dsnopek/panopoly/builds/50962029

dsnopek’s picture

Hide some files, so we only see the necessary ones.

dsnopek’s picture

Status: Needs review » Needs work

Blergh! Other unrelated tests are failing on Travis-CI due to disabling Clean URLs. So, I guess we shouldn't do that! However, I'm not sure what to do to move this forward.

Here are the options:

  1. Ideally, drush runserver would be updated to allow images styles to work with Clean URLs:

    https://github.com/drush-ops/drush/issues/1189

    But I can't see an obvious fix in the drush code, and I don't know when someone more knowledgeable will be able to help out. :-/

  2. There is the issue about running our tests through Apache or nginx, which would fix it: #2183021: Should Travis-CI use Apache/nginx rather than the builtin PHP webserver?
  3. Or, we could remove or disable the one test that actually needs image styles to work correctly until we have a better solution

At the moment I'm leaning towards #3 since that would involve the least amount of work!

dsnopek’s picture

Status: Needs work » Needs review
FileSize
17.47 KB

Ok, I took the easy way out. :-) I've commented out the test that's checking for a valid image and created a follow-up issue:

#2428097: Image styles won't generate on Travis-CI (breaking tests that need valid images)

This also means the patch against the main Panopoly repo is no longer needed!

I'll run on Travis-CI again in a moment to verify that everything is working again.

EDIT: Here is the Travis-CI build: https://travis-ci.org/dsnopek/panopoly/builds/51107078

dsnopek’s picture

Status: Needs review » Needs work

Pff.. Still failing on Travis-CI because connection to MySQL server is timing out. Here's a link about increasing it:

http://stackoverflow.com/questions/1644432/mysql-server-has-gone-away-in...

dsnopek’s picture

FileSize
749 bytes

On an entirely unrelated note, I just noticed that the panopoly_core patch has the wrong version of the FPP patch. Here's an update patch! The above testing issues are still not fixed.

dsnopek’s picture

I'm attempting to come up with a fix to the MySQL time errors on this issue:

#2231631: Tests randomly fail with "MySQL server has gone away" (take 4)

Here's a Travis build attempting these tests with that patch:

https://travis-ci.org/dsnopek/panopoly/builds/52323403

dsnopek’s picture

Status: Needs work » Needs review
FileSize
17.53 KB

Ok! Here is a re-roll of the panopoly_test patch against the current -dev.

I'm going to do a test run on Travis-CI, and then if that is working, it might be time to actually merge this!

EDIT: Here's the Travis-CI build: https://travis-ci.org/panopoly/panopoly/jobs/52426341

dsnopek’s picture

Blergh! So, the tests are passing, except the build takes so long that Travis always errors out. :-/ I guess this is now dependent on:

#2437927: [META] Improve test performance on Travis-CI

dsnopek’s picture

Issue tags: +sprint
dsnopek’s picture

Just merged #2447839: Audit Behat code for slowness and optimize which should allow these tests to finish without going over the limit!

Here is a new build on Travis-CI: https://travis-ci.org/panopoly/panopoly/builds/53460114

dsnopek’s picture

FileSize
827 bytes

Rerolled patch for panopoly_core.

EDIT: Here's the Travis-CI: https://travis-ci.org/panopoly/panopoly/builds/53473718

mglaman’s picture

Status: Needs review » Reviewed & tested by the community

Test past, manually tested. Revision table only populates when saved. Thanks for running with this dsnopek! This will be a crucial step if/when #2427403: Migration path to use ECK comes to fruition.

  • dsnopek committed 383a5fa on 7.x-1.x
    Update Panopoly Magic, Core and Test for Issue #2398347 by dsnopek,...
dsnopek’s picture

Status: Reviewed & tested by the community » Fixed

Committed! Finally!

Status: Fixed » Closed (fixed)

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