Cloning display into another display also stores options that are not supported by the new display type

Just tagging and retitling as a bug for now. Will review soon.

Also I made the display crud test exempt from schema checking to avoid hitting this bug in #2385805-31: Views tests don't pass strict schema checking, this patch will need to undo that change to the display crud test. Assuming that lands first of course.

Similar issue with access plugin #2387627: Changing access plugins in views leaves invalid settings around.

1. +++ b/core/modules/views/src/Entity/View.php
   @@ -242,6 +243,40 @@ public function &getDisplay($display_id) {
   +    $displays[$new_display_id] = NestedArray::mergeDeep($displays[$new_display_id], $display_duplicate);
   +    $displays[$new_display_id]['id'] = $new_display_id;
   +
   +    // First set the displays.
   +    $this->set('display', $displays);
   +
   +    // Ensure that we just copy display options, which are provided by the new
   +    // display plugin.
   +    $executable = $this->getExecutable();
   +    $executable->setDisplay($new_display_id);
   +
   +    $defined_options = $executable->display_handler->getDefinedOptions();
   +    $executable->display_handler->filterByDefinedOptions($displays[$new_display_id]['display_options'], $defined_options);
   +    // Update the display settings.
   +    $this->set('display', $displays);
   

   This looks a bit rough but I understand this is probably the extent the views API makes possible now. We first set invalid settings so that we have the new display and can instantiate it, so we can ask it for its options and then set it again with filtered options, so its valid.

   Ideally we would set a display with valid options but that would need a display instance ahead of time (eg. create it with the options and then set it on the view), but I think that is not how views work ATM? That is, we can just set the data and later instantiate, not set an instance to be a component of the view.

   In short, unless we can do this by creating a valid instance first, I understand this is the best for now.

2. +++ b/core/modules/views/src/ViewStorageInterface.php
   @@ -30,4 +38,19 @@ public function &getDisplay($display_id);
   +   * This allows you to clone a page display as a block display.
   

   Minor: "For example clone a page display to a block display."

All right AFAIS the only thing to do here is #4 undo the FALSE for strict schema checking in DisplayCRUDTest.php that we added in #2385805-31: Views tests don't pass strict schema checking. The views testbase has TRUE, so we can just remove that section from DisplayCRUDTest.php.

Yeah it should :) So looks as good as it gets to me.

Just some questions, trying to understand this.

display options depends on the configured options

Can you point me an example of this?

1. +++ b/core/modules/views/src/Entity/View.php
   @@ -242,6 +243,40 @@ public function &getDisplay($display_id) {
   +    $executable = $this->getExecutable();
   ...
   +    $executable = $this->getExecutable();
   

   Why the second getExecutable()?

2. +++ b/core/modules/views/src/Entity/View.php
   @@ -242,6 +243,40 @@ public function &getDisplay($display_id) {
   +    $display = $executable->newDisplay($new_display_type);
   ...
   +    $defined_options = $executable->display_handler->getDefinedOptions();
   

   How is the first $display different from $executable->display_handler? I don't see where we are changing that.

+++ b/core/modules/views/src/Entity/View.php
@@ -242,6 +243,40 @@ public function &getDisplay($display_id) {
+    $displays[$new_display_id] = NestedArray::mergeDeep($displays[$new_display_id], $display_duplicate);

Can we explain this line with comments?

Ok, thanks.

+++ b/core/modules/views/src/Plugin/views/PluginBase.php
@@ -177,6 +177,27 @@ protected function setOptionDefaults(array &$storage, array $options) {
+   * @param array $options
+   *   The defined options.
+   */
+  public function filterByDefinedOptions(array &$storage, array $options) {

+++ b/core/modules/views/src/Plugin/views/display/DisplayPluginBase.php
@@ -505,6 +505,17 @@ public function defaultableSections($section = NULL) {
+   * Gets the defined options.
+   *
+   * @see ::defineOptions()
+   *
+   * @return array
+   */
+  public function getDefinedOptions() {

To use filterByDefinedOptions, I need defined options. Either move getDefinedOptions() to PluginBase, or remove getDefinedOptions and make the second param $options for filterByDefinedOptions optional? Any reason why not add it to ViewsPluginInterface?

Only found one missing dot to fix. Otherwise looks good. I agree internalizing getting the definitions is fine. As discussed, due to API limitations, the code is a bit hacky anyway, so I was not calling for this before :)

+++ b/core/modules/views/src/Tests/TestHelperPlugin.php
@@ -25,4 +32,24 @@ public function testSetOptionDefaults(&$storage, $options, $level = 0) {
+  /**
+   * {@inheritdoc}
+   */
+  protected function defineOptions() {
+    return $this->definedOptions;
+  }

Considering we have setDefinedOptions this probably should be getDefinedOptions - it definitely should not be defineOptions because that sounds like a setter not a getter.

#20: Thanks, that worked for me. Postponed #2387627: Changing access plugins in views leaves invalid settings around on this.

#25: setDefinedOptions was added to TestHelperPlugin only to test this new filterByDefinedOptions.

Committed 4250561 and pushed to 8.0.x. Thanks!