Convert logging and error settings to configuration system

Working on this...

Ok, this turned out to be not so novice after all, but I think I got it covered.

- The logging settings are split between system.module, dblog.module and syslog.module
- So each of them now has a $module.logging config file
- I decided to use logging for two reasons: a) dblog and syslog are really about only logging. b) while the system one is actually only about errors, the settings form is called *logging_settings* and it's consistent with the others.
- syslog.module has a dynamic default value, which is now set in hook_install()
- Update functions for each of them.

Testbot, go for it!

Oh, almost forgot, I also removed an IMHO pointless test assertion that that directly verified the variable directly in the database, *additionally* to the API.

It would really help if I'd be able to spell unserialize correctly... D'oh.

 function dblog_uninstall() {
-  variable_del('dblog_row_limit');
+  config('dblog.logging')->delete();
+}

Is this necessary now?

Hm, I guess the upgrade path tests fail because the basic upgrade path that creates the initial directories and stuff isn't in yet, right?

@catch: That's what I understood from what beejebus explained to me. I guess that's something that could be automated once we have file naming patterns and stuff like that place, similar to the added submit functions. It probably also makes sense to push/decide on the system_settings_form() issue before converting too many settings forms.

Few things I update in patch from #6

• rerolled system update hook
• Uninstall hook will not be needed once #1444766: Determine method for automatically cleaning up configuration after a module is uninstalled lands not sure if we still want to include config removals until that happens though
• If there is only one usage of $config object I just chained as much as possible

We should be deleting after we have saved to ensure that data is not lost in the conversion process in the update hooks.

• Updated error.test, syslog.test to fix the broken tests for "Syslog functionality" and "Drupal error handlers". The other 3 failing tests haven't been investigated yet.
• Also, now that we're using config->save() in a submit callback instead of system_settings_form(), the "The configuration options have been saved." message no longer appears, which caused one test failure. I added the message back into system_logging_settings_submit(). Is that what has been done in other areas we replaced system_settings_form()?

Oops, it looks my patch does not include the .xml config files. Those files initially showed up as untracked files when I applied the patch at #10, so I tried using git add to add them in. But it looks like the files don't show up when I ran git diff to produce my patch.

I've dug a little bit more into the failing upgrade tests. An example failure is

file_put_contents(sites/default/files/simpletest/config_simpletest694263/system.logging.xml): failed to open stream: No such file or directory

The issue is that the directory 'sites/default/files/simpletest/config_simpletest694263' does not exist when system_update_8006() runs, causing the $config->save() to attempt to create a file in a non-existent directory.

Looking at config_get_config_directory(), I see that a special config directory is used when SimpleTest runs, so it's hard to tell if it's a test issue or a legitimate issue.

From what I understand, as said in #9, the upgrade path tests fail because there is no upgrade path yet at all for the new config system, which first needs to create the directory and so on. Then, the upgrade functions here need to be configured with hook_update_dependencies() to run that one first.

The upgrade test issue has been fixed, trying a new patch.

I also don't think the current hook_update_N() implementations would work if a variable did not have an entry in the variable table. This can happen when variable_set() is never called for a variable (in which case variable_get() falls back to the default value). Here is an example:

$value = db_query("SELECT value FROM {variable} WHERE name = 'dblog_row_limit'")->fetchField();
$config->set('row_limit', unserialize($value));

I wrote a nice helper method to transfer variables to config to solve that problem.

A helper function already exists update_variables_to_config for updating variables.

Currently, update_variables_to_config() has no usages outside unit tests, so I merged the two helper methods together rather than stick with the old one. In the upgrade path, I prefer to explicitly state the default values instead of relying on the default values loaded from the default configuration files because:

• As I was writing my tests, it seemed that config() may return an empty configuration object during the D7 > D8 upgrade process instead of the default configuration, and in any case I would not assume the default configuration would be available during the upgrade process unless a unit test proves that to me. The unit tests for update_variables_to_config() would not catch this issue because they are not running in a true upgrade environment.
• In some cases, the default is dynamically determined (e.g.: syslog_facility)/
• Defaults sometimes change between Drupal versions (e.g.: error_level from D6 to D7). In that case, the preference often is to use the new (and improved) default for new site installs but still use the old default for upgraded sites, so as not to disturb anything.

