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.

CommentFileSizeAuthor
#104 drupal8-Convert-comment-module-configuration-to-cmi-1776076_24.patch38.73 KBjulien
#102 drupal8-Convert-comment-module-configuration-to-cmi-1776076_24.patch36.14 KBjulien
#95 1776076_95.patch30.01 KBchx
#95 interdiff.txt2.79 KBchx
#94 drupal8-Convert-comment-module-configuration-to-cmi-1776076.patch30.59 KBjulien
#91 1776076_91.patch29.95 KBchx
#91 interdiff.txt5.88 KBchx
#90 drupal8-Convert-comment-module-configuration-to-cmi-1776076.patch30.65 KBjulien
#87 drupal8-Convert-comment-module-configuration-to-cmi-1776076.patch30.73 KBjulien
#86 drupal8-Convert-comment-module-configuration-to-cmi-1776076.patch27.86 KBjulien
#84 drupal8-Convert-comment-module-configuration-to-cmi-1776076_19.patch27.82 KBjulien
#82 drupal8-Convert-comment-module-configuration-to-cmi-1776076_19.patch27.87 KBjulien
#81 drupal8-Convert-comment-module-configuration-to-cmi-1776076_19.patch51.18 KBjulien
#80 drupal8-Convert-comment-module-configuration-to-cmi-1776076_19.patch26.43 KBjulien
#78 drupal8-Convert-comment-module-configuration-to-cmi-1776076_19.patch26.32 KBjulien
#76 drupal8-Convert-comment-module-configuration-to-cmi-1776076.patch30.1 KBjulien
#75 drupal8-Convert-comment-module-configuration-to-cmi-1776076.patch28.37 KBjulien
#74 drupal8-Convert-comment-module-configuration-to-cmi-1776076.patch27.95 KBjulien
#73 drupal8-Convert-comment-module-configuration-to-cmi-1776076.patch27.61 KBjulien
#72 drupal8-Convert-comment-module-configuration-to-cmi-1776076.patch29.34 KBjulien
#69 drupal8-Convert-comment-module-configuration-to-cmi-1776076.patch29.56 KBjulien
#64 drupal8-Convert-comment-module-configuration-to-cmi-1776076.patch29.56 KBjulien
#62 drupal8-convert-comment-module-configuration-to-cmi-1776076.patch26.59 KBjulien
#60 drupal8-convert-comment-module-configuration-to-cmi-1776076.patch26.39 KBjulien
#58 drupal8-convert-comment-module-configuration-to-cmi-1776076.patch25.99 KBjulien
#56 drupal8-convert-comment-module-configuration-to-cmi-1776076.patch25.81 KBjulien
#55 drupal8-convert-comment-module-configuration-to-cmi-1776076.patch25.27 KBjulien
#54 drupal8-convert-comment-module-configuration-to-cmi-1776076.patch25.27 KBjulien
#52 drupal8-convert-comment-module-configuration-to-cmi-1776076.patch25.11 KBjulien
#50 drupal8-convert-comment-module-configuration-to-cmi-1776076.patch20.63 KBjulien
#48 drupal8-convert-comment-module-configuration-to-cmi-1776076.patch19.74 KBjulien
#47 drupal8-convert-comment-module-configuration-to-cmi-1776076.patch19.78 KBjulien
#45 drupal8-convert-comment-module-configuration-to-cmi-1776076.patch19.78 KBjulien
#42 drupal8-convert-comment-module-configuration-to-cmi-1776076.patch20.98 KBjulien
#38 drupal8-Convert-comment-module-configuration-to-cmi-1776076.patch17.88 KBjulien
#36 drupal8-Convert-comment-module-configuration-to-cmi-1776076.patch17.34 KBjulien
#35 drupal8-Convert-comment-module-configuration-to-cmi-1776076.patch17.85 KBjulien
#33 drupal8-Convert-comment-module-configuration-to-cmi-1776076.patch6.27 KBjulien
#28 drupal8-Convert-comment-module-configuration-to-cmi-1776076.patch8.28 KBjulien
#23 drupal8-Convert-comment-module-configuration-to-cmi-1776076.patch9 KBjulien
#18 drupal8-Convert-comment-module-configuration-to-cmi-1776076.patch11.18 KBjulien
#14 drupal8-Convert-comment-module-configuration-to-cmi-1776076.patch7.91 KBjulien
#13 drupal8-Convert-comment-module-configuration-to-cmi-1776076.patch9.96 KBjulien
#12 drupal8-Convert-comment-module-configuration-to-cmi-1776076.patch9.96 KBjulien
#11 drupal8-Convert-comment-module-configuration-to-cmi-1776076.patch9.82 KBjulien
#9 drupal8-Convert-comment-module-configuration-to-cmi-1776076.patch9.87 KBjulien
#5 drupal8-Convert-comment-module-configuration-to-cmi-1776076.patch3.82 KBjulien
#1 drupal8-Convert-comment-module-configuration-to-cmi-1776076.patch3.71 KBjulien
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

