Conversion was done in #731724: Convert comment settings into a field to make them work with CMI and non-node entities

The comment settings need to be converted to use the new configuration system. I think all these are found in comment_form_node_type_form_alter() and used throughout comment module for node-type-specific settings. These are comment module settings, and thus I think they should live in comment module and their config should be in comment.* files, however they are inextricably tied to node types/bundles and thus need to be accessible by that system and acknowledge its naming conventions. We may need to wait for the field system conversion to get a little farther before tackling this, but I wanted an issue open for it.

Files: 

Comments

Status:Active» Needs review
StatusFileSize
new3.71 KB
FAILED: [[SimpleTest]]: [MySQL] 41,198 pass(es), 22 fail(s), and 8 exception(s).
[ View ]

made a patch for this, but it needs work. I don't think that using dynamic variable names can be use with the yml file, i'm more thinking to use the db if it's dynamic.

Status:Needs review» Needs work

The last submitted patch, drupal8-Convert-comment-module-configuration-to-cmi-1776076.patch, failed testing.

Status:Needs work» Needs review

What will probably need to happen is that the YAML file names will be dynamic based on the node type, but the data inside them will be the same in each one. However, I also think this needs some feedback from the Field API conversion folks (swentel or yched) before we go on, to make sure we are in line with their plans.

If you are interested in other CMI conversion, I recommend you read http://drupal.org/node/1667896 to give you an overview of how this works. http://heyrocker.com/how-use-drupal-8-configuration-system is also a useful reference. The architecture data is somewhat out of date, but the implementation and API sections are still completely valid.

Status:Needs review» Needs work

Status:Needs work» Needs review
StatusFileSize
new3.82 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch drupal8-Convert-comment-module-configuration-to-cmi-1776076_0.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

yes i need to readup about dynamic yml files, maybe if someone figured it out already, he will be able to help. Just for now, i made .module changes and a first yml static file (that needs to be made dynamic).

Status:Needs review» Needs work

The last submitted patch, drupal8-Convert-comment-module-configuration-to-cmi-1776076.patch, failed testing.

As discussed in #irc, I'm willing to have a crack at converting comment to a field, even though it was described as 'a bit nutty'.

that's your opinion but i did used the documents on #3
it works pretty well actually.
but yeah go nuts, convert the all module to field.

Status:Needs work» Needs review
StatusFileSize
new9.87 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch drupal8-Convert-comment-module-configuration-to-cmi-1776076_1.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

as discussed on irc earlier, i did use the docs, and it's ok, it need refactoring to manage several content type, but it works as a proof of concept on one, article.

Status:Needs review» Needs work

The last submitted patch, drupal8-Convert-comment-module-configuration-to-cmi-1776076.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new9.82 KB
PASSED: [[SimpleTest]]: [MySQL] 41,234 pass(es).
[ View ]

retrying, bad git diff

StatusFileSize
new9.96 KB
PASSED: [[SimpleTest]]: [MySQL] 41,234 pass(es).
[ View ]

fixing the ugly scary query syntax

StatusFileSize
new9.96 KB
PASSED: [[SimpleTest]]: [MySQL] 41,231 pass(es).
[ View ]

StatusFileSize
new7.91 KB
FAILED: [[SimpleTest]]: [MySQL] 41,229 pass(es), 0 fail(s), and 14 exception(s).
[ View ]

Thanks for the conversion! Two coding style problems: the default_values need to be broken out into variables or other ways of multiple lines , this is way, way too long to be nicely readable. Yes, the unset in form_state_values_clean is very long too but that doesn't contain complicated logic, just a simple list of things. Also comment_submit_config needs doxygen. Thanks again.

Status:Needs review» Needs work

The last submitted patch, drupal8-Convert-comment-module-configuration-to-cmi-1776076.patch, failed testing.

i'm on it. i also need to make the content types dynamic and also make it work on the node edit form. after that i think it will pretty much near alpha :)

StatusFileSize
new11.18 KB
PASSED: [[SimpleTest]]: [MySQL] 41,235 pass(es).
[ View ]

so here is one who manage the two default content type, doc added and line cleanup

Status:Needs work» Needs review