I also dropped the try-catch(all) block because it was suppressing the exception (although it did log it), which could cause the update to incorrectly succeed.

This patch has a few more comments for some discussion points in #21 (variables may not have an entry in the variable table) and #24 (config() may return an empty configuration object during the D7 > D8 upgrade process).

I would suggest if you want to change the update helper function that you split this out into a seperate issue becaue there are at least 3 patches based on the existing helper function that are ready for final review.

And if we are going to change the behaviour I think it needs wider discussion than being part of this patch.

The upgrade has been discussed elsewhere and basically and it is the responsibility of the upgrading function to load their default config eg config_install_default_config('search'); and discussed here http://drupal.org/node/1497132#comment-5798610. So the default config will always be available maybe some docs need updating?

Thanks for pointing me to that thread, marcingy, when I have some time I'll reactivate it with some input of mine from building this patch.

Tagging

Simplified version of patch.

this install locally without issue...

#31: logging.patch queued for re-testing.

Ah 8009 ha been merged will fix tomorrow.

-  $error_level = variable_get('error_level', ERROR_REPORTING_DISPLAY_ALL);
+  $error_level = config('system.logging')->get('error_level');

This change concerns me a bit. Currently variable_get() doesn't hit the database (or anything apart from the $conf global), but config() will, so what happens if you get an exception due to something the config system itself relies upon like the database? Also is the config system definitely always available when this code is called?

Just a re-roll for now will try to look ito catch's question in the next couple of days.

This should pass tests.

+++ b/core/modules/syslog/syslog.installundefined
@@ -6,10 +6,31 @@
 /**
- * Implements hook_uninstall().
+ * Implements hook_install().
  */
