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.

CommentFileSizeAuthor
#180 config.site_.180.patch52.93 KBsun
#180 interdiff.txt5.45 KBsun
#176 config.site_.176.patch54.26 KBsun
#176 interdiff.txt723 bytessun
#175 config.site_.173.patch54.36 KBsun
#175 interdiff.108-173.txt3.7 KBsun
#172 1496542-site_information_cmi-172.patch57.45 KBaspilicious
#170 1496542-site_information_cmi-170.patch57.3 KBaspilicious
#168 interdiff.108-161.txt7.5 KBsun
#161 1496542-site_information_cmi-161.patch54.78 KBandypost
#161 1496542-interdiff-158vs161.txt1.09 KBandypost
#158 drupal-1496542-site_information_cmi-158.patch54.78 KBdisasm
#158 interdiff.txt49.17 KBdisasm
#148 1496542_148_site_information_cmi.patch55.21 KBvasi1186
#148 interdiff_144_148.txt3.34 KBvasi1186
#144 1496542_144_site_information_cmi.patch55.45 KBvasi1186
#141 1496542_141_site_information_cmi.patch54.31 KBvasi1186
#141 interdiff_136_141.txt1.84 KBvasi1186
#136 1496542_136_site_information_cmi.patch57.33 KBaspilicious
#132 1496542_132_site_information_cmi.patch54.28 KBcosmicdreams
#129 1496542_129_site_information_cmi.patch57.06 KBcosmicdreams
#124 1496542_124_site_information_cmi.patch54.71 KBcosmicdreams
#119 1496542_119_site_information_cmi.patch54.68 KBcosmicdreams
#113 1496542_113.patch50.52 KBchx
#112 config.site_.112.patch50.38 KBchx
#108 config.site_.108.patch50.09 KBsun
#108 interdiff.txt1.82 KBsun
#103 config.site_.103.patch48.28 KBsun
#100 config.site_.100.patch48.18 KBsun
#95 1496542_95_system-site-cmi.patch104.44 KBcosmicdreams
#88 interdiff.txt1.31 KBchx
#88 1496542_88.patch63.42 KBchx
#83 config.site_.83.patch63.25 KBsun
#81 config.site_.81.patch64.14 KBalexpott
#79 config.site_.79.patch64.03 KBalexpott
#79 interdiff-73-79.txt4.22 KBalexpott
#73 config.site_.73.patch62.91 KBsun
#73 interdiff.txt11.86 KBsun
#71 config.site_.71.patch51.99 KBsun
#71 interdiff.txt4.11 KBsun
#69 config.site_.69.patch49.04 KBsun
#67 config.site_.67.patch48.5 KBsun
#64 config.site_.64.patch48.9 KBsun
#61 1496542_awesome_patch.patch46.3 KBcosmicdreams
#58 1496542_59_site_information_cmi.patch94.34 KBcosmicdreams
#57 1496542_57_site_information_cmi.patch47.25 KBcosmicdreams
#53 1496542_53_site_information_cmi.patch46.39 KBcosmicdreams
#51 1496542_51_site_information_cmi.patch46.39 KBcosmicdreams
#48 1496542_48_site_information.patch46.21 KBcosmicdreams
#46 1496542_46_site_information.patch46.12 KBcosmicdreams
#43 1496542_43_site_information.patch46.42 KBcosmicdreams
#41 1496542_41_site_information.patch46.19 KBcosmicdreams
#34 1496542_34_site_information_cmi.patch47.72 KBcosmicdreams
#30 1496542_30_site_information_cmi.patch47.71 KBcosmicdreams
#27 1496542_27_account_cmi.patch47.87 KBcosmicdreams
#26 1496542_26_account_cmi.patch48.43 KBcosmicdreams
#22 1496542_22_account_cmi.patch47.23 KBcosmicdreams
#21 1496542_21_account_cmi.patch46.09 KBcosmicdreams
#20 1496542_20_account_cmi.patch45.06 KBcosmicdreams
#13 1496542_13_account_cmi.patch42.74 KBcosmicdreams
#3 1496542-site-info-cmi.patch33.87 KBPedro Lozano
#2 drupal-site-information-form-1496542-2.patch6.02 KBnadavoid
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

nadavoid’s picture

