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

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

justafish’s picture

Status: Active » Needs review
FileSize
1.27 KB
alexpott’s picture

Status: Needs review » Needs work

Needs a system_update_N to migrate this variable from a Drupal 7 variable to Drupal 8 config.

justafish’s picture

justafish’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 1824762-Convert-admin_compact_mode-to-CMI-3.patch, failed testing.

alexpott’s picture

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

aspilicious’s picture

Status: Needs work » Needs review

go bot!

gdd’s picture

Should the change to mergeDeepArray() be split off into its own issue? Seems like that shouldn't be hidden away inside a conversion issue patch.

justafish’s picture

Status: Needs review » Postponed
justafish’s picture

Status: Postponed » Needs review
FileSize
1.86 KB

Array merging has been fixed now, so here's the patch from #3 with the system update version number bumped

aspilicious’s picture

Status: Needs review » Reviewed & tested by the community

Looks good to me

Dries’s picture

Asking for a re-test. I believe this may need a reroll.

Dries’s picture

webchick’s picture

Status: Reviewed & tested by the community » Fixed

Committed and pushed to 8.x. Thanks!

sun’s picture

Status: Fixed » Needs review

Hm.

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:

admin_mode: compact|elaborate

...which still does not clarify at all that it is about descriptions on admin overview/category pages though, so actually much rather this:

admin_category_descriptions: show|hide

...or alternatively, if you prefer a Boolean value, inject the "show" nature into the config key:

admin_show_category_descriptions: 1|0

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

msonnabaum’s picture

Status: Needs review » Fixed

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

sun’s picture

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

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

Anonymous’s picture

Issue summary: View changes

Update issue link