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.
Comment | File | Size | Author |
---|---|---|---|
#36 | 1938654-36.patch | 59.78 KB | damiankloip |
#36 | interdiff-1938654-36.patch | 47 KB | damiankloip |
#28 | 1938654-28.patch | 12.78 KB | damiankloip |
#28 | interdiff-1938654-28.txt | 6.2 KB | damiankloip |
#26 | 1938654-26.patch | 12.42 KB | damiankloip |
Comments
Comment #1
Fabianx CreditAttribution: Fabianx commentedThere 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?
Comment #2
tstoecklerRe #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:
My gut reaction would be to say "all of the above". I will post a patch soon that tries to achieve that.
Comment #3
tstoecklerNote 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:
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.
Comment #4
dawehnerRegarding #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.
Comment #5
tstoecklerThat 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.
Comment #6
dawehnerSo 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.
Comment #7
tstoecklerAhh, @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.
Comment #8
damiankloip CreditAttribution: damiankloip commentedSo 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 :)
Comment #10
tim.plunkettShouldn't this be able to iterate without the all()? Since PluginBag implements Iterator.
Comment #11
damiankloip CreditAttribution: damiankloip commentedYeah, good point. We don't need that at all :)
Comment #13
dawehnerCould we call a method on the Display Plugin, this feels better.
Then we should be able to call $display->getPlugin instead. Afaik this doesn't respect overridden and default values.
Comment #14
damiankloip CreditAttribution: damiankloip commentedYeah, you're right. Having that kind of code there isn't the best.
Comment #15
damiankloip CreditAttribution: damiankloip commentedMaybe more like this?
Comment #16
dawehnerShouldn't we include handlers and all the other options as well?
Comment #17
dawehnerShouldn't we also check whether it's overridden here?
Comment #18
damiankloip CreditAttribution: damiankloip commentedThis should be closer, not finished yet but I'm going getting away from the computer for the day now!
Comment #19
dawehnerMy 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.
Comment #21
dawehnerWhat about something like this?
Comment #22
damiankloip CreditAttribution: damiankloip commentedBuilding 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...
Comment #24
damiankloip CreditAttribution: damiankloip commentedHmm, 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.
Comment #25
damiankloip CreditAttribution: damiankloip commentedHow 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?
Comment #26
damiankloip CreditAttribution: damiankloip commentedAdded 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
and can just call
Also, if we need this for any updates. It makes that a bit simpler too.
Comment #27
tim.plunkettThis 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')
Tests
Missing docblock
\Drupal
Comment #28
damiankloip CreditAttribution: damiankloip commentedGreat, thanks Tim! All good points.
Comment #30
damiankloip CreditAttribution: damiankloip commented#28: 1938654-28.patch queued for re-testing.
Comment #31
dawehnerThat's looking great now!
Comment #32
alexpottThe current patch does not solve the issue reported in the summary... if I
I get the following diff...
Comment #33
alexpottLooks like we need to update the default views config
Comment #34
damiankloip CreditAttribution: damiankloip commentedIt 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.
Comment #35
alexpottMy preference is to get this done here for consistency...
Comment #36
damiankloip CreditAttribution: damiankloip commentedok, let's do this! Here are updated default views with default defined options merged in.
Comment #38
dawehnerOh really the interdiff failed ^^
Let's wait until
Just this line is already an improvement, as it's so much easier to read!
Comment #39
alexpottCommitted 9825bac and pushed to 8.x. Thanks!