| Project: | Drupal core |
| Version: | 8.x-dev |
| Component: | views.module |
| Category: | task |
| Priority: | normal |
| Assigned: | Unassigned |
| Status: | fixed |
| Issue tags: | VDC |
Issue Summary
Note: VDC folks may be able to explain this better, please edit if that is the case.
Use-case
- Add a view that lists nodes and save it with the default settings, e.g. 10 nodes.
- Add the corresponding config file to version control.
- Now decide that you want to show 15 instead of 10 nodes. Go to the view edit that setting and save.
Expected result
The version-control system shows a diff of the config file as:
- items_per_page: '10'
+ items_per_page: '15'Actual result
The version-control system shows a diff of the config file as:
+ options:
+ items_per_page: '15'
+ offset: '0'
+ id: '0'
+ total_pages: ''
+ tags:
+ previous: '‹ previous'
+ next: 'next ›'
+ first: '« first'
+ last: 'last »'
+ expose:
+ items_per_page: '0'
+ items_per_page_label: 'Items per page'
+ items_per_page_options: '5, 10, 20, 40, 60'
+ items_per_page_options_all: '0'
+ items_per_page_options_all_label: '- All -'
+ offset: '0'
+ offset_label: Offset
+ quantity: '9'Problem
Because the handler settings form that controls the number of items per page is only loaded when you click on the appropriate link in the Views edit form, by default that form's submit handler is not called. Therefore the corresponding config keys are not set.
Solution
When saving a view iterate over all plugins and handlers and set each config key to the default value.
Downside
The problem with this approach is that
A. the config files become just that much larger
B. even though the diff becomes clearer when modifying settings with this, when *adding* a field, for example, you would get a huge hunk in the diff that adds a billion default values, which makes the diff less reviewable in this case.
Because we really have to choose one or the other here I think there is no way around Views behaving like all other config in core. Therefore I think the proposed solution, despite the downsides, is the way to go.
Comments
#1
There is another possibility:
Have config be aware of defaults and just not output the default values. (like an internal array diff again the default values) That way:
* Your example outputs only +15, the config keeps being small and diffs are readable.
It is true however that in this case you don't know the default value (the -10), but perhaps we can add some override to the config diffing code to show that as a follow-up?
I think it would not be terribly difficult to implement that and I think it is _much_ better than any of the two behaviors you proposed (current vs. export all).
Thoughts?
#2
Re #1: I thought about this a lot and while I'm not sure that I am totally against it, I think it would be a pretty big change at this point. Because we would have to enforce that for all config files not just views. The major downside of that approach would be that a single config files is not independent of the system anymore. It relies on the default config, which is usually shipped with the module's config files, or in Views' case in PHP code. The fact that the defaults live in module-shipped config would also mean that we would constantly have to load those files on runtime in order to get the "actual" config (saved + default). So while I agree that this issue will make views config files even larger to the point where it hurts, I don't think we can avoid that with the current system. If you want to pursue that change I encourage to open a separate issue ("Don't save default values in configuration files") and fight off all attackers. :-)
Regarding this issue, I spoke to @dawehner a bit during the #SprintWeekend and how we want to fix this issue really boils down to which of the following we want to support in terms of the resulting configuration being "complete" in the sense explained in the original post:
<?php$view = entity_create('view', array(...));
$view->save();
?>
<?php$view->set('foo', 'bar');
$view->save();
?>
My gut reaction would be to say "all of the above". I will post a patch soon that tries to achieve that.
#3
Note that 3. in #2 includes a subtlety that I did not mention:
When setting a value, or a bunch of values, you can change the plugin_id of a handler which also results in different plugin options and, thus, different default values. For example:
<?php$view = entity_create('view', array(
'display' => array(
// Some levels down...
'plugin_id' => 'foo',
'foo_option_1' => 'default',
'foo_option_2' => 'default2',
),
));
$view->set('display', array(
// Those same levels down...
'plugin_id' => 'bar',
));
$display = $view->get('display');
// At this point those same levels in the display key should be:
'plugin_id' => 'bar',
'bar_option_A' => FALSE,
'bar_obtion_B' => TRUE,
));
?>
This example uses the 'display' key because @dawehner said that is the only one that we need to cater for in this patch, because that's basically where all the dynamic/magic-y stuff happens in a view.
#4
Regarding #1
We discussed this in IRC and said it's not worth and even bad. We had a lot of trouble with changing default values in D7/D6, so let's not reintroduce this problem.
#5
That would mean, though, that we will be sticking diffs like in the OP in people's faces, which I would seriously hope we can avoid.
Can you clarify in detail what would be "bad" about this? If you think it's not worth it, I guess I can't force you to ;-), but I don't mean what you mean by bad.
#6
So let's assume we just export the non-default values.
The UX team realized that we want to expose CSS classes in the field configuration all the time. Okay easy: Let's change defineOptions() in FieldPluginBase() and we are ready, but OHH this would also change the behavior of all existing views, as this default values are used to construct the actual values of the options on runtime. To summarize: If we force to export only the non-default-values we can't change any default setting during the full release cycle, which you might not believe, but happens to be required to be changed at least a few times during 7.x-3.x.
#7
Ahh, @dawehner, I thought #4 was regarding the proposal in the OP. I missed the "Regarding #1" part; I saw that just now. That's why I was a little defensive in #5 :-). I totally agree with your assessment.
#8
So maybe we could do something like this to populate the defaults before saving? I've attached a do not test patch, as well as a patch that should* work that contains the patch from #1886894: Add an all() method to PluginBag to add the all() method to the displayBag.
The patch needs a bit more work, for sure. I need to change the ->options used to load the plugin instance, for example. Reviews welcome though :)
#9
The last submitted patch, 1938654-with-all-method.patch, failed testing.
#10
+++ b/core/modules/views/lib/Drupal/views/ViewStorageController.phpundefined@@ -42,6 +42,23 @@ protected function attachLoad(&$queried_entities, $revision_id = FALSE) {
+ foreach ($executable->displayHandlers->all() as $display) {
Shouldn't this be able to iterate without the all()? Since PluginBag implements Iterator.
#11
Yeah, good point. We don't need that at all :)
#12
The last submitted patch, 1938654-11.patch, failed testing.
#13
+++ b/core/modules/views/lib/Drupal/views/ViewStorageController.phpundefined@@ -42,6 +42,23 @@ protected function attachLoad(&$queried_entities, $revision_id = FALSE) {
+ protected function preSave(EntityInterface $entity) {
Could we call a method on the Display Plugin, this feels better.
+++ b/core/modules/views/lib/Drupal/views/ViewStorageController.phpundefined@@ -42,6 +42,23 @@ protected function attachLoad(&$queried_entities, $revision_id = FALSE) {
+ $plugin = Views::pluginManager($type)->createInstance($display->options[$type]['type']);
+ $plugin->init($executable, $display);
+ $display->display['display_options'][$type]['options'] += $plugin->options;
Then we should be able to call $display->getPlugin instead. Afaik this doesn't respect overridden and default values.
#14
Yeah, you're right. Having that kind of code there isn't the best.
#15
Maybe more like this?
#16
+++ b/core/modules/views/lib/Drupal/views/Plugin/views/display/DisplayPluginBase.phpundefined@@ -2724,6 +2724,18 @@ public function getPagerText() {
+ foreach (array('access', 'cache', 'query', 'exposed_form', 'pager', 'style', 'row') as $type) {
Shouldn't we include handlers and all the other options as well?
#17
+++ b/core/modules/views/lib/Drupal/views/Plugin/views/display/DisplayPluginBase.phpundefined@@ -2724,6 +2724,18 @@ public function getPagerText() {
+ if (isset($this->display['display_options'][$type]['type'])) {
Shouldn't we also check whether it's overridden here?
#18
This should be closer, not finished yet but I'm going getting away from the computer for the day now!
#19
My idea actually have been to put this information onto the defineOptions() and then just execute from there one.
You might could get inspired by the Drupal 7 code of views.
#20
The last submitted patch, 1938654-18.patch, failed testing.
#21
What about something like this?
#22
Building on that idea, how about something more like this? I added a test too.
The thing I'm not happy about is having the switch the plural handler types...
#23
The last submitted patch, 1938654-22.patch, failed testing.
#24
Hmm, I think there are issues that mergeDefaults is called really early, in installation, and then it goes boom when we try to load a plugin when the config entity (view) is saved.
#25
How about we just do with in the wizard? We can also invoke this if we add any new options in updates etc...
Let me know what you think, then I can add a UI based test, and maybe a method on the view executable that invokes mergeDefaults() on each display?
#26
Added a quick test for the wizard, mainly to make sure that default values have been merged, as the unit test asserts that the actual option arrays are identical.
Also added a mergeDefaults method on ViewExecutable, mainly so I don;t have to keep doing
<?php$view->initDisplay();
foreach ($view->displayHandlers as $display) {
$display->mergeDefaults()
}
?>
and can just call
<?php$view->mergeDefaults();
?>
Also, if we need this for any updates. It makes that a bit simpler too.
#27
+++ b/core/modules/views/lib/Drupal/views/Plugin/views/display/DisplayPluginBase.phpundefined
@@ -598,27 +605,34 @@ protected function defineOptions() {
+ 'merge_defaults' => 'mergeHandler',
@@ -2725,6 +2739,65 @@ public function getPagerText() {
+ if (!isset($definition['merge_defaults']) || !method_exists($this, $definition['merge_defaults'])) {
This worries me, because it means for someone to override the merge_defaults callback, it has to be a method on the DisplayPlugin. Why not make it a callable, use call_user_func(), and make them specify something like
'merge defaults' => array($this, 'mergeHandler')+++ b/core/modules/views/lib/Drupal/views/Tests/Plugin/DisplayUnitTest.phpundefined@@ -0,0 +1,93 @@
+ * Test the default display options.
Tests
+++ b/core/modules/views/lib/Drupal/views/Tests/Wizard/BasicTest.phpundefined@@ -150,4 +150,27 @@ protected function testWizardForm() {
+ public function testWizardDefaultValues() {
Missing docblock
+++ b/core/modules/views/lib/Drupal/views/Tests/Wizard/BasicTest.phpundefined@@ -150,4 +150,27 @@ protected function testWizardForm() {
+ // @see Drupal\views\Tests\Plugin\DisplayUnitTest
\Drupal
#28
Great, thanks Tim! All good points.
#29
The last submitted patch, 1938654-28.patch, failed testing.
#30
#28: 1938654-28.patch queued for re-testing.
#31
That's looking great now!
#32
The current patch does not solve the issue reported in the summary... if I
I get the following diff...
diff sites/default/files/config_ahx1VVwjOrVILFef4cBybleh7zDz6bInJFEUKRCKBEY/active/views.view.frontpage.yml core/modules/node/config/views.view.frontpage.yml96d95
< type: full
99,115c98
< offset: '0'
< id: '0'
< total_pages: ''
< tags:
< previous: '‹ previous'
< next: 'next ›'
< first: '« first'
< last: 'last »'
< expose:
< items_per_page: '0'
< items_per_page_label: 'Items per page'
< items_per_page_options: '5, 10, 20, 40, 60'
< items_per_page_options_all: '0'
< items_per_page_options_all_label: '- All -'
< offset: '0'
< offset_label: Offset
< quantity: '9'
---
> type: full
149c132
< position: '0'
---
> position: 0
156c139
< position: '1'
---
> position: 1
161c144
< position: '2'
---
> position: 2
171,172c154
< tag: default
< uuid: c368c19b-8e00-4a89-a502-2f44a0608044
---
> tag: 'default'
#33
Looks like we need to update the default views config
#34
It does solve the issue in the summary, but frontpage is a default view. So this has not been modified in this issue. Should we do that here? Seems like too much for one issue, as we will want to update all the default views.
This patch is only meant to add these defaults in when a new view is created from the wizard, not when config is saved.
#35
My preference is to get this done here for consistency...
#36
ok, let's do this! Here are updated default views with default defined options merged in.
#37
The last submitted patch, interdiff-1938654-36.patch, failed testing.
#38
Oh really the interdiff failed ^^
Let's wait until
+++ b/core/modules/node/config/views.view.frontpage.ymlundefined@@ -93,17 +104,41 @@ display:
row:
+ type: node
options:
build_mode: teaser
comments: '0'
links: '1'
- type: node
+ view_mode: teaser
Just this line is already an improvement, as it's so much easier to read!
#39
Committed 9825bac and pushed to 8.x. Thanks!