Convert Node module variables/settings into configuration

This issue is only about converting the variables. The node types themselves will need to become ConfigEntities (i think that's what Thingies(TM) ended up being called :) ).

Clarifying issue title.

Thanks for kick-starting this! :)

Let's make sure to follow the guidelines: http://drupal.org/node/1667896

Great work, you got it working with the tests. Looks like the errors point to common problems in the patch. There's likely some low hanging fruit here.

Great progress! :)

However, please note that the variables belonging to individual node types should not be converted here. They belong to the node type configuration objects, whose conversion is subject to #111715: Convert node/content types into configuration

Thus, only the variables of node.settings are relevant here.

Thanks! :)

I have a couple of remarks:

+++ b/core/modules/node/config/node.settings.yml
@@ -0,0 +1,12 @@
+node_admin_theme: '0'
...
+default_nodes_main: '10'

Let's remove "node" from these.

+++ b/core/modules/node/config/node.settings.yml
@@ -0,0 +1,12 @@
+cron_last: '0'

This variable is not configuration (but "state" information instead) and should thus be left alone.

+++ b/core/modules/node/config/node.settings.yml
@@ -0,0 +1,12 @@
+recent_block_count: '10'

This should be block.recent.limit

+++ b/core/modules/node/config/node.settings.yml
@@ -0,0 +1,12 @@
+rank_relevance: '0'
+rank_sticky: '0'
+rank_promote: '0'
+rank_recent: '0'

Let's use sub-keys for these; e.g., rank.relevance.

Speaking of, rank.relevance is the only one out of these which is not self-descriptive. Unfortunately, the patch/diff context doesn't show what this rank variable actually means and refers to.

+++ b/core/modules/node/node.api.php
@@ -380,7 +380,7 @@ function hook_node_grants_alter(&$grants, $account, $op) {
-  $restricted = variable_get('example_restricted_roles', array());
+  $restricted = config('node.settings')->get('example_restricted_roles');

@@ -968,7 +968,7 @@ function hook_node_info() {
-  if (variable_get('vote_node_enabled', TRUE)) {
+  if (config('node.settings')->get('vote_node_enabled')) {

These are example code snippets that demonstrate how to implement node module hooks and thus do not actually belong to Node module's settings. :)

However, we should convert them nevertheless, but into fake settings of hypothetical modules; e.g.: vote_node_enabled becomes vote.settings:node.enabled

+++ b/core/modules/node/node.install
@@ -474,17 +474,17 @@ function node_uninstall() {
   // Delete node search ranking variables.
   // @see node_ranking(), _node_rankings()
-  variable_del('node_rank_relevance');
...
   // Delete remaining general module variables.
-  variable_del('node_access_needs_rebuild');

The configuration of a module is uninstalled automatically, so these deletions can just simply be removed.

Looks good except

+++ b/core/modules/node/config/node.settings.ymlundefined
@@ -0,0 +1,13 @@
+cron_last: '0'

As mentionet above stis is a state not a variable, see #1175054: Add a storage (API) for persistent non-configuration state

I didn't read everything, I've been out of core for a while so I can be wrong in some places.

+++ b/core/modules/node/content_types.incundefined
@@ -361,16 +367,25 @@ function node_type_form_submit($form, &$form_state) {
     $variable_new = $key . '_' . $type->type;
     $variable_old = $key . '_' . $type->old_type;
 
     if (is_array($value)) {
       $value = array_keys(array_filter($value));
     }
+    $config->set($configvariable_new , $value)->save();
     variable_set($variable_new, $value);
 
     if ($variable_new != $variable_old) {
+      $config->clear($variable_old)->save();
       variable_del($variable_old);

Why do we need the $variable stuff in here?

+++ b/core/modules/node/lib/Drupal/node/NodeFormController.phpundefined
@@ -248,7 +252,8 @@ class NodeFormController extends EntityFormController {
+    $preview_mode = isset($preview_type) ? $preview_type : DRUPAL_OPTIONAL;

I think with the newer version of php we can write this as isset($preview_type) ?: DRUPAL_OPTIONAL

+++ b/core/modules/node/lib/Drupal/node/Tests/NodeQueryAlterTest.phpundefined
@@ -196,7 +196,7 @@ class NodeQueryAlterTest extends NodeTestBase {
+    config('node_access_test.settings')->set('node_test_node_access_all_uid', $this->noAccessUser->uid)->save();

node_access_test_settings sounds more like state

+++ b/core/modules/node/node.installundefined
@@ -601,6 +594,24 @@ function node_update_8004() {
+ ¶

trailing whitespace

+++ b/core/modules/node/node.moduleundefined
@@ -565,7 +565,20 @@ function node_type_save($info) {
+  // Set some default config variables
+  // if it is created programatically
+  // should be removed when the node config object
+  // issue is finished.
+  // see http://drupal.org/node/111715
+  if (!config('node.type.settings')->get($type->type . '.permissions')) {
+    config('node.type.settings')->set($type->type . '.permissions', 1)->save();

Why aren't the comments wrapped at 80 chars.

+++ b/core/modules/node/tests/modules/node_access_test/config/node_access_test.settings.ymlundefined
@@ -0,0 +1,3 @@
+node_test_node_access_all_uid: '0'
+node_access_test_secret_catalan: '0'
+node_access_test_private: '0'

In general I have a problem with test only config elements. These never will be changed and smell like state (as I said before)

Marking postponed as for #45: #111715: Convert node/content types into configuration

I've put together a separate patch for the search-module integration (aka node_rank_-variables) in #1831632: Convert node_rank_ $rank variables to use configuration system. That part can be attackad independantly from the whole business over at #111715: Convert node/content types into configuration.

#111715: Convert node/content types into configuration actually covered most of this. And other issues are being split out to smaller more manageable tasks eg: #2066145: Convert node_admin_theme to the configuration system and #1831632: Convert node_rank_ $rank variables to use configuration system

update issue summary