Problem/Motivation

2. When duplicating displays, what ViewEditForm::submitDuplicateDisplayAsType() does is it removes the display_title and display_plugin but otherwise clones ALL the settings of the display. That is ending up with all kinds of cruft in the new display setting, such as in the test a path for a block. Not good. So what I thought we can do there is to ask the display for their options and then filter the options from the old display with the possible options of the new. That is not possible because defineOptions() is protected on PluginBase and not intended as a public method. I previously also thought display extenders would be a problem here, but getting their stuff integrated is part of defineOptions() on DisplayPluginBase. The $view->options array is actually public, but sounds like it would be a major sin to use that directly, haha... #evilgabor

Proposed resolution

Filter the options using the defined options, so we don't end up with crap entries in the configuration.

Remaining tasks

User interface changes

API changes

Add a new API function to filter out invalide items.

Beta phase evaluation

Reference: https://www.drupal.org/core/beta-changes
Issue category Bug because it makes it impossible for configs to validate the schema.
Disruption Not disruptive at all, because it doesn't change anything beside internal details.
Files: 
CommentFileSizeAuthor
#27 interdiff.txt576 bytesdawehner
#27 2387157-27.patch12.27 KBdawehner
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 82,067 pass(es). View
#23 interdiff.txt586 bytesGábor Hojtsy
#23 2387157-22.patch12.12 KBGábor Hojtsy
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 82,120 pass(es). View
#20 interdiff.txt5.31 KBdawehner
#20 2387157-20.patch12.12 KBdawehner
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 82,113 pass(es). View
#17 interdiff.txt583 bytesdawehner
#17 2387157-17.patch10.85 KBdawehner
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 82,150 pass(es). View
#13 interdiff.txt649 bytesdawehner
#13 2387157-13.patch10.89 KBdawehner
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 82,151 pass(es). View

Comments

dawehner’s picture

Issue tags: +VDC

Working on it.

dawehner’s picture

Status: Active » Needs review
FileSize
10.57 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 82,094 pass(es). View

Alright, here is a patch.

Gábor Hojtsy’s picture

Title: Cloning display into another display should just store options, which are provided by the other display » Cloning display into another display also stores options that are not supported by the new display type
Issue tags: +Configuration schema

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

Gábor Hojtsy’s picture

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.

Gábor Hojtsy’s picture

Issue tags: +Configuration system
Gábor Hojtsy’s picture

Issue tags: +D8 upgrade path
olli’s picture

dawehner’s picture

@olli
Let's share reviews ...

dawehner’s picture

FileSize
10.51 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 82,100 pass(es). View
1.32 KB

Let's get rid of the debug() statements

Gábor Hojtsy’s picture

  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."

dawehner’s picture

FileSize
10.52 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 82,094 pass(es). View
613 bytes

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.

Well yeah, its kinda a hack, but sadly display options depends on the configured options. I can't think for a better solution for it at the moment.
Maybe someone else though has.

Gábor Hojtsy’s picture

Status: Needs review » Needs work

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.

dawehner’s picture

Status: Needs work » Needs review
FileSize
10.89 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 82,151 pass(es). View
649 bytes

Oh right, this landed in the meantime.

"Luckily" when we remove that switch, it still passes.

Gábor Hojtsy’s picture

Status: Needs review » Reviewed & tested by the community

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

olli’s picture

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.

jibran’s picture

+++ 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?

dawehner’s picture

FileSize
10.85 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 82,150 pass(es). View
583 bytes

display options depends on the configured options

Sure, just have a look at DisplayPluginBase:

  if (!$this->usesPager()) {
    $options['defaults']['default']['pager'] = FALSE;
    $options['pager']['contains']['type']['default'] = 'some';
  }
How is the first $display different from $executable->display_handler? I don't see where we are changing that.

Well, my idea was to reinitialize the display, in order to ensure that the updated $display is propagated.

olli’s picture

Ok, thanks.

olli’s picture

Status: Reviewed & tested by the community » Needs work
+++ 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?

dawehner’s picture

Status: Needs work » Needs review
FileSize
12.12 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 82,113 pass(es). View
5.31 KB

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?

Let's do it properly, add the method to the interface, make $options optional and fetch it internally.

Gábor Hojtsy’s picture

Issue tags: +Ghent DA sprint
dawehner’s picture

Adding the tag.

Gábor Hojtsy’s picture

Status: Needs review » Reviewed & tested by the community
FileSize
12.12 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 82,120 pass(es). View
586 bytes

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 :)

dawehner’s picture

Issue summary: View changes

Update the IS.

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
+++ 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.

olli’s picture

#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.

dawehner’s picture

Status: Needs work » Reviewed & tested by the community
FileSize
12.27 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 82,067 pass(es). View
576 bytes

Alright, added some line of documentation here.

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Committed 4250561 and pushed to 8.0.x. Thanks!

  • alexpott committed 4250561 on 8.0.x
    Issue #2387157 by dawehner, Gábor Hojtsy: Cloning display into another...

Status: Fixed » Closed (fixed)

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