+function comment_node_submit_config($form, &$form_state) {
+  $config = config('comment.settings');
+  $i = 0;
+  // Remove internal Form API values to get
+  // the values we need at the end of the form_state array.
+  unset($form_state['values']['form_id'], $form_state['values']['form_token']);
+  unset($form_state['values']['form_build_id'], $form_state['values']['op']);
+  unset($form_state['values']['menu_parent'], $form_state['values']['menu_options']);
+  $valkey = array_reverse($form_state['values']);
+  foreach ($valkey as $key => $val) {
+    if ($i < 7) {
+      $value = db_query('SELECT value FROM {variable} WHERE name = :name', array(':name' => $key . '_' . $form['#node_type']->type))->fetchField();
+      $config->set($key . '_' . $form['#node_type']->type, $value);
+      db_delete('variable')->condition('name', $key . '_' . $form['#node_type']->type)->execute();
+      $i++;
+    }
   }
+  $config->save();
+  watchdog('comment', 'CMI configuration variables updated for node type %type', array('%type' => $form['#node_type']->type));

This is very very confusing. The three lines with db_query, $config->set(), db_delete() belongs to comment_update_N.

This function should do a number of $config->set() commands based on form values. See http://api.drupal.org/api/drupal/core%21modules%21aggregator%21aggregato... as an example.

you're actually right, as we discuss on irc, i've been tricked by the doc on #3 or how to use the d8 cmi system step 5.
it's not used anyway. but yeah if we could have some feedback that at least the two conf for the two default content type work on the node type form, that will be cool

Really, I do not understand any of this unset, array_reverse, loop etc that is happening there. Both the article you are looking at, at Step 2 gives you the structure of the submit handler and the example link I gave you gives it: your submit handler should contain 1 line of $config = config('something'); N lines of $config->set() and 1 line of $config->save() and nothing else. The reason this is not automated because sometimes those $config->set() need type casting -- the config API can save integers for example while the user submitted input will be string.

StatusFileSize
new9 KB
FAILED: [[SimpleTest]]: [MySQL] 41,226 pass(es), 6 fail(s), and 0 exception(s).
[ View ]

remove the variable db queries

Status:Needs review» Needs work

The last submitted patch, drupal8-Convert-comment-module-configuration-to-cmi-1776076.patch, failed testing.

Please see #22, thanks.

yes, i'm checking it, thanks for the review. i also saw that i don't have to use this function:
http://api.drupal.org/api/drupal/core%21lib%21Drupal%21Core%21Config%21C...

The reason this is not automated because sometimes those $config->set() need type casting -- the config API can save integers for example while the user submitted input will be string.

that's weird, it's not what i understand from the api castvalue function which is only call from set()

The configuration system only saves strings or arrays. Any scalar non-string value is cast to a string.

anyway, regarding coding N lines of ->set like in the example is not very flexible to me, if you change the name of fields in the form later on or the number of fields, for example, with the code i did, you only have to increment the 7 integer variable (which must be a sum of the form fields really.), also the fields name are taken from the form.
hope that make sense.

While doing that sort of future proofing seems prudent at first it makes the submit function extremely hard to read and so it does not really worth it.

The castvalue in CMI is about to removed AFAIK so that we can actually use different data types.

Status:Needs work» Needs review
StatusFileSize
new8.28 KB
FAILED: [[SimpleTest]]: [MySQL] 41,240 pass(es), 7 fail(s), and 0 exception(s).
[ View ]

allrighty, here is the new patch with the requested changes :)

Status:Needs review» Needs work

The last submitted patch, drupal8-Convert-comment-module-configuration-to-cmi-1776076.patch, failed testing.

White reviewing #28 patch, I was struck by how many times long string xyz was repeated in each set function in the format of xyz . custom . xyx For the sake of code readability, does it make sense to a helper function that does essentially(xyz + custom + xyz)?

I am not sure if this would impact performance, but it sure would make identifying potential problems during patch reviews easier!!

Cheers, Lars

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

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

The last submitted patch, drupal8-Convert-comment-module-configuration-to-cmi-1776076.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new6.27 KB
FAILED: [[SimpleTest]]: [MySQL] 41,244 pass(es), 6 fail(s), and 0 exception(s).
[ View ]

removed useless code due to misunderstanding of doc after reading again.

Status:Needs review» Needs work

The last submitted patch, drupal8-Convert-comment-module-configuration-to-cmi-1776076.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new17.85 KB
FAILED: [[SimpleTest]]: [MySQL] 41,129 pass(es), 141 fail(s), and 20 exception(s).
[ View ]

added forum default conf variables, and changed variable_get and variable_set. still need work to pass all the other module tests i think.

StatusFileSize
new17.34 KB
FAILED: [[SimpleTest]]: [MySQL] 41,143 pass(es), 113 fail(s), and 3 exception(s).
[ View ]

