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
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. |
Comment | File | Size | Author |
---|---|---|---|
#27 | interdiff.txt | 576 bytes | dawehner |
#27 | 2387157-27.patch | 12.27 KB | dawehner |
#23 | interdiff.txt | 586 bytes | Gábor Hojtsy |
#23 | 2387157-22.patch | 12.12 KB | Gábor Hojtsy |
#20 | interdiff.txt | 5.31 KB | dawehner |
Comments
Comment #1
dawehnerWorking on it.
Comment #2
dawehnerAlright, here is a patch.
Comment #3
Gábor HojtsyJust tagging and retitling as a bug for now. Will review soon.
Comment #4
Gábor HojtsyAlso 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.
Comment #5
Gábor HojtsyComment #6
Gábor HojtsyComment #7
olli CreditAttribution: olli commentedSimilar issue with access plugin #2387627: Changing access plugins in views leaves invalid settings around.
Comment #8
dawehner@olli
Let's share reviews ...
Comment #9
dawehnerLet's get rid of the
debug()
statementsComment #10
Gábor HojtsyThis 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.
Minor: "For example clone a page display to a block display."
Comment #11
dawehnerWell 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.
Comment #12
Gábor HojtsyAll 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.
Comment #13
dawehnerOh right, this landed in the meantime.
"Luckily" when we remove that switch, it still passes.
Comment #14
Gábor HojtsyYeah it should :) So looks as good as it gets to me.
Comment #15
olli CreditAttribution: olli commentedJust some questions, trying to understand this.
Can you point me an example of this?
Why the second getExecutable()?
How is the first $display different from $executable->display_handler? I don't see where we are changing that.
Comment #16
jibranCan we explain this line with comments?
Comment #17
dawehnerSure, just have a look at
DisplayPluginBase
:Well, my idea was to reinitialize the display, in order to ensure that the updated $display is propagated.
Comment #18
olli CreditAttribution: olli commentedOk, thanks.
Comment #19
olli CreditAttribution: olli commentedTo 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?
Comment #20
dawehnerLet's do it properly, add the method to the interface, make $options optional and fetch it internally.
Comment #21
Gábor HojtsyComment #22
dawehnerAdding the tag.
Comment #23
Gábor HojtsyOnly 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 :)
Comment #24
dawehnerUpdate the IS.
Comment #25
alexpottConsidering we have
setDefinedOptions
this probably should begetDefinedOptions
- it definitely should not bedefineOptions
because that sounds like a setter not a getter.Comment #26
olli CreditAttribution: olli commented#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 newfilterByDefinedOptions
.Comment #27
dawehnerAlright, added some line of documentation here.
Comment #28
alexpottCommitted 4250561 and pushed to 8.0.x. Thanks!