The node settings need to be converted to use the new configuration system. Not node types themselves, those need to be configentities so maybe those will be in another issue.

Files: 
CommentFileSizeAuthor
#45 1779486_last.patch39.25 KBjulien
PASSED: [[SimpleTest]]: [MySQL] 41,485 pass(es). View
#43 1779486_last.patch39.36 KBjulien
PASSED: [[SimpleTest]]: [MySQL] 41,299 pass(es). View
#41 1779486_last.patch39.31 KBjulien
FAILED: [[SimpleTest]]: [MySQL] 40,871 pass(es), 701 fail(s), and 404 exception(s). View
#38 1779486_last.patch40.19 KBjulien
PASSED: [[SimpleTest]]: [MySQL] 41,299 pass(es). View
#36 1779486.patch40.21 KBjulien
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1779486_12.patch. Unable to apply patch. See the log in the details link for more information. View
#34 1779486.patch39.76 KBjulien
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1779486_11.patch. Unable to apply patch. See the log in the details link for more information. View
#32 1779486.patch37.24 KBjulien
FAILED: [[SimpleTest]]: [MySQL] 41,258 pass(es), 9 fail(s), and 0 exception(s). View
#30 1779486.patch40.18 KBjulien
FAILED: [[SimpleTest]]: [MySQL] 41,170 pass(es), 31 fail(s), and 1 exception(s). View
#26 1779486.patch35.7 KBjulien
FAILED: [[SimpleTest]]: [MySQL] 41,002 pass(es), 55 fail(s), and 6 exception(s). View
#28 1779486.patch35.7 KBjulien
FAILED: [[SimpleTest]]: [MySQL] 41,053 pass(es), 40 fail(s), and 5 exception(s). View
#18 1779486_3.patch11.41 KBjulien
FAILED: [[SimpleTest]]: [MySQL] 41,252 pass(es), 2 fail(s), and 0 exception(s). View
#25 1779486.patch15.35 KBjulien
PASSED: [[SimpleTest]]: [MySQL] 41,246 pass(es). View
#24 1779486.patch14.39 KBjulien
PASSED: [[SimpleTest]]: [MySQL] 41,248 pass(es). View
#21 1779486.patch11.33 KBjulien
PASSED: [[SimpleTest]]: [MySQL] 41,244 pass(es). View
#19 1779486_3.patch10.48 KBjulien
FAILED: [[SimpleTest]]: [MySQL] 41,243 pass(es), 1 fail(s), and 0 exception(s). View
#14 1779486.patch10.81 KBjulien
FAILED: [[SimpleTest]]: [MySQL] 41,231 pass(es), 10 fail(s), and 112 exception(s). View
#10 1779486.patch24.14 KBjulien
FAILED: [[SimpleTest]]: [MySQL] 39,640 pass(es), 561 fail(s), and 746 exception(s). View
#8 1779486.patch58.94 KBjulien
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1779486_1.patch. Unable to apply patch. See the log in the details link for more information. View
#7 1779486.patch58.94 KBjulien
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1779486_0.patch. Unable to apply patch. See the log in the details link for more information. View
#6 1779486.patch17.21 KBjulien
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1779486.patch. Unable to apply patch. See the log in the details link for more information. View
#3 drupal8-convert-node-module-configuration-to-cmi-1779486.patch3.65 KBjulien
FAILED: [[SimpleTest]]: [MySQL] 41,230 pass(es), 9 fail(s), and 0 exception(s). View

Comments

chx’s picture

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

sun’s picture

Title: Convert node module configuration to CMI » Convert Node module variables/settings into configuration

Clarifying issue title.

julien’s picture

Title: Convert Node module variables/settings into configuration » Convert node module configuration to CMI
Status: Active » Needs review
FileSize
3.65 KB
FAILED: [[SimpleTest]]: [MySQL] 41,230 pass(es), 9 fail(s), and 0 exception(s). View

one very simple patch to start with, with the most general ones.

Status: Needs review » Needs work

The last submitted patch, drupal8-convert-node-module-configuration-to-cmi-1779486.patch, failed testing.

sun’s picture

Title: Convert node module configuration to CMI » Convert Node module variables/settings into configuration

Thanks for kick-starting this! :)

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

julien’s picture

FileSize
17.21 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1779486.patch. Unable to apply patch. See the log in the details link for more information. View

few more functions change. even if the yml structure is not right, it will just need to adapt the config object name and variables name.

julien’s picture

Status: Needs work » Needs review
FileSize
58.94 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1779486_0.patch. Unable to apply patch. See the log in the details link for more information. View

ok an update even if some tests fail.

julien’s picture

FileSize
58.94 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1779486_1.patch. Unable to apply patch. See the log in the details link for more information. View

simpler one.

Status: Needs review » Needs work

The last submitted patch, 1779486.patch, failed testing.

julien’s picture

Status: Needs work » Needs review
FileSize
24.14 KB
FAILED: [[SimpleTest]]: [MySQL] 39,640 pass(es), 561 fail(s), and 746 exception(s). View

another one which actually save content type settings

Status: Needs review » Needs work

The last submitted patch, 1779486.patch, failed testing.

cosmicdreams’s picture

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.

sun’s picture

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.

julien’s picture

