Early Bird Registration for DrupalCon Portland 2024 is open! Register by 23:59 PST on 31 March 2024, to get $100 off your ticket.
The maintenance mode settings at admin/config/development/maintenance need to be converted to use the new configuration system.
Tasks
- Create a default config file and add it to the system module.
- Convert the admin UI in system.admin.inc to read to/write from the appropriate config.
- Convert any code that currently accesses the variables used to use the config system.
- Write an upgrade path from D7 -> D8 to migrate existing data to the new system and drop the appropriate variables.
This would be a good task for someone wanting to get up to speed on how the new config system works. Feel free to ping me on IRC if you need help.
Comment | File | Size | Author |
---|---|---|---|
#63 | system-1496458-63.patch | 661 bytes | no_commit_credit |
#63 | interdiff.txt | 427 bytes | no_commit_credit |
#60 | drupal8.config-system-maintenance.60.patch | 658 bytes | sun |
#57 | config.maintenance.57.patch | 12.62 KB | sun |
#57 | interdiff.txt | 1.97 KB | sun |
Comments
Comment #1
pcambraComment #2
pcambraComment #3
Lars Toomre CreditAttribution: Lars Toomre commentedA couple of things about this function...
a) // Get all setting names defined in system.rss-publishing.xml is the wrong config file... needs correction.
b) //Get any variables currently defined that match the new setting names in
+ //the config file. ... needs a space between // and the first word of both lines
c) + // Update the config system settings to use the values previously stored in
+ // the variable table.
+ if (!empty($var_names)) {
I am unclear why this is again checking if $var_names is empty. Perhaps this should be a check for not empty $var_values?
Comment #4
pcambraYup, that was my intention.
Fixed all comments, patch attached.
Comment #5
pcambraComment #6
pcambraMakint this one dependant of #1497132: Provide a generalised function to update variables from Drupal 7 to Drupal 8's configuration management system
Comment #7
pcambraNew atch attached, dependent on #1497132: Provide a generalised function to update variables from Drupal 7 to Drupal 8's configuration management system
Comment #8
gddLetting the bot take a shot at it.
Comment #9
gddOh duh its just going to fail since the other function isn't committed yet. Nevermind.
Comment #10
gddOK the upgrade function got committed so we can get back to work on this.
Comment #11
gdd#7: 1496458-maintenance_mode_settings-7.patch queued for re-testing.
Comment #13
pcambraThis needs an update using the new update function
Comment #14
pcambraHum, nope, the update function is there with the correct update_N number, the tests are complaining about config table not existing (?). Let's do another try for the testbot.
Comment #15
pcambra#7: 1496458-maintenance_mode_settings-7.patch queued for re-testing.
Comment #17
swentel CreditAttribution: swentel commented$config is unknown in both places.
Trying new patch.
-29 days to next Drupal core point release.
Comment #18
swentel CreditAttribution: swentel commentedSetting to review
Comment #20
swentel CreditAttribution: swentel commentedOk, the problem with this maintenance patch is that update.php is now trying to read the maintenance status from a table that doesn't exist yet.
This is a bit like the same problem we ran in another issue with D8 language updates/upgrades.
Attached a patch which makes all tests pass already, except for the update functionality. Trying to grasp there what goes wrong.
What changed:
- added update_prepare_d8_configuration() to update.php
- added a file_exists to config_get_config_directory() - now *THIS* was a fun one to debug, but it makes sense.
- removed the updates in system.install, it really needs to happen earlier - especially for upgrade tests.
- changed the submit button value on the maintenance page to those tests will pass well.
Let's see what the bot thinks now.
Comment #22
swentel CreditAttribution: swentel commentedThis one should pass, it was actually easier than I thought. (wrong conversion in update_finished()).
Comment #23
swentel CreditAttribution: swentel commentedNote: the fix added in config.inc is also used in #1496510: Convert search settings to configuration system. Once we agree this is the right way to do (or another fix is proposed), the patch there needs to be updated. It probably affects others as well (like #1496534: Convert account settings to configuration system)
Comment #24
swentel CreditAttribution: swentel commentedPatch uses file_prepare_directory() instead of mkdir() and file_exists().
Comment #26
swentel CreditAttribution: swentel commentedNever ever manually update patch :)
Comment #27
swentel CreditAttribution: swentel commentedAlso, one thing I've noticed is that CMI now generates a lot of CMI directories in the simpletest directory which aren't cleaned up after test are run, is that a known issue or should we open one for this ?
- Edit - Created an issue for it: #1512676: Simpletest doesn't clean up the config directories generated by the configuration
Comment #28
aspilicious CreditAttribution: aspilicious commentedLooks good. Tempted to mark this rtbc.
Comment #29
catchThis shouldn't use system_schema(), this has to work upgrading to any version of Drupal 8, and it's conceivable that we'll change the config schema after beta or rc. So it should use the literal schema definition as it is now, then updates can be run on it previously.
However having typed that, I'm less sure now, because the update system depends on the config system, so the schema has to be absolutely up-to-date. Either way this is going to complicate any schema changes to the config system later down the line - at least once we support 8.x-8.x updates.
Comment #30
swentel CreditAttribution: swentel commentedGood point, I think having the latest schema is a must though. As you point out, during 8.x upgrades, this could potententialy be split into something like this:
Wow, now, that looks really ugly isn't it ? :) On the other hand, I think if we'd introduce a really dramatic change in the config schema or even storage, we're into a big problem anyway. Not sure how to handle this further, let's get some more opinions here from say heyrocker, beejeebus, sun and others.
Comment #31
marcingy CreditAttribution: marcingy commented#1517138: SimpleTest does not create configuration folder causing test failures has now been committed to get round the folder issue this likely needs a re-roll.
Comment #32
swentel CreditAttribution: swentel commentedUpdated
Comment #33
gddUnfortunately this needs a reroll to HEAD because the system update function numbers have changed. Otherwise, I think its looking pretty good.
Comment #34
marcingy CreditAttribution: marcingy commentedThis should be a straight reroll aflso takes into account tests moving. This might fail on upgrades due to the refractoring of tests going on at the moment.
Comment #36
kbasarab CreditAttribution: kbasarab commentedThis should be just a straight reroll of #34. Only difference should be moving:
Over to:
SimpleTest may still not pass though with all the other testing issues happening.
Comment #38
ZenDoodles CreditAttribution: ZenDoodles commentedWith upgrade path failures this does not look like a novice issue anymore. Be sure to add "config novice" back to tags if it's suitable when the next action is clarified.
Comment #39
kbasarab CreditAttribution: kbasarab commentedRerunning test based on: http://qa.drupal.org/node/158
Comment #40
kbasarab CreditAttribution: kbasarab commented#36: maintenance_mode-1496458-36.patch queued for re-testing.
Comment #42
kbasarab CreditAttribution: kbasarab commentedRerolled against current 8.x core. This passes upgrade tests locally as well. Didn't run a full test locally though. No interdiff due to core 8.x changes since last roll of this. Made interdiff 3+ mb.
Comment #44
kbasarab CreditAttribution: kbasarab commentedLooks like some of the settings aren't reading. In system.admin.inc I did some var_dump inside function system_site_maintenance_mode() { and got this:
Comment #45
kbasarab CreditAttribution: kbasarab commentedWondering here if http://drupal.org/node/1447686#comment-6062768 might have something to do with this here. Seems like All the calls are correct for defining the maintenance setting but its not being read. Its correct in the XML inside system/config/system.maintenance.xml but if you grep the config files you get:
So the messages aren't saving as I expected in #44. Not sure how to force a refresh though of file system config files. Answer might be in issue I cited though.
Comment #46
gdd- The last couple patches don't actually include the system.maintenance.xml file
- The file needs to be updated to YAML instead of XML
- The naming needs to be adjusted to match the conventions defined at http://drupal.org/node/1667896
Comment #47
n3or CreditAttribution: n3or commentedI created a new patch based on #42. In this patch the rejects of #42 are fixed and the YAML config file is added. Additionally, the naming is adjusted to match the new conventions.
I tried to create a interdiff, but my attempts unfortunately failed.
Comment #48
sunThanks for working on this, @n3or! :)
This looks almost ready. I'm missing the new .yml file in the patch though?
Have a look at the changes to system.admin.inc in the interdiff on #1493108-72: Convert logging and error settings to configuration system to see how the new system_config_form() is supposed to be used.
Let's revert this :)
Leading TAB (should be two spaces instead).
These calls can be chained; i.e.:
Trailing white-space.
Comment #49
n3or CreditAttribution: n3or commentedProblems with current D8 core and problems mentioned in #48 fixed.
Comment #50
sunDidn't we want to rename "mode" into "enabled" ?
Let's change the phpDoc summary into:
"Form submission handler for system_site_maintenance_mode()."
...and then remove the @see.
One TAB left ;)
Comment #51
alexpottWe missing an upgrade path for the
maintenance_mode_message
variable. So we need a system_update_N function in system.install to do this.Also, the config keys should be sorted alphabetically
Comment #52
sunAdding the other missing tags, and unassigning @kbasarab, since @n3or seems to be actively working on this patch. :)
Comment #53
n3or CreditAttribution: n3or commentedFixed problems of #50 and #51. Thank you guys!
Comment #55
suna99b185 Attempt to fix update.php's temporary global override of maintenance mode.
Comment #56
alexpottcore/modules/system/lib/Drupal/system/Tests/Menu/RouterTest.php has couple of
variable_set('maintenance_mode', TRUE);
that need converting toconfig('system.maintenance')->set('enabled', TRUE)->save();
.Comment #57
sun5920cf3 Fixed Menu\RouterTest.
Comment #58
alexpottPatch apply and works as expected. Thanks for all the work!
I wonder if we should mention in the maintenance message field description that it now supports replacing
@site
with the site name in custom messages?Comment #59
webchickCommitted and pushed to 8.x. Thanks!
Comment #60
sunFor some reason, we forgot to add a update here.
Comment #61
aspilicious CreditAttribution: aspilicious commentedIf green this is rdy to go!
Comment #62
sun#60: drupal8.config-system-maintenance.60.patch queued for re-testing.
Comment #63
no_commit_credit CreditAttribution: no_commit_credit commentedComment #64
xjmMinor docblock cleanup attached above. Hook implementations get the imperative for their summaries, because they are user-facing:
http://drupal.org/node/1354#hookimpl
Comment #65
Dries CreditAttribution: Dries commentedCommitted the missing update function.
Comment #67
amonteroLinking to overlapping functionality issue: #97650: Token support for site maintenance mode message
Feedback is appreciated.