julien’s picture

Status: Active » Needs review
FileSize
3.71 KB

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

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.

gdd’s picture

Status: Needs review » Needs work
julien’s picture

Status: Needs work » Needs review
FileSize
3.82 KB

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
larowlan’s picture

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

julien’s picture

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.

julien’s picture

Status: Needs work » Needs review
FileSize
9.87 KB

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
julien’s picture

Status: Needs work » Needs review
FileSize
9.82 KB

retrying, bad git diff

julien’s picture

fixing the ugly scary query syntax

julien’s picture

julien’s picture

chx’s picture

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
julien’s picture

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

julien’s picture

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

julien’s picture

Status: Needs work » Needs review
chx’s picture

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

julien’s picture

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

chx’s picture

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.

julien’s picture

remove the variable db queries

Status: Needs review » Needs work
chx’s picture

Please see #22, thanks.

julien’s picture

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.

chx’s picture

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.

julien’s picture

Status: Needs work » Needs review
FileSize
8.28 KB

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

Status: Needs review » Needs work
Lars Toomre’s picture

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

julien’s picture

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

Status: Needs review » Needs work
Issue tags: +Configuration system
julien’s picture

Status: Needs work » Needs review
FileSize
6.27 KB

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

Status: Needs review » Needs work
julien’s picture

Status: Needs work » Needs review
FileSize
17.85 KB

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

julien’s picture

Status: Needs review » Needs work
julien’s picture

julien’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work
chx’s picture

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.

julien’s picture

Status: Needs work » Needs review
FileSize
20.98 KB

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
chx’s picture

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!

julien’s picture

Status: Needs review » Needs work
julien’s picture

Status: Needs work » Needs review
FileSize
19.78 KB

typo

julien’s picture

Status: Needs review » Needs work
julien’s picture

Status: Needs work » Needs review
FileSize
20.63 KB

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

Status: Needs review » Needs work
julien’s picture

Status: Needs work » Needs review
FileSize
25.11 KB

comment test update

Status: Needs review » Needs work
julien’s picture

Status: Needs work » Needs review
FileSize
25.27 KB
julien’s picture

julien’s picture

Status: Needs review » Needs work
julien’s picture

Status: Needs work » Needs review
FileSize
25.99 KB

Status: Needs review » Needs work
julien’s picture

Status: Needs work » Needs review
FileSize
26.39 KB

Status: Needs review » Needs work
julien’s picture

Status: Needs work » Needs review
FileSize
26.59 KB

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.

julien’s picture

added update_hook for D7 to D8

chx’s picture

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.

julien’s picture

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.

chx’s picture

Status: Needs work » Needs review

Opsie, crosspost.

Status: Needs review » Needs work
julien’s picture

Status: Needs work » Needs review
FileSize
29.56 KB

removed all the intval

Status: Needs review » Needs work
chx’s picture

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

julien’s picture

Status: Needs work » Needs review
FileSize
29.34 KB

one more to try a green flag, changing as 71

julien’s picture

julien’s picture

julien’s picture

added the blog yml file.

julien’s picture

julien’s picture

need to test the code cleanup now.

julien’s picture

Status: Needs review » Needs work
julien’s picture

Status: Needs work » Needs review
FileSize
26.43 KB

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

julien’s picture

julien’s picture

Status: Needs review » Needs work
julien’s picture

Status: Needs work » Needs review
FileSize
27.82 KB

Status: Needs review » Needs work
julien’s picture

Status: Needs work » Needs review
FileSize
27.86 KB
julien’s picture

Status: Needs review » Needs work
Issue tags: -Configuration system
julien’s picture

Status: Needs work » Needs review
Issue tags: +Configuration system
julien’s picture

chx’s picture

Status: Needs review » Reviewed & tested by the community
FileSize
5.88 KB
29.95 KB

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.

chx’s picture

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

chx’s picture

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.

julien’s picture

Status: Needs work » Needs review
FileSize
30.59 KB
chx’s picture

Status: Needs review » Reviewed & tested by the community
FileSize
2.79 KB
30.01 KB

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.

julien’s picture

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

ok i misunderstood then.

julien’s picture

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

chx’s picture

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

julien’s picture

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

sun’s picture

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?

julien’s picture

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.

julien’s picture

Status: Needs work » Needs review
FileSize
36.14 KB

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
julien’s picture

julien’s picture

Status: Needs work » Needs review
gdd’s picture

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.

julien’s picture

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

xjm’s picture

larowlan’s picture

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

larowlan’s picture

Issue summary: View changes

Updated issue summary.

andypost’s picture

Issue summary: View changes
Status: Postponed » Closed (duplicate)
andypost’s picture

Issue summary: View changes