Part of #1696224: Remove system_settings_form()
forum_admin_settings uses system_settings_form() and needs to use system_config_form().

Files: 
CommentFileSizeAuthor
#37 drupal8.config-forum-settings.37.patch22.58 KBsun
PASSED: [[SimpleTest]]: [MySQL] 39,750 pass(es). View
#34 1696902_34_forum_cmi.patch21.47 KBtobiasb
FAILED: [[SimpleTest]]: [MySQL] 39,736 pass(es), 1 fail(s), and 0 exception(s). View
#31 1696902_31_forum_cmi.patch20.61 KBcosmicdreams
FAILED: [[SimpleTest]]: [MySQL] 39,750 pass(es), 1 fail(s), and 0 exception(s). View
#29 1696902_29_form_cmi.patch20.67 KBcosmicdreams
FAILED: [[SimpleTest]]: [MySQL] 39,760 pass(es), 1 fail(s), and 0 exception(s). View
#25 drupal8.config-forum-settings.25.patch20.73 KBsun
FAILED: [[SimpleTest]]: [MySQL] 37,226 pass(es), 1 fail(s), and 0 exception(s). View
#25 interdiff.txt595 bytessun
#24 drupal8.config-forum-settings.24.patch20.73 KBsun
FAILED: [[SimpleTest]]: [MySQL] 37,217 pass(es), 1 fail(s), and 0 exception(s). View
#24 interdiff.txt793 bytessun
#23 drupal8.config-forum-settings.23.patch20.78 KBsun
FAILED: [[SimpleTest]]: [MySQL] 37,216 pass(es), 1 fail(s), and 0 exception(s). View
#14 drupal8.config-forum-settings.14.patch20.67 KBsun
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: PHP Fatal error encountered during run_tests.sh. See review log for details.. View
#14 interdiff.txt16.37 KBsun
#11 convert-forum-to-cmi-1696902-11.patch21.81 KBlarowlan
PASSED: [[SimpleTest]]: [MySQL] 37,106 pass(es). View
#10 convert-forum-to-cmi-1696902-10.patch20.34 KBlarowlan
FAILED: [[SimpleTest]]: [MySQL] 37,083 pass(es), 4 fail(s), and 1 exception(s). View
#7 convert-forum-to-cmi-1696902-7.patch21.48 KBheyrocker
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: PHP Fatal error encountered during run_tests.sh. See review log for details.. View
#5 convert-forum-to-cmi-1696902-5.patch21.19 KBlarowlan
FAILED: [[SimpleTest]]: [MySQL] 37,097 pass(es), 1 fail(s), and 0 exception(s). View
#1 convert-forum-to-cmi-1696902-1.patch19.72 KBlarowlan
FAILED: [[SimpleTest]]: [MySQL] 37,068 pass(es), 5 fail(s), and 1 exception(s). View

Comments

larowlan’s picture

Status: Active » Needs review
FileSize
19.72 KB
FAILED: [[SimpleTest]]: [MySQL] 37,068 pass(es), 5 fail(s), and 1 exception(s). View
19.72 KB
FAILED: [[SimpleTest]]: [MySQL] 37,068 pass(es), 5 fail(s), and 1 exception(s). View

note does not convert node_options_forum variable, this one depends on node.module being converted first - added a todo.

larowlan’s picture

sorry mister testbot, network gremlins resulted in same patch attached twice :)

sun’s picture

Title: Convert forum_admin_settings to new configuration system » Convert forum_admin_settings() to new configuration system
Issue tags: +Novice, +Configuration system, +Config novice

Status: Needs review » Needs work

The last submitted patch, convert-forum-to-cmi-1696902-1.patch, failed testing.

larowlan’s picture

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

Updating node and system tests, not sure what's happening with the module api tests but this fixes the path and node access tests.

Status: Needs review » Needs work

The last submitted patch, convert-forum-to-cmi-1696902-5.patch, failed testing.

heyrocker’s picture

Status: Needs work » Needs review
FileSize
21.48 KB
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: PHP Fatal error encountered during run_tests.sh. See review log for details.. View

I spent quite a bit of time looking at this test failure today. The fundamental problem is related to the fact that forum module tracks vid as a unique identifier, but also hardcodes a machine name of 'forum'. The fact that this code works in Drupal 7 is actually a bug, because forum module doesn't clean up the 'forum_nav_vocabulary' after it gets uninstalled, and thus the value is maintained between installations (which forum_enable() expects.)

Ultimately, in Drupal 8, all references to taxonomy vocabularies should be changed to refer to machine names. I assume this will happen in conjunction with vocabularies moving to configuration. In the meantime, I've added another check to guarantee that a second duplicate taxonomy doesn't get created in this situation.