-function syslog_uninstall() {
-  variable_del('syslog_identity');
-  variable_del('syslog_facility');
-  variable_del('syslog_format');
+function syslog_install() {
+  // The default facility setting depends on the operating system, so it needs
+  // to be set dynamically during installation.
+  config('syslog.logging')
+    ->set('syslog_facility', defined('LOG_LOCAL0') ? LOG_LOCAL0 : LOG_USER)
+    ->save();

syslog_facility is initialized here.

A static default value is not possible. The only possible thing would be to define an empty placeholder, not sure if that makes sense.

Powered by Dreditor.

Re-roll with empty value in xml.

Not sure why this fails applies locally and my code base is current.

We need to set facility on update if it isn't in the db. Not sure if this fixes the issues or not because my local install is giving inconsistent results.

Last patch had extra stuff in it :(

#49: logging-new-49.patch queued for re-testing.

Re-attaching the patch that passes - if this pass it seems like the issue is outsides cmi and head has issues in some ways.

Yes, the five upgrade tests randomly fail for other issues as well.

Note this the patch to review http://drupal.org/node/1493108#comment-6014680 i'll re-upload it latter when I am at my desktop.

Correct patch version

Rerolls against HEAD. No other changes.

update_variables_to_config() was changed and now requires the second argument, that's why the tests fail. Modules are now required to explicitly pass the variable => config mapping in there.

Ahh. Thanks Berdir. Updated functionality for this and ran tests patch effects locally. Had a couple fails but when trying to mimic the config->save options locally from CLI I would get an error cause of file permissions. Gonna see what happens on testbot.

So it seems as though my variables aren't setting for the tests or maybe my error test handler isn't working correctly. If I try to manually change the error settings on the install via the UI it works and if I manually enable the error test module all is working as well.

I did find an error though in my testing and rerolling that removed the default system.logging.xml and though I'm sure that's not the only problem as I'm still getting the error locally here it is so we have the most recent version to work off of. Any ideas to point me in the right direction?

language_negotiation_include() is in language.module. I think in the parent::setup() function above, you need to pass array('language') to enable the language module.

Yeah. I think you are right disasm. The one I can't seem to clear is this:

Found error message: Exception: Drupal is awesome in error_test_trigger_exception() (line . Other ErrorHandlerTest.php 98 Drupal\system\Tests\System\ErrorHandlerTest->testExceptionHandler()

Found DatabaseExceptionWrapper in error page. Other ErrorHandlerTest.php 104 Drupal\system\Tests\System\ErrorHandlerTest->testExceptionHandler()

Found SELECT * FROM bananas_are_awesome in error page. Other ErrorHandlerTest.php 105 Drupal\system\Tests\System\ErrorHandlerTest->testException

Found 'in error_test_trigger_pdo_exception() (line ' in error page. Other ErrorHandlerTest.php 107 Drupal\system\Tests\System\ErrorHandlerTest->testExceptionHandler()

Those are the fails that I get running the tests individually rather than all together. When I try to do manual testing of those I'm able to replicate and get the expected text that the test run is looking for.

Those are in this portion of patch:

--- a/core/modules/system/lib/Drupal/system/Tests/System/ErrorHandlerTest.php
+++ b/core/modules/system/lib/Drupal/system/Tests/System/ErrorHandlerTest.php
@@ -29,6 +29,7 @@ class ErrorHandlerTest extends WebTestBase {
    * Test the error handler.
    */
   function testErrorHandler() {
+    $config = config('system.logging');
     $error_notice = array(
       '%type' => 'Notice',
       '!message' => 'Undefined variable: bananas',
@@ -49,7 +50,7 @@ class ErrorHandlerTest extends WebTestBase {
     );
 
     // Set error reporting to collect notices.
-    variable_set('error_level', ERROR_REPORTING_DISPLAY_ALL);
+    $config->set('error_level', ERROR_REPORTING_DISPLAY_ALL)->save();
     $this->drupalGet('error-test/generate-warnings');
     $this->assertResponse(200, t('Received expected HTTP status code.'));
     $this->assertErrorMessage($error_notice);
@@ -57,7 +58,7 @@ class ErrorHandlerTest extends WebTestBase {
     $this->assertErrorMessage($error_user_notice);
 
     // Set error reporting to not collect notices.
-    variable_set('error_level', ERROR_REPORTING_DISPLAY_SOME);
+    $config->set('error_level', ERROR_REPORTING_DISPLAY_SOME)->save();
     $this->drupalGet('error-test/generate-warnings');
     $this->assertResponse(200, t('Received expected HTTP status code.'));
     $this->assertNoErrorMessage($error_notice);
@@ -65,7 +66,7 @@ class ErrorHandlerTest extends WebTestBase {
     $this->assertErrorMessage($error_user_notice);
 
     // Set error reporting to not show any errors.
-    variable_set('error_level', ERROR_REPORTING_HIDE);
+    $config->set('error_level', ERROR_REPORTING_HIDE)->save();
     $this->drupalGet('error-test/generate-warnings');
     $this->assertResponse(200, t('Received expected HTTP status code.'));
     $this->assertNoErrorMessage($error_notice);

working on this tonight. More to come.

All config system conversions are API changes, so tagging as such.

I've worked up a new patch for this based on the patch in #64. The issues identified in #67 will likely still be issues with this patch.

Things in this patch:
- converted xml to yml
- added yml files for dblog and syslog
- modified naming of config values to use our new standards
- renamed the system.logging.yml file to system.logging.settings.yml (let's discuss)

Thanks for working on this! :)

+++ b/core/includes/errors.inc
@@ -172,7 +172,7 @@ function _drupal_render_exception_safe($exception) {
+  $error_level = config('system.logging.settings')->get('error_level');

Oh, hm, there shouldn't be a .settings suffix if a module has multiple config objects containing settings. The $module.settings standard only applies to modules that only have one set of settings. Perhaps we need to clarify the howto/tutorial?

I've performed a range of adjustments.

+++ b/core/modules/dblog/config/dblog.settings.yml
@@ -0,0 +1 @@
\ No newline at end of file

Bleh. I thought I had fixed all of them in the re-roll...

Thanks sun, I figured there was a detail that would modify the naming rule.

#73: drupal8.config-system-logging.73.patch queued for re-testing.

+++ b/core/includes/errors.incundefined
@@ -10,22 +10,22 @@ use Symfony\Component\HttpFoundation\Response;
+const ERROR_REPORTING_HIDE = 'hide';

This feels like an API change, I don't think it's a bad one but
1) is it needed here?
2) if we keep it in here (I'm fine with that) we need to make sure we make a change notification for that.

Other than that ==> rtbc it is I think

-26 days to next Drupal core point release.

whoops

I think it makes sense to keep it here, since, frankly, values like 0, 1, 2, 3 do not make any sense in a configuration file.

The good news is that all of core is using the error level constants already, so the actual values of those constants is completely irrelevant for the runtime code. :)

For that reason, I'm also not sure whether the change would require a change notice. All PHP code should be using the constants anyway already, and people will have to rewrite/rebuild their entire configuration for D8 anyway...

That said, the system module update does not translate the old numeric value to the new string identifier yet.

Hmmm that means we need to update the variables before converting.
I would like to push #1348162: Add update_variable_*() as fast as possible...

That issue is waiting for months now...

Or we simply update the value.

Yeah thats fine to...
But I still want the other issue being fixed... :p

I would like to have a test for this upgrade. When thats done this is rtbc.

It looks like we have sufficient tests ;)

2715c60 Fixed upgrade path for error_level for sites that do not have error_level configured.

Good to go now :). Thnx!

I think we might have an issue in system_update_8014 as we'll be calling code with error_level set to an invalid value.

+++ b/core/modules/system/system.installundefined
@@ -1972,6 +1972,27 @@ function system_update_8013() {
+function system_update_8014() {
+  update_variables_to_config('system.logging', array(
+    'error_level' => 'error_level',
+  ));
+  // Update the numeric error_level value to a string identifier.
+  $config = config('system.logging');
+  $map = array(
+    '0' => 'hide',
+    '1' => 'some',
+    '2' => 'all',
+    '3' => 'verbose',
+    // Also map the system.logging:error_level default value.
+    'all' => 'all',
+  );
+  $config->set('error_level', $map[$config->get('error_level')]);
+  $config->save();
+}

The call to update_variables_to_config() will set error_level incorrectly and then we map it to the new values and save. It seems odd/inefficient/wrong to set the value twice in the same update. We could do something like this...

$error_level = db_result(db_query("SELECT value FROM {variable} WHERE name = 'error_level'"));
// Only do the conversion if we have a value and it is numeric.
if ($error_level && is_numeric($error_level)) {
  $config = config('system.logging');
  $map = array(
    '0' => 'hide',
    '1' => 'some',
    '2' => 'all',
    '3' => 'verbose',
  );
  // Update error_level value to a string identifier.
  $config->set('error_level', $map[$error_level]);
  $config->save();
  // Delete the migrated variable.
  db_delete('variable')->condition('name', 'error_level')->execute();
}

Changing issue status to needs review...

@aspilicious pointed out on IRC that we need to do other things from update_varaibles_to_config().

Patch attached solves this.

If we're OK with update 8014 then the rest of this patch looks good. Recommend RTBC

Cool. Asked Greg eyeball this at #mwds, and he said it was good to go!

Committed and pushed to 8.x. Thanks! :D

+++ b/core/modules/system/system.install
@@ -1972,6 +1972,32 @@ function system_update_8013() {
+  $error_level = db_query("SELECT value FROM {variable} WHERE name = 'error_level'")->fetchField();

All variables are stored as serialized PHP.

+++ b/core/modules/system/system.install
@@ -1972,6 +1972,32 @@ function system_update_8013() {
+  // Only do the conversion if we have a value and it is numeric.
+  if ($error_level && is_numeric($error_level)) {
...
+      '0' => 'hide',

If the value was '0', then the $error_level condition equals FALSE. You wanted to do a strict type check on !== FALSE here.

Doh! My bad.

Sun's patch is great. Applied and ran manual tests both with and without error_level in the variables table and everything works as expected.

Committed/pushed the follow-up, thanks!