Status:Needs review» Needs work

The last submitted patch, drupal8-Convert-comment-module-configuration-to-cmi-1776076.patch, failed testing.

StatusFileSize
new17.88 KB
FAILED: [[SimpleTest]]: [MySQL] 41,132 pass(es), 112 fail(s), and 3 exception(s).
[ View ]

callback fix

Status:Needs work» Needs review

Status:Needs review» Needs work

The last submitted patch, drupal8-Convert-comment-module-configuration-to-cmi-1776076.patch, failed testing.

You are making fantastic work on a hard problem. It doesn't end here, alas.

In comment_form_node_type_form_alter please change config('comment.settings')->get($form['#node_type']->type . '.comment') into $config->get('comment') and put $config = config('comment.settings.' . $form['#node_type']->type); to the top of the function. Adjust other calls as necessary.

For example: config('comment.settings')->get($node->type . '.comment_default_mode') becomes config("comment.settings.$node->type")->get('comment_default_mode').

Please ship with core/modules/comment/config/comment.settings.page.yml, core/modules/comment/config/comment.settings.article.yml, core/modules/comment/config/comment.settings.forum.yml instead of core/modules/comment/config/comment.settings.yml. We have discussed this on IRC and I showed you http://privatepaste.com/187c0faa10 this as an example.

Also in general you have function_call() ? function_call() : DEFAULT_VALUE then you should save the results of function_call() into a variable. But, in this case, the terniary is wrong for two reasons. One, config()->get() returns NULL in case of not found, so you should be testing for that instead of just anything empty (which can be 0, a valid value). Ie. $foo = $config->get('something'); $bar = isset($foo) ? $foo : DEFAULT_VALUE;. Two, as we already ship with comment.settings.page.yml etc, it is not possible to config()->get() to return a NULL. You need to react to hook_node_type_insert and save a default comment settings for this node type instead of trying to determine them run-time.

Status:Needs work» Needs review
StatusFileSize
new20.98 KB
FAILED: [[SimpleTest]]: [MySQL] 40,780 pass(es), 702 fail(s), and 412 exception(s).
[ View ]

thanks for this detailled review. that definitely help to get it right.
First i agree that the way i calling the conf object was not right, i see that it was buggy on block_save. So i changed it as it should be like :

$config = config('comment.settings');
$config->set('key', 'value');
$config->save();

instead of this, that was not setting right on block_save. calling twice config('comment.settings') doesn't contain the same data. i was only doing this to avoid to have this line on top of every function ($config = config('comment.settings'))
config('comment.settings')->set('key', 'value');
config('comment.settings')->save();

i changed the conditional logic as you suggested, here it's just pure php missknowledge. thanks :)
I did create the other .yml file as suggested, but i think it is not necessary to have one per content type, as there is default value in the form now, when a new content type is created to actually store the data.

Status:Needs review» Needs work

The last submitted patch, drupal8-convert-comment-module-configuration-to-cmi-1776076.patch, failed testing.

Status:Needs work» Needs review

A few notes to the already very nicely shaping work.

While indeed the node type form is altered, you still should implement that hook because it is possible someone calls node_type_save to create a node type manually.

Also, contrary to my previous comment, I agree with how the default values are handled, because it is possible that the node type is just being created so you do need that in comment_form_node_type_form_alter().

In comment_node_view, it's enough to load $config once. On the other hand, in comment_node_prepare and similar, if config is used just once then it is not necessary to create a $config variable . The point is to call config() once per function call, if at all possible. Edit: And if it is called more than once then put the results into a variable.

In comment_add you have added some line end whitespace, please remove.

In the new yml files, please remove the top keys, they are not necessary. I see this is my fault as this was erroneus in the pastebin I showed to you, sorry about that. So instead of

+++ b/core/modules/comment/config/comment.settings.page.yml
@@ -0,0 +1,9 @@
+page:
+ comment: '2'

just

+++ b/core/modules/comment/config/comment.settings.page.yml
@@ -0,0 +1,9 @@
+comment: '2'

because your code now uses ->get('comment') and not ->get('page.comment');

Thanks again!

StatusFileSize
new19.78 KB
FAILED: [[SimpleTest]]: [MySQL] 40,902 pass(es), 676 fail(s), and 296 exception(s).
[ View ]

retry with yml changes

Status:Needs review» Needs work

The last submitted patch, drupal8-convert-comment-module-configuration-to-cmi-1776076.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new19.78 KB
FAILED: [[SimpleTest]]: [MySQL] 41,091 pass(es), 157 fail(s), and 575 exception(s).
[ View ]

