This is part of #1775842: [meta] Convert all variables to state and/or config systems

Instructions are at https://drupal.org/node/1667896

CommentFileSizeAuthor
#7 2066145-convert-node_admin_theme-7.patch7.45 KBmtift
#7 interdiff.txt793 bytesmtift
#4 2066145-convert-node_admin_theme-4.patch7.51 KBmtift
#4 interdiff.txt311 bytesmtift
#3 2066145-convert-node_admin_theme-3.patch7.5 KBmtift
#2 2066145-convert-node_admin_theme-2.patch5.64 KBmtift
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

mtift’s picture

mtift
he/him
Primary language English
Location Minnesota, USA
CreditAttribution: mtift commented
Assigned: Unassigned » mtift
mtift’s picture

mtift
he/him
Primary language English
Location Minnesota, USA
CreditAttribution: mtift commented
Status: Active » Needs review
Issue tags: +Configuration system
FileSize
2066145-convert-node_admin_theme-2.patch5.64 KB

Patch is attached. In cases where we are currently using TRUE (booleans), I converted them to '1'.

mtift’s picture

mtift
he/him
Primary language English
Location Minnesota, USA
CreditAttribution: mtift commented
FileSize
2066145-convert-node_admin_theme-3.patch7.5 KB

The previous patch was wrong on multiple levels, so please ignore it.

Changing node_admin_them to admin_theme could potentially cause confusion with the admin_theme in the system module (especially in ThemeTest.php). I discussed this with @msonnabaum, who suggested use_admin_theme_for_editing_nodes, which seemed more descriptive, but would more accurately need to be named use_admin_theme_for_editing_or_creating_nodes. That seemed too long. @effulgentsia and I thought use_admin_theme would be sufficiently descriptive so that is what I used.

mtift’s picture

mtift
he/him
Primary language English
Location Minnesota, USA
CreditAttribution: mtift commented
FileSize
interdiff.txt311 bytes
2066145-convert-node_admin_theme-4.patch7.51 KB

Updated patch to make config in node.settings.yml alphabetical.

aspilicious’s picture

aspilicious CreditAttribution: aspilicious commented
Status: Needs review » Reviewed & tested by the community

Actually this looks ok :).

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott commented
Status: Reviewed & tested by the community » Needs work 
+++ b/core/modules/node/config/node.settings.ymlundefined
@@ -1 +1,2 @@
+use_admin_theme: '1'

It seems like we're changing the default behaviour here. When it was a variable the default was false and now it true.

+++ b/core/modules/node/node.installundefined
@@ -430,7 +430,7 @@ function node_uninstall() {
+  Drupal::config('node.settings')->delete('use_admin_theme');

There's no need for this line. Default config is automatically removed on uninstall. And delete() does not clear a single key anyway :)

mtift’s picture

mtift
he/him
Primary language English
Location Minnesota, USA
CreditAttribution: mtift commented
Status: Needs work » Needs review
FileSize
interdiff.txt793 bytes
2066145-convert-node_admin_theme-7.patch7.45 KB

Updated patch attached

aspilicious’s picture

aspilicious CreditAttribution: aspilicious commented
Status: Needs review » Reviewed & tested by the community

Back to rtbc unless I'm missing something again?

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch commented
Status: Reviewed & tested by the community » Fixed

Committed/pushed to 8.x, thanks!

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

Anonymous’s picture

(not verified) CreditAttribution: commented
Issue summary: View changes

Updated issue summary.