search

artfulrobot / uk.artfulrobot.civicrm.gocardless

A CiviCRM extension providing GoCardless integration to handle UK Direct Debits.
GNU Affero General Public License v3.0
5 stars 18 forks source link

Remove extra newlines from logging #80

Closed jmdh closed 4 years ago

jmdh commented 4 years ago

Some log processing tools don't handle empty lines well, so remove them. This also improves readability, attaching timestamps to content more closely.

Note that the core logging functionality also introduces additional newlines, which should arguably be removed as well. I've applied the trivial patch to my local installation to fix this there, but I need to look more carefully at callers to see what the impact is. Meanwhile this patch should be harmless.

artfulrobot commented 4 years ago

Thanks for explaining your problem with the logs.

Currently:

The two instances you found introduce newlines (but not blank lines) after the short identifier line. This means that when viewing the logs from the command line you get to see more of the content - a full terminal-width row of characters. I suppose I had done it this way deliberately, but I don't have a strong view on it.

We're using a bit of a low level logging function because this is the only way to put these logs in a dedicated file/facility and it's very helpful to have them all together, separate from other logs. I think CiviCRM core is adding a blank line at the end of each log item.

Newlines are not to be feared in this logging; it's very crude and is not supposed to adhere to any particular format. Also, some of what gets logged might include new lines (and possibly blank lines) itself.

So I could accept this PR, which would help you with these particular log lines, but it would not help your more general issue: there will still be new and blank lines in the log - there are other places in this extension that write logs, e.g. a stacktrace and the Upgrader/Base.php file which is generated by civix writes a new line at the end of database updates during upgrades.

jmdh commented 4 years ago

Thanks for the reply and manage apologies for going quiet for so long... Yes, after sending this patch I realised just how unlike a traditional system log CiviCRM's logging was. It may have been a mistake for me to treat it as such. I decided not to send the core patch because I realised, as you point out, that it would never be a complete solution, because there is nothing stopping other sources from sprinkling newlines around.

I'm probably going to carry on running with this patch (and the corresponding core one) as it does fix the specific, common case of the audit logs from this plugin, which I am very grateful for. (Ultimately, this is a usability/formatting issue, nothing terrible will happen if I receive newlines, so a 90% solution that works for my case is still worthwhile)

But I understand that you might not want to merge this :)

artfulrobot commented 4 years ago

FWIW I find logging in Civi really annoying too. And the interplay with Drupal's watchdog is not fun, either (that isn't a standard logging system either). Thanks anyway!