Early Bird Registration for DrupalCon Portland 2024 is open! Register by 23:59 PST on 31 March 2024, to get $100 off your ticket.
Part of #1696224: Remove system_settings_form()
forum_admin_settings uses system_settings_form() and needs to use system_config_form().
Comment | File | Size | Author |
---|---|---|---|
#37 | drupal8.config-forum-settings.37.patch | 22.58 KB | sun |
#34 | 1696902_34_forum_cmi.patch | 21.47 KB | tobiasb |
#31 | 1696902_31_forum_cmi.patch | 20.61 KB | cosmicdreams |
#29 | 1696902_29_form_cmi.patch | 20.67 KB | cosmicdreams |
#25 | drupal8.config-forum-settings.25.patch | 20.73 KB | sun |
Comments
Comment #1
larowlannote does not convert node_options_forum variable, this one depends on node.module being converted first - added a todo.
Comment #2
larowlansorry mister testbot, network gremlins resulted in same patch attached twice :)
Comment #3
sunComment #5
larowlanUpdating node and system tests, not sure what's happening with the module api tests but this fixes the path and node access tests.
Comment #7
gddI 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.
Comment #9
larowlanThis should use the block.num instead of block_num
same again
Comment #10
larowlanChanges block configure/save hooks to use block.num_* format as recommended by heyrocker
Comment #11
larowlanMissed the patches to node/system tests.
Comment #12
gddThis should be
To properly represent the hierarchy.
I think otherwise we are good to go though.
Comment #13
sunAll config system conversions are API changes, so tagging as such.
Comment #14
sunIt'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.:
- 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.
Comment #15
gddI 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.
Comment #17
sunOh. 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).
Comment #18
larowlanWas 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
to be
for the sake of readability whilst we're at it.
Will re-roll on Monday unless someone else beats me to it.
Comment #19
sunAdditionally, 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.
Comment #20
gddupdate_variables_to_config() only runs in the update function, don't we still need config_install_default_config() for new installs?
Comment #21
sunNope, module_enable() installs the default configuration of a module already.
Comment #22
gddOh right, you would think I would remember that since I wrote the code!
Comment #23
sunExecuted 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:
No other changes. (i.e., in-patch renames only)
Comment #24
sunSorry, forgot some essential fixes.
Comment #25
sunSorry, that wasn't complete either. This one is.
Comment #26
sunok, 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.
Comment #27
aspilicious CreditAttribution: aspilicious commentedGet doesn't have 2 arguments...
Is this correct? Can we pass a second argument to ->get()
-27 days to next Drupal core point release.
Comment #28
gddNo that is incorrect, config->get() does not take the default value like variable_get() does
Comment #29
cosmicdreams CreditAttribution: cosmicdreams commentedRerolled #25
fixed the typo shown in #26
migrated a few lines to using the fluid syntax.
Comment #31
cosmicdreams CreditAttribution: cosmicdreams commentedWhy 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.
Comment #32
gddThat return should be completely irrelevant. There is no difference functionally from returning and just falling out of the function.
Comment #34
tobiasb* Uses taxonomy_vocabulary_load() again in forum_enable().
Comment #36
sunModuleApiTest: line 239:
Comment #37
sunRe-applying @heyrocker's fix from #7 for forum_install().
Comment #38
aspilicious CreditAttribution: aspilicious commentedRdy to fly I think!
Comment #39
Dries CreditAttribution: Dries commentedAnother great CMI conversion. Very happy to see that momentum. Committed the patch to 8.x - including the new yml file (schrug). Thanks all!