The site information settings at admin/config/system/site-information 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.
- Some of these settings are first set in the installer, which may or may not make life more complicated.
- Write an upgrade path from D7 -> D8 to migrate existing data to the new system and drop the appropriate variables.
This is possibly a more involved task than the other novice ones because of the number of variables and the fact that you might need to touch the installer. However if you're feeling brave feel free to give it a start. Any amount of progress would be helpful.
Comment | File | Size | Author |
---|---|---|---|
#180 | config.site_.180.patch | 52.93 KB | sun |
#180 | interdiff.txt | 5.45 KB | sun |
#176 | config.site_.176.patch | 54.26 KB | sun |
#176 | interdiff.txt | 723 bytes | sun |
#175 | config.site_.173.patch | 54.36 KB | sun |
Comments
Comment #1
nadavoid CreditAttribution: nadavoid commentedComment #2
nadavoid CreditAttribution: nadavoid commentedAttached patch is a work in progress, but should complete "Create a default config file and add it to the system module" and take care of most of "Convert the admin UI in system.admin.inc to read to/write from the appropriate config."
What remains on the admin form is to properly detect clean urls and take care of the remaining variable_get() calls.
Comment #3
Pedro Lozano CreditAttribution: Pedro Lozano commentedReplaced the calls to variable_get variable_set in most of the places I found it.
I didn't replace the function in node_update_8000() because it is a previous update function and I'm not sure how these should be handled.
Also I get errors when trying to do a clean install. Calls to config()->get() done from install.php produce:
Comment #4
gddHm, I think the problem there is that something in the installer is attempting to save some config before the database is initialized. What step is this happening on?
As far as the update function, you should convert it to use the config system. Node module runs after system module, so all this data should have already been migrated by that point.
Comment #5
David_Rothstein CreditAttribution: David_Rothstein commentedI don't think node_update_8000() is actually guaranteed to run after system_update_8003() unless you force it to be. But that's easy to do with hook_update_dependencies().
Comment #6
gddFixing title
Comment #7
gddI had thought that update hooks were done in order of module weight but apparently I am wrong on that one. So yes, we would need to add an update dependency to make that happen.
I tried the patch in #3 and sure enough, drupal_is_front_page() is called in the installer before we've set up the database. However, it really shouldn't crash because it comes from the following:
The problem there is that we didn't have
use Exception;
in theme.inc, although when I add that it crashes with[Sun Mar 25 14:47:11 2012] [error] [client 127.0.0.1] PHP Fatal error: Class 'Drupal\\Core\\Cache\\InstallBackend' not found in /Users/gdd/Sites/drupal/core/includes/theme.inc on line 5
Don't have much time to chase this down anymore but maybe someone can pick it up from here.
Comment #8
Pedro Lozano CreditAttribution: Pedro Lozano commentedAnother problem that I found is that some uses of variable_get() have a dynamic default value. How should this be translated into the configuration management system without losing the functionality of getting the php value for sendmail_from?
Comment #9
cosmicdreams CreditAttribution: cosmicdreams commentedIt appears that this patch is being implemented in another issue as well.
#1496534: Convert account settings to configuration system
We should combine efforts
Comment #10
cosmicdreams CreditAttribution: cosmicdreams commentedCan I mark this one as a duplicate?
Comment #11
cosmicdreams CreditAttribution: cosmicdreams commentedswitching to review so patches can go through testbot
Comment #13
cosmicdreams CreditAttribution: cosmicdreams commentedHere is a patch that follows the recent advice from #1496534: Convert account settings to configuration system and removes all changes to things that aren't in system.admin.inc.
If this patch succeeds or if the number of failed tests are reasonable then work should continue completing the config conversions for the rest of the settings.
Comment #15
cosmicdreams CreditAttribution: cosmicdreams commentedOnce again, these failures do not occur for me when I test these locally.
P.S.: I just updated and reran the update tests locally and they all still pass. I don't know what's wrong with them. Please help.
Comment #16
marcingy CreditAttribution: marcingy commentedThese tests simply can't pass until #1541958: Split setUp() into specific sub-methods is merged in.
Comment #17
marcingy CreditAttribution: marcingy commented#13: 1496542_13_account_cmi.patch queued for re-testing.
Comment #19
cosmicdreams CreditAttribution: cosmicdreams commentedCool! Now that the patch mentioned above landed, There's one remaining test that this needs to pass before it goes green. However, it should be noted that this patch is incomplete because it does nothing for the site 403 and 404 settings. I'm looking to merge this patch with the previous work to make sure all of that is covered.
Once that is done, I'll need to remove the band-aid I have in place to ensure that I'm setting both the $GLOBALS and the config for these settings.
Comment #20
cosmicdreams CreditAttribution: cosmicdreams commentedHey all,
This patch is still incomplete but I just wanted to see what how this patch performs before I start changing the 'default_node_main' setting that is on the site information administrative page.
This setting, unlike the others, seems to have a lot of use in the node module and is apart of previous uninstall functions. We are faced with the decision of do we change that uninstall function or do we keep it. I'm going to explore both.
Comment #21
cosmicdreams CreditAttribution: cosmicdreams commentedHa, sorry. that patch didn't convert the "sets" just the "gets" for site_404 and "site_403". this one converts them all.
Comment #22
cosmicdreams CreditAttribution: cosmicdreams commentedThis patch throws caution to the wind and removes the band-aid I had in place to ensure that $GLOBALS and config settings are saved in the system_settings_form_submit function found in system.module. If this patch has a lot of fails I'll add it back.
Oh, and it:
Comment #24
cosmicdreams CreditAttribution: cosmicdreams commentedThese tests fail because I'm not supposed to be using system_settings_form_submit but instead using my own submit hook. Except for the "HTML in page title" test. I have no idea why that's failing here and not for me locally.
Comment #25
nadavoid CreditAttribution: nadavoid commentedHey cosmicdreams, thanks for your work on this. I haven't been able to get back to this issue recently, so I'm unassigning it from myself. Feel free to assign it to yourself if you want.
Comment #26
cosmicdreams CreditAttribution: cosmicdreams commentedHere's a patch that implements that submit function, let's see how this improves things.
Comment #27
cosmicdreams CreditAttribution: cosmicdreams commentedforgot to revert the change to system.module that would have put every system configuration setting into the system.site.xml config file.
Comment #29
cosmicdreams CreditAttribution: cosmicdreams commentedAfter reviewing previous patches, I'm going to do the following.
Comment #30
cosmicdreams CreditAttribution: cosmicdreams commentedRemoved the setting of configuration for default_country and default time zone. These are settings that are made in a different form and probably should be set in this patch, but I'm starting to think that it shouldn't. That's a whole set of changes that could be done in another patch.
Comment #31
gddCan you elaborate on what the issues are with default country and timezone?
Comment #33
cosmicdreams CreditAttribution: cosmicdreams commented@heyrocker:
These two settings are not set or retrieved on the system_site_information_settings form.
Further in system.admin.inc there's a whole other set of variables that are retrieved and set by another form. So, I just want to make sure I've got this set of changes right first before moving on.
Comment #34
cosmicdreams CreditAttribution: cosmicdreams commentedmight fix 5 of the fails, but I still don't know what's going on with the Page title fail. @heyrocker can you please help with that?
Comment #36
marcingy CreditAttribution: marcingy commentedThe issue is this line
isn't valid XML and is attempted to being saved to/loaded from config - this of course introduces an interesting question
Comment #37
cosmicdreams CreditAttribution: cosmicdreams commented....that we should be cleaning these values before they are saved? or that we shouldn't allow markup?
You're killing me what's the question?
Comment #38
cosmicdreams CreditAttribution: cosmicdreams commentedTalked this over with marcingy on IRC. What's going on with this remaining fail is that the test tries to submit improperly formatted xhtml and as a result fails when the config('system.site_information')->set('site_name')->save(); occurs.
This whole class of error, may apparently fix if #1470824: XML encoder can only handle a small subset of PHP arrays, so switch to YAML is committed. Otherwise, we'll have a regression.
Comment #39
sunI agree that default country and timezone settings should be tackled in a separate issue. Both of them are rather related to Locale module, just happen to be located in System module (oh my...).
I disagree with the suggested config object name though. What's wrong with
'system.site'
? I'd prefer that.Comment #40
cosmicdreams CreditAttribution: cosmicdreams commentedTechnically there's nothing wrong with it. The site_information seemed more descriptive I can change it back if brevity is preferred.
Comment #41
cosmicdreams CreditAttribution: cosmicdreams commentedThis should simply replace all the system.site_information with system.site
Comment #43
cosmicdreams CreditAttribution: cosmicdreams commentedsecond try
Comment #45
cosmicdreams CreditAttribution: cosmicdreams commented#34: 1496542_34_site_information_cmi.patch queued for re-testing.
Retesting the patch that had 1 fail to see if it still does. I don't know how renaming the name of the config file and the every config function to use that renamed file would lead to more fails.
EDIT: OK, looks like I have to reroll patch 34 to see where it's at.
Comment #46
cosmicdreams CreditAttribution: cosmicdreams commentedAnother try. Haven't really found anything wrong with the patch yet.
This attempt try to restart with the patch in 34 and redoes the changes described in comments starting there and beyond.
Comment #48
cosmicdreams CreditAttribution: cosmicdreams commentedAh well at least that's something. This patch fixes the reuse of the update function.
Comment #50
alexpottNeed to remove this line from node_uninstall() in node.install
And we probably don't want to remove the blank line here
Comment #51
cosmicdreams CreditAttribution: cosmicdreams commentedThis patch implements those comments.
Comment #53
cosmicdreams CreditAttribution: cosmicdreams commentedupdated the update function's number so this can pass.
Comment #54
cosmicdreams CreditAttribution: cosmicdreams commentedAlso if this passes it should be postponed until #1470824: XML encoder can only handle a small subset of PHP arrays, so switch to YAML and #1324618: Implement automated config saving for simple settings forms gets committed.
Comment #56
cosmicdreams CreditAttribution: cosmicdreams commentedso I need to debug these tests to understand why they fail. Hopefully I can get this patch ready so it can get in shortly after the Yaml and system_settings_form refactoring makes it in.
Comment #57
cosmicdreams CreditAttribution: cosmicdreams commentedTests are still failing but I need to brain dump this and get back it later:
-- uses yml now instead of xml.
Comment #58
cosmicdreams CreditAttribution: cosmicdreams commentedOops, I meant this patch.
Comment #59
cosmicdreams CreditAttribution: cosmicdreams commented#58: 1496542_59_site_information_cmi.patch queued for re-testing.
Comment #60
Dave ReidThis code is causing a config() call no matter if a [site:name], [site:mail], or [site:slogan] tokens are being used at all. This logic really should go into the switch/case statements themselves.
Comment #61
cosmicdreams CreditAttribution: cosmicdreams commentedThis patch does what Dave says.
Comment #63
cosmicdreams CreditAttribution: cosmicdreams commented#58: 1496542_59_site_information_cmi.patch queued for re-testing.
Comment #64
sunTaking this over.
Code of attached patch lives in the config-site-1496542-sun branch.
Comment #65
Dave ReidBecause ini_get should be considered untrusted, this would need to be escaped using check_plain if $sanitize is TRUE?
Comment #66
sunReviewed myself - will incorporate the following in a minute:
(and elsewhere) I had serious issues in trying to understand the code, because $config can mean each and everything. Let's make sure to use self-descriptive variable names; e.g., $site_config.
Weird resulting variable name; hints at the exact opposite (the ini_get value). Let's clean this up.
I guess this should stay, and instead should remove the value from system.site.
Overall, this badly needs to be cleaned up though. :(
Replace with @todo + @see to config settings form issue.
Let's re-order those. Wasn't aware this code was new.
This needs to happen earlier; very early actually.
Comment #67
sunre: #65: That's an entirely bogus change in the first place. system.site:mail is a required config value that ought to always exist. The existing code doesn't fall back to sendmail_from either. Reverted that.
Attached patch fixes all from #66.
Except for the upgrade path. Not sure how to handle that yet.
Working on the failing tests first.
Comment #69
sunFixed those two test failures. As usual, I forgot to update installation profiles accordingly (since I'm grepping on /core only).
Comment #70
cosmicdreams CreditAttribution: cosmicdreams commentedwow thanks for being the closer of on this sun. However, it went green once before I'll send your most recent patch to retest and if it passes I'll do a quick review and consider marking this RTBC.
Comment #71
sunAttached patch adjusts the remaining puzzle piece: The upgrade of system site variables to config.
It also reverts the change to a node module update that was contained before, because it is incorrect. We will have to double-check and verify the exact order of module update execution at some later point in the cycle.
Because of that, this patch introduces a new @defgroup
config_upgrade
, which allows us to locate and track all update functions that are dealing with variables and/or config conversions more easily.I consider this patch ready to be committed.
Comment #73
sunLuckily, update_variables_to_config() is completely bogus and does not work at all, which explains the test failures of #71.
Attached patch fixes update_variables_to_config() to work correctly, to incorporate the absolute default configuration of a module's configuration object, and to correctly merge and migrate the variables into the configuration object.
It also (semi-)introduces formal configuration object name namespaces, as stated in #1560060: [meta] Configuration system roadmap and architecture clean-up and also hinted at in #1505090: Clean up all (non-default) configuration (files) when a module is uninstalled
Comment #74
sunI have nothing to add to the patch to #73.
Comment #75
alexpott#73 update_variables_to_config() is not complete bogus.
If the update hook in #71 had been - it would have worked
Which is what #1589174: Configuration upgrade path is broken was about. The patch on this issue was suggesting that we should have the ability to load a specific file from a module's config directory so it would be something like this:
I think this approach might be better because then update_variables_to_config is not duplicating the functionality of config_install_default_config.
Comment #76
alexpottFixing tags
Comment #77
sun@alexpott: The approach taken here by the new update_variables_to_config() is superior, because it additionally adds a validation to the upgrade path, which verifies that a module provides a default configuration file for the variables being migrated.
Second, the desired and expected functionality is different, too: We do not want to silently install/migrate all configuration object files that may exist in the new module. We only want to migrate the exact specified one. Migrating all would only hide upgrade path mistakes.
Comment #78
sunAny further reviews? This patch pretty much blocks all other config conversions, due to the required upgrade path fixes.
Comment #79
alexpottPatch attached fixes the variable migration test name.
Patch attached re-implements the ability to call update_variables_to_config() without a variable map for an improved DX.
Patch adds a test to ensure variables are removed from variable table by update_variables_to_config()
Patch add tests for calling update_variables_to_config() without a variable map.
I'm still not the hugest fan of update_variables_to_config() doing the initial import of config from the module's supplied default as I think importing config and migrating variables are two different concerns which is why I think the following code would be better:
Comment #81
alexpottReroll so patch applies against latest 8.x
Comment #82
alexpottReroll so patch applies against latest 8.x
Comment #83
sunHrm. Took me some time to distill and merge your changes. Can we use the sandbox, if any further work is required, please? :)
That said, I've taken over the additional test assertion that verifies the removal of variables after migration.
But I did not merge the change that makes the $variable_map optional. That is, because I want the to be migrated variables to be specified (and thus also documented) explicitly in the upgrade path. This has nothing to do with DX or whatsoever, but with pure upgrade/migration path workflows, semantics, and clarity. Catch-all mechanisms involving magic that relies on new data - which can potentially change further during D8 development - are prone to errors. We don't do that.
(Furthermore, most variables will be renamed during or after the conversion either way, so there is no use-case for the direct mapping in the first place. See #1599554: Tutorial/guidelines for how to convert variables into configuration)
Comment #85
aspilicious CreditAttribution: aspilicious commentedI can't reproduce the failure...
Comment #86
aspilicious CreditAttribution: aspilicious commented#83: config.site_.83.patch queued for re-testing.
Comment #87
sunThis patch looks RTBC to me.
Comment #88
chx CreditAttribution: chx commentedI merely added a line of comment and rerolled with the block test move.
Comment #89
catchThis appears to be implementing the guidelines from #1599554: Tutorial/guidelines for how to convert variables into configuration, but that issue is under discussion, so I'll hold off committing this until that one's dealt with.
Comment #90
Gábor HojtsyAs per a quick discussion with @catch, given that we need something in D8MI to get going with prototyping for config language, it looks like this is either postponed on #1599554: Tutorial/guidelines for how to convert variables into configuration or needs work since it makes modifications in patterns that are not yet agreed on.
Comment #91
cosmicdreams CreditAttribution: cosmicdreams commentedWhat can be committed now? Everything but the how the configuration variables are named? It would be great to get the majority of this work in and push the remainder in later.
Comment #92
gddI agree with catch that this should not be committed with the variable name changes until #1599554: Tutorial/guidelines for how to convert variables into configuration is resolved. Also I'm not in favor of wrapping the wholesale changes to update_variables_to_config() into this patch. It seems like that should be taken up in a separate issue. Is there anything specific to this issue that prevents that?
Comment #93
cosmicdreams CreditAttribution: cosmicdreams commentedIn order to move this issue along, I opened #1606980: Rename site information config to new naming convention for the purpose of focusing on the naming convention changes so that the remainder of this issue may be committed. Patch to follow later with a rewinding of the name changes here so they can be done in #1606980: Rename site information config to new naming convention.
Comment #94
sun#1589174: Configuration upgrade path is broken
Comment #95
cosmicdreams CreditAttribution: cosmicdreams commentedThis patch attempts to surgically remove the name changes to the config and the changes that sun linked to here: #1589174: Configuration upgrade path is broken.
If #1589174: Configuration upgrade path is broken is committed then it would probably be easier to reroll the patch in #88, then do the name changes. Or better yet we reach consensus on the name changes and we can just reroll this patch.
Comment #96
cosmicdreams CreditAttribution: cosmicdreams commentedgo testbot
Comment #97
sun#88: 1496542_88.patch queued for re-testing.
Comment #98
cosmicdreams CreditAttribution: cosmicdreams commentedHi sun:
Is it ok with you if we land this patch without the name changes of the variables, then handle the name changes later?
Comment #99
cosmicdreams CreditAttribution: cosmicdreams commentedsun, do you want me to reroll #88 for you and get that patch to go green, or can you please review 95 and let me know if it's RTBC?
Comment #100
sunMerged latest HEAD.
Not sure whether this passes, as I'm not able to run any tests due to #1617776: Fatal error: require(): Cannot redeclare class drupal\core\config\drupalconfig in UniversalClassLoader
Comment #102
aspilicious CreditAttribution: aspilicious commentedprobably a bootstrap.test conflict
Comment #103
sunjeez. These manual merge conflict resolutions for PSR-0 test patches suck big time. Why doesn't git's rename/copy detection kick in?
Comment #104
aspilicious CreditAttribution: aspilicious commentedBecause bootstrap.test contained more than 1 class and the file is now split in several files. You can't rename one file in 10 files.
Comment #107
Gábor HojtsyTagging for D8MI as one of two paths where we can experiment with adding language support sooner than later. The other one is #1588422: Convert contact categories to configuration system which might not be as further along depending on how you see it.
Comment #108
sunConverted site variables in new DrupalKernel code.
Comment #109
chx CreditAttribution: chx commentedDoesn't get simpler than that. Awesome job.
Comment #110
cosmicdreams CreditAttribution: cosmicdreams commentedThe last thing I want to see is this patch stalled for yet another week. +RTBC even though it implements the changes to variable names that held this issue up in #88 and #92.
Comment #111
chx CreditAttribution: chx commentedI am revoking the RTBC and rerolling. Just a sec.
Comment #112
chx CreditAttribution: chx commentedRerolled without name changes.
Comment #113
chx CreditAttribution: chx commentedksenzee pointed out that i missed site_mail and the core/modules/system/config/system.site.yml file. Here.
Comment #114
cosmicdreams CreditAttribution: cosmicdreams commentedReviewed the patch. the only think I objected to was a few places where there were some extra parenthesis in a situations like this. But that seems to be a coding standard as I found it in previously committed code as well. Since this is minor, and the patch is green, I think it's RTBC.
an example of extra parenthesis
If this patch is committed, and if sun doesn't beat me to it, I'll create a patch to rename the variables to sun's syntax in #1606980: Rename site information config to new naming convention
Comment #115
cosmicdreams CreditAttribution: cosmicdreams commentedlol, I meant RTBC not needs work
Comment #116
cosmicdreams CreditAttribution: cosmicdreams commented@sun: I know you disagree with the using the old way of naming variables here. Can we either discuss or allow #1606980: Rename site information config to new naming convention handle the renaming of the variables?
Comment #117
catch#113: 1496542_113.patch queued for re-testing.
Comment #119
cosmicdreams CreditAttribution: cosmicdreams commentedrerolled, as the location of a few of these tests have changed and break the patch.
Comment #121
Gábor Hojtsy@cosmicdreams: Fails now :/
Comment #122
webflo CreditAttribution: webflo commented#119: 1496542_119_site_information_cmi.patch queued for re-testing.
Comment #124
cosmicdreams CreditAttribution: cosmicdreams commentedIn review of the patch in #119, here's what I found:
I forget where, but I was using the site_name variable where I should have used the site_frontpage variable.
I also found in system.api.php many instances the use of Drupal\Database\Query\AlterableInterface as a parameter. That interface is at Drupal\Core\Database\Query\AlterableInterface instead, and it makes my IDE scream at me.
I'm filing #1630108: Improper reference to Drupal\Database\Query\AlterableInterface for that. If I'm wrong about the need for this change please discuss there.
Comment #126
aspilicious CreditAttribution: aspilicious commented#124: 1496542_124_site_information_cmi.patch queued for re-testing.
Comment #128
aspilicious CreditAttribution: aspilicious commentedI think system_update_8011 need to come before system_update_8002
We probably just can switch these functions (and its number). We are not yet in alpha so we don't have to care about d8 to d8 upgrades.
15 days to next Drupal core point release.
Comment #129
cosmicdreams CreditAttribution: cosmicdreams commentedAfter changing the order the update functions occur, here is a new patch. Let's see how it fares.
I though it would be best to make sure the the creation of the config table happens before we convert the variables into config.
Comment #130
cosmicdreams CreditAttribution: cosmicdreams commentedComment #132
cosmicdreams CreditAttribution: cosmicdreams commentedThis attempt is the patch in #124 but instead of doing what #129 tries, this reverts what update 8002 is trying to do in system.install.
Comment #134
disasm CreditAttribution: disasm commentedI've been running this through a debugger on install, and it crashes here:
( ! ) Fatal error: Call to undefined function Drupal\Core\Config\db_query() in /Users/sbl5007/htdocs/drupal-git/core/lib/Drupal/Core/Config/DatabaseStorage.php on line 23
My assumption is this is related to the failures in the upgrade_path tests that are failing in the current code.
The root of the issue is in the drupal_is_front_page() function on line 301:
$is_front_page = (current_path() == config('system.site')->get('site_frontpage'));
Here's a backtrace:
Fatal error: Call to undefined function Drupal\Core\Config\db_query() in /Users/sbl5007/htdocs/drupal-git/core/lib/Drupal/Core/Config/DatabaseStorage.php on line 23
Call Stack
# Time Memory Function Location
1 0.1352 649576 {main}( ) ../install.php:0
2 5.3350 1320608 install_drupal( ??? ) ../install.php:36
3 11.9062 16849568 install_display_output( ???, ??? ) ../install.core.inc:109
4 11.9073 16854520 theme( ???, ??? ) ../install.core.inc:757
5 11.9074 16855248 theme_install_page( ??? ) ../theme.inc:1104
6 11.9075 16856328 theme( ???, ??? ) ../theme.maintenance.inc:147
7 11.9076 16858368 template_preprocess( ???, ??? ) ../theme.inc:1068
8 11.9077 16860904 _template_preprocess_default_variables( ) ../theme.inc:2406
9 11.9078 16864384 drupal_is_front_page( ) ../theme.inc:2440
10 18.5082 16865480 config( ???, ??? ) ../path.inc:301
11 108.9604 17028232 Drupal\Core\Config\DrupalConfig->__construct( ??? ) ../config.inc:87
12 123.8664 17028232 Drupal\Core\Config\DrupalConfig->read( ) ../DrupalConfig.php:37
13 123.8665 17028232 Drupal\Core\Config\DatabaseStorage->read( ) ../DrupalConfig.php:44
I'm afraid this is beyond my knowledge of drupal core at this point. Someone with a little more experience than me is going to have to take a look at this.
Comment #135
aspilicious CreditAttribution: aspilicious commentedBut I do find it strange that this worked before...
I'm going to create a test patch...
Comment #136
aspilicious CreditAttribution: aspilicious commentedLets try this one. It probably will fail but hopefully not the update tests :)
Comment #138
aspilicious CreditAttribution: aspilicious commentedI changed this
And it is still failing, so the problem isn't cause by that.
EDIT:
It maybe is possible it isn't failing on that line anymore but a different one. Can someone step debug this again. With my patch?
Comment #139
sunThat is because database.inc is not loaded. #1605324: Configuration system cleanup and rearchitecture and inherently #1626584: Combine configuration system changes to verify they are compatible fixes that by not relying on database.inc in the first place.
Comment #140
disasm CreditAttribution: disasm commentedaspilicious, the backtrace is different, so yeah, it's dying in another place.
I checked line 2822 of theme.inc, and it is config('system.site'), which is making the same call to DrupalStorage sun alluded to above.
1 1.3531 232608 {main}( ) ../install.php:0
2 26.2050 240168 install_drupal( ??? ) ../install.php:36
3 26.2834 1346040 install_display_output( ???, ??? ) ../install.core.inc:109
4 26.2842 1349560 theme( ???, ??? ) ../install.core.inc:757
5 26.2842 1350000 theme_install_page( ??? ) ../theme.inc:1104
6 26.2843 1350800 theme( ???, ??? ) ../theme.maintenance.inc:147
7 51.2041 1357552 template_preprocess_maintenance_page( ???, ??? ) ../theme.inc:1068
8 51.2052 1370624 config( ???, ??? ) ../theme.inc:2822
9 51.2058 1399728 Drupal\Core\Config\DrupalConfig->__construct( ??? ) ../config.inc:87
10 51.2058 1399776 Drupal\Core\Config\DrupalConfig->read( ) ../DrupalConfig.php:37
11 51.2058 1399848 Drupal\Core\Config\DatabaseStorage->read( ) ../DrupalConfig.php:44
Comment #141
vasi1186 CreditAttribution: vasi1186 commentedI did some very small changes in the patch from #136:
This generated a fatal error, in the Upgrade Path test cases, because the upgrade is not performed in the setUp() method, so the "config" table is not yet created. I removed it because from what I've seen, the site_mail should be migrated to the config table in system_update_8011().
I reset that variable to make sure that the next call to drupal_is_front_page will not return a cached value.
Just made the change for the correct check of the current path, as it was already specified in the comment.
Let's see if it will pass now the tests.
Comment #143
aspilicious CreditAttribution: aspilicious commentedMmmm menu.test and upgrade.tests are replaced with psr-0 files now. Srry :)
Comment #144
vasi1186 CreditAttribution: vasi1186 commentedAttached the new patch.
One more thing that I did, and that should most probably be changed, is this:
I did this because, as also the comment in the function says, during the installation, the db_query function is not available, so this will generate an error.
Ideas about how this should be refactored are welcome!
Comment #145
effulgentsia CreditAttribution: effulgentsia commentedHere's how that hunk is being dealt with in #1576322: page_cache_without_database doesn't return cached pages without the database:
Comment #146
Gábor HojtsyIs that the type of solution we need?
Comment #147
gddI just talked to vasi and recommended that he wrap drupal_is_front_page() with a check if we're in maintenance mode (which would include in the installer) and if so just return FALSE. This is the solution we came up with for other variables used in the installer like js/css compression. If the above check for function_exists() ever gets in, we should probably revert those but for the time being this is how we've been handling it.
Comment #148
vasi1186 CreditAttribution: vasi1186 commentedAttached a new patch that removes the ugly function_exists() check. Also, besides the check in the drupal_is_front_page(), another issue appeared in template_preprocess_maintenance_page(), which I solved usign the same approach. Also attached an interdiff between the last two patches to point out the changes.
Comment #149
aspilicious CreditAttribution: aspilicious commentedLooks good now :)
Comment #150
vasi1186 CreditAttribution: vasi1186 commented#148: 1496542_148_site_information_cmi.patch queued for re-testing.
Comment #151
vasi1186 CreditAttribution: vasi1186 commented#148: 1496542_148_site_information_cmi.patch queued for re-testing.
Comment #152
effulgentsia CreditAttribution: effulgentsia commentedIs the issue summary up to date for someone who might want to review this without reading 150 comments? Are there debates in any other issues that this is blocked on?
Comment #153
Gábor Hojtsy@effulgentsia: I'd have the same question? What is still holding this up? Any complaints against the latest patch?
Comment #154
gddI realize that above I had recommended we not hold up this issue on #1599554: Tutorial/guidelines for how to convert variables into configuration however i suspect that will resolve in the next 24 hours. So I think we should hold off since resolution of that issue will require a reroll of this one.
Comment #155
cosmicdreams CreditAttribution: cosmicdreams commentedCool, have we reached that consensus?
Comment #156
Gábor Hojtsy@heyrocker: Now four times your estimated time passed. There has not been any comments on #1599554: Tutorial/guidelines for how to convert variables into configuration for 23 days. Are you advocating postponing this issue on that indefinitely?
Comment #157
gdd#1599554: Tutorial/guidelines for how to convert variables into configuration is fixed, so this needs a reroll.
Comment #158
disasm CreditAttribution: disasm commentedattached is a reroll of the latest patch that meets the guidelines to the best of my knowledge.
Comment #160
andypost#158: drupal-1496542-site_information_cmi-158.patch queued for re-testing.
Comment #161
andypostFixed one missed variable name
Looks like dots in key-names leads to broken config values
Comment #162
Gábor HojtsyBroken? It is just nested. What is broken?
Comment #163
andypostIt's not broken, I got sometimes values duplicated after update, but probably this caused by previous version of config which was not nested. A clean install works fine for me
Comment #164
disasm CreditAttribution: disasm commentedThat would be my fault. I didn't know the proper yml syntax for sub keys. I've tested andypost's patch, and it works as expected.
Comment #165
effulgentsia CreditAttribution: effulgentsia commentedI reviewed #161 and it looks good to me. A couple minor points:
This might be a good idea, but I'm not clear whether it's necessary in this patch. If tests pass without it, I think we should remove this, and address it in a follow-up.
Removing this entirely seems wrong to me. #141 explains why we can't use config() here, but I suspect we need some way to deal with this.
#1599554-36: Tutorial/guidelines for how to convert variables into configuration landed, so this can be removed, and instead change the later call from system_settings_form() to system_config_form().
Comment #166
effulgentsia CreditAttribution: effulgentsia commentedAlso, we're changing this form element name to match the config key, but we're not changing any of the other form element names to match their new config key names. Should we change all the form element names, or is that being deferred to a follow-up?
Comment #167
gddThis is a good point. We should definitely make them match since that has been consistent throughout core for a long time and I don't want to change that standard without reason.
I would love to do this right now, but there are some small questions about how this naming should work and those questions should probably be discussed in #1599554: Tutorial/guidelines for how to convert variables into configuration and added onto those standards. For instance in situations like the example here, would we change the form element to just be '404' or 'page.404'? The latter is more explicit but introduces a dot in element names which may or may not be safe. The former is simpler but could in some edge cases create name collision (not sure that is necessarily worth worrying about.) So I'm OK with keeping that out of this patch while we figure it out and having it be addressed in a followup. Lets get this thing in.
-1 days to next Drupal core point release.
Comment #168
sunI'm sorry, but I need to postpone this on #1605324: Configuration system cleanup and rearchitecture — which is RTBC already and should (finally) land today/tomorrow.
That is, because this patch introduces too many changes for handling edge-case situations of the installer and other situations, which should be entirely obsolete after that patch has landed.
Second, I disagree with the variable/key name change of the 'default_nodes_main' variable in this patch. That is, because this variable actually does not belong into system.site. Node module is optional. I already considered multiple times whether it shouldn't be reverted entirely from this patch. It belongs to Node module only, and Node module should form alter the site information settings form to inject it. However, since we're not able to support variables and new configuration in the same form, we have to convert it into system.site for now. There should, or actually has to be, a follow-up issue to move this setting from system.site into node.main or whatever. Searching for existing issues yielded one that primarily focuses on UX, and I didn't want to hi-jack that, so let's create a new one. AFAICS, creating the follow-up issue is a hard requirement for this patch to land.
Third, I disagree with changing the array elements in the form structure. That's the entire topic of #1324618: Implement automated config saving for simple settings forms and/or #1648930: Introduce configuration schema and use for translation. Performing any kind of premature change in any conversion has a probability of 99% for being bogus and unnecessary. One of the existing conversion patches already introduced a bogus premature change, which won't ever happen in that form, so it introduced code that's completely off, and I almost guess that if I hadn't reviewed that patch post-commit no one would have ever noticed. We need to clarify #1599554: Tutorial/guidelines for how to convert variables into configuration for that detail. But that does not affect this patch. Except for the 'default_nodes_main' rename in the form structure, but that's unnecessary/bogus in the first place, as explained above.
PS: @heyrocker: Your version of Dreditor is very outdated. ;)
Comment #169
aspilicious CreditAttribution: aspilicious commentedI'm going trying to reroll
Comment #170
aspilicious CreditAttribution: aspilicious commentedNot sure if this is going to work and if it covers all the reviews. I don't think I covered Sun his second point. Not sure if it's actually needed at the moment.
Comment #172
aspilicious CreditAttribution: aspilicious commentedNext try
Comment #174
aspilicious CreditAttribution: aspilicious commentedApparantly we still need the maintenance mode checks on upgrade.
Comment #175
sunAttached is #172 with default_nodes_main reverted.
Comment #176
sun64738e2 Fixed bogus preemptive attempt of using configuration system in upgrade path tests.
Comment #177
aspilicious CreditAttribution: aspilicious commentedMakes sense...
Comment #178
andypostFiled #1666074: Move 'default_nodes_main' setting to node module to start work on 'default_nodes_main'
So there's no actual reason to convert this variable twice
I think this need better comment to explain a usage of $this->variable_set()
Maybe be we leave this as is and convert to node variable in filed issue?
And this related to node module too?
Comment #179
aspilicious CreditAttribution: aspilicious commentedSee #168
"However, since we're not able to support variables and new configuration in the same form, we have to convert it into system.site for now"
But yes that variable_set can use an explanation, certainly for the future. Does that mean variable_set and variable_get will be kept for stuff like this in drupal 8?
Comment #180
sun#1666074: Move 'default_nodes_main' setting to node module landed. Thus:
160a51a Merge branch '8.x' into config-site-1496542-sun
3c8c22d Reverted default_nodes_main changes.
Comment #181
sunThis has been RTBC before. The last patch only adjusted for #1666074: Move 'default_nodes_main' setting to node module
Comment #182
Jose Reyero CreditAttribution: Jose Reyero commented+1 Looks good to me
Comment #183
webchickCommitted and pushed to 8.x. Thanks!
This will likely need a change notice to refer to the new way for installation profiles et al to get at those variables.
Comment #184
Gábor HojtsyThanks!
Comment #185
tim.plunkettChange record added: http://drupal.org/node/1668806
Comment #186
sunThanks!
Comment #187
Tor Arne Thune CreditAttribution: Tor Arne Thune commented