Everywhere in the code, you do this:

watchdog('Apache Solr', nl2br(check_plain($e->getMessage())), NULL, WATCHDOG_ERROR);

Which is wrong because this:

nl2br(check_plain($e->getMessage()))

Will get displayed through t() function. See watchdoc PHPdoc:

/**
 * Log a system message.
 * ..
 *
 * @param $message
 *   The message to store in the log. See t() for documentation
 *   on how $message and $variables interact. Keep $message
 *   translatable by not concatenating dynamic values into it!
 * @param $variables
 *   Array of variables to replace in the message on display or
 *   NULL if message is already translated or not possible to
 *   translate.
 * ..
 */

You should write your watchdog events like this:

watchdog('Apache Solr', "!e", array("!e" => nl2br(check_plain($e->getMessage()))), WATCHDOG_ERROR);

Everywhere, this should avoid the locale table oversizing because of exceptions messages getting translated. May be you want the exceptions messages to be translated, but you have to ensure those message comes from your own exceptions doing better exception catching than "Exception $e".
Other exception messages should remain in their original language, debugging wrong translated messages is the hell in person.

This is just an opinion, but you should think about it.

CommentFileSizeAuthor
#20 watchdog.patch1.62 KBrobertdouglass
#18 non-t-watchdog-664818-18.patch894 bytespwolanin
#14 watchdog.patch11.67 KBrobertdouglass
#12 watchdog.patch12.45 KBrobertdouglass
#10 apachesolr-6.x-2.x-dev-watchdog.patch7.57 KBpounard
#3 apachesolr-2.x-dev-watchdog.patch8.28 KBpounard

Comments

robertdouglass’s picture

robertdouglass commented

Yes, agree. Can you help us patch?

pounard’s picture

pounard
Primary language French
commented

I can, expect patches until tonight.

pounard’s picture

pounard
Primary language French
commented
StatusFileSize
new apachesolr-2.x-dev-watchdog.patch8.28 KB

Here is some patches, please review do not commit without further testing. You may not like the function I created.

Scott Reynolds’s picture

Scott Reynolds commented
Status: Active » Needs work

I think creating a function that expects a Exception or a String is pretty confusing. Can we set this up as two functions? 

function apachesolr_exception(Exception $e, $variables, $display) {
  apachesolr_error_message('!e', array('!e' => nl2br(....)), $display);
}

function apachesolr_error_message($message, $variables, $display) {
  // Write the message to log and / or screen.
}
pounard’s picture

pounard
Primary language French
commented

I'm ok with it.

EDIT: despite the fact this is how polymorphism can be implemented in PHP :D (just a joke, btw this is true).

Scott Reynolds’s picture

Scott Reynolds commented

Actually, after giving it more thought, I don't like a one line function.

I think I would prefer the pattern