typo

StatusFileSize
new19.74 KB
FAILED: [[SimpleTest]]: [MySQL] 41,125 pass(es), 139 fail(s), and 20 exception(s).
[ View ]

Status:Needs review» Needs work

The last submitted patch, drupal8-convert-comment-module-configuration-to-cmi-1776076.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new20.63 KB
FAILED: [[SimpleTest]]: [MySQL] 41,131 pass(es), 135 fail(s), and 18 exception(s).
[ View ]

added other default yml file for other core content type, to help tests runs.

Status:Needs review» Needs work

The last submitted patch, drupal8-convert-comment-module-configuration-to-cmi-1776076.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new25.11 KB
FAILED: [[SimpleTest]]: [MySQL] Invalid PHP syntax in core/modules/comment/lib/Drupal/comment/CommentFormController.php.
[ View ]

comment test update

Status:Needs review» Needs work

The last submitted patch, drupal8-convert-comment-module-configuration-to-cmi-1776076.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new25.27 KB
FAILED: [[SimpleTest]]: [MySQL] Invalid PHP syntax in core/modules/comment/lib/Drupal/comment/CommentFormController.php.
[ View ]

StatusFileSize
new25.27 KB
FAILED: [[SimpleTest]]: [MySQL] 41,129 pass(es), 170 fail(s), and 70 exception(s).
[ View ]

StatusFileSize
new25.81 KB
FAILED: [[SimpleTest]]: [MySQL] 41,129 pass(es), 170 fail(s), and 70 exception(s).
[ View ]

Status:Needs review» Needs work

The last submitted patch, drupal8-convert-comment-module-configuration-to-cmi-1776076.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new25.99 KB
FAILED: [[SimpleTest]]: [MySQL] 41,036 pass(es), 140 fail(s), and 44 exception(s).
[ View ]

Status:Needs review» Needs work

The last submitted patch, drupal8-convert-comment-module-configuration-to-cmi-1776076.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new26.39 KB
FAILED: [[SimpleTest]]: [MySQL] 41,224 pass(es), 49 fail(s), and 22 exception(s).
[ View ]

Status:Needs review» Needs work

The last submitted patch, drupal8-convert-comment-module-configuration-to-cmi-1776076.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new26.59 KB
FAILED: [[SimpleTest]]: [MySQL] 41,221 pass(es), 48 fail(s), and 22 exception(s).
[ View ]

sorry to flood with that amount of patches, but it's quicker for me to go through all the tests there, rather than locally, which is way too slow on my machine yet. i think the fact that the config->get return a string instead of variable_get was return an integer can be the cause of those tests fails.

The last submitted patch, drupal8-convert-comment-module-configuration-to-cmi-1776076.patch, failed testing.

StatusFileSize
new29.56 KB
FAILED: [[SimpleTest]]: [MySQL] 41,247 pass(es), 14 fail(s), and 4 exception(s).
[ View ]

added update_hook for D7 to D8

Status:Needs review» Needs work

Failing patches are no problem, I recommend picking one test and run that locally and making sure it passes before uploading a new patch. Likely you have very few actual problems left so fixing one test very likely will fix others too. Thanks for getting this done.

One the patch in general is missing is an update hook. Just call update_variables_to_config with the array of variables necessary it'll do everything for you.

the update_variables_to_config is already in this patch on top.
did run some last tests locally (only a very few), and that's seems to be ok. let's wait for the testbot results.

Status:Needs work» Needs review

Opsie, crosspost.

Status:Needs review» Needs work

The last submitted patch, drupal8-Convert-comment-module-configuration-to-cmi-1776076.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new29.56 KB
FAILED: [[SimpleTest]]: [MySQL] 41,249 pass(es), 14 fail(s), and 4 exception(s).
[ View ]

removed all the intval

Status:Needs review» Needs work

The last submitted patch, drupal8-Convert-comment-module-configuration-to-cmi-1776076.patch, failed testing.

The update function should iterate the node types returned by _update_7000_node_get_types().

Status:Needs work» Needs review
StatusFileSize
new29.34 KB
FAILED: [[SimpleTest]]: [MySQL] 41,251 pass(es), 1 fail(s), and 0 exception(s).
[ View ]

one more to try a green flag, changing as 71

StatusFileSize
new27.61 KB
FAILED: [[SimpleTest]]: [MySQL] 41,163 pass(es), 21 fail(s), and 0 exception(s).
[ View ]

