Use watchdog_severity_levels() instead of duplicating information

[BWPanda]

A function exists that pairs the various watchdog severity levels with their human-readable names: watchdog_severity_levels()

And yet there are places where that information in duplicated unnecessarily:

• hook_watchdog()
• system_logging_settings()
• dblog_overview()

We should consolidate...

[ghost]

PR: https://github.com/backdrop/backdrop/pull/3397

[indigoxela]

I suspect, you're mixing stuff in your PR.

classes: WATCHDOG_DEBUG => 'dblog-debug'

human readable name: WATCHDOG_DEBUG => t('Debug'),

lowercase name: WATCHDOG_DEBUG => t('debug'),

It might be possible, though, to consolidate the names. It depends where these are used. We should first check, why there are lowercase and ucfirst versions of the names.

[indigoxela]

I took a look at the places, where these strings are in use:

• admin/config/development/logging - the uppercase (first char) versions of the level
• admin/reports/dblog, and individual log item pages - the lowercase versions of the level
• admin/reports/dblog - the css classes

The PR works as expected.

The code is looking good, too, but I hesitate to set it RTBC, because it might need a decision if the "ucfirst" approach for translated strings is OK. I couldn't tell if this might be problematic with certain languages. Although backdrop_ucfirst() handles utf-8 strings.

[klonos]

I couldn't tell if this might be problematic with certain languages.

It shouldn't (at least I wasn't able to find any issues for it in d.org and by googling). But I like this clean-up, so let's just do it 😅 ...we could revise later if anyone reports that this makes it hard for their language to be translated accurately.

[ghost]

I realised that there might be an issue with dblog.admin.inc in that we're using translated strings as class names. So technically that means the class names could change depending on the language of the site, right? I'm not sure classes should change based on the language, so we might need to do something different there... Unless I'm mistaken?

[klonos]

🤔 ...I was thinking that perhaps we could define the levels as (untranslated) english strings via constants elsewhere, and then wrap them in t() in watchdog_severity_levels(). Alternatively, we could introduce a new parameter in watchdog_severity_levels() (like function watchdog_severity_levels($raw = FALSE)), and then in dblog_overview() we call it with TRUE for the classes.

[ghost]

I was thinking of the latter.

[indigoxela]

Or we simply leave it as is - before we waste time to fix something that's not broken. And break it.

[ghost]

Or we simply leave it as is

I don't agree with leaving things as they are just because they're not broken...

[jlfranklin]

it might need a decision if the "ucfirst" approach for translated strings is OK. I couldn't tell if this might be problematic with certain languages. Although backdrop_ucfirst() handles utf-8 strings.

It's less a matter of does backdrop_ucfirst() handle UTF8, and more is it stylistically appropriate to ucfirst the strings in all languages.

perhaps we could define the levels as (untranslated) English strings via constants elsewhere, and then wrap them in t() in watchdog_severity_levels().

Doesn't that make it much harder to find the strings to translate? Calls to t() are supposed to use string literals because the translation tools search for those calls and provide the string literal to translators.

we could introduce a new parameter in watchdog_severity_levels() (like function watchdog_severity_levels($raw = FALSE))

I'm thinking roughly the same thing. It would also remove the same for-loop in several places in the code. Minor nit - I'd suggest $machine_name instead of $raw.

[indigoxela]

The related PR is in "needs work" state since November 2020. Sadly, it now also has conflicts. And, there seem to be some plans for substantial changes regarding the implementation, anyway, so I'm removing the milestone.

[ghost]

Here's a new PR that I think does a better job of accounting for all situations in one function: https://github.com/backdrop/backdrop/pull/3968

[klonos]

I took a quick look at the code, and it looks good. I'll get around to testing soon (unless anyone else beats me to it).

[ghost]

PR rebased and all tests passing. Would be great to get some reviews on this please!

[klonos]

Thanks @BWPanda 🙏🏼 ...code looks good to me (although I've left a small suggestion that can be ignored).

Now back to finding time to test this. I guess I'll have to do it on my local though.

[quicksketch]

I reviewed this PR and it looks great. I think we should leave intact the current docblock for watchdog() to list the available values directly, as that's the main place where people are going to need to know the parameter value. I reverted that one docblock and resolved conflicts in the PR.

[quicksketch]

Thanks folks! Finally merged this into 1.x and 1.26.x.