try {
}
catch (Exception $e) {
  apachesolr_error_message('!e', array('!e' => nl2br(....), $display);
}

In this way, there is one function to log messages, and it accepts string, array(), boolean.

This of course brings up the question as to why we need to put it through one function? We already have watchdog, which adds the hook_watchdog, so that you can log messages where you want them. Seems like maybe we should just fix the 'bad' watchdog logs and not introduce a new function.

What is the advantage of this new abstraction?

pounard’s picture

pounard
Primary language French
commented

The advantage was to dispense the developer writing the n2lbr(check_plain()) everywhere. It's not really needed, it's just more convenient to write. And also it gives only one entry point for all error messages, which could eventually allow, if needed, to put a debug mode setting which could allow, for example, to store exception full stack trace, or in the opposite, do not log anything when not in debug mode.

pounard’s picture

pounard
Primary language French
commented

If you say no to this abstraction, tell me, I'll re-do my patch without it.

robertdouglass’s picture

robertdouglass commented

It's too bad that Drupal doesn't have exception handling of any sort. I'd love to catch more specific exceptions (instead of Exception $e all the time), and then throw new DrupalExceptions that have watchdog and string cleaning built in.

I think the function in the example is an unneeded layer of complexity. I like the convenience that it brings, but I don't like the idea of having a special custom error handling mechanism just for this module. Let's start with a patch that just fixes the original problem everywhere it occurs. We can come back to the idea of better error handling later.

pounard’s picture

pounard
Primary language French
commented
StatusFileSize
new apachesolr-6.x-2.x-dev-watchdog.patch7.57 KB

Re-done the patch. I noticed that sometime, watchdog type was 'Apache Solr', and sometime it was 'apachesolr'. I did put 'apachesolr' everywhere.
If you prefer 'Apache Solr' (which looks like quite inconsistent with other module usage of watchdog), you can still do:

  sed -i "s/watchdog('apachesolr/watchdog('Apache Solr/g" apachesolr-6.x-2.x-dev-watchdog.patch
robertdouglass’s picture

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

robertdouglass commented
StatusFileSize
new watchdog.patch12.45 KB

Made all watchdog calls consistent (not just the ones with $e->getMessage()).

robertdouglass’s picture

robertdouglass commented
Version: 6.x-2.x-dev » 6.x-1.x-dev
Status: Needs review » Needs work

#664818 by pounard, robertDouglass | Scott Reynolds: Fixed Wrong watchdog() usage.

Committing to 6.2.

robertdouglass’s picture

robertdouglass commented
Status: Needs work » Needs review
StatusFileSize
new watchdog.patch11.67 KB

Here's the 6.1 patch.

robertdouglass’s picture

robertdouglass commented
Version: 6.x-1.x-dev » 5.x-2.x-dev
Status: Needs review » Patch (to be ported)

committing to 6.1.

pwolanin’s picture

pwolanin commented
Version: 5.x-2.x-dev » 6.x-1.x-dev
Status: Patch (to be ported) » Needs work

About to roll this patch back. It's an incorrect use of the APi.

http://api.drupal.org/api/function/watchdog/6

$variables Array of variables to replace in the message on display or NULL if message is already translated or not possible to translate.

Note the special use of NULL - this patch in fact makes the message go through t() when it was NOT previously.

pwolanin’s picture

pwolanin commented

see: http://api.drupal.org/api/function/_dblog_format_message/6

If $variables is NULL (serializes to N;) then t() is NOT called. What was the basis for this original report of these messages going into the translation table?

pwolanin’s picture

pwolanin commented
Status: Needs work » Needs review
StatusFileSize
new non-t-watchdog-664818-18.patch894 bytes

Looks like there was a single wrong call to be corrected.

pwolanin’s picture

pwolanin commented
Version: 6.x-1.x-dev » 6.x-2.x-dev
Status: Needs review » Patch (to be ported)

committing this - I suggest #12 be rolled back and any actual incorrect calls be fixed also in 6.x-2.x.

Given that all but the one call used 'Apache Solr', I'm sticking with that as the consistent $type.

robertdouglass’s picture

robertdouglass commented
StatusFileSize
new watchdog.patch1.62 KB

Committing attached to 6.2 after rolling #12 back, so now the branches are at least in sync.

robertdouglass’s picture

robertdouglass commented
Status: Patch (to be ported) » Fixed

@pounard - does @pwolanin's API clarification satisfy your concern here? If not, please re-open.

pounard’s picture

pounard
Primary language French
commented

Ok, I misread watchdog() doc.

EDIT: BTW watchdog() function usage is weird because of this special 'NULL' variable array.

pwolanin’s picture

pwolanin commented

Yes, this is a weird/stupid api in D6+, but with that said, I think we are using it as it is expected to be used.

Status: Fixed » Closed (fixed)

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

claudiu.cristea’s picture

claudiu.cristea
Primary language Romanian
Location Arad 🇷🇴
commented
Version: 6.x-2.x-dev » 5.x-2.x-dev

Fixed also in 5.2