StatusFileSize
new27.95 KB
FAILED: [[SimpleTest]]: [MySQL] 41,175 pass(es), 17 fail(s), and 0 exception(s).
[ View ]

Fix for pager test

StatusFileSize
new28.37 KB
FAILED: [[SimpleTest]]: [MySQL] 41,185 pass(es), 14 fail(s), and 0 exception(s).
[ View ]

added the blog yml file.

StatusFileSize
new30.1 KB
PASSED: [[SimpleTest]]: [MySQL] 41,260 pass(es).
[ View ]

need to test the code cleanup now.

StatusFileSize
new26.32 KB
FAILED: [[SimpleTest]]: [MySQL] 40,700 pass(es), 548 fail(s), and 867 exception(s).
[ View ]

Status:Needs review» Needs work

Status:Needs work» Needs review
StatusFileSize
new26.43 KB
FAILED: [[SimpleTest]]: [MySQL] 39,551 pass(es), 495 fail(s), and 768 exception(s).
[ View ]

another one, i'm not sure if the tests are actually ok to achieve this yet.

StatusFileSize
new51.18 KB
FAILED: [[SimpleTest]]: [MySQL] 40,466 pass(es), 797 fail(s), and 1,085 exception(s).
[ View ]

StatusFileSize
new27.87 KB
FAILED: [[SimpleTest]]: [MySQL] 40,484 pass(es), 790 fail(s), and 992 exception(s).
[ View ]

sorry wrong one

Status:Needs review» Needs work

Status:Needs work» Needs review
StatusFileSize
new27.82 KB
FAILED: [[SimpleTest]]: [MySQL] 40,682 pass(es), 547 fail(s), and 862 exception(s).
[ View ]

Status:Needs review» Needs work

Status:Needs work» Needs review
StatusFileSize
new27.86 KB
FAILED: [[SimpleTest]]: [MySQL] 40,701 pass(es), 546 fail(s), and 865 exception(s).
[ View ]

StatusFileSize
new30.73 KB
PASSED: [[SimpleTest]]: [MySQL] 41,264 pass(es).
[ View ]

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

The last submitted patch, drupal8-Convert-comment-module-configuration-to-cmi-1776076.patch, failed testing.

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

StatusFileSize
new30.65 KB
PASSED: [[SimpleTest]]: [MySQL] 41,254 pass(es).
[ View ]

Status:Needs review» Reviewed & tested by the community
StatusFileSize
new5.88 KB
new29.95 KB
PASSED: [[SimpleTest]]: [MySQL] 41,248 pass(es).
[ View ]

Congratulations! I think this is ready with some minimal tidying, which I did. Mostly no function change at all, the only function change is that uninstall was doing nothing: just calling clear on a config object without save does not persist the removal of that key. But that's the smaller problem, the bigger problem is that we do not want to unset specific keys, we want the whole config object to be removed so changed to delete().

The formatting change in comment_block_save might feel arbitrary because I did leave in another comment->set->save chain elsewhere on a single line; this one simply felt too long.

The only question I have, do we want a test for this update function, in this issue, or a followup?

Status:Reviewed & tested by the community» Needs work

Sorry, on a second look the new update needs to be moved into comment_update_8001. About testing, the problem is that creating a database dump for every variation of variables updated in comment_update_8001 is a huge pita, so I think let's skip that.

Status:Needs work» Needs review
StatusFileSize
new30.59 KB
PASSED: [[SimpleTest]]: [MySQL] 41,260 pass(es).
[ View ]

Status:Needs review» Reviewed & tested by the community
StatusFileSize
new2.79 KB
new30.01 KB
PASSED: [[SimpleTest]]: [MySQL] 41,250 pass(es).
[ View ]

OK, half-my bad because I didnt verify whether 8001 already existed. I simply thought that you patched the only update function existing. The point is that you should never, ever change an update function, that's bad. Also #94 reverted some of the change I made, so I rerolled #91 with the new update function.

Sorry, on a second look the new update needs to be moved into comment_update_8001.

ok i misunderstood then.

thanks for your patience for reviewing and helping me to get it right. really appreciate it. did learn heaps on that one. cheers!

Yes, I should have written "into a new comment_update_N".

that's ok, we just got back on this today, warm up one.

Status:Reviewed & tested by the community» Needs work
Issue tags:+Configurables

Wow, I somehow completely missed this issue. :-/

