Problem/Motivation
Original problem report, as amended by @mpp:
- Create a paged view with an exposed block and expose the "items per page" option to allow a user to change the amount of results.
- When applying a sort on the view and then changing the amount of items per page, the sort is lost.
Steps to reproduce:
- Installed fresh drupal(8.4.x), enabled devel, devel_generate modules
- Generated 50 contents(page, articles) from devel generate page -'/admin/config/development/generate/content'
- Created view page '/test' & select Title, Id fields
- Selected table format & enabled Title, Id as sort-able in it's table settings
- Selected Pager as full, and in it's settings checked 'Allow user to control the number of items displayed in this view' under 'EXPOSED OPTIONS' section
- Under Advanced settings, enabled 'Exposed form in block' option (Yes). And saved the view
- Next, i'v enabled this filter block from block layout page in one of the region & only for '/test' page
- Now, i visited '/test' page and clicked on title to sort it. Results came as expected & url is - http://d8.dev/test?items_per_page=5&order=title&sort=asc
- Next, from the block filter i'v changed no.of items to 25 & clicked apply. Results came only for this filter But sort queries gone in the url. url is - http://d8.dev/test?items_per_page=25
Also see this related issue which may be encountered when testing this bug: #2887144: Views exposed form block options are not updated immediately when adding additional sorts, filters, etc (Caching?).
Proposed resolution
Atm the views exposed form filters out get parameters in ViewsExposedForm::buildForm:
$form['#action'] = $view->hasUrl() ? $view->getUrl()->toStrin- g() : Url::fromRoute('<current>')->toString();
A quick workaround would be to alter the exposed block form to add current sort & order query parameters.
Remaining tasks
User interface changes
(New or changed features/functionality in the user interface, modules added or removed, changes to URL paths, changes to user interface text.)
API changes
(API changes/additions that would affect module, install profile, and theme developers, including examples of before/after code if appropriate.)
Data model changes
(Database or configuration data changes that would make stored data on an existing site incompatible with the site's updated codebase, including changes to hook_schema(), configuration schema or keys, or the expected format of stored data, etc.)
Original report by [username]
(Text of the original report, for legacy issues whose initial post was not the issue summary. Use rarely.)
| Comment | File | Size | Author |
|---|---|---|---|
| #99 | 2823541-99-rerolled-on-11.2.4.patch | 28.66 KB | rsych |
| #92 | 2823541-rerolled-on-10.4.x.patch | 36.5 KB | afsch |
| #88 | 2823541-rerolled-on-10.3.x.patch | 36.49 KB | lovejit_singh |
| #86 | 2823541-rerolled-on-10.2.x.patch | 36.39 KB | edmoreta |
| #81 | 2823541-81.patch | 35.25 KB | berdir |
Issue fork drupal-2823541
Show commands
Start within a Git clone of the project using the version control instructions.
Or, if you do not have SSH keys set up on git.drupalcode.org:
Comments
Comment #2
strykaizerHavent tested yet, but this sounds like views exposed filters stripping out existing get parameters.
If this is the case, its a views exposed filter bug.
Comment #3
mpp commentedI agree with StrykKaizer,
$form['#action'] = $view->hasUrl() ? $view->getUrl()->toString() : Url::fromRoute('')->toString();ViewsExposedForm::buildFormremoves existing get parameters:Comment #4
mpp commentedComment #5
mpp commentedComment #6
dawehnerIdeally we would fix it in 8.3.x/8.2.x
Comment #7
dawehnerMaybe we should have a special route which also contains the query parameters.
Comment #9
Anonymous (not verified) commentedSteps taken to confirm that this is still an issue:
Based on testing with this configuration, I was not able to replicate this issue. See video:
https://www.dropbox.com/s/xfdl27too26xvob/screencast_2017-02-19_14-37-09...
Issues observed (these are not related the reported issue, but I'm documenting here in case they may be related and/or helpful):
Summary: At this point, this bug is not confirmed. However, the exposed filter in a block doesn't appear to be working 100% correctly.
Comment #10
krknth commentedI'm able to reproduce this, here the steps i'v followed
Comment #11
samirjusic commentedI'm working with @xjm on this in the Drupal Montreal Camp sprint and I'm trying to follow the steps from @krknth in #10 and @anneeasterling #9 so I could continue the work done so far.
Comment #12
samirjusic commented@xjm and I went through the @anneeasterling #9 steps and were able to see the same behaviour @anneeasterling reported. In addition, and to rule out that as a potential source of problem, we added another option to the Sort By drop down (authored by field) and confirmed it worked as expected. I'd agree that we were not able to reproduce the originally reported issue.
@anneeasterling also mentioned:
We verified that there indeed exist an issue with the exposed filters block and how it is updated every time the list of fields to be shown is changed. Whenever a new element is added the exposed form block will take a few refreshes before it actually shows the new option in the dropdown (or whether the filtering option is shown/hidden altogether). It does not seem to be about whether there are parameters in the URL or not. For example
(you can do the above if you are adding or removing an existing exposed filter option - perhaps Items Per Page item - and it will also take a few refreshes on the page before it kicks-in with the changes)
This might be a separate issue relating to cache invalidation of the exposed block. I'll try to see whether there is an existing issue reported.
I'm attaching the view that I've used to follow the steps and get to this point.
Comment #13
samirjusic commented@xjm and I were able to follow the steps outlined in @krknth's comment #10 and saw the behaviour as what @krknth wrote there.
As per @xjm, there is most likely an existing issues somewhere in the views/contrib views issue queues that complains about this problem and has been there for some time now. I'll try and find something and report back here.
Comment #14
stpaultim commentedComment #15
stpaultim commentedComment #16
mathuaerknedam commentedComment #17
stpaultim commentedComment #18
stpaultim commentedAfter multiple attempts to recreate this problem without success, we're closing this issue.
One new issues was created as a result of the work in this issue. #2823541: Table clicksort is lost when using views exposed filter & Pager exposed '#items'
Comment #19
strykaizer@stpaultim can you share the new issue? You seem to have linked to this issue instead
Comment #20
xjm@stpaultim, can you clarify how you were not able to reproduce it? We clearly reproduced it as outlined in #13, less than a week before your comment. It'd be good to post a more detailed comment explaining what you were unable to reproduce before unilaterally closing the entire issue. An exported view that reproduces it is even provided.
Comment #21
xjm"Closed (cannot reproduce)" is really extreme; we should give people a chance to respond before changing to that status. #10 is reproducible. It might be a feature request to make table click sorting work with the exposed filter in a block, but until we locate that issue, we should not close this one, and if we do locate that issue, it should be closed as a duplicate.
Comment #22
xjmThe summary even has a proposed resolution so we definitely should not close this issue without looking into that.
Comment #23
xjmYeah, #10 is definitely the bug described. Adding those STR to the summary and clarifying the title.
I don't think this is a major bug and I still think there is probably a duplicate somewhere. However, we should find such duplicates and confirm they are up to date before closing this issue. This is different from the cache invalidation bug described in the related issue. (This one is probably just a normal.)
Comment #24
xjmComment #25
xjm(Adding issue credit for major triage participation. Thanks!)
Comment #27
stpaultim commentedSorry, I working with a small group and we must have missed something. Personally, I understood that two other people had tried but failed to recreate the original problem.
Comment #9 "At this point, this bug is not confirmed"
Comment #12 "I'd agree that we were not able to reproduce the originally reported issue."
It was my understanding that both comment #9 and comment #12 did see a NEW issue - which was reported here: #2887144: Views exposed form block options are not updated immediately when adding additional sorts, filters, etc (Caching?) (unfortunately, I linked to the wrong issue) and must have misunderstood the connect between the two issues.
Based upon our understanding that 2 other people had failed to recreate the original issue, based upon our own failed attempts to recreate the original issue - using the steps outlined in previous comments, the fact that a new issue had been opened, and after consulting with sprint leaders - we closed the issue.
I should have been more descriptive, but it was the end of a long day and a long camp. :-( Will do better next time.
Thanks everyone for their hard work on this and all the other issues.....
Comment #28
j2r commentedThis issue still exist.
Create simple node listing view with table, make all fields sortable, add 1/2 expose filter. Now sort on node title after that use expose filter, with response the sorting on node title will be reset to default.
Please let me know if you need more detail to reproduce this issue.
Comment #29
pwaterz commentedThis issue is reproducible. If you setup a solr view with facets. Expose items per page. You will see that the active facets that are in the url are ignored.
Comment #31
samuel.mortensonI ran into something similar in #2962525: Create a field widget for the Media library module, and added some Javascript to ensure that the original query parameters are always added to the exposed filter request. The IS doesn't mention AJAX, so I'm going to try to see about a PHP fix as well.
Comment #33
samuel.mortensonUnfortunately I think the bug I was fixing in #31 was specific to AJAX and unrelated to the issue you're having. :-(
I did replicate the bug from the issue summary, but was not able to determine the best path forward to fix this problem. Since the exposed form method is GET, and a form's action cannot contain query parameters, you would need JS or PHP to add hidden input elements for every query string key. Even if you implemented this, it would not help if the view described in the IS used AJAX. I'm not sure how to address the problem in the most complete manner.
One idea I had was to add order, sort, and page as hidden input elements to the exposed form. That should fix non-AJAX forms, but for AJAX you would need to refresh the exposed form block as well when the table sort changes. This is a very tricky issue!
Comment #34
marcoscanoI was having some errors due to
settings.view_querynot being set in some circumstances, so I'm uploading a small check for that.Maybe we could open a separate issue for the patch in #31? I do believe it solves part of the issue, for example when you want your exposed filter's AJAX calls to persist the query params.
Comment #36
idebr commentedComment #38
idebr commentedReroll against 8.7.x
Comment #41
mbovan commentedReroll against
8.8.x.By following the steps described in the issue summary and in #10 I can reproduce the problem with views that use ajax as well as with non-ajax views.
However, the patch from #31 does not fix views that have ajax enabled (as per steps from #10). To proof that we would need to write a failing test based on #12.
Additionally, I noticed that the current patch always appends
_wrapper_format=drupal_ajaxon next ajax requests. So the query string becomes_wrapper_format=drupal_ajax&_wrapper_format=drupal_ajaxin the second ajax request,_wrapper_format=drupal_ajax&_wrapper_format=drupal_ajax&_wrapper_format=drupal_ajaxin the third one and so on.Comment #44
berdirAt some point the core implementation for view_path changed and this didn't catch up with that change, the initial patch used something else.
This is a problem because on ajax requests, getPathInfo() then returns /views/ajax, while path.current is correct as views is setting that.
Same for the query arguments, the ViewAjaxController removes a bunch of them, but not from the query string, so instead of using that as-is, we have to build it again, and we also have to remove the new view_query parameter.
Also, there was a mixup with leading ?, because window.location.search did start with a ? but our setting didn't, so I changed the logic to only slice off the first character from window.location.search.
This is now passing the existing test which ensures that no extra stuff is being added as shown in #41, still need to extend tests to cover the problem this fixes.
Comment #45
berdirReroll for 9.0.x, strangely on ajax_view.js conflicted, but not ajax_view.es6.js. At least now I know how to build those things, hopefully did that correctly.
Comment #46
xjmSince we should fix this bug in D8 too, I'm filing it against 8.8.x (which is the current bugfix support branch). Patches can be tested against other branches as needed when they're uploaded.
The issue will be automatically updated to 8.9.x after the last 8.8.x bugfix release. Thanks!
Comment #47
lendudeMoving to needs work for the additional test coverage
Comment #48
oknateAdding test coverage. No interdiff is needed as this is the same as #45 except for test coverage.
The test coverage is essentially testing this assertion:
With the bug, when clicking filter, the sort goes away. When the bug is fixed, the sort persists.
Comment #49
lendude@oknate thanks for looking at this! some nits to pick (besides the test failing :)
First: do we really need the massive new view? Isn't there something simpler in core already we can modify within the test to match our needs? (If nothing useful exists can we at least strip this down to the essentials for this test?)
waitForElement doesn't check if the element exists after waiting, so we should check for that
It's not obvious why it shouldn't be showing after sorting
Comment #50
oknate1. I'll look through the existing views in core to see if there's one that allows us to do what we need, or I'll pare down the new view.
2. Really, I just need a way of testing that the view updated. Perhaps I can just check when the form id updates. I remember other tests do that. Then I wouldn't need to create new nodes, I think.
3. I don't know why the test failed, it passed locally with Drupal 9 rc1. So I'm sure it's a small thing.
Comment #51
oknateResponding to feedback in #49.
1. I reduced the size of the new view. It still may be possible to find an existing view or test view to demonstrate the issue. Also, there's probably more settings that can be removed from this view.
2. I reworked the test so it doesn't need new nodes. It can test for refresh by testing the number of items shown.
3. I don't know how I saw it working last night locally, as I'm not seeing it working this morning. As far as I see this morning, #45 doesn't fix the issue in the issue summary. I think the test I created demonstrates the bug, but with #45 applied, it still happens. The sort goes away when changing the filters, when the exposed form is in a separate block. This was testing on Drupal 9 rc1.
Next patch around this comment should be removed:
// Wait for the dialog to close.That's a cut and paste error, and isn't relevant.
Comment #52
oknateUpdated test case. I'm leaving the bug fix code out for now, as it doesn't work for this test. See #51.3.
Comment #53
oknateHere's a video of the bug, as demonstrated by placing a sleep statement in the functional js test in #52.
Comment #54
rishabhthakur commentedComment #55
hardik_patel_12 commentedRe-rolled for 8.9.x-dev.
Comment #57
berdirNote: The current patch does _not_ fix the original problem described in the issue summary, it only fixes exposed filters and clicksort. Handling the block settings is fixed by #2605218: Views Block Display skips preBlockBuild() call on ajax rebuild which in turn depends on the view_query stuff introduced here.
We should possibly merge the two issues together and close one as duplicate. I would assume that the other issue will pass with the test added here. Thoughts on that?
@Primsi has been working a bit on this issue as well, but possibly without knowledge of the other one. We'll post something here soon. Don't work on it for now.
Comment #58
primsi commentedJust quickly trying with the combination of the patch from @oknate + 9.1.x version from #2605218: Views Block Display skips preBlockBuild() call on ajax rebuild. Both are re-rolls for 9.1.x.
Comment #60
johnpitcairn commentedShouldn't this be 9.3.x now?
Comment #61
johnpitcairn commentedFor anyone trying to apply this for current Drupal core (9.2.x) - the patch at #58 applies, but does not fix the issue as described by the issue title.
For a page view that is not using ajax, click-sorting a table column header then applying an exposed filter loses the original click-sort and reverts to the default column-sort. The sort parameters are removed from the URL, with or without the patch.
For a page view that is using ajax, the patch at #58 does appear to fix the issue.
The issue summary makes no mention of ajax, and I'm not sure why that has been the focus. The title alone describes the base issue pretty well. Any fix here should also fix the issue for non-ajax views.
Comment #62
tim_djI did a reroll for 9.2.x because patch #58 no longer applies to 9.2.7 a reroll for 9.3.x is less straight forward because of the changes to modules/views/src/Plugin/views/display/Block.php
Comment #65
megha_kundar commentedComment #66
suresh prabhu parkala commentedTried to fix custom failures of #65.
Comment #67
xjmComment #68
berdirReroll for 9.5. Didn't figure out how to format that conditional line, converted to a regular if.
Comment #69
berdirForgot to build the .js file.
Comment #72
berdirReroll for D10. Did not yet look into the test fails.
Comment #74
berdirRerolled the patch for D9.5, the patch for D10 is above.
Comment #76
berdirAnother reroll for 9.5.0-rc2.
Comment #77
needs-review-queue-bot commentedThe Needs Review Queue Bot tested this issue. It either no longer applies to Drupal core, or fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".
Apart from a re-roll or rebase, this issue may need more work to address feedback in the issue or MR comments. To progress an issue, incorporate this feedback as part of the process of updating the issue. This helps other contributors to know what is outstanding.
Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.
Comment #78
berdirReroll for 10.1
Comment #80
berdirAnother D9.5 patch update.
Comment #81
berdirAnother reroll for 10.1
Comment #85
berdirDid a reroll as a merge request for 10.2.3+, still test fails to figure out.
Comment #86
edmoreta commentedre-posting MR patch
Comment #87
jennypanighetti commentedStill not working for me, unfortunately.
New view > fields of some nodes > Table display with all columns sortable > some exposed filters
Go to page, choose a sort.
Enter a search term
->
table is re-sorted by the default sort
Comment #88
lovejit_singh commentedComment #90
samitk commentedHI,
I have fixed other issue reported by GitLab pipeline, but getting some errors with
PHPUnit Functional JavascriptandNightWatch, Post reviewing the errors, they looks like related to core.https://git.drupalcode.org/issue/drupal-2823541/-/jobs/2599151
https://git.drupalcode.org/issue/drupal-2823541/-/jobs/2599078
Thanks
Samit K.
Comment #91
johnvComment #92
afschRe-rolled for D10.4
Comment #93
colanIs this also supposed to fix the issue where the column click-sorting only sorts the visible rows? That is, if I have 10 in the exposed filter pager, and re-sorting should include a row on page 2, it doesn't. Patching from here doesn't seem to help.
Is this a separate issue, or was it designed to work that way (only sort the visible rows)?
Comment #94
berdirNote: We've seen some issues with reset that persists on following requests and resets the filters again and again.
Comment #95
rsych commentedRe-roll for Drupal Core 11.2.4
Comment #96
oily commentedThere is a failing test that looks related. Here is the output:
One of the assertions that is failing here is at lines 115- 116 in PaginationAJAXTest.php:
Comment #97
oily commentedHere is the test-only output:
Comment #98
oily commentedIt seems that the update in core/modules/views/tests/fixtures/update/views.view.test_table_css_class.yml is triggering the deprecation at lines 293 to 296 of core/modules/views/src/ViewsConfigUpdater.php.
@berdir Should/ can views.view.test_table_css_class.yml and core/modules/views/tests/fixtures/update/test_table_css_class.php be removed? Should that be done in a follow up?
Comment #99
rsych commented