We missed converting one the variable_get('error_level') in the original conversion. This patch fixes this bug.

Part of #1775842: [meta] Convert all variables to state and/or config systems

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

andreiashu’s picture

Title: Convert error_level to CMI » Finish conversion of error_level to CMI
Priority: Normal » Critical

Spoke with @alexpott and it seems this has already been addressed in a separate issue but we discovered one place where it was missed. Patch to come

andreiashu’s picture

Status: Active » Needs review
FileSize
688 bytes

patch attached

andreiashu’s picture

+++ b/core/includes/errors.incundefined
@@ -250,7 +250,7 @@ function _drupal_log_error($error, $fatal = FALSE) {
+      $error_level = config('system.logging')->get('error_level') ?: ERROR_REPORTING_DISPLAY_ALL;

@alexpott: does it make sense to have to default to a value at code level? We already have a default in system.logging.yml

andreiashu’s picture

Status: Needs review » Needs work
andreiashu’s picture

Issue tags: +Configuration system

tagging

andreiashu’s picture

Status: Needs work » Needs review
FileSize
657 bytes

Removed the redundant default value

alexpott’s picture

Category: task » bug
Status: Needs review » Reviewed & tested by the community

Looks great.

Thanks for the work!

webchick’s picture

Status: Reviewed & tested by the community » Needs review

Just a quick question. Is this actually causing a bug to appear somewhere? If so, we need tests to illustrate the failure.

alexpott’s picture

Yep webchick you're right we need a test...

So here it is. The .test.only.patch should fail :)

Status: Needs review » Needs work

The last submitted patch, 1798756-9.error_level.test-only.patch, failed testing.

alexpott’s picture

Status: Needs work » Needs review

Setting back to RTBC as the added test proves the bug and the fix!

aspilicious’s picture

Status: Needs review » Reviewed & tested by the community

Looks good

Lars Toomre’s picture

Small nit going forward.... please do not use t() around assert messages. Thanks.

alexpott’s picture

Lars that t() is in all the ErrorHandlerTest's - copy/paste mistake :)

Lars Toomre’s picture

There are patches waiting review/commit removing all of the t() from assert messages. Hopefully, soon there won't be bad examples going forward.

webchick’s picture

Status: Reviewed & tested by the community » Fixed

Awesome, thanks a lot!

Committed and pushed to 8.x.

Lars Toomre’s picture

For reference, this is the issue cleaning up t() around assert messages in the referenced test file: #1797926: Remove t() from asserts from System sub-system tests.

andreiashu’s picture

@alexpott: re #7: thanks for the guidance and helping out :)

andypost’s picture

Yep, one t() added with commit

+++ b/core/modules/system/lib/Drupal/system/Tests/System/ErrorHandlerTest.phpundefined
@@ -53,6 +53,15 @@ class ErrorHandlerTest extends WebTestBase {
+    $this->assertResponse(200, t('Received expected HTTP status code.'));

this place should be fixed in #1797926: Remove t() from asserts from System sub-system tests

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

Anonymous’s picture

Issue summary: View changes

Updated issue summary.