Note: VDC folks may be able to explain this better, please edit if that is the case.

Use-case

  1. Add a view that lists nodes and save it with the default settings, e.g. 10 nodes.
  2. Add the corresponding config file to version control.
  3. 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.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Fabianx’s picture

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?

tstoeckler’s picture

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:

  1. Creating and saving views from the UI
  2. $view = entity_create('view', array(...));
    $view->save();
    
  3. $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.

tstoeckler’s picture

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:

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

dawehner’s picture

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.

tstoeckler’s picture

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.

dawehner’s picture

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.

tstoeckler’s picture

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.

damiankloip’s picture

Status: Active » Needs review
FileSize
3.72 KB
2.13 KB

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

Status: Needs review » Needs work

The last submitted patch, 1938654-with-all-method.patch, failed testing.

tim.plunkett’s picture

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

damiankloip’s picture

Status: Needs work » Needs review
FileSize
2.12 KB

Yeah, good point. We don't need that at all :)

Status: Needs review » Needs work

The last submitted patch, 1938654-11.patch, failed testing.

dawehner’s picture

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

damiankloip’s picture

Yeah, you're right. Having that kind of code there isn't the best.

damiankloip’s picture

Status: Needs work » Needs review
FileSize
1.91 KB
2.59 KB

Maybe more like this?

dawehner’s picture

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

dawehner’s picture

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

damiankloip’s picture

This should be closer, not finished yet but I'm going getting away from the computer for the day now!

dawehner’s picture

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.

Status: Needs review » Needs work

The last submitted patch, 1938654-18.patch, failed testing.

dawehner’s picture

FileSize
4.78 KB
6.52 KB

What about something like this?

damiankloip’s picture

Status: Needs work » Needs review
FileSize
10.16 KB

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

Status: Needs review » Needs work

The last submitted patch, 1938654-22.patch, failed testing.

damiankloip’s picture

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.

damiankloip’s picture

Status: Needs work » Needs review
FileSize
3.04 KB
10.49 KB

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?

damiankloip’s picture

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

$view->initDisplay();
foreach ($view->displayHandlers as $display) {
  $display->mergeDefaults()
}

and can just call

$view->mergeDefaults();

Also, if we need this for any updates. It makes that a bit simpler too.

tim.plunkett’s picture

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

damiankloip’s picture

Great, thanks Tim! All good points.

Status: Needs review » Needs work
Issue tags: -VDC

The last submitted patch, 1938654-28.patch, failed testing.

damiankloip’s picture

Status: Needs work » Needs review
Issue tags: +VDC

#28: 1938654-28.patch queued for re-testing.

dawehner’s picture

Status: Needs review » Reviewed & tested by the community

That's looking great now!

alexpott’s picture

Status: Reviewed & tested by the community » Needs work

The current patch does not solve the issue reported in the summary... if I

  1. Install standard profile
  2. Edit frontpage view
  3. Change listed items to 15
  4. Change listed items to 10
  5. Press save...

I get the following diff...

diff sites/default/files/config_ahx1VVwjOrVILFef4cBybleh7zDz6bInJFEUKRCKBEY/active/views.view.frontpage.yml core/modules/node/config/views.view.frontpage.yml
96d95
<         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'
alexpott’s picture

Looks like we need to update the default views config

damiankloip’s picture

Status: Needs work » Needs review

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.

alexpott’s picture

My preference is to get this done here for consistency...

damiankloip’s picture

ok, let's do this! Here are updated default views with default defined options merged in.

Status: Needs review » Needs work

The last submitted patch, interdiff-1938654-36.patch, failed testing.

dawehner’s picture

Status: Needs work » Reviewed & tested by the community

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!

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Committed 9825bac and pushed to 8.x. Thanks!

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