Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
Issue Summary
Created from #1775842: [meta] Convert all variables to state and/or config systems: [meta] Convert all variables to state and/or config systems.
The following variable/file pairs need to be converted:
admin_compact_mode system.module
Comment | File | Size | Author |
---|---|---|---|
#10 | 1824762-convert_admin_compact_mode_to_cmi-10.patch | 1.86 KB | justafish |
#6 | 3-6-interdiff.txt | 3.33 KB | alexpott |
#6 | 1824762-Convert-admin_compact_mode-to-CMI-6.patch | 5.29 KB | alexpott |
#3 | 1824762-Convert-admin_compact_mode-to-CMI-3.patch | 1.87 KB | justafish |
#3 | interdiff.txt | 822 bytes | justafish |
Comments
Comment #1
justafishComment #2
alexpottNeeds a system_update_N to migrate this variable from a Drupal 7 variable to Drupal 8 config.
Comment #3
justafishComment #4
justafishComment #6
alexpottThis patch has uncovered a nasty issue with
update_variables_to_config()
and merging numeric keys such as page.403 and page.404 - the 403 and 404 will be numeric keys. By default, NestedArray::mergeDeepArray() will not preserve numeric keys.The attached patch fixes this by adding the ability to preserve keys to
NestedArray::mergeDeepArray()
and a new merge method to config so that we have a consistent way of merging data into a configuration object.@Justafish and I discussed this at length and agreed that providing a generic method to merge on config object will ensure that we have a consistent way of merging configuration data.
Comment #7
aspilicious CreditAttribution: aspilicious commentedgo bot!
Comment #8
gddShould the change to mergeDeepArray() be split off into its own issue? Seems like that shouldn't be hidden away inside a conversion issue patch.
Comment #9
justafishPostponed on #1825466: Allow NestedArray::mergeDeepArray() to preserve integer keys
Comment #10
justafishArray merging has been fixed now, so here's the patch from #3 with the system update version number bumped
Comment #11
aspilicious CreditAttribution: aspilicious commentedLooks good to me
Comment #12
Dries CreditAttribution: Dries commentedAsking for a re-test. I believe this may need a reroll.
Comment #13
Dries CreditAttribution: Dries commented#10: 1824762-convert_admin_compact_mode_to_cmi-10.patch queued for re-testing.
Comment #14
webchickCommitted and pushed to 8.x. Thanks!
Comment #15
sunHm.
1) I wonder whether we really need this configuration setting in the first place, and whether we cannot simply default to TRUE/FALSE?
2) If we keep it, "admin_compact_mode" with a value of 0 or 1 is not really descriptive for a configuration setting. E.g., a better, and much more self-descriptive setting would be:
...which still does not clarify at all that it is about descriptions on admin overview/category pages though, so actually much rather this:
...or alternatively, if you prefer a Boolean value, inject the "show" nature into the config key:
Let's make sure to follow the guidelines for how to convert variables into configuration in all of the variable to config/state conversions.
The important aspect is: What previously might have made sense as a variable name almost always does no longer make sense as a config object key. But reality thus far is, almost all of our previous variable names were poorly named and designed in the first place. They did not make any sense as a standalone variable name, and they make even less sense as a key within a config object.
So, in general, if we convert all variable names directly into config object keys, then we'll end up with config files that are impossible to understand, and the one and only change we'll have achieved is where the respective values are stored.
This ought to be taken into account as hard requirement for all config conversions. We've written those guidelines exactly for that purpose. Most often, the final evaluation I'm personally applying myself is: "Am I able to understand which effect this config object key value has, without looking up its usage throughout the code base?"
With the concrete example here, I've no idea what "admin_compact_mode" is referring to — the name does not describe the scope and context and effect of what the setting is doing.
Lastly, we should also not be afraid of 1) using sub-keys à la 'admin.category.show_descriptions', as well as introducing new config objects (e.g., 'system.admin'), which quite potentially makes sense here, since this setting is only ever needed on /admin* category/overview pages, but not in the 99.9% of all other cases in which system.site is loaded (pretty much for every single request that hits a site).
Comment #16
msonnabaum CreditAttribution: msonnabaum commentedThis is bad naming regardless of whether its a variable or CMI. Fixing non-intention-revealing names is always worth doing, but it has nothing to do with CMI conversion.
This issue is done and committed, create a followup for anything outside the scope of variable to CMI conversion.
Comment #17
sunWell, we authored those guidelines to ensure that config conversions are happening in a sane way. They were created before the master/parent issue of this issue was created. I think it it wrong to brush aside the concerns being raised in #15, because they are doing exactly what is supposed to be done for every config conversion, as clearly documented in the guidelines.
Comment #18.0
(not verified) CreditAttribution: commentedUpdate issue link