Follow-up to #2358265: Some views schemas are (still) missing, maybe

Problem/Motivation

While fixing #2358265: Some views schemas are (still) missing, maybe found that while views stores plugin options under the plugins, it does not clear out the [plugin]_options form element and therefore saves extra items in views that are an artefact of the form structure and then not used later on. See:

abstract class DisplayPluginBase extends PluginBase {
  public function submitOptionsForm(&$form, FormStateInterface $form_state) {
      case 'access_options':
      case 'cache_options':
      case 'exposed_form_options':
      case 'pager_options':
      case 'row_options':
      case 'style_options':
        // Submit plugin options. Every section with "_options" in it, belongs to
        // a plugin type, like "style_options".
        $plugin_type = str_replace('_options', '', $section);
        if ($plugin = $this->getPlugin($plugin_type)) {
          $plugin_options = $this->getOption($plugin_type);
          $plugin->submitOptionsForm($form[$plugin_type . '_options'], $form_state);
          $plugin_options['options'] = $form_state->getValue($section);
          $this->setOption($plugin_type, $plugin_options);
        }
        break;
  }
}

Proposed resolution

Remove the artefacts.

Remaining tasks

1. Patch, automated tests.
2. Review
3. Commit.

User interface changes

None.

API changes

None

Files: 
CommentFileSizeAuthor
#12 2385107-no-persist-options-12.patch2.17 KBGábor Hojtsy
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 81,759 pass(es), 24 fail(s), and 1 exception(s). View
#12 interdiff.txt1.14 KBGábor Hojtsy
#9 2385107-unset-options-9.patch2.12 KBGábor Hojtsy
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 81,752 pass(es), 24 fail(s), and 1 exception(s). View
#7 2385107-unset-options-7-with-test.patch2.11 KBGábor Hojtsy
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 81,734 pass(es), 24 fail(s), and 1 exception(s). View
#4 2385107-unset-options-2.patch717 bytesGábor Hojtsy
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 81,828 pass(es). View
#3 2385107-test.patch1.41 KBGábor Hojtsy
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 81,741 pass(es), 25 fail(s), and 1 exception(s). View

Comments

vijaycs85’s picture

Issue summary: View changes
vijaycs85’s picture

Issue tags: +VDC
FileSize
717 bytes

Initial patch...

Gábor Hojtsy’s picture

Issue tags: -Needs tests
FileSize
1.41 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 81,741 pass(es), 25 fail(s), and 1 exception(s). View

Here is a very quick test only patch that proves pager_options are still saved. Thanks to the views wizard tests checking for strict config schema.

Gábor Hojtsy’s picture

FileSize
717 bytes
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 81,828 pass(es). View

Duh, strange crosspost....

Gábor Hojtsy’s picture

Title: Cleanup legacy [plugin]_options from » Cleanup legacy [plugin]_options from views displays
Issue summary: View changes

Completed the title :)

The last submitted patch, 3: 2385107-test.patch, failed testing.

Gábor Hojtsy’s picture

FileSize
2.11 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 81,734 pass(es), 24 fail(s), and 1 exception(s). View

Merged the two patches.

Status: Needs review » Needs work

The last submitted patch, 7: 2385107-unset-options-7-with-test.patch, failed testing.

Gábor Hojtsy’s picture

Status: Needs work » Needs review
FileSize
2.12 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 81,752 pass(es), 24 fail(s), and 1 exception(s). View

So this just sets it to NULL. Let's use unsetValue() :) Sorry no interdiff because I hand-edited. The change is in the first hunk, only one line.

Status: Needs review » Needs work

The last submitted patch, 9: 2385107-unset-options-9.patch, failed testing.

Gábor Hojtsy’s picture

Note that pager_options and exposed_form_options are both in the defaultable list in DisplayPluginBase::defaultableSections():

      'pager' => array('pager', 'pager_options'),
      'pager_options' => array('pager', 'pager_options'),

      'exposed_form' => array('exposed_form', 'exposed_form_options'),
      'exposed_form_options' => array('exposed_form', 'exposed_form_options'),

OTOH exposed_form_options and pager_options are not listed in DisplayPluginBase::defineOptions(). But they are under pager.options and exposed_form.options respectively.

So would it suffice to not list the pager_options and exposed_form_options in defaultableSections() or is that needed for some reason? Other options are not listed.

Gábor Hojtsy’s picture

Status: Needs work » Needs review
FileSize
1.14 KB
2.17 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 81,759 pass(es), 24 fail(s), and 1 exception(s). View

An attempt at that approach.

Status: Needs review » Needs work

The last submitted patch, 12: 2385107-no-persist-options-12.patch, failed testing.

olli’s picture

So would it suffice to not list the pager_options and exposed_form_options in defaultableSections() or is that needed for some reason? Other options are not listed.

#2331793: Changing pager settings for this display only also changes pager settings for other display looks related.

Gábor Hojtsy’s picture

Status: Needs work » Closed (duplicate)
Issue tags: -sprint

This looks like a clear duplicate of that one.