Additionally, I made the block variables nested as 'block.num_new' and 'block.num_active'. I was thinking that the 'block' part might be able to be made slightly more clear, but didn't come up with anything off the top of my head. Suggestions welcome.

Status: Needs review » Needs work

The last submitted patch, convert-forum-to-cmi-1696902-7.patch, failed testing.

larowlan’s picture

+++ b/core/modules/forum/forum.moduleundefined
@@ -659,7 +661,7 @@ function forum_block_info() {
+  $form['block_num_' . $delta] = array('#type' => 'select', '#title' => t('Number of topics'), '#default_value' => config('forum')->get('block_num_' . $delta), '#options' => drupal_map_assoc(array(2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16, 17, 18, 19, 20)));

This should use the block.num instead of block_num

+++ b/core/modules/forum/forum.moduleundefined
@@ -667,7 +669,9 @@ function forum_block_configure($delta = '') {
+  $config->set('block_num_' . $delta, $edit['block_num_' . $delta]);

same again

larowlan’s picture

Status: Needs work » Needs review
FileSize
20.34 KB
FAILED: [[SimpleTest]]: [MySQL] 37,083 pass(es), 4 fail(s), and 1 exception(s). View

Changes block configure/save hooks to use block.num_* format as recommended by heyrocker

larowlan’s picture

FileSize
21.81 KB
PASSED: [[SimpleTest]]: [MySQL] 37,106 pass(es). View

Missed the patches to node/system tests.

heyrocker’s picture

Status: Needs review » Needs work
+++ b/core/modules/forum/config/forum.ymlundefined
@@ -0,0 +1,7 @@
+block.num_active: 5

This should be

block:
  num_new: 5
  num_active: 5

To properly represent the hierarchy.

I think otherwise we are good to go though.

sun’s picture

Issue tags: +API change

All config system conversions are API changes, so tagging as such.

sun’s picture

Status: Needs work » Needs review
FileSize
16.37 KB
20.67 KB
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: PHP Fatal error encountered during run_tests.sh. See review log for details.. View

It's... interesting that #11 was even able to pass tests with that fundamental config file syntax glitch... ;)

Note:

- All config values are casted to strings for storage compatibility. YAML serializes and expects strings by design/default, so the syntax for numbers is a bit different. Essentially, you need to wrap any value that is not a string in single quotes; e.g.:

hot_topic: '15'

- The default settings configuration object for a module (if there are no other default configuration objects) should be $module.settings. I very recently clarified that on http://drupal.org/node/1667896 — FWIW, I also created #1701014: Validate config object names to prevent the suggested (bogus) conversion as in this patch in the future.

- &$form_state needs to be taken by reference in form constructors. Otherwise, a copy of it is passed forward to subsequently invoked functions, such as system_config_form().

- We do not change the form structure of affected settings forms in these conversions, so the array keys always stay the same as before.

- Let's not invent new "patterns" or methods for saving submitted form values in a config/settings form submit handler. The form submit handler also should not check whether the submitted values are different to the existing. Just simply set the submitted form values on the config objects.

And:

- forum.settings:containers is borderline NOT configuration, but content instead. This is so much borderline that I will require and want to see a follow-up [task] issue to exist before this patch can be committed. (Given the overall impact, it looks fine to convert it here, but the value's presence in config has to be discussed.)

Furthermore:

- I'd like 'nav_vocabulary' to be renamed to just 'vocabulary'. I don't see what the "nav" part tries to say or mean.

heyrocker’s picture

I think this is going to fail because this patch is missing the fix from #7, but letting it run anyways.

I agree about 'nav_vocabulary' name change, that makes sense.

Status: Needs review » Needs work

The last submitted patch, drupal8.config-forum-settings.14.patch, failed testing.

sun’s picture

Oh. I'm sorry. I didn't realize that this code stemmed from earlier discussions on a more complex sub-topic.

Let's add an explicit @todo for that change in the forum_enable() code in the next re-roll, so this won't happen again and we know that there's another major glitch involved (to be resolved by #1552396: Convert vocabularies into configuration).

larowlan’s picture

Was not aware of casting/strings issue - thanks for pointing that out.
Missed the reference to .settings for default config - thanks.
Yes, I agree on nav_vocabulary becoming vocabulary.
Does the $form_state reference need backport to D7?
We should clean up the

drupal_map_assoc(array(2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16, 17, 18, 19, 20))

to be

drupal_map_assoc(range(2, 20));

for the sake of readability whilst we're at it.
Will re-roll on Monday unless someone else beats me to it.

sun’s picture

+++ b/core/modules/forum/forum.install
@@ -249,8 +251,8 @@
 function forum_update_8000() {
...
+  config_install_default_config('forum.settings');

Additionally, update_variables_to_config() "installs" (merges) the default configuration of a module already (if appropriate), so this call to config_install_default_config() should be removed.

heyrocker’s picture

update_variables_to_config() only runs in the update function, don't we still need config_install_default_config() for new installs?

sun’s picture

Nope, module_enable() installs the default configuration of a module already.

heyrocker’s picture

Oh right, you would think I would remember that since I wrote the code!

sun’s picture

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

Executed the necessary adjustments.

Three of the settings apply to topics, so I've grouped them.

Also, since each block is its own unit, and all blocks will most likely converted into atomic plugins for D8, I've split the config values for each block into an own section.

Leading to the following result in total:

block:
    active:
        limit: '5'
    new:
        limit: '5'
containers: []
topics:
    hot_threshold: '15'
    order: '1'
    page_limit: '25'
vocabulary: '0'

No other changes. (i.e., in-patch renames only)

sun’s picture

FileSize
793 bytes
20.73 KB
FAILED: [[SimpleTest]]: [MySQL] 37,217 pass(es), 1 fail(s), and 0 exception(s). View

Sorry, forgot some essential fixes.

sun’s picture

FileSize
595 bytes
20.73 KB
FAILED: [[SimpleTest]]: [MySQL] 37,226 pass(es), 1 fail(s), and 0 exception(s). View

Sorry, that wasn't complete either. This one is.

sun’s picture

Status: Needs review » Needs work
+++ b/core/modules/forum/config/forum.settings.yml
@@ -0,0 +1,11 @@
+vocabulary: forums

+++ b/core/modules/forum/forum.admin.inc
@@ -71,7 +71,7 @@ function forum_form_forum($form, &$form_state, $edit = array()) {
+  $form['vid'] = array('#type' => 'hidden', '#value' => config('forum.settings')->get('vocabulary'));

@@ -181,7 +183,7 @@ function forum_form_container($form, &$form_state, $edit = array()) {
   $form['vid'] = array(
...
+    '#value' => $config->get('vocabulary'),

@@ -271,8 +286,9 @@ function forum_admin_settings($form) {
+  $vid = $config->get('vocabulary');
   $vocabulary = taxonomy_vocabulary_load($vid);

@@ -326,7 +342,7 @@ function _forum_parent_select($tid, $title, $child_type) {
+  $vid = config('forum.settings')->get('vocabulary');
   $children = taxonomy_get_tree($vid, $tid);

+++ b/core/modules/forum/forum.install
@@ -28,7 +30,8 @@ function forum_enable() {
+  $vocabulary = taxonomy_vocabulary_machine_name_load($config->get('vocabulary'));

+++ b/core/modules/forum/forum.module
@@ -219,7 +219,7 @@ function forum_menu_local_tasks_alter(&$data, $router_item, $root_path) {
+  if ($vid = config('forum.settings')->get('vocabulary')) {

@@ -264,7 +264,7 @@ function _forum_node_check_node_type(Node $node) {
+  $vid = config('forum.settings')->get('vocabulary');

@@ -593,7 +595,7 @@ function forum_field_storage_pre_update($entity_type, $entity, &$skip_fields) {
+  $vid = config('forum.settings')->get('vocabulary');

@@ -612,7 +614,7 @@ function forum_form_taxonomy_form_vocabulary_alter(&$form, &$form_state, $form_i
+  $vid = config('forum.settings')->get('vocabulary');

@@ -768,7 +773,8 @@ function forum_forum_load($tid = NULL) {
+  $vid = $config->get('vocabulary');

@@ -1059,7 +1065,8 @@ function forum_preprocess_block(&$variables) {
+  $vid = $config->get('vocabulary');
   $vocabulary = taxonomy_vocabulary_load($vid);

+++ b/core/modules/forum/lib/Drupal/forum/Tests/ForumTest.php
@@ -215,7 +215,7 @@ class ForumTest extends WebTestBase {
+    $vid = config('forum.settings')->get('vocabulary');
     $tree = taxonomy_get_tree($vid);

@@ -329,7 +329,7 @@ class ForumTest extends WebTestBase {
+    $vid = config('forum.settings')->get('vocabulary');
     $original_settings = taxonomy_vocabulary_load($vid);

+++ b/core/modules/node/lib/Drupal/node/Tests/NodeAccessPagerTest.php
@@ -65,7 +65,7 @@ class NodeAccessPagerTest extends WebTestBase {
+    $vid = config('forum.settings')->get('vocabulary', 0);
     $this->assertTrue($vid, t('Forum navigation vocabulary found.'));

+++ b/core/modules/system/lib/Drupal/system/Tests/Path/UrlAlterFunctionalTest.php
@@ -63,7 +63,7 @@ class UrlAlterFunctionalTest extends WebTestBase {
+    $forum_vid = config('forum.settings')->get('vocabulary');
     $tid = db_insert('taxonomy_term_data')

ok, obviously, it's too late here and I'm wrong. :-/

The configuration should refer a machine name (since a vid means the world and nothing in terms of staging), but implementing that change to its full extent will require massive changes down the line.

So I guess... let's go with the vid for now.

aspilicious’s picture

+++ b/core/modules/node/lib/Drupal/node/Tests/NodeAccessPagerTest.phpundefined
@@ -65,7 +65,7 @@ class NodeAccessPagerTest extends WebTestBase {
+    $vid = config('forum.settings')->get('vocabulary', 0);

Get doesn't have 2 arguments...

+++ b/core/modules/node/lib/Drupal/node/Tests/NodeAccessPagerTest.phpundefined
@@ -65,7 +65,7 @@ class NodeAccessPagerTest extends WebTestBase {
+    $vid = config('forum.settings')->get('vocabulary', 0);

Is this correct? Can we pass a second argument to ->get()

-27 days to next Drupal core point release.

heyrocker’s picture

No that is incorrect, config->get() does not take the default value like variable_get() does

cosmicdreams’s picture

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

Rerolled #25

fixed the typo shown in #26

migrated a few lines to using the fluid syntax.

Status: Needs review » Needs work

The last submitted patch, 1696902_29_form_cmi.patch, failed testing.

cosmicdreams’s picture

Status: Needs work » Needs review
FileSize
20.61 KB
FAILED: [[SimpleTest]]: [MySQL] 39,750 pass(es), 1 fail(s), and 0 exception(s). View
+++ b/core/modules/forum/forum.admin.incundefined
@@ -117,8 +118,8 @@ function forum_form_submit($form, &$form_state) {
-  return;

Why do we omit this return?

I think it might be what is failing the patch.

Also here's a revision of the previous patch that moves the save function to where the setting is saved in forum_form_submit.
Hopefully doing this fixes the fail, but I suspect it won't. If it doesn't I will even more suspicious that the lack of a return here is the culprit.

heyrocker’s picture

That return should be completely irrelevant. There is no difference functionally from returning and just falling out of the function.

Status: Needs review » Needs work

The last submitted patch, 1696902_31_forum_cmi.patch, failed testing.

tobiasb’s picture

Assigned: larowlan » Unassigned
Status: Needs work » Needs review
FileSize
21.47 KB
FAILED: [[SimpleTest]]: [MySQL] 39,736 pass(es), 1 fail(s), and 0 exception(s). View

* Uses taxonomy_vocabulary_load() again in forum_enable().

Status: Needs review » Needs work

The last submitted patch, 1696902_34_forum_cmi.patch, failed testing.

sun’s picture

ModuleApiTest: line 239:

    // Enable forum module again, which should enable both the poll module and
    // php module. But, this time do it with poll module declaring a dependency
    // on a specific version of php module in its info file. Make sure that
    // module_enable() still works.
    variable_set('dependency_test', 'version dependency');
    drupal_static_reset('system_rebuild_module_data');
    $result = module_enable(array('forum'));                  // <-----
    $this->assertTrue($result, t('module_enable() returns the correct value.'));
Integrity constraint violation: Duplicate entry 'forums' for key 'machine_name':

INSERT INTO {taxonomy_vocabulary} (vid, langcode, name, machine_name, description, hierarchy, weight) VALUES (default, ....); Array
(
    [:db_insert_placeholder_0] => en
    [:db_insert_placeholder_1] => Forums
    [:db_insert_placeholder_2] => forums
    [:db_insert_placeholder_3] => Forum navigation vocabulary
    [:db_insert_placeholder_4] => 1
    [:db_insert_placeholder_5] => -10
)
sun’s picture

Status: Needs work » Needs review
FileSize
22.58 KB
PASSED: [[SimpleTest]]: [MySQL] 39,750 pass(es). View

Re-applying @heyrocker's fix from #7 for forum_install().

aspilicious’s picture

Status: Needs review » Reviewed & tested by the community

Rdy to fly I think!

Dries’s picture

Status: Reviewed & tested by the community » Fixed

Another great CMI conversion. Very happy to see that momentum. Committed the patch to 8.x - including the new yml file (schrug). Thanks all!

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