If a view uses ajax, and user can able to change Items per page value + pager exits, the pager won't work as well, if 'items per page' has different value, than the original view's pager option.
To reproduce: Create a view pane, Set pager to 'Paged output, full pager', and set 'Items to display' on views pager settings form to 10. On pane configuration let the 'Allow settings' check 'Items per page'.
Then add a new pane, and set 'Item per page' on pane input form to 1.
On page load, the 1 item will show up with pager. Then use pager, the result will be 10 item, as in the original view was set up.
This is, because the ajax callback of views just load the default view settings, and give back $this->preview() as result. No chance to add the pane's own parameters.
I'm not sure how do we need to handle this.. I tried to extend the preview() in views_content_plugin_display_panel_pane.inc, but I didn't get any relevant information from exisiting variables. The one of my idea to pass number of items by url, but I'm not sure, this is the best way (I think, we could have other pane settings, which would be lost)
Comment | File | Size | Author |
---|---|---|---|
#84 | views_content-keyword-substitution-1910608-84.patch | 12.05 KB | dsnopek |
| |||
#79 | 1910608-ctools-ajax-settings-lost-79.patch | 17.39 KB | Jorrit |
| |||
#56 | views_content-keyword-substitution-1910608-56.patch | 12.07 KB | mpotter |
#51 | views_content-ajax-1910608-51.patch | 767 bytes | iSoLate |
#48 | ctools_cache_table_fix_1910608_48.patch | 665 bytes | rklawson |
Comments
Comment #1
merlinofchaos CreditAttribution: merlinofchaos commentedThis is a known, unfixable issue, and there is warning text in the pane settings that addresses this.
Well, okay, I just went and looked; that warning text actually doesn't appear due to D6 -> D7 upgrade changes that got missed. Go figure. I'll have to fix that.
Comment #2
vflirt CreditAttribution: vflirt commentedHi,
all i can say is that none of the allowed settings (pager element, offset, items per page, etc.) actually work with ajax. I see that in views module : includes/ajax.inc that is a line :
$view->display[$display_id]->handler->set_option('pager_element', $pager_element);
that should set he pager element but it has 0 influence on the result.
The easiest step to reproduce :
create test page , create test view with content , add content page display , set it to ajax , allow page settings to be set in the panel and set the pager element to be 0 and items per page to be 2 (for the test) , attach the view pane in the panel and set page element to be 1 and items per page to be 1. Add 5 nodes so we have some content.
Now when you open the page /test (or whatever is the url) that we will see 1 node with pager as expected and the linkst are correct page=0,1 or page=0,2 etc.
But when you click any of the pager it just reloads 2 nodes and loads the first page not the one you selected and when you check the pager links they are page=1,1 or page = 2,1 etc. So the pager handler has 0 clue of the settings that were posted trough ajax.
I will try to dig more into it this days and hope to find solution as I need to for one of my projects but I can't promise how far I will go and if the client won't just accept to reload the page as then the listings are working correctly.
Kind Regards
Comment #3
vflirt CreditAttribution: vflirt commentedHi,
a few more lines i can add to this but all are connected with view module.
Instead of :
you should use the pager array options :
and then setup the ones posted from ajax :
then we will have to add :
a bit above so we actually have this setting.
and in theme/theme.inc in the template_preprocess_views_view function in the ajax settings we will have to add :
as the pager_element is added.
Probably adding a function to the plugin display base class might look cleaner approach in order to handle this situation but that should also require to use array for pager settings in the ajax so other modules could add settings that would need but i guess that would require too much change in the views module.
I think that this could be related more to the views module to provide either option so other modules could add their own javascript settings and also to process them but again that is too much to ask. I am not that good to write such patch for the views module to provide such extendibility but the above changes does work quite well for me so i will commit a patch soon.
Kind Regards
Comment #4
hefox CreditAttribution: hefox commentedThis is more a proof of concept but can defiantly be rolled into a true patch if think it's worth it.
Patch applies after #1901106: Allow querystring to (optionally) override empty exposed filters. as too lazy to make two patches for a POC type patch
So what patch does
1) Moves the 'changing of the view' into the display handler in a method pane_process_conf
2) in set_pane_conf, set the pane config both to the object and to the cache (via cache_set( with a CID that includes the dom id to cache table, @todo move that to own cache table, perhaps one done similar to cache_form that uses expires to last through cron)
3) Add a hook_views_pre_view that triggers pane_process_conf
4) in pane_process_conf if no pane config, look for it in cache and use/set that if exists.
5) Adds a views_content_views_pre_view that triggers the function, and uses module implements to execute it first so anyone else currently doing views_pre_view can use the config (had stuff break when trying to do it in ..pre_execute?)
Use of cache is because
1) didn't want to try and figure out where that view was coming from and loading that
2) didn't want to pass around the settings in a way a user could manipulate
Comment #5
beeradb CreditAttribution: beeradb commentedThe previous patch failed to work with clickable table sorting. Pane configuration would trump everything else. This updated patch checks for clicksorting before applying the pane sort configuration.
Comment #6
beeradb CreditAttribution: beeradb commentedOops, looks like the previous version included the first patch. Here's a retry.
Comment #7
beeradb CreditAttribution: beeradb commentedIt looks like this patch was also based on #2016559: Non-numeric pager_id causes a loop that just won't stop, here's a re-roll based on that as well.
So to get this patch working:
Apply #1901106: Allow querystring to (optionally) override empty exposed filters.
Apply #2016559: Non-numeric pager_id causes a loop that just won't stop
Apply this patch.
Comment #8
SocialNicheGuru CreditAttribution: SocialNicheGuru commentedpatch does not apply cleanly
I did follow the instructions and applied the two others before the one attached in 7
july 5 ctools
patch -p1 < 1910608-views_content-ajax-7.patch
patching file views_content/plugins/content_types/views_panes.inc
Hunk #2 FAILED at 163.
1 out of 2 hunks FAILED -- saving rejects to file views_content/plugins/content_types/views_panes.inc.rej
patching file views_content/plugins/views/views_content_plugin_display_panel_pane.inc
Hunk #2 succeeded at 472 (offset 1 line).
patching file views_content/views_content.module
Hunk #1 succeeded at 333 (offset 13 lines).
Comment #9
boyan.borisov CreditAttribution: boyan.borisov commentedHi guys,
I had the same problem but I found a view's related solution. See the link bellow
https://drupal.org/node/2065975#comment-7757969
Comment #10
vflirt CreditAttribution: vflirt commentedHi,
as pointed the patch doesn't apply clean.
I applied it manually and it is working great and fixes number of other issues with views in panels and ajax.
There is one minor issue though. If you set the content pane to allow path overwrite and then enter path in panel config it produces call to unidentified function ctools_context_keyword_substitute on line 544 of ctools/views_content/plugins/views/views_content_plugin_display_pane.inc
the code that does it is :
all i did was to add
ctools_include('context');
before that call of that function and it was working like a charm.I think what this patch does is great and should be used.
Kind Regards,
Dobromir
Comment #11
mpotter CreditAttribution: mpotter commentedOK, let's take a cleaner approach here. This patch is too intertwined with #1901106: Allow querystring to (optionally) override empty exposed filters.. That patch is adding additional functionality. The patch in this issue really addresses the fundamental issue of ajax losing options from the pane config. So it's really this issue that should be committed first (and soon) and then I will go re-roll 1901106 so it applies *after* this patch.
Also, while #9 addresses the specific pager settings in views, it doesn't address the more fundamental problem here in ctools related to other config options being lost by Ajax.
Here is a re-roll of this patch for the current 1.4 version of ctools. It has no other patch dependencies.
While the diff is ugly, what this patch does is relatively simple. It is taking code *out* of the views_content_views_panes_content_type_render() function in views_content/plugins/content_types/views_panes.inc and moving it *into* a new pane_process_conf() function in the handler views_content/plugins/views/views_content_plugin_display_panel_pane.inc.
By moving this code into the new pane_process_conf() function of the handler, it can then be called from both the original views_content_views_panes_content_type_render() function as well as a new views_content_views_pre_view() hook which preserves all of the pane config when using ajax.
Hopefully some others can review this. Don't be scared away by the size of the diff. I believe this puts the common code into the proper location and handles the ajax more generally for all config options.
We really need to get this patch committed asap so other functionality patches like 1901106 can be reviewed. Because any other patch that tries to add pane config options conflicts with this patch since this patch moves the code to a different file.
Comment #12
mpotter CreditAttribution: mpotter commentedAck, patch in #11 was missing a line. Here is the fixed patch.
Comment #13
mpotter CreditAttribution: mpotter commentedOK, 3rd time is a charm! The patch in #12 contained some contamination from the #1901106: Allow querystring to (optionally) override empty exposed filters. issue. This patch should be clean. Crossing fingers.
Comment #14
mansspams CreditAttribution: mansspams commented13: 1910608-views_content-ajax-13.patch queued for re-testing.
Comment #15
pokurek CreditAttribution: pokurek commentedHi,
there is a small bug in #13 around line 557.
$view->set_exposed_input() has to be $this->view->set_exposed_input().
Regards,
Petr
Comment #16
vordude CreditAttribution: vordude commentedThis works for me.
My tests were specifically focusing on using panel pane configuration to override 'items per page' returned in a view pane with an ajax pager.
#13 still applies cleanly, I've rerolled with the addition from #15.
Comment #17
ParisLiakos CreditAttribution: ParisLiakos commented#16 works great for me..i tested it with both offset and items per page
Comment #22
ParisLiakos CreditAttribution: ParisLiakos commented#10 is still an issue though
Comment #23
mkhamash CreditAttribution: mkhamash commentedRe roll for patch #16 against ctools-1.6 the current dev. this will need another review since views_content_views_panes_content_type_render have been edited since the last time this patch have been updated.
Comment #24
mkhamash CreditAttribution: mkhamash commentedComment #25
redndahead CreditAttribution: redndahead commentedComment #26
gmercer CreditAttribution: gmercer commentedWe found that the previous patch #24 wasn't working when we pass view context filter/arguments to a content pane on a page.
In the new patch we use the pane_contexts passed in to get the view filter/arguments for the pane.
Comment #27
gmercer CreditAttribution: gmercer commentedAh... we found a small issue in my previous patch. We weren't getting the panel args correctly in all cases. Using $panel_args turns out to be a better (yeah) approach. So here's another try.
Comment #28
redndahead CreditAttribution: redndahead commentedForgot to get the panel args variable from the conf variable.
Comment #29
mkhamash CreditAttribution: mkhamash commentedRe-roll Patch #28 against 7.x-1.7 to apply, also added the the edit introduced in issue [2416589] to views_content_views_panes_content_type_render() to the new views_content_plugin_display_panel_pane extends views_plugin_display() function.
Comment #32
hefox CreditAttribution: hefox commentedHaha, I'm glad I saw this when I did and not a minute too soon, cause was about to reroll it, and bam, the patch was posted less then a minute before! Thanks, applies clean (haven't tested past that)
Comment #33
eyilmazWith the latest patch, the keyword substitution doesn't work.
Removed code from views_content_views_panes_content_type_render() :
added instead:
should be:
Refreshed patch as attachment.
Comment #34
joelstein CreditAttribution: joelstein commentedThe patch at #33 worked for me. Thanks!
Comment #35
mdeltito CreditAttribution: mdeltito at Phase2 commentedCame here to report the same issue as described in #33. Thanks for the updated patch @eyilmaz
Comment #36
lucas.constantino CreditAttribution: lucas.constantino at Taller commentedHello,
I've had an issue with the ctools_context_keyword_substitute when the pane configs get cached. The problem is the function is, sometimes, not defined in the time of calling. The solution would be to simply invoke "ctools_include('context')" before the function usage. Anyway, I did not understand if this include was not in code by purpose or if it was simply forgotten. Most times your page would have already loaded CTools contexts anyway, so on most situations the issue would not occur.
Here goes patch #33 with the addition of ctools_include.
Comment #37
IRuslan CreditAttribution: IRuslan as a volunteer and at DrupalJedi commented#36 works for me.
+1 RTBC
Comment #38
mkhamash CreditAttribution: mkhamash as a volunteer commentedAlso RTBC #36 working perfectly fine for some time now.
Comment #39
rivimeyPatch applies cleanly to current 7.x-1.x
PHP code snifer is complaining about the class views_content_plugin_display_panel_pane because access modifiers are not defined for functions or (some) variables. However, this appears to be the normal thing for such plugins.
I have set up the scenario indicated (requires panels, page manager, devel-generate) and the patched code works correctly, in that the pager is now using 1 item per page.
Good to go, I think.
Comment #40
rivimeyComment #42
japerryUsually something this big wouldn't be committed, but after reading #11, the code, and talking to rivimey about testing, I feel somewhat okay about committing this. Fixed!
Comment #44
bobodrone CreditAttribution: bobodrone commentedAfter this change I experience a huge "cache" table (>5gb) in just the last 10 days. Since yesterday when I clear the cache I have a "cache" table of:
19647 rows in total
19600 of those are beginning with: "view_panel_pane_"[HASH]
Table size in the last 12 hours: 1GB (and growing every minute)
In the commited patch you can find:
I have no clue myself what´s going on there.
Anyone knows what the problem can be?
// Fredric
Comment #45
rivimeyHi @bobodrone, thanks for your report.
I cannot explain the issues you are having, but the lines you highlighted are creating and caching the saved configuration for a pane (I think mostly for use by Views AJAX calls). The cache_set call accepts up to 4 args; the first two (present here) are the key name and the data to save. The last two are the cache name and the timeout, which defaults to permanent.
On looking at this code I am somewhat confused as to why the caching is happening at all, but it would seem reasonable to set the timeout to some form of 'temporary', else the cached entries will not be removed when a general clear is done.
This change could at least assist with cache cleaning:
to
Which means it can (not will) now be cleaned up by a general cache-clear after an hour's life.
Overall I think some more investigation is needed as to why there are so many cache_set operations being performed in the first place. Perhaps some judicious logging code in the set_pane_conf() function would do that?
Comment #46
KarlSheaThis patch breaks any changes to views arguments set when making an AJAX call.
Comment #47
KarlSheaIf I change line 492 to
My AJAX call stops failing. Does that break the intentions of this patch?
Comment #48
rklawson CreditAttribution: rklawson commentedI've also experienced the cache table growing excessively large as a result of this patch. Since the patch here has already been released, I rolled a light-weight patch to change the default CACHE_PERMANENT of cache_set() to CACHE_TEMPORARY. This will probably end up needing a separate issue.
I agree with @rivimey and also do not understand why caching has been implemented in this way and think that the implementation here should be re-examined because of its negative impact on performance (and disk space).
Comment #49
geek-merlin#440: added a new issue for that: #2828620: Regression: View pane rendering misuses and floods cache
Comment #50
jastraat CreditAttribution: jastraat commentedComment #51
iSoLate CreditAttribution: iSoLate at Randstad Digital for Government of Flanders commentedIt seems it is setting the cache one sided not taking into account the cache settings on a views pane. In my case, this is breaking my glossary pager.
Quickly added a small patch.
Comment #52
japerryRe-opening because I had to revert it.
Comment #53
mpotter CreditAttribution: mpotter at Phase2 commentedCommit was reverted on 2017-01-19. Adding the 1.13 release as a parent. This needs to get resolved since Open Atrium sites are using this functionality and removing it from the next release will break sites.
Comment #54
Alan D. CreditAttribution: Alan D. commentedPre-revert, it was potentially killing sites on smaller hosting plans, it's appeared to have sucked up 350 MB within a week for one of our clients.
I would assume a direct revert, but I didn't check. So to get the committed patch for the revert:
http://cgit.drupalcode.org/ctools/commit/?id=b4d7d11
Comment #55
DamienMcKennaThis didn't get into 1.13, maybe it will get into 1.14.
We need to try getting this to RTBC.
Comment #56
mpotter CreditAttribution: mpotter at Phase2 commentedHere is a reroll of #36 that applies to 1.14. Doesn't address any of the caching issues that were raised, so those still need work.
Need to decide if the small fix from #51 should also be added.
Comment #57
mpotter CreditAttribution: mpotter at Phase2 commentedOK, after reviewing all the related issues, going to leave #56 as is and we should address the cache fix in #2843333: Fix the Ajax settings fix's broken cache usage. Then I'll go update the related #1901106: Allow querystring to (optionally) override empty exposed filters..
This is getting awfully complicated, but we really need to fix these Ajax issues as there are thousands of Atrium sites that need this (without breaking thousands of more sites with large cache issues).
Anybody else out there have some time to help get all these issues to RTBC?
Comment #59
joelstein CreditAttribution: joelstein commentedThank you for the patch in #56! It applies cleanly and fixes the missing pane settings on ajax. Also, the caching issue seems to be resolved with the patch in #2843333-9: Fix the Ajax settings fix's broken cache usage.
Comment #60
joseph.olstad+1 yes we are depending on this patch too.
we have an exposed form in our view on a panel page, with an apply and reset button
the reset button without this patch redirects to the home page rather than resetting the form
with the patch, the reset button works correctly.
Comment #61
Jorrit CreditAttribution: Jorrit at nCode for DOM Digital Online Media GmbH commentedPatch #56 is causing problems for me, which appeared after I updated to OpenAtrium 2.6.29. The symptom is that the preview AJAX callback on the the views edit page gives the following error:
This is caused by the following line of code:
if ($set_cache || $conf != $this->get_cached_conf()) {
It happens because
!=
compares objects with recursive properties. The only recursive objects are in the'pane_contexts'
key of the$conf
variable. These objects are quite large and I think it would be beneficial also for performance when those large objects would not be serialized at all in the Drupal cache.For me, this can be reproduced on OpenAtrium on the `admin/structure/views/view/oa_fullcalendar/edit/oa_calendar_month` page when it is visited after (in the same session, by the same user) a calendar with the output of this view is displayed.
Comment #62
surendramohan CreditAttribution: surendramohan as a volunteer commentedI 100% agree with you Jorrit, and as far as Open Atrium is concerned, it's not limited to an edit page of any view.
We should be able to even reproduce it on each and every page that uses AJAX; especially the "Load more" button (which I tested and am able to reproduce each time I try to use this feature; regardless of the page).
And as correctly said, this issue popped up after the latest OA upgrade, that is, openatrium-7.x-2.629) .
Comment #63
surendramohan CreditAttribution: surendramohan as a volunteer commentedI applied patch #56 and it works great at my end. Thanks Mr. Potter for this patch.
In case of Open Atrium 7.x-2.629, just the below mentioned part of the patch does the job; other code lines from the patch are already applied to the module:
File name:
/views_content/plugins/views/views_content_plugin_display_panel_pane.inc
Comment #64
joseph.olstadI just triggered a php 7.2 test on patch 56
7.2 is now supported by Drupal 7.x core (dev branch) , the next tagged release of 7.x core (7.60) will support php 7.2 so we are best to start testing contrib now. if there's any 7.2 issues we should open a new issue and put a reference to it here.
Comment #65
joseph.olstadphp 7.2 passes
Comment #66
DamienMcKennaComment #67
debain CreditAttribution: debain commentedFor openatrium-7.x-2.631 (fresh install > ajax error) the function should look like this:
Comment #68
bobbygryzyngerLike @debain, in OpenAtrium
7.x-2.631
I am finding that the patch from #56 doesn't seem to have been completely applied to the distribution. The if statement fromset_pane_conf
looks like this in the distributed code:Edit: it appears this patch is getting overridden by another applied to
views_content_plugin_display_panel_pane.inc
in OpenAtrium. See #3000618: Patches to CTools reverse one another.Comment #69
Jorrit CreditAttribution: Jorrit at nCode for DOM Digital Online Media GmbH commentedI don't think the entire $conf needs to be changed, but just (some of) the results of
pane_process_conf()
. For instance, a lot of the contents of $conf is needed to determine the $args of the view. I think it is much easier to just cache these $args and not the $conf that was used to create them.Comment #70
Jorrit CreditAttribution: Jorrit at nCode for DOM Digital Online Media GmbH commentedIn #3000618: Patches to CTools reverse one another I have posted a comprehensive patch that fixes this issue while keeping the cache size small and not causing any errors. The patch also includes #1000146: Edit text of "More Link" from Panel config, #1858922: views_content_views_panes_content_type_render() should not set $view->get_total_rows and #1901106: Allow querystring to (optionally) override empty exposed filters.. I am hoping to get some feedback, if anyone is still interested in this problem.
Comment #71
joelpittetComment #72
Jorrit CreditAttribution: Jorrit at nCode for DOM Digital Online Media GmbH commentedI want to separate the AJAX settings fix from #3000618 so it can be considered to be merged on its own in a future ctools release.
Comment #73
Jorrit CreditAttribution: Jorrit at nCode for DOM Digital Online Media GmbH commentedI propose the attached patch. It reduces the size of the cached configuration by first calculating the changes to the views settings using the panel config and contexts, caching that, and reading from that cache on AJAX callbacks. In this way the context objects don't need to be serialized/deserialized.
Comment #74
Jorrit CreditAttribution: Jorrit at nCode for DOM Digital Online Media GmbH commentedThis patch fixed a notice, thanks andyuq.
Comment #75
andyuq CreditAttribution: andyuq commentedThat was quick Jorrit. It works great now, thanks a lot for your contribution.
Comment #76
Jorrit CreditAttribution: Jorrit at nCode for DOM Digital Online Media GmbH commentedMy patch broke exposed forms in block. The attached patch fixes this.
Comment #77
ChaseOnTheWebNeeds reroll against current -dev
Comment #78
joelpittetComment #79
Jorrit CreditAttribution: Jorrit at nCode for DOM Digital Online Media GmbH commentedRerolled
Comment #80
joelpittetThis patch is quite tricky to review, includes schema changes, lots of deletes, it's hard to know if it will cause regressions... any way this could get a couple tests and/or simplify the fix?
Also there are some minor nitpicks around coding standards (at a glance extra whitespace on the end of lines)
Comment #81
Jorrit CreditAttribution: Jorrit at nCode for DOM Digital Online Media GmbH commentedTo be honest, I don't expect that this patch will ever be committed. If it were important to anyone, many more people would have found this issue and commented on it. It is a very complicated change in complicated code. The code worked fine with the project I needed it for, but I am no longer involved in it and I can't contribute time to add tests.
Comment #82
joelpittet@Jorrit thanks for the update, I'll take this off my hitlist and wait for the other followers of this issue to chime in.
Comment #83
frodri CreditAttribution: frodri commentedPatch #79 breaks our view pre-render hooks, so I'm rerolling #56 to account for recent changes on ctools.
Comment #84
dsnopekHere's a re-roll of patch #83, but where the cache entries expire after 1 hour, in order to work around the cache ballooning issue.