When creating a new view (/admin/structure/views/add
) the "Page" widget "Use a Pager" setting is not stored.
To reproduce:
- Create a new view at
/admin/structure/views/add
- Fill in base required options (view name) and choose the "Create a Page" option
- In the "page" options, fill-in base required options (page name, etc.) and uncheck the "Use a Pager" option.
- 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.
Comments
Comment #2
rjacobs CreditAttribution: rjacobs as a volunteer commentedIt 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 anisset()
to an!empty()
. Patch to come shortly.Comment #3
rjacobs CreditAttribution: rjacobs as a volunteer commentedHere'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?
Comment #4
rjacobs CreditAttribution: rjacobs as a volunteer commentedFix title.
Comment #5
stevectorThis change looks solid to me. Here is a new patch that includes a test of both conditions, select a pager and selecting no pager.
Comment #6
Lendudesorting? Bit of a copy paste left over.
for ($i = 0; $i < 12; $i++) ?
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.
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.
Comment #7
rjacobs CreditAttribution: rjacobs as a volunteer commentedHere's an update that should address the points in #6.
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.
Comment #9
Lendude@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.
Comment #10
rjacobs CreditAttribution: rjacobs as a volunteer commentedThanks Len for the reviews and generating that missing interdiff (sorry I missed that). Thanks also to Steve for creating the test!
Comment #11
catchThere'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!