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.
Comment | File | Size | Author |
---|
Comments
Comment #1
julien CreditAttribution: julien commentedmade 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.
Comment #3
gddWhat 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.
Comment #4
gddComment #5
julien CreditAttribution: julien commentedyes 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).
Comment #7
larowlanAs 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'.
Comment #8
julien CreditAttribution: julien commentedthat'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.
Comment #9
julien CreditAttribution: julien commentedas 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.
Comment #11
julien CreditAttribution: julien commentedretrying, bad git diff
Comment #12
julien CreditAttribution: julien commentedfixing the ugly scary query syntax
Comment #13
julien CreditAttribution: julien commentedComment #14
julien CreditAttribution: julien commentedComment #15
chx CreditAttribution: chx commentedThanks 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.
Comment #17
julien CreditAttribution: julien commentedi'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 :)
Comment #18
julien CreditAttribution: julien commentedso here is one who manage the two default content type, doc added and line cleanup
Comment #19
julien CreditAttribution: julien commentedComment #20
chx CreditAttribution: chx commentedThis 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.
Comment #21
julien CreditAttribution: julien commentedyou'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
Comment #22
chx CreditAttribution: chx commentedReally, 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.
Comment #23
julien CreditAttribution: julien commentedremove the variable db queries
Comment #25
chx CreditAttribution: chx commentedPlease see #22, thanks.
Comment #26
julien CreditAttribution: julien commentedyes, 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...
that's weird, it's not what i understand from the api castvalue function which is only call from set()
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.
Comment #27
chx CreditAttribution: chx commentedWhile 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.
Comment #28
julien CreditAttribution: julien commentedallrighty, here is the new patch with the requested changes :)
Comment #30
Lars Toomre CreditAttribution: Lars Toomre commentedWhite 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
Comment #31
julien CreditAttribution: julien commented#28: drupal8-Convert-comment-module-configuration-to-cmi-1776076.patch queued for re-testing.
Comment #33
julien CreditAttribution: julien commentedremoved useless code due to misunderstanding of doc after reading again.
Comment #35
julien CreditAttribution: julien commentedadded forum default conf variables, and changed variable_get and variable_set. still need work to pass all the other module tests i think.
Comment #36
julien CreditAttribution: julien commentedComment #38
julien CreditAttribution: julien commentedcallback fix
Comment #39
julien CreditAttribution: julien commentedComment #41
chx CreditAttribution: chx commentedYou 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.Comment #42
julien CreditAttribution: julien commentedthanks 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 :
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'))
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.
Comment #44
chx CreditAttribution: chx commentedA 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
just
because your code now uses ->get('comment') and not ->get('page.comment');
Thanks again!
Comment #45
julien CreditAttribution: julien commentedretry with yml changes
Comment #47
julien CreditAttribution: julien commentedtypo
Comment #48
julien CreditAttribution: julien commentedComment #50
julien CreditAttribution: julien commentedadded other default yml file for other core content type, to help tests runs.
Comment #52
julien CreditAttribution: julien commentedcomment test update
Comment #54
julien CreditAttribution: julien commentedComment #55
julien CreditAttribution: julien commentedComment #56
julien CreditAttribution: julien commentedComment #58
julien CreditAttribution: julien commentedComment #60
julien CreditAttribution: julien commentedComment #62
julien CreditAttribution: julien commentedsorry 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.
Comment #64
julien CreditAttribution: julien commentedadded update_hook for D7 to D8
Comment #65
chx CreditAttribution: chx commentedFailing 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.
Comment #66
julien CreditAttribution: julien commentedthe 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.
Comment #67
chx CreditAttribution: chx commentedOpsie, crosspost.
Comment #69
julien CreditAttribution: julien commentedremoved all the intval
Comment #71
chx CreditAttribution: chx commentedThe update function should iterate the node types returned by _update_7000_node_get_types().
Comment #72
julien CreditAttribution: julien commentedone more to try a green flag, changing as 71
Comment #73
julien CreditAttribution: julien commentedComment #74
julien CreditAttribution: julien commentedFix for pager test
Comment #75
julien CreditAttribution: julien commentedadded the blog yml file.
Comment #76
julien CreditAttribution: julien commentedComment #77
julien CreditAttribution: julien commentedneed to test the code cleanup now.
Comment #78
julien CreditAttribution: julien commentedComment #80
julien CreditAttribution: julien commentedanother one, i'm not sure if the tests are actually ok to achieve this yet.
Comment #81
julien CreditAttribution: julien commentedComment #82
julien CreditAttribution: julien commentedsorry wrong one
Comment #84
julien CreditAttribution: julien commentedComment #86
julien CreditAttribution: julien commentedComment #87
julien CreditAttribution: julien commentedComment #89
julien CreditAttribution: julien commented#87: drupal8-Convert-comment-module-configuration-to-cmi-1776076.patch queued for re-testing.
Comment #90
julien CreditAttribution: julien commentedComment #91
chx CreditAttribution: chx commentedCongratulations! 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.
Comment #92
chx CreditAttribution: chx commentedThe only question I have, do we want a test for this update function, in this issue, or a followup?
Comment #93
chx CreditAttribution: chx commentedSorry, 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.
Comment #94
julien CreditAttribution: julien commentedComment #95
chx CreditAttribution: chx commentedOK, 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.
Comment #96
julien CreditAttribution: julien commentedok i misunderstood then.
Comment #97
julien CreditAttribution: julien commentedthanks for your patience for reviewing and helping me to get it right. really appreciate it. did learn heaps on that one. cheers!
Comment #98
chx CreditAttribution: chx commentedYes, I should have written "into a new comment_update_N".
Comment #99
julien CreditAttribution: julien commentedthat's ok, we just got back on this today, warm up one.
Comment #100
sunWow, 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: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?
Comment #101
julien CreditAttribution: julien commentedi 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.
Comment #102
julien CreditAttribution: julien commentedDid 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.
Comment #104
julien CreditAttribution: julien commentedforgot the yml files.
Comment #105
julien CreditAttribution: julien commentedComment #106
gddI 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:
Which I think makes a lot of sense.
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.
Comment #107
julien CreditAttribution: julien commentedI'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
Comment #108
xjmMeta issue: #1802750: [Meta] Convert configurable data to ConfigEntity system
Comment #109
larowlanI 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
Comment #109.0
larowlanUpdated issue summary.
Comment #110
andypostDuplicate of #731724: Convert comment settings into a field to make them work with CMI and non-node entities
Comment #111
andypost