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

CommentFileSizeAuthor
#12 2385107-no-persist-options-12.patch2.17 KBGábor Hojtsy
#12 interdiff.txt1.14 KBGábor Hojtsy
#9 2385107-unset-options-9.patch2.12 KBGábor Hojtsy
#7 2385107-unset-options-7-with-test.patch2.11 KBGábor Hojtsy
#4 2385107-unset-options-2.patch717 bytesGábor Hojtsy
#3 2385107-test.patch1.41 KBGábor Hojtsy
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

vijaycs85’s picture

vijaycs85
Location London, UK
CreditAttribution: vijaycs85 commented
Issue summary: View changes
vijaycs85’s picture

vijaycs85
Location London, UK
CreditAttribution: vijaycs85 commented
Issue tags: +VDC
FileSize
2385107-unset-options-2.patch717 bytes

Initial patch...

Gábor Hojtsy’s picture

Gábor Hojtsy
he/him
Primary language Hungarian
Location Hungary
CreditAttribution: Gábor Hojtsy commented
Issue tags: -Needs tests
FileSize
2385107-test.patch1.41 KB

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

Gábor Hojtsy
he/him
Primary language Hungarian
Location Hungary
CreditAttribution: Gábor Hojtsy commented
FileSize
2385107-unset-options-2.patch717 bytes

Duh, strange crosspost....

Gábor Hojtsy’s picture

Gábor Hojtsy
he/him
Primary language Hungarian
Location Hungary
CreditAttribution: Gábor Hojtsy commented
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

Gábor Hojtsy
he/him
Primary language Hungarian
Location Hungary
CreditAttribution: Gábor Hojtsy commented
FileSize
2385107-unset-options-7-with-test.patch2.11 KB

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

Gábor Hojtsy
he/him
Primary language Hungarian
Location Hungary
CreditAttribution: Gábor Hojtsy commented
Status: Needs work » Needs review
FileSize
2385107-unset-options-9.patch2.12 KB

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

Gábor Hojtsy
he/him
Primary language Hungarian
Location Hungary
CreditAttribution: Gábor Hojtsy commented

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

Gábor Hojtsy
he/him
Primary language Hungarian
Location Hungary
CreditAttribution: Gábor Hojtsy commented
Status: Needs work » Needs review
FileSize
interdiff.txt1.14 KB
2385107-no-persist-options-12.patch2.17 KB

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

olli CreditAttribution: olli commented

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

Gábor Hojtsy
he/him
Primary language Hungarian
Location Hungary
CreditAttribution: Gábor Hojtsy commented
Status: Needs work » Closed (duplicate)
Issue tags: -sprint

This looks like a clear duplicate of that one.