Assigned: Unassigned » nadavoid
nadavoid’s picture

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

Pedro Lozano’s picture

FileSize
33.87 KB

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

Fatal error: Call to undefined function Drupal\Core\Config\db_query() in /Users/peterlozano/Sites/core/8.x/core/lib/Drupal/Core/Config/DrupalVerifiedStorageSQL.php on line 22
gdd’s picture

Hm, 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.

David_Rothstein’s picture

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.

I 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().

gdd’s picture

Title: Convert site information blocking to config system » Convert site information to config system

Fixing title

gdd’s picture

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


  // drupal_is_front_page() might throw an exception.
  try {
    $variables['is_front'] = drupal_is_front_page();
  }
  catch (Exception $e) {
    // If the database is not yet available, set default values for these
    // variables.
    $variables['is_front'] = FALSE;
    $variables['db_is_active'] = FALSE;
  }

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.

Pedro Lozano’s picture

Status: Active » Needs work
+++ b/core/includes/mail.incundefined
@@ -118,7 +118,7 @@ define('MAIL_LINE_ENDINGS', isset($_SERVER['WINDIR']) || strpos($_SERVER['SERVER
-  $default_from = variable_get('site_mail', ini_get('sendmail_from'));
+  $default_from = config('system.site_information')->get('site_mail');

Another 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?

cosmicdreams’s picture

It appears that this patch is being implemented in another issue as well.

#1496534: Convert account settings to configuration system

We should combine efforts

cosmicdreams’s picture

Can I mark this one as a duplicate?

cosmicdreams’s picture

Status: Needs work » Needs review

switching to review so patches can go through testbot

Status: Needs review » Needs work

The last submitted patch, 1496542-site-info-cmi.patch, failed testing.

cosmicdreams’s picture

Status: Needs work » Needs review
FileSize
42.74 KB

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

Status: Needs review » Needs work

The last submitted patch, 1496542_13_account_cmi.patch, failed testing.

cosmicdreams’s picture

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

marcingy’s picture

These tests simply can't pass until #1541958: Split setUp() into specific sub-methods is merged in.

marcingy’s picture

Status: Needs work » Needs review
Issue tags: -Configuration system, -Config novice

#13: 1496542_13_account_cmi.patch queued for re-testing.

Status: Needs review » Needs work
Issue tags: +Configuration system, +Config novice

The last submitted patch, 1496542_13_account_cmi.patch, failed testing.

cosmicdreams’s picture

Cool! 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.

cosmicdreams’s picture

Status: Needs work » Needs review
FileSize
45.06 KB

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

cosmicdreams’s picture

FileSize
46.09 KB

Ha, sorry. that patch didn't convert the "sets" just the "gets" for site_404 and "site_403". this one converts them all.

cosmicdreams’s picture

FileSize
47.23 KB

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

  • converts all of the default_nodes_main settings
  • corrects the syntax of the system.site.xml file

Status: Needs review » Needs work

The last submitted patch, 1496542_22_account_cmi.patch, failed testing.

cosmicdreams’s picture

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

nadavoid’s picture

Assigned: nadavoid » Unassigned

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

cosmicdreams’s picture

Status: Needs work » Needs review
FileSize
48.43 KB

Here's a patch that implements that submit function, let's see how this improves things.

cosmicdreams’s picture

FileSize
47.87 KB

forgot to revert the change to system.module that would have put every system configuration setting into the system.site.xml config file.

Status: Needs review » Needs work

The last submitted patch, 1496542_27_account_cmi.patch, failed testing.

cosmicdreams’s picture

After reviewing previous patches, I'm going to do the following.

  1. Rename config file to system.site-information.xml
  2. Remove config changes to default time zone
  3. Look to conditionally set some of these settings.
cosmicdreams’s picture

Status: Needs work » Needs review
FileSize
47.71 KB

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

gdd’s picture

Can you elaborate on what the issues are with default country and timezone?

Status: Needs review » Needs work

The last submitted patch, 1496542_30_site_information_cmi.patch, failed testing.

cosmicdreams’s picture

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

cosmicdreams’s picture

Status: Needs work » Needs review
FileSize
47.72 KB

might 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?

Status: Needs review » Needs work

The last submitted patch, 1496542_34_site_information_cmi.patch, failed testing.

marcingy’s picture

The issue is this line

$title = '</title><script type="text/javascript">alert("Title XSS!");</script> & < > " \' ';

isn't valid XML and is attempted to being saved to/loaded from config - this of course introduces an interesting question

cosmicdreams’s picture

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

cosmicdreams’s picture

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

sun’s picture

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

cosmicdreams’s picture

Technically there's nothing wrong with it. The site_information seemed more descriptive I can change it back if brevity is preferred.

cosmicdreams’s picture

Status: Needs work » Needs review
FileSize
46.19 KB

This should simply replace all the system.site_information with system.site

Status: Needs review » Needs work

The last submitted patch, 1496542_41_site_information.patch, failed testing.

cosmicdreams’s picture

Status: Needs work » Needs review
FileSize
46.42 KB

second try

Status: Needs review » Needs work
Issue tags: -Configuration system, -Config novice

The last submitted patch, 1496542_43_site_information.patch, failed testing.

cosmicdreams’s picture

Status: Needs work » Needs review
Issue tags: +Configuration system, +Config novice

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

cosmicdreams’s picture

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

Status: Needs review » Needs work

The last submitted patch, 1496542_46_site_information.patch, failed testing.

cosmicdreams’s picture

Status: Needs work » Needs review
FileSize
46.21 KB

Ah well at least that's something. This patch fixes the reuse of the update function.

Status: Needs review » Needs work

The last submitted patch, 1496542_48_site_information.patch, failed testing.

alexpott’s picture

Need to remove this line from node_uninstall() in node.install

    variable_del('default_nodes_main');

And we probably don't want to remove the blank line here

--- a/core/includes/theme.inc
+++ b/core/includes/theme.incundefined
@@ -2529,18 +2529,18 @@ function template_preprocess_html(&$variables) {
     $type = theme_get_setting('favicon_mimetype');
     drupal_add_html_head_link(array('rel' => 'shortcut icon', 'href' => drupal_strip_dangerous_protocols($favicon), 'type' => $type));
   }
-
+  $config = config('system.site');
cosmicdreams’s picture

Status: Needs work » Needs review
FileSize
46.39 KB

This patch implements those comments.

Status: Needs review » Needs work

The last submitted patch, 1496542_51_site_information_cmi.patch, failed testing.

cosmicdreams’s picture

Status: Needs work » Needs review
FileSize
46.39 KB

updated the update function's number so this can pass.

cosmicdreams’s picture

Status: Needs review » Needs work

The last submitted patch, 1496542_53_site_information_cmi.patch, failed testing.

cosmicdreams’s picture

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

cosmicdreams’s picture

Status: Needs work » Needs review
FileSize
47.25 KB

Tests are still failing but I need to brain dump this and get back it later:

-- uses yml now instead of xml.

cosmicdreams’s picture

Oops, I meant this patch.

cosmicdreams’s picture

Dave Reid’s picture

+++ b/core/modules/system/system.tokens.incundefined
@@ -140,22 +140,24 @@ function system_tokens($type, $tokens, array $data = array(), array $options = a
+  $config = config('system.site');
+  $site_mail = $config->get('site_mail');

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

cosmicdreams’s picture

FileSize
46.3 KB

This patch does what Dave says.

Status: Needs review » Needs work
Issue tags: -Configuration system, -Config novice

The last submitted patch, 1496542_awesome_patch.patch, failed testing.

cosmicdreams’s picture

Status: Needs work » Needs review
Issue tags: +Configuration system, +Config novice
sun’s picture

Component: configuration system » system.module
Assigned: Unassigned » sun
FileSize
48.9 KB

Taking this over.

Code of attached patch lives in the config-site-1496542-sun branch.

Dave Reid’s picture

+++ b/core/modules/system/system.tokens.incundefined
@@ -138,24 +138,24 @@ function system_tokens($type, $tokens, array $data = array(), array $options = a
+          $replacements[$original] = empty($default_from) ? ini_get('sendmail_from') : $default_from;

Because ini_get should be considered untrusted, this would need to be escaped using check_plain if $sanitize is TRUE?

sun’s picture

Status: Needs review » Needs work

Reviewed myself - will incorporate the following in a minute:

+++ b/core/includes/theme.inc
@@ -2574,6 +2575,7 @@ function template_preprocess_html(&$variables) {
+  $config = config('system.site');

@@ -2784,17 +2786,18 @@ function template_preprocess_maintenance_page(&$variables) {
+  $config = config('system.site');

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

+++ b/core/modules/contact/contact.install
@@ -68,11 +68,13 @@ function contact_schema() {
+  $site_mail = config('system.site')->get('mail');
+  $default_from = empty($site_mail) ? ini_get('sendmail_from') : $site_mail;
...
+      'recipients' => $default_from,

+++ b/core/modules/system/system.admin.inc
@@ -1462,6 +1462,9 @@ function system_ip_blocking_delete_submit($form, &$form_state) {
+  $site_mail = $config->get('mail');
+  $default_from = empty($site_mail) ? ini_get('sendmail_from') : $site_mail;

@@ -1469,19 +1472,19 @@ function system_site_information_settings() {
+    '#default_value' => $default_from,

Weird resulting variable name; hints at the exact opposite (the ini_get value). Let's clean this up.

+++ b/core/modules/node/node.install
@@ -475,7 +475,6 @@ function node_uninstall() {
-  variable_del('default_nodes_main');

I guess this should stay, and instead should remove the value from system.site.

Overall, this badly needs to be cleaned up though. :(

+++ b/core/modules/system/system.admin.inc
@@ -1519,18 +1522,35 @@ function system_site_information_settings() {
+  //@todo: remove dependence on system_settings_from as this will fire the system_settings_form_submit action.
   return system_settings_form($form);

Replace with @todo + @see to config settings form issue.

+++ b/core/modules/system/system.admin.inc
@@ -1519,18 +1522,35 @@ function system_site_information_settings() {
+    ->set('page.front', $form_state['values']['site_frontpage'])
+    ->set('default_nodes_main', $form_state['values']['default_nodes_main'])
+    ->set('page.403', $form_state['values']['site_403'])
+    ->set('page.404', $form_state['values']['site_404'])

Let's re-order those. Wasn't aware this code was new.

+++ b/core/modules/system/system.install
@@ -1888,6 +1888,13 @@ function system_update_8010() {
 /**
+ * Converts configuration storage for site information to use static files
+ */
+function system_update_8011() {
+  config_install_default_config('system');
+  update_variables_to_config('system.site');
+}
+/**

This needs to happen earlier; very early actually.

sun’s picture

Status: Needs work » Needs review
FileSize
48.5 KB

re: #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.

Status: Needs review » Needs work

The last submitted patch, config.site_.67.patch, failed testing.

sun’s picture

Status: Needs work » Needs review
FileSize
49.04 KB

Fixed those two test failures. As usual, I forgot to update installation profiles accordingly (since I'm grepping on /core only).

cosmicdreams’s picture

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

sun’s picture

FileSize
4.11 KB
51.99 KB

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

Status: Needs review » Needs work

The last submitted patch, config.site_.71.patch, failed testing.

sun’s picture

Status: Needs work » Needs review
FileSize
11.86 KB
62.91 KB

Luckily, 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

sun’s picture

Issue tags: -Config novice

I have nothing to add to the patch to #73.

alexpott’s picture

Issue tags: +Config novice

#73 update_variables_to_config() is not complete bogus.

If the update hook in #71 had been - it would have worked

function system_update_8011() {
  config_install_default_config('system');
  update_variables_to_config('system.site', array(
    'name' => 'site_name',
    'mail' => 'site_mail',
    'slogan' => 'site_slogan',
    'default_nodes_main' => 'default_nodes_main',
    'page.front' => 'site_frontpage',
    'page.403' => 'site_403',
    'page.404' => 'site_404',
  ));
}

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:

function system_update_8011() {
  config_install_default_config('system', 'system.site');
  update_variables_to_config('system.site', array(
    'name' => 'site_name',
    'mail' => 'site_mail',
    'slogan' => 'site_slogan',
    'default_nodes_main' => 'default_nodes_main',
    'page.front' => 'site_frontpage',
    'page.403' => 'site_403',
    'page.404' => 'site_404',
  ));
}

I think this approach might be better because then update_variables_to_config is not duplicating the functionality of config_install_default_config.

alexpott’s picture

Fixing tags

sun’s picture

Issue tags: -Config novice

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

sun’s picture

Any further reviews? This patch pretty much blocks all other config conversions, due to the required upgrade path fixes.

alexpott’s picture

FileSize
4.22 KB
64.03 KB

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

function system_update_8010() {
  config_install_default_config('system', 'system.rss-publishing');
  update_variables_to_config('system.rss-publishing');
}

Status: Needs review » Needs work

The last submitted patch, config.site_.79.patch, failed testing.

alexpott’s picture

FileSize
64.14 KB

Reroll so patch applies against latest 8.x

alexpott’s picture

Status: Needs work » Needs review

Reroll so patch applies against latest 8.x

sun’s picture

FileSize
63.25 KB

Hrm. 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)

Status: Needs review » Needs work

The last submitted patch, config.site_.83.patch, failed testing.

aspilicious’s picture

I can't reproduce the failure...

aspilicious’s picture

Status: Needs work » Needs review

#83: config.site_.83.patch queued for re-testing.

sun’s picture

This patch looks RTBC to me.

chx’s picture

Status: Needs review » Reviewed & tested by the community
FileSize
63.42 KB
1.31 KB

I merely added a line of comment and rerolled with the block test move.

catch’s picture

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

Gábor Hojtsy’s picture

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

cosmicdreams’s picture

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

gdd’s picture

Status: Reviewed & tested by the community » Needs work

I 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?

cosmicdreams’s picture

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

sun’s picture

cosmicdreams’s picture

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

cosmicdreams’s picture

Status: Needs work » Needs review

go testbot

sun’s picture

#88: 1496542_88.patch queued for re-testing.

cosmicdreams’s picture

Hi sun:

Is it ok with you if we land this patch without the name changes of the variables, then handle the name changes later?

cosmicdreams’s picture

sun, 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?

sun’s picture

FileSize
48.18 KB

Merged 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

Status: Needs review » Needs work

The last submitted patch, config.site_.100.patch, failed testing.

aspilicious’s picture

probably a bootstrap.test conflict

sun’s picture

Status: Needs work » Needs review
FileSize
48.28 KB

jeez. These manual merge conflict resolutions for PSR-0 test patches suck big time. Why doesn't git's rename/copy detection kick in?

aspilicious’s picture

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

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

The last submitted patch, config.site_.103.patch, failed testing.

Issue tags: +Configuration system

The last submitted patch, config.site_.103.patch, failed testing.

Gábor Hojtsy’s picture

Issue tags: +D8MI, +sprint

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

sun’s picture

Status: Needs work » Needs review
FileSize
1.82 KB
50.09 KB

Converted site variables in new DrupalKernel code.

chx’s picture

Status: Needs review » Reviewed & tested by the community

Doesn't get simpler than that. Awesome job.

cosmicdreams’s picture

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

chx’s picture

Status: Reviewed & tested by the community » Needs work

I am revoking the RTBC and rerolling. Just a sec.

chx’s picture

Status: Needs work » Needs review
FileSize
50.38 KB

Rerolled without name changes.

chx’s picture

FileSize
50.52 KB

ksenzee pointed out that i missed site_mail and the core/modules/system/config/system.site.yml file. Here.

cosmicdreams’s picture

Status: Needs review » Needs work

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

+++ b/core/includes/mail.inc
@@ -118,7 +118,8 @@ define('MAIL_LINE_ENDINGS', isset($_SERVER['WINDIR']) || strpos($_SERVER['SERVER
+  $default_from = (!empty($site_mail) ? $site_mail : ini_get('sendmail_from'));

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

cosmicdreams’s picture

Status: Needs work » Reviewed & tested by the community

lol, I meant RTBC not needs work

cosmicdreams’s picture

@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?

catch’s picture

Issue tags: -Configuration system, -D8MI, -sprint

#113: 1496542_113.patch queued for re-testing.

Status: Reviewed & tested by the community » Needs work
Issue tags: +Configuration system, +D8MI, +sprint

The last submitted patch, 1496542_113.patch, failed testing.

cosmicdreams’s picture

Status: Needs work » Needs review
FileSize
54.68 KB

rerolled, as the location of a few of these tests have changed and break the patch.

Status: Needs review » Needs work

The last submitted patch, 1496542_119_site_information_cmi.patch, failed testing.

Gábor Hojtsy’s picture

@cosmicdreams: Fails now :/

webflo’s picture

Status: Needs work » Needs review
Issue tags: -Configuration system, -D8MI, -sprint

Status: Needs review » Needs work
Issue tags: +Configuration system, +D8MI, +sprint

The last submitted patch, 1496542_119_site_information_cmi.patch, failed testing.

cosmicdreams’s picture

Status: Needs work » Needs review
FileSize
54.71 KB

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

Status: Needs review » Needs work
Issue tags: -Configuration system, -D8MI, -sprint

The last submitted patch, 1496542_124_site_information_cmi.patch, failed testing.

aspilicious’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work
Issue tags: +Configuration system, +D8MI, +sprint

The last submitted patch, 1496542_124_site_information_cmi.patch, failed testing.

aspilicious’s picture

+++ b/core/modules/system/system.installundefined
@@ -1754,9 +1754,9 @@ function system_update_8001() {
 function system_update_8002() {
-  $front_page = variable_get('site_frontpage');
+  $front_page = config('system.site')->get('site_frontpage');
   if (!isset($front_page)) {
-    variable_set('site_frontpage', 'node');
+    config('system.site')->set('site_frontpage', 'node')->save();
   }
   $theme = variable_get('theme_default');
   if (!isset($theme)) {
@@ -1924,6 +1924,23 @@ function system_update_8010() {

@@ -1924,6 +1924,23 @@ function system_update_8010() {
 }
 
 /**
+ * Moves site system settings from variable to config.
+ *
+ * @ingroup config_upgrade
+ */
+function system_update_8011() {
+  update_variables_to_config('system.site', array(
+    'site_name' => 'site_name',
+    'site_mail' => 'site_mail',
+    'site_slogan' => 'site_slogan',
+    'default_nodes_main' => 'default_nodes_main',
+    'site_frontpage' => 'site_frontpage',
+    'site_403' => 'site_403',
+    'site_404' => 'site_404',
+  ));

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

cosmicdreams’s picture

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

cosmicdreams’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 1496542_129_site_information_cmi.patch, failed testing.

cosmicdreams’s picture

Status: Needs work » Needs review
FileSize
54.28 KB

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

Status: Needs review » Needs work

The last submitted patch, 1496542_132_site_information_cmi.patch, failed testing.

disasm’s picture

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

aspilicious’s picture

But I do find it strange that this worked before...
I'm going to create a test patch...

aspilicious’s picture

Status: Needs work » Needs review
FileSize
57.33 KB

Lets try this one. It probably will fail but hopefully not the update tests :)

Status: Needs review » Needs work

The last submitted patch, 1496542_136_site_information_cmi.patch, failed testing.

aspilicious’s picture

Status: Needs work » Needs review

I changed this

-    $is_front_page = (current_path() == variable_get('site_frontpage', 'user'));
+    $is_front_page = (current_path() == 'user'); //config('system.site')->get('site_frontpage'));

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?

sun’s picture

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

disasm’s picture

aspilicious, 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

vasi1186’s picture

I did some very small changes in the patch from #136:

--- a/core/modules/system/tests/upgrade/upgrade.test
+++ b/core/modules/system/tests/upgrade/upgrade.test
@@ -127,9 +127,6 @@ abstract class UpgradePathTestCase extends WebTestBase {
     // Generate and set a D8-compatible session cookie.
     $this->prepareD8Session();
 
-    // Restore necessary variables.
-    config('system.site')->set('site_mail', 'simpletest@example.com')->save();
-
     drupal_set_time_limit($this->timeLimit);
     $this->setup = TRUE;
   }

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().

--- a/core/modules/system/lib/Drupal/system/Tests/Theme/ThemeTest.php
+++ b/core/modules/system/lib/Drupal/system/Tests/Theme/ThemeTest.php
@@ -77,6 +77,7 @@ class ThemeTest extends WebTestBase {
     // Set the current path to node because theme_get_suggestions() will query
     // it to see if we are on the front page.
     config('system.site')->set('site_frontpage', 'node')->save();
+    drupal_static_reset('drupal_is_front_page');
     _current_path('node');
     $suggestions = theme_get_suggestions(array('node'), 'page');

I reset that variable to make sure that the next call to drupal_is_front_page will not return a cached value.

--- a/core/includes/path.inc
+++ b/core/includes/path.inc
@@ -298,7 +298,8 @@ function drupal_is_front_page() {
   $is_front_page = &$drupal_static_fast['is_front_page'];
 
   if (!isset($is_front_page)) {
-    $is_front_page = (current_path() == 'user'); //config('system.site')->get('site_frontpage'));
+    $is_front_page = (current_path() == config('system.site')->get('site_frontpage'));
   }

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.

Status: Needs review » Needs work

The last submitted patch, 1496542_141_site_information_cmi.patch, failed testing.

aspilicious’s picture

Mmmm menu.test and upgrade.tests are replaced with psr-0 files now. Srry :)

vasi1186’s picture

Status: Needs work » Needs review
FileSize
55.45 KB

Attached the new patch.

One more thing that I did, and that should most probably be changed, is this:

--- a/core/lib/Drupal/Core/Config/DatabaseStorage.php
+++ b/core/lib/Drupal/Core/Config/DatabaseStorage.php
@@ -20,6 +20,9 @@ class DatabaseStorage extends StorageBase {
     // handle it if need be.
     $data = array();
     try {
+      if (!function_exists('db_query')) {
+        throw new Exception("no db_query");
+      }
       $raw = db_query('SELECT data FROM {config} WHERE name = :name', array(':name' => $this->name))->fetchField();
       if ($raw !== FALSE) {
         $data = $this->decode($raw);

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!

effulgentsia’s picture

Here's how that hunk is being dealt with in #1576322: page_cache_without_database doesn't return cached pages without the database:

+  if (function_exists('db_query')) {
      try {
        $raw = db_query('SELECT data FROM {config} WHERE name = :name', array(':name' => $this->name))->fetchField();
        if ($raw !== FALSE) {
          $data = $this->decode($raw);
        }
      }
      catch (Exception $e) {
      }
+  }
Gábor Hojtsy’s picture

Is that the type of solution we need?

gdd’s picture

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

vasi1186’s picture

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

aspilicious’s picture

Looks good now :)

vasi1186’s picture

vasi1186’s picture

effulgentsia’s picture

Is 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?

Gábor Hojtsy’s picture

@effulgentsia: I'd have the same question? What is still holding this up? Any complaints against the latest patch?

gdd’s picture

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

cosmicdreams’s picture

Cool, have we reached that consensus?

Gábor Hojtsy’s picture

@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?

gdd’s picture

Status: Needs review » Needs work
disasm’s picture

Status: Needs work » Needs review
FileSize
49.17 KB
54.78 KB

attached is a reroll of the latest patch that meets the guidelines to the best of my knowledge.

Status: Needs review » Needs work
Issue tags: -Configuration system, -D8MI, -sprint

The last submitted patch, drupal-1496542-site_information_cmi-158.patch, failed testing.

andypost’s picture

Status: Needs work » Needs review
Issue tags: +Configuration system, +D8MI, +sprint
andypost’s picture

Fixed one missed variable name

Looks like dots in key-names leads to broken config values

# cat system.site.yml
name: Site-Install
mail: admin@example.com
slogan: ''
max_posts: '10'
page.front: user
page.404: ''
page.403: ''
page:
    front: node
    403: ''
    404: ''
Gábor Hojtsy’s picture

Broken? It is just nested. What is broken?

andypost’s picture

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

disasm’s picture

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

effulgentsia’s picture

Status: Needs review » Needs work

I reviewed #161 and it looks good to me. A couple minor points:

+++ b/core/modules/system/lib/Drupal/system/Tests/Theme/ThemeTest.php
@@ -77,7 +77,8 @@ class ThemeTest extends WebTestBase {
+    drupal_static_reset('drupal_is_front_page');

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.

+++ b/core/modules/system/lib/Drupal/system/Tests/Upgrade/UpgradePathTestBase.php
@@ -134,10 +134,6 @@ abstract class UpgradePathTestBase extends WebTestBase {
-    // Restore necessary variables.
-    // @todo Convert into config('system.site')->set('mail')?
-    $this->variable_set('site_mail', 'simpletest@example.com');
-

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.

+++ b/core/modules/system/system.admin.inc
@@ -1530,6 +1536,8 @@ function system_site_information_settings() {
+  // @todo Replace with configuration system settings form.
+  $form['#submit'][] = 'system_site_information_settings_submit';

#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().

effulgentsia’s picture

+++ b/core/modules/system/system.admin.inc
@@ -1495,17 +1501,17 @@ function system_site_information_settings() {
-  $form['front_page']['default_nodes_main'] = array(
+  $form['front_page']['max_posts'] = array(

Also, 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?

gdd’s picture

Also, 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?

+++ b/core/modules/system/system.admin.incundefined
@@ -1522,7 +1528,7 @@ function system_site_information_settings() {
   $form['error_page']['site_404'] = array(
...
-    '#default_value' => variable_get('site_404', ''),
+    '#default_value' => $site_config->get('page.404'),

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

sun’s picture

Status: Needs work » Postponed
FileSize
7.5 KB

I'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. ;)

aspilicious’s picture

Assigned: sun » aspilicious
Status: Postponed » Active

I'm going trying to reroll

aspilicious’s picture

Status: Active » Needs review
FileSize
57.3 KB

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

Status: Needs review » Needs work

The last submitted patch, 1496542-site_information_cmi-170.patch, failed testing.

aspilicious’s picture

Status: Needs work » Needs review
FileSize
57.45 KB

Next try

Status: Needs review » Needs work

The last submitted patch, 1496542-site_information_cmi-172.patch, failed testing.

aspilicious’s picture

Apparantly we still need the maintenance mode checks on upgrade.

sun’s picture

Assigned: aspilicious » sun
Status: Needs work » Needs review
FileSize
3.7 KB
54.36 KB

Attached is #172 with default_nodes_main reverted.

sun’s picture

FileSize
723 bytes
54.26 KB

64738e2 Fixed bogus preemptive attempt of using configuration system in upgrade path tests.

aspilicious’s picture

Makes sense...

andypost’s picture

Filed #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

+++ b/core/modules/system/lib/Drupal/system/Tests/Upgrade/UpgradePathTestBase.phpundefined
@@ -135,7 +135,6 @@ abstract class UpgradePathTestBase extends WebTestBase {
     // Restore necessary variables.
-    // @todo Convert into config('system.site')->set('mail')?
     $this->variable_set('site_mail', 'simpletest@example.com');

I think this need better comment to explain a usage of $this->variable_set()

+++ b/core/modules/system/system.admin.incundefined
@@ -1496,17 +1502,17 @@ function system_site_information_settings() {
-    '#default_value' => variable_get('default_nodes_main', 10),
+    '#default_value' => $site_config->get('default_nodes_main'),

Maybe be we leave this as is and convert to node variable in filed issue?

+++ b/core/modules/taxonomy/taxonomy.pages.incundefined
@@ -43,7 +43,7 @@ function taxonomy_term_page(Term $term) {
-  if ($nids = taxonomy_select_nodes($term->tid, TRUE, variable_get('default_nodes_main', 10))) {
+  if ($nids = taxonomy_select_nodes($term->tid, TRUE, config('system.site')->get('default_nodes_main'))) {

And this related to node module too?

aspilicious’s picture

See #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?

sun’s picture

FileSize
5.45 KB
52.93 KB

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

sun’s picture

Status: Needs review » Reviewed & tested by the community

This has been RTBC before. The last patch only adjusted for #1666074: Move 'default_nodes_main' setting to node module

Jose Reyero’s picture

+1 Looks good to me

webchick’s picture

Title: Convert site information to config system » [Change notice] Convert site information to config system
Priority: Normal » Critical
Status: Reviewed & tested by the community » Active
Issue tags: +Needs change record

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

Gábor Hojtsy’s picture

Issue tags: -sprint

Thanks!

tim.plunkett’s picture

Status: Active » Needs review

Change record added: http://drupal.org/node/1668806

sun’s picture

Priority: Critical » Normal
Status: Needs review » Fixed
Issue tags: -Needs change record

Thanks!

Tor Arne Thune’s picture

Title: [Change notice] Convert site information to config system » Convert site information to config system

Automatically closed -- issue fixed for 2 weeks with no activity.