When creating a new view (/admin/structure/views/add) the "Page" widget "Use a Pager" setting is not stored.

To reproduce:

  1. Create a new view at /admin/structure/views/add
  2. Fill in base required options (view name) and choose the "Create a Page" option
  3. In the "page" options, fill-in base required options (page name, etc.) and uncheck the "Use a Pager" option.
  4. Click "Save and Edit" to proceed to the full views UI.

After the steps above, the view will be configured to use a pager despite having deselected "Use a Pager" on the add view widget. Changing the pager settings via the full views UI works as expected.

I suspect that there is a mismatch issue with the option type for the pager checkbox on the add view page widget vs the pager options on the full views UI. On the add view page widget it's a boolean checkbox, but what's ultimately stored in the view configuration is a string key ('some', 'none', 'full' or 'mini'). The widget's "Use a Pager" value of FALSE might need to be converted into a string of 'none' for the add view page widget setting to stick.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

rjacobs created an issue. See original summary.

rjacobs’s picture

Component: views_ui.module » views.module

It looks like the classes in question are actually not part of views_ui:

Drupal\views\Plugin\views\wizard\WizardPluginBase::buildForm() appears to handle the rendering of those add page view form elements.

Drupal\views\Plugin\views\wizard\WizardPluginBase::pageDisplayOptions() appears to handle the translation of the boolean pager option (from the add view page widget) to a proper string key value.

When checking to see if the pager bool was checked the pageDisplayOptions() method only detects a deselected pager option if the "use pager" form value was not set. The problem seems to be that a bool value of 0 is of course still a set value. So I think this may be a simple case of flipping an isset() to an !empty(). Patch to come shortly.

rjacobs’s picture

Status: Active » Needs review
FileSize
753 bytes

Here's a patch. This allows the unchecked pager option (on the add view page widget) to be detected instead of ignored.

The end result is that a deselected pager option on the add view page widget becomes a formal pager value of 'some'. This appears to be the original UX intent, however I do wonder if a value of 'none' would actually be more appropriate?

rjacobs’s picture

Title: Pager option no processed on views add form page widget » Pager option not saved on views add form page widget

Fix title.

stevector’s picture

This change looks solid to me. Here is a new patch that includes a test of both conditions, select a pager and selecting no pager.

Lendude’s picture

Status: Needs review » Needs work
Issue tags: +VDC
  1. +++ b/core/modules/views/src/Tests/Wizard/PagerTest.php
    @@ -0,0 +1,70 @@
    +  /**
    +   * Tests the sorting functionality.
    +   */
    +  public function testSorting() {
    +    // Create nodes, each with a different creation time so that we can do a
    +    // meaningful sort.
    

    sorting? Bit of a copy paste left over.

  2. +++ b/core/modules/views/src/Tests/Wizard/PagerTest.php
    @@ -0,0 +1,70 @@
    +    $i = 1;
    +    while ($i <= 11) {
    

    for ($i = 0; $i < 12; $i++) ?

  3. +++ b/core/modules/views/src/Tests/Wizard/PagerTest.php
    @@ -0,0 +1,70 @@
    +    $path_with_pager = $this->randomMachineName(16);
    ...
    +    $path_with_no_pager = $this->randomMachineName(16);
    ...
    +    $view['label'] = $this->randomMachineName(16);
    +    $view['id'] = strtolower($this->randomMachineName(16));
    ...
    +    $view['page[title]'] = $this->randomMachineName(16);
    

    I think it is now preferred to use known values and not random values. Random values lead to random fails seems to be the argument.

  4. +++ b/core/modules/views/src/Tests/Wizard/PagerTest.php
    @@ -0,0 +1,70 @@
    +    $this->assertTrue(empty($elements), 'Full pager found.');
    

    Should be 'not found' in the second test.

Patch still applies. Bug still exists. Manually tested the patch and the fix is good. I checked how this works in D7 and this patch gets the functionality back in line with how it works there.

rjacobs’s picture

Here's an update that should address the points in #6.

I think it is now preferred to use known values and not random values. Random values lead to random fails seems to be the argument.

For the programmatically-created test view paths (with/without pager) using a static value probably makes good sense, especially given that we are activity passing that parameter around during the test. So the updated patch changes those values to static strings. For all other values (view ID, label, title) it seems that the other views Wizard tests in core have somewhat set a precedent for the use of randomMachineName(), so I figured it's probably best to stick with that for consistency.

Lendude’s picture

Status: Needs review » Reviewed & tested by the community
FileSize
1.83 KB

@rjacobs, thanks for the new patch (please include an interdiff next time to make life for the reviewer a little easier). Patch looks good to me now.

Manually tested this again and it still fixes the issue there too.

rjacobs’s picture

Thanks Len for the reviews and generating that missing interdiff (sorry I missed that). Thanks also to Steve for creating the test!

catch’s picture

Status: Reviewed & tested by the community » Fixed

There's an issue open to remove randomness in tests, but that's not in yet so consistency is OK for now.

Committed/pushed to 8.1.x and cherry-picked to 8.0.x. Thanks!

  • catch committed 0d83833 on 8.1.x
    Issue #2579931 by rjacobs, stevector: Pager option not saved on views...

  • catch committed 61bc688 on 8.0.x
    Issue #2579931 by rjacobs, stevector: Pager option not saved on views...

Status: Fixed » Closed (fixed)

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