Views UI preview does not retain Contextual filters on pagination

What happened when you have done the same in Drupal 7?

added steps to reproduce

SO this is clearly a bug.

formatting

Raising to major, since #2237001: Remove no longer needed _current_path() fallback for early bootstrap is postponed on this.

trying...

#2237001: Remove no longer needed _current_path() fallback for early bootstrap is postponed on this, so adding the "blocker" tag.

Note: #2362227: Replace all instances of current_path() is also removing the call to current_path()

I could apply the patch, so no reroll is needed.

Thank you for working on this issue!! I'm curious whether you have checked how it works in Drupal 7. I remember that it used to work,
maybe it used to be an easier solution ... but in general it seems pretty sane what the patch does.

--- /dev/null
+++ b/core/modules/views/tests/modules/views_test_config/test_views/views.view.test_contextual_filter_paging.yml

+++ b/core/modules/views/tests/modules/views_test_config/test_views/views.view.test_contextual_filter_paging.yml
@@ -0,0 +1,249 @@

@@ -0,0 +1,249 @@
+id: test_contextual_filter_paging

Just in case, I'd prefer to name it test_argument_paging, because this is still the internal technical term for it.

Triage notes:

1. Summary is up-to-date.
2. Bug is still present: Reproduced the issue on simplytest.me.
3. No duplicate issues found.
4. Beta evaluation added to summary.

(Saving proposed issue credit for discussion and triage participants at LA.)

Didn't apply anymore, did a reroll.

Reroll.

@xjm @alexpott @cilefen @dawehner and I agreed that this is not a major bug because it only affects the Views Preview UI, and only under certain circumstances (contextual filters and pager navigation).

+++ b/core/modules/views_ui/src/Tests/PreviewTest.php
@@ -239,6 +239,42 @@ public function testPreviewWithPagersUI() {
+    $this->drupalPostForm(NULL, $edit = [], t('Update preview'));
...
+    $this->drupalPostForm(NULL, $edit = ['view_args' => 'page'], t('Update preview'));

I don't think we need the t around these buttons as this view is not translated.
This is the only nitpick I could find, otherwise this patch looks very good.

Almost ready I think

1. +++ b/core/modules/views_ui/src/Tests/PreviewTest.php
   @@ -17,7 +17,14 @@ class PreviewTest extends UITestBase {
   +    'test_argument_paging'
   

   per codesniffer: missing a trailing comma

2. +++ b/core/modules/views_ui/src/Tests/PreviewTest.php
   @@ -322,7 +365,7 @@ protected function getPreviewAJAX($view_name, $panel_id, $row_count) {
   -  protected function clickPreviewLinkAJAX($url, $row_count) {
   +  protected function clickPreviewLinkAJAX($url, $row_count = NULL) {
   
   @@ -335,7 +378,9 @@ protected function clickPreviewLinkAJAX($url, $row_count) {
   -    $this->assertPreviewAJAX($result, $row_count);
   +    if ($row_count) {
   +      $this->assertPreviewAJAX($result, $row_count);
   +    }
   

   Why are we changing this and not just adding a row count to the call? It wouldn't hurt to test that the pager works, right?

Nice, more coverage!

1. +++ b/core/modules/views_ui/src/Tests/PreviewTest.php
   @@ -266,20 +266,29 @@ public function testPreviewPagingWithContextualFilters() {
   +      // Set the contextual filter to show special content type.
   

   special? 'specific' I'd say.

2. +++ b/core/modules/views_ui/src/Tests/PreviewTest.php
   @@ -266,20 +266,29 @@ public function testPreviewPagingWithContextualFilters() {
   +      // Check 2 pages after contextual filter settings.
   

   Check that there are two pages after setting the contextual filter.

3. +++ b/core/modules/views_ui/src/Tests/PreviewTest.php
   @@ -266,20 +266,29 @@ public function testPreviewPagingWithContextualFilters() {
   +      $this->verbose($this->content);
   

   debug?

4. +++ b/core/modules/views_ui/src/Tests/PreviewTest.php
   @@ -266,20 +266,29 @@ public function testPreviewPagingWithContextualFilters() {
   +      // Check that all results is expected type.
   

   Check that all results are of the expected type.

5. +++ b/core/modules/views_ui/src/Tests/PreviewTest.php
   @@ -266,20 +266,29 @@ public function testPreviewPagingWithContextualFilters() {
   +      $this->assertEqual(count($elements), 5, 'Expected items of "Page" type.');
   

   Yeah use $type here. Also if we have 10 results, 5 page five article and the sorting is by type, this would still be true. So we'd need to test this on the first page of the result too if we want to be sure the filter worked.

@vaplas, thanks, look good to me now. Updated the IS a bit to include the findings in #31 that seem relevant to the removal of a big section of code here.

+++ b/core/modules/views_ui/src/Tests/PreviewTest.php
@@ -239,6 +246,52 @@ public function testPreviewWithPagersUI() {
+      $this->assertTrue(empty($this->xpath('//a[@title = :title]', [':title' => 'Go to page 3'])));

Shouldn't this have an assertFalse() for go to page 4 as well?

Fixed deprecation code and coding standard.

Hola, this came up in the #bugsmash channel as the Views daily triage target.

As we haven't had an update in over four years, if there's no update in the next three months, this issue may be closed.

But it is such an annoyance that this functionality is missing ... for everyone who develops using views ...

The tests look great, needs a re-roll of course but better to move this to a MR.