There have been a lot of architectural discussions around the particular topic of "dependent/enhancing module config for another module's config" (sorry, despite the discussions, there's no proper term for them yet).

This issue/patch here attempts to not only convert the Comment module (global/static) settings, but also the dynamic configuration for each node type. The former is straight-forward to do, but the latter is a very complex topic of its own.

The consensus that was formed thus far foresees that Comment module's settings for each node type would live within the node type config — or would be formally "attached" to it. In any case, Comment module's node type settings have to be Configurables/ConfigEntities, since they need to be handled appropriately when importing configuration. Unfortunately, the current patch here is doing something different...

I'm fully aware that this probably sounds demotivating to you :(, but I think we need to handle the dynamic/attached config in a separate and dedicated issue. I know it's tough, but hope you can understand the reasoning.

This will allow us to focus on the bare Comment module settings in this issue.

With regard to those, I have a couple of remarks on these changes:

1) The settings for the block should live in a dedicated section for the block within the comment.settings object. The pattern is block.[name].[setting], and you can find a couple of examples in HEAD for that already; e.g., pasting from forum.settings:

block:
  active:
    limit: '5'
  new:
    limit: '5'

2) We should tweak the comment setting names to follow the guidelines for converting variables to config; see the handbook page How to upgrade variables into configuration — specifically, the keys in comment.settings should not repeat the term "comment", and they should be as self-descriptive as possible.

3) Does 'node_cron_comments_scale' really belong into comment.settings?

i think we need a default yml file for content type specific config, that will be use to store all the core module content type variable, and that can also be used in the patched update_variables_to_config function when no yml file exist during upgrade of existing content.

Status:Needs work» Needs review
StatusFileSize
new36.14 KB
FAILED: [[SimpleTest]]: [MySQL] 40,699 pass(es), 527 fail(s), and 891 exception(s).
[ View ]

Did applied 2 and 3, to follow the guidelines properly. For 3, it actually does, i think mixing the word cron and comment in the same variable makes finding to which module it belongs confusing.

Status:Needs review» Needs work

StatusFileSize
new38.73 KB
PASSED: [[SimpleTest]]: [MySQL] 41,263 pass(es).
[ View ]

forgot the yml files.

Status:Needs work» Needs review

The consensus that was formed thus far foresees that Comment module's settings for each node type would live within the node type config — or would be formally "attached" to it.

I don't believe we ever talked about including config from module x into the config for module y. I'm kind of against that plan. Over in #1739928-100: Generalize language configuration on content types to apply to terms and other entities I suggested a convention like this:

entity_type.bundle.yml
entity_type.bundle.comment.settings.yml

Which I think makes a lot of sense.

In any case, Comment module's node type settings have to be Configurables/ConfigEntities, since they need to be handled appropriately when importing configuration.

Is that really true? If we have a naming convention as above, then the bundle ConfigEntity can easily grab and deal with the bundle-specific comment settings, and the global ones are just plain data. I dont think the bundle-specific comment settings have any use on their own, or any context in which to live outside of being part of a bundle.

I've been thinking about the best way to achieve this also. I'm not sure if each module which have content type or entity type specific variables should have his own config file like "entity_type.bundle.comment.settings.yml". looking at the number of contrib modules who will maybe need to do this, we will end up with lots of files. As for the upgrade to D8, we actually don't know how many content type there is, and how many variables attached to it exists, so it's difficult to me, to start with a static file structure. it has to be dynamic.
On node.module for example, i finally did manage to avoid have that kind of file. It just store them in config('node.type.settings').('typename.variable') instead, but there is actually no yml file for node.type.settings.
If every module, could store his content type variable in a general store like this, it will be way cleaner.
for example i store, config('node.type.settings').('page.submitted'), but we could imagine that the comment module will store, config('node.type.settings').('page.comment.anonymous') for example.
If you look at the content type form submit in content.type.inc, to store those values, it loops through the content type values, comment values, menu values of the form. If all those content type values goes in the same content type store, that will be handy.
http://drupal.org/node/1779486#comment-6476316

Status:Needs review» Postponed

I believe this should be postponed pending #731724: Convert comment settings into a field to make them work with CMI and non-node entities which will move most of the comment config to field/instance settings.
I've discussed this in irc and believe that with feature freeze looming, efforts would be better spent in the decouple issue.
If #731724: Convert comment settings into a field to make them work with CMI and non-node entities doesn't get in before feature freeze, we'll come back to this issue.
@julien we'd love your help in that issue - if you're interested

Issue summary:View changes

Updated issue summary.

Issue summary:View changes
Status:Postponed» Closed (duplicate)

Issue summary:View changes