Status: Needs work » Needs review
FileSize
10.81 KB
FAILED: [[SimpleTest]]: [MySQL] 41,231 pass(es), 10 fail(s), and 112 exception(s). View

Status: Needs review » Needs work

The last submitted patch, 1779486.patch, failed testing.

sun’s picture

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.

julien’s picture

Status: Needs review » Needs work

Thanks for the review.

julien’s picture

Status: Needs work » Needs review
FileSize
11.41 KB
FAILED: [[SimpleTest]]: [MySQL] 41,252 pass(es), 2 fail(s), and 0 exception(s). View

Here is the latest one.
Regarding cron_last, i understand but i need more details on how you want to convert it, as it was used with variable_set and when a node get indexed.

  // Save the changed time of the most recent indexed node, for the search
  // results half-life calculation.
  config('node.settings')->set('cron_last', $node->changed)->save()

All the remainings one are done.

julien’s picture

Status: Needs work » Needs review
FileSize
10.48 KB
FAILED: [[SimpleTest]]: [MySQL] 41,243 pass(es), 1 fail(s), and 0 exception(s). View

Oops, forgot something.

Status: Needs review » Needs work

The last submitted patch, 1779486_3.patch, failed testing.

julien’s picture

Status: Needs work » Needs review
FileSize
11.33 KB
PASSED: [[SimpleTest]]: [MySQL] 41,244 pass(es). View

This one is just to be sure that the general settings variables are ok with all the tests.

Status: Needs review » Needs work
Issue tags: -Configuration system

The last submitted patch, 1779486.patch, failed testing.

julien’s picture

Status: Needs work » Needs review
Issue tags: +Configuration system

#21: 1779486.patch queued for re-testing.

julien’s picture

FileSize
14.39 KB
PASSED: [[SimpleTest]]: [MySQL] 41,248 pass(es). View
julien’s picture

FileSize
15.35 KB
PASSED: [[SimpleTest]]: [MySQL] 41,246 pass(es). View
julien’s picture

Status: Needs work » Needs review
FileSize
35.7 KB
FAILED: [[SimpleTest]]: [MySQL] 41,002 pass(es), 55 fail(s), and 6 exception(s). View

Status: Needs review » Needs work

The last submitted patch, 1779486.patch, failed testing.

julien’s picture

Status: Needs work » Needs review
FileSize
35.7 KB
FAILED: [[SimpleTest]]: [MySQL] 41,053 pass(es), 40 fail(s), and 5 exception(s). View

Status: Needs review » Needs work

The last submitted patch, 1779486.patch, failed testing.

julien’s picture

FileSize
40.18 KB
FAILED: [[SimpleTest]]: [MySQL] 41,170 pass(es), 31 fail(s), and 1 exception(s). View

Status: Needs review » Needs work

The last submitted patch, 1779486.patch, failed testing.

julien’s picture

Status: Needs work » Needs review
FileSize
37.24 KB
FAILED: [[SimpleTest]]: [MySQL] 41,258 pass(es), 9 fail(s), and 0 exception(s). View

Status: Needs review » Needs work

The last submitted patch, 1779486.patch, failed testing.

julien’s picture

Status: Needs work » Needs review
FileSize
39.76 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1779486_11.patch. Unable to apply patch. See the log in the details link for more information. View

Status: Needs review » Needs work

The last submitted patch, 1779486.patch, failed testing.

julien’s picture

Status: Needs work » Needs review
FileSize
40.21 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1779486_12.patch. Unable to apply patch. See the log in the details link for more information. View

Status: Needs review » Needs work

The last submitted patch, 1779486.patch, failed testing.

julien’s picture

Status: Needs work » Needs review
FileSize
40.19 KB
PASSED: [[SimpleTest]]: [MySQL] 41,299 pass(es). View

Status: Needs review » Needs work
Issue tags: -Configuration system

The last submitted patch, 1779486_last.patch, failed testing.

julien’s picture

Status: Needs work » Needs review
Issue tags: +Configuration system

#38: 1779486_last.patch queued for re-testing.

julien’s picture

Status: Needs work » Needs review
FileSize
39.31 KB
FAILED: [[SimpleTest]]: [MySQL] 40,871 pass(es), 701 fail(s), and 404 exception(s). View

Status: Needs review » Needs work

The last submitted patch, 1779486_last.patch, failed testing.

julien’s picture

FileSize
39.36 KB
PASSED: [[SimpleTest]]: [MySQL] 41,299 pass(es). View
andypost’s picture

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

julien’s picture

FileSize
39.25 KB
PASSED: [[SimpleTest]]: [MySQL] 41,485 pass(es). View

You're right, the last patch is not up to date as this issue #1175054: Add a storage (API) for persistent non-configuration state have been created after the latest patch.
Looking at the last patch there, i think that maybe more node module related variables could go in the state storage instead, but before that, i'm still looking forward that this issue get resolved first. see #111715: Convert node/content types into configuration

aspilicious’s picture

Status: Needs review » Needs work

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)

pcambra’s picture

Status: Needs work » Postponed
znerol’s picture

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.

aspilicious’s picture

Status: Postponed » Active
alexpott’s picture

Status: Active » Closed (duplicate)
alexpott’s picture

Issue summary: View changes

update issue summary