Database log message is truncated at 56 characters and not configurable

[jromine]

Description of the need

The database log report at admin/reports/dblog limits the log message length to 56 characters before truncation. This is inadequate for my use case and window size, and requires much clicking of links to see log details.

It was possible in Drupal 7 to alter the log display by overriding the dblog_message theme function, however that capability was removed in https://github.com/backdrop/backdrop/pull/629.

Proposed solution

Provide a numeric setting for log message line length on the admin/config/development/logging page.

Draft of feature description for Press Release (1 paragraph at most)

Backdrop now includes a setting to limit dblog message length before truncation.

[jromine]

[bugfolder]

I would love to have this ability (restored).

Tests are failing, though.

[jromine]

I think the problem is the log length setting is created in update.php, and some log messages are being generated before that can run. I'll update the PR to provide a default value.

[jromine]

I updated the PR. I'm not sure if I need to initiate a re-test, or that will happen automatically.

[jromine]

After updating the PR, the tests look good now.

[indigoxela]

Great, many thanks for your PR.

I wonder, if the config value has to get removed from the system.core json, when uninstalling the dblog module. :thinking: But I noticed, that this also doesn't happen with the other values.

[jromine]

@indigoxela I agree, other settings for some core modules seem to go in system.core rather than their own module-specific config file, and don't get removed when uninstalling the module. The existing settings are also named log_* rather than dblog_*. I tried to follow existing practice.

[bugfolder]

Code reviewed: LGTM.

Tested:

• Set value to 10, worked.
• Set value to 10.5, got an error (appropriately).
• Set value to 1000, got an error (appropriately) for max length of 255.

So, works for me.

Restarted tests. Tests failed twice, but different failures both times, and neither appeared related to these changes. So I'm guessing it's the test gremlin. Or, put differently, between the two runs, every test passed at least once.

[hosef]

We should move the install hook to system.install since the setting is going to live in the System module. That way all the functionality that changes that setting lives in one place.

[bugfolder]

It looks like a simple change to get this across the finish line. @jromine, would you be open to a replacement PR (or a PR against your PR)?

[jromine]

It looks like a simple change to get this across the finish line. @jromine, would you be open to a replacement PR (or a PR against your PR)?

I have no objections.

[jromine]

@hosef Existing update hooks in dblog.install affect settings in system.core. Will they all be moved or are we splitting up these hooks into two locations?

[bugfolder]

Would like to get @hosef's input on this, but my understanding is that moving existing update hooks between modules is problematic because existing systems will have recorded those hooks as having run. But new ones can be moved before they're merged in.

[quicksketch]

Putting log updates in system.install makes sense to me. I think this PR is ready for review again.

[klonos]

@bugfolder I would like to suggest the following:

• We should change the label of the field and/or its description to clarify that this is NOT about truncating the length of the log messages that are being saved in the database, rather than the length of the text being displayed in the log report page. Perhaps Log message display length for the label, and for the help text something along the lines of:

  Limit the mount of characters for each log entry shown in the reports page (does not trim the message that is actually being saved in the database).

  Perhaps something shorter than what I came up with bove.

• Add a "characters" suffix to the field.

• Move all fields provided by the dblog module into a fieldset (always expanded I guess), since if you also enable syslog it gets kinda messy to be able to tell which settings belong to dblog and which to syslog. (I will open a separate issue to add syslog messages to a fieldset as well).

• I would also like to see these config settings being moved into a dblog.settings.json file, which is removed when the dblog module is uninstalled (but that's also a thing for a separate issue).

PS: taking the opportunity to mention that https://github.com/backdrop-contrib/views_watchdog is only a few lines of code, and if that functionality was added to core + the log report page converted to a view, then people would be able to not only tweak the length/trimming of the entries, but also do other things with the messages (such as add filtering by keywords or date range, besides type/severity etc.). I think that that would provide more value.

[bugfolder]

We should change the label of the field and/or its description to clarify that this is NOT about truncating the length of the log messages that are being saved in the database

Language changed. Perhaps noting that this is the "displayed" message length makes sufficient distinction that we don't need to add more words about what is stored.

Add a "characters" suffix to the field.

Added.

Move all fields provided by the dblog module into a fieldset

Added.

Agreed that the other bits are worthy of separate follow-up issues. (And being a fan of putting as much as possible into Views, I quite agree with the absorption of views_watchdog.)

I also found (and fixed) a hard-coded reference to the 56-character truncation limit in a docblock.

[klonos]

Thanks @bugfolder 🙏🏼 ...I'll make sure to file separate follow-ups for the other tasks.

Code looks OK to me, and the form looks neat now. There is still an odd failure in the phpcs check that throws Error: Command failed: phpcs --version, but I guess we can ignore that for now(?).

[bugfolder]

PR updated with your suggestion, thanks.

Dunno why the phpcs command is failing, but it doesn't seem related to any of the code changes.

[klonos]

PR updated with your suggestion, thanks.

Thank you @bugfolder 🙏🏼 ...that has caused PHPCS to complain about a missing comma now. Can you please quickly fix that too?

[bugfolder]

Fixed that, but more PHPCS complaints in layout.admin.inc, which is not part of this PR.

[klonos]

Thanks again :)

@indigoxela do you have any idea? Is it #5721 or #5848 or a separate issue?

[bugfolder]

I rebased it, and PHPCS is now happy. Tests restarting.

[klonos]

Thanks @bugfolder 🙏🏼 ...this is still RTBC and good to go 👍🏼

Follow-up issues for the things I mentioned in my comment above:

• I have added the fieldset around the syslog-related fields as part of my PR for #4615, so no need for a new issue.
• I have filed #5897 for moving the settings of the dblog and syslog modules out of system.core.json.

[quicksketch]

One suggestion on the PR's update hook: https://github.com/backdrop/backdrop/pull/4288/files#r1058661197

[bugfolder]

Suggestion accepted. Back to review.

[quicksketch]

Code LGTM, I'd like to give it a try locally to check on the upgrade path.

[jenlampton]

In testing, the display of messages on the admin/reports/dblog page was shorter, but I was surprised to see the complete message is still displayed on /admin/reports/event/* pages.

I think I was mislead by the label Displayed message length, which seems like it should apply every time the message is displayed. What about something like Abbreviated message length, or Shortened message length?

[jenlampton]

I seem to be unable to make a PR against your PR @bugfolder, but I added some alternative label/description text options on the PR for consideration. (I chose Abbreviated)

[bugfolder]

Sounds good to me. Both changes made, PR updated.

[quicksketch]

I tested the upgrade path and it works great. Also tested the functionality of adjusting the limit and it takes effect immediately. Nice job!