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

CommentFileSizeAuthor
#9 1798756-9.error_level.patch2.98 KBalexpott
#9 1798756-9.error_level.test-only.patch2.34 KBalexpott
#6 1798756-error_level_2.patch657 bytesandreiashu
#2 1798756-error_level_1.patch688 bytesandreiashu

Comments

andreiashu’s picture

andreiashu commented
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

andreiashu commented
Status: Active » Needs review
StatusFileSize
new 1798756-error_level_1.patch688 bytes

patch attached

andreiashu’s picture

andreiashu commented 
+++ 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

andreiashu commented
Status: Needs review » Needs work
andreiashu’s picture

andreiashu commented
Issue tags: +Configuration system

tagging

andreiashu’s picture

andreiashu commented
Status: Needs work » Needs review
StatusFileSize
new 1798756-error_level_2.patch657 bytes

Removed the redundant default value

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Category: task » bug
Status: Needs review » Reviewed & tested by the community

Looks great.

Thanks for the work!

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
commented
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

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
StatusFileSize
new 1798756-9.error_level.test-only.patch2.34 KB
new 1798756-9.error_level.patch2.98 KB

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

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Needs work » Needs review

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

aspilicious’s picture

aspilicious commented
Status: Needs review » Reviewed & tested by the community

Looks good

lars toomre’s picture

lars toomre commented

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

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented

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

lars toomre’s picture

lars toomre commented

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

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
commented
Status: Reviewed & tested by the community » Fixed

Awesome, thanks a lot!

Committed and pushed to 8.x.

lars toomre’s picture

lars toomre commented

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

andreiashu commented

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

andypost’s picture

andypost
he/him
Primary language Russian
commented

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

(not verified) commented
Issue summary: View changes

Updated issue summary.