The logging and error settings at admin/config/development/logging need to be converted to use the new configuration system.

Tasks

  • Create a default config file named system.logging_and_errors.xml 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 these variables to use the config system.
  • Write an upgrade path from D7 -> D8 to migrate existing data to the new system and drop the appropriate variables.

This would be a good task for someone wanting to get up to speed on how the new config system works. Feel free to ping me on IRC if you need help.

Files: 
CommentFileSizeAuthor
#91 drupal8.config-system-logging.91.patch1.3 KBsun
PASSED: [[SimpleTest]]: [MySQL] 39,743 pass(es). View
#88 config.system-logging.88.patch16.24 KBalexpott
PASSED: [[SimpleTest]]: [MySQL] 37,256 pass(es). View
#88 84-88-interdiff.txt1.41 KBalexpott
#84 config.system-logging.84.patch16 KBsun
PASSED: [[SimpleTest]]: [MySQL] 37,227 pass(es). View
#84 interdiff.txt490 bytessun
#81 drupal8.config-system-logging.81.patch15.92 KBsun
FAILED: [[SimpleTest]]: [MySQL] 37,219 pass(es), 0 fail(s), and 8 exception(s). View
#81 interdiff.txt594 bytessun
#73 drupal8.config-system-logging.73.patch15.63 KBsun
PASSED: [[SimpleTest]]: [MySQL] 37,222 pass(es). View
#72 drupal8.config-system-logging.72.patch15.66 KBsun
PASSED: [[SimpleTest]]: [MySQL] 37,117 pass(es). View
#72 interdiff.txt6.96 KBsun
#71 1493108_71_logging_cmi.patch15.2 KBcosmicdreams
PASSED: [[SimpleTest]]: [MySQL] 37,119 pass(es). View
#64 1493108-64.patch14.71 KBkbasarab
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: PHP Fatal error encountered during run_tests.sh. See review log for details.. View
#62 1493108-62.patch13.58 KBkbasarab
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: PHP Fatal error encountered during run_tests.sh. See review log for details.. View
#62 interdiff.txt1.68 KBkbasarab
#59 1493108-59-reroll.patch13.34 KBkbasarab
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: PHP Fatal error encountered during run_tests.sh. See review log for details.. View
#58 logging-new-49.patch14.19 KBmarcingy
PASSED: [[SimpleTest]]: [MySQL] 36,616 pass(es). View
#53 logging-new-48.patch21.69 KBmarcingy
FAILED: [[SimpleTest]]: [MySQL] 36,225 pass(es), 5 fail(s), and 0 exception(s). View
#49 logging-new-49.patch14.19 KBmarcingy
FAILED: [[SimpleTest]]: [MySQL] 36,247 pass(es), 5 fail(s), and 0 exception(s). View
#48 logging-new-48.patch21.69 KBmarcingy
PASSED: [[SimpleTest]]: [MySQL] 36,569 pass(es). View
#46 logging-new-46.patch13.86 KBmarcingy
FAILED: [[SimpleTest]]: [MySQL] 36,257 pass(es), 5 fail(s), and 0 exception(s). View
#44 logging-new-1493108-44.patch13.85 KBmarcingy
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch logging-new-1493108-44.patch. Unable to apply patch. See the log in the details link for more information. View
#40 logging-new.patch13.83 KBmarcingy
PASSED: [[SimpleTest]]: [MySQL] 36,602 pass(es). View
#38 logging-new.patch13.81 KBmarcingy
FAILED: [[SimpleTest]]: [MySQL] 36,611 pass(es), 1 fail(s), and 3 exception(s). View
#31 logging.patch13.81 KBmarcingy
FAILED: [[SimpleTest]]: [MySQL] Invalid PHP syntax in core/modules/system/system.install. View
#26 1493108-26.patch24.17 KBMike Wacker
PASSED: [[SimpleTest]]: [MySQL] 35,076 pass(es). View
#24 1493108-24.patch23.81 KBMike Wacker
PASSED: [[SimpleTest]]: [MySQL] 35,056 pass(es). View
#21 1493108-21.patch17.17 KBMike Wacker
PASSED: [[SimpleTest]]: [MySQL] 35,064 pass(es). View
#14 1493108-14.patch14.59 KBMike Wacker
FAILED: [[SimpleTest]]: [MySQL] 35,664 pass(es), 19 fail(s), and 8 exception(s). View
#10 1493108-log_settings.patch12.9 KBRok Žlender
FAILED: [[SimpleTest]]: [MySQL] 35,624 pass(es), 21 fail(s), and 8 exception(s). View
#6 doh.patch13.32 KBBerdir
FAILED: [[SimpleTest]]: [MySQL] 35,612 pass(es), 21 fail(s), and 8 exception(s). View
#2 convert_logging_settings.patch13.32 KBBerdir
FAILED: [[SimpleTest]]: [MySQL] 35,570 pass(es), 22 fail(s), and 3 exception(s). View

Comments

Berdir’s picture

Assigned: Unassigned » Berdir

Working on this...

Berdir’s picture

Status: Active » Needs review
FileSize
13.32 KB
FAILED: [[SimpleTest]]: [MySQL] 35,570 pass(es), 22 fail(s), and 3 exception(s). View

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!

Berdir’s picture

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.

beejeebus’s picture

Issue tags: -Config novice

oh nice. just removing the 'Config novice' tag.

Status: Needs review » Needs work

The last submitted patch, convert_logging_settings.patch, failed testing.

Berdir’s picture

Status: Needs work » Needs review
FileSize
13.32 KB
FAILED: [[SimpleTest]]: [MySQL] 35,612 pass(es), 21 fail(s), and 8 exception(s). View

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

Status: Needs review » Needs work

The last submitted patch, doh.patch, failed testing.

catch’s picture

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

Is this necessary now?

Berdir’s picture

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.

Rok Žlender’s picture

Status: Needs work » Needs review
FileSize
12.9 KB
FAILED: [[SimpleTest]]: [MySQL] 35,624 pass(es), 21 fail(s), and 8 exception(s). View

Few things I update in patch from #6

Berdir’s picture

Assigned: Berdir » Unassigned

Status: Needs review » Needs work

The last submitted patch, 1493108-log_settings.patch, failed testing.

marcingy’s picture

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

Mike Wacker’s picture

FileSize
14.59 KB
FAILED: [[SimpleTest]]: [MySQL] 35,664 pass(es), 19 fail(s), and 8 exception(s). View
  • 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()?
beejeebus’s picture

Status: Needs work » Needs review

for bot

Mike Wacker’s picture

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.

Status: Needs review » Needs work

The last submitted patch, 1493108-14.patch, failed testing.

heyrocker’s picture

#10: No we should not include the config removals, I'm pretty sure that patch will land and things will take care of themselves.

Mike Wacker’s picture

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.

Berdir’s picture

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.

Mike Wacker’s picture

FileSize
17.17 KB
PASSED: [[SimpleTest]]: [MySQL] 35,064 pass(es). View

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.

Mike Wacker’s picture

Status: Needs work » Needs review
marcingy’s picture

Status: Needs review » Needs work

A helper function already exists update_variables_to_config for updating variables.

Mike Wacker’s picture

FileSize
23.81 KB
PASSED: [[SimpleTest]]: [MySQL] 35,056 pass(es). View

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.

Mike Wacker’s picture

Status: Needs work » Needs review
Mike Wacker’s picture

FileSize
24.17 KB
PASSED: [[SimpleTest]]: [MySQL] 35,076 pass(es). View

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

marcingy’s picture

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?

Mike Wacker’s picture

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.

heyrocker’s picture

Status: Needs review » Needs work

I agree with marcingy, this should be reverted to use the existing update_variables_to_config() and the changes to that function should be broken out into a new patch. The same goes for the test around that functionality.

Once that is done I think this will probably be really close to getting in.

cosmicdreams’s picture

Issue tags: +Config novice

Tagging

marcingy’s picture

Status: Needs work » Needs review
FileSize
13.81 KB
FAILED: [[SimpleTest]]: [MySQL] Invalid PHP syntax in core/modules/system/system.install. View

Simplified version of patch.

Status: Needs review » Needs work

The last submitted patch, logging.patch, failed testing.

marcingy’s picture

Status: Needs work » Needs review

this install locally without issue...

marcingy’s picture

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

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

The last submitted patch, logging.patch, failed testing.

marcingy’s picture

Ah 8009 ha been merged will fix tomorrow.

catch’s picture

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

marcingy’s picture

Status: Needs work » Needs review
FileSize
13.81 KB
FAILED: [[SimpleTest]]: [MySQL] 36,611 pass(es), 1 fail(s), and 3 exception(s). View

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

Status: Needs review » Needs work

The last submitted patch, logging-new.patch, failed testing.

marcingy’s picture

Status: Needs work » Needs review
FileSize
13.83 KB
PASSED: [[SimpleTest]]: [MySQL] 36,602 pass(es). View

This should pass tests.

heyrocker’s picture

Status: Needs review » Needs work
+++ b/core/modules/syslog/syslog.moduleundefined
@@ -45,18 +45,19 @@ function syslog_help($path, $arg) {
+      '#default_value' => $config->get('syslog_facility'),

'syslog_facility' appears to be missing from the default config file.

Other than that I think this is good to go.

12 days to next Drupal core point release.

Berdir’s picture

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

heyrocker’s picture

Ah I see. I would like to have an empty placeholder in the default config, just so we can see everything that is defined by looking at the initial file.

marcingy’s picture

Status: Needs work » Needs review
FileSize
13.85 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch logging-new-1493108-44.patch. Unable to apply patch. See the log in the details link for more information. View

Re-roll with empty value in xml.

Status: Needs review » Needs work

The last submitted patch, logging-new-1493108-44.patch, failed testing.

marcingy’s picture

Status: Needs work » Needs review
FileSize
13.86 KB
FAILED: [[SimpleTest]]: [MySQL] 36,257 pass(es), 5 fail(s), and 0 exception(s). View

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

Status: Needs review » Needs work

The last submitted patch, logging-new-46.patch, failed testing.

marcingy’s picture

Status: Needs work » Needs review
FileSize
21.69 KB
PASSED: [[SimpleTest]]: [MySQL] 36,569 pass(es). View

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.

marcingy’s picture

FileSize
14.19 KB
FAILED: [[SimpleTest]]: [MySQL] 36,247 pass(es), 5 fail(s), and 0 exception(s). View

Last patch had extra stuff in it :(

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

The last submitted patch, logging-new-49.patch, failed testing.

marcingy’s picture

Status: Needs work » Needs review

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

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

The last submitted patch, logging-new-49.patch, failed testing.

marcingy’s picture

FileSize
21.69 KB
FAILED: [[SimpleTest]]: [MySQL] 36,225 pass(es), 5 fail(s), and 0 exception(s). View

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

marcingy’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, logging-new-48.patch, failed testing.

Tor Arne Thune’s picture

Status: Needs work » Needs review

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

marcingy’s picture

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.

marcingy’s picture

FileSize
14.19 KB
PASSED: [[SimpleTest]]: [MySQL] 36,616 pass(es). View

Correct patch version

kbasarab’s picture

FileSize
13.34 KB
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: PHP Fatal error encountered during run_tests.sh. See review log for details.. View

Rerolls against HEAD. No other changes.

Status: Needs review » Needs work

The last submitted patch, 1493108-59-reroll.patch, failed testing.

Berdir’s picture

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.

kbasarab’s picture

Assigned: Unassigned » kbasarab
Status: Needs work » Needs review
FileSize
1.68 KB
13.58 KB
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: PHP Fatal error encountered during run_tests.sh. See review log for details.. View

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.

Status: Needs review » Needs work

The last submitted patch, 1493108-62.patch, failed testing.

kbasarab’s picture

Status: Needs work » Needs review
FileSize
14.71 KB
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: PHP Fatal error encountered during run_tests.sh. See review log for details.. View

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?

Status: Needs review » Needs work

The last submitted patch, 1493108-64.patch, failed testing.

disasm’s picture

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.

kbasarab’s picture

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);
heyrocker’s picture

On top of any other problems

- This file needs to be converted to YAML per #1470824: XML encoder can only handle a small subset of PHP arrays, so switch to YAML
- The names need to match the new naming conventions laid out at http://drupal.org/node/1667896

cosmicdreams’s picture

working on this tonight. More to come.

sun’s picture

Issue tags: +API change

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

cosmicdreams’s picture

Status: Needs work » Needs review
FileSize
15.2 KB
PASSED: [[SimpleTest]]: [MySQL] 37,119 pass(es). View

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)

sun’s picture

FileSize
6.96 KB
15.66 KB
PASSED: [[SimpleTest]]: [MySQL] 37,117 pass(es). View

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.

sun’s picture

FileSize
15.63 KB
PASSED: [[SimpleTest]]: [MySQL] 37,222 pass(es). View
+++ 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...

cosmicdreams’s picture

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

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

The last submitted patch, drupal8.config-system-logging.73.patch, failed testing.

alexpott’s picture

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

Status: Needs review » Needs work
+++ 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.

aspilicious’s picture

Status: Needs work » Needs review

whoops

sun’s picture

Assigned: kbasarab » Unassigned

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.

aspilicious’s picture

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

sun’s picture

FileSize
594 bytes
15.92 KB
FAILED: [[SimpleTest]]: [MySQL] 37,219 pass(es), 0 fail(s), and 8 exception(s). View

Or we simply update the value.

aspilicious’s picture

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.

Status: Needs review » Needs work

The last submitted patch, drupal8.config-system-logging.81.patch, failed testing.

sun’s picture

Status: Needs work » Needs review
FileSize
490 bytes
16 KB
PASSED: [[SimpleTest]]: [MySQL] 37,227 pass(es). View

It looks like we have sufficient tests ;)

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

aspilicious’s picture

Status: Needs review » Reviewed & tested by the community

Good to go now :). Thnx!

alexpott’s picture

Status: Reviewed & tested by the community » Needs work

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();
}
alexpott’s picture

Status: Needs work » Needs review

Changing issue status to needs review...

alexpott’s picture

FileSize
1.41 KB
16.24 KB
PASSED: [[SimpleTest]]: [MySQL] 37,256 pass(es). View

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

Patch attached solves this.

cosmicdreams’s picture

Status: Needs review » Reviewed & tested by the community

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

webchick’s picture

Status: Reviewed & tested by the community » Fixed

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

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

sun’s picture

Status: Fixed » Needs review
FileSize
1.3 KB
PASSED: [[SimpleTest]]: [MySQL] 39,743 pass(es). View
+++ 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.

alexpott’s picture

Status: Needs review » Reviewed & tested by the community

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.

catch’s picture

Status: Reviewed & tested by the community » Fixed

Committed/pushed the follow-up, thanks!

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