Support from Acquia helps fund testing for Drupal Acquia logo

Comments

larowlan’s picture

Status: Active » Needs review
FileSize
19.72 KB
19.72 KB

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

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.

gdd’s picture

Status: Needs work » Needs review
FileSize
21.48 KB

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

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

larowlan’s picture

Missed the patches to node/system tests.

gdd’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

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.

gdd’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.

gdd’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.

gdd’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

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

Sorry, forgot some essential fixes.

sun’s picture

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.

gdd’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

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

gdd’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

* 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

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.