Tracker and Action module calling t() or format_plural() inside watchdog() function call, but this is not necessary since watchog() process translation strings internally.

First I found bug in tracker and after that I create this regex: /watchdog\([^,]+, ?(t\(|format_plural\()/ and found one more bug in Action module.

CommentFileSizeAuthor
d7_watchdog_action_tracker.patch1.46 KBwojtha
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

meba’s picture

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

Sounds fine to me

wojtha’s picture

wojtha CreditAttribution: wojtha commented

The bug in action.inc affects also D6 branch (Drupal 6.19, action.inc:358).

Dries’s picture

Dries CreditAttribution: Dries commented

I don't understand the format_plural() change. AFAIK, watchdog() doesn't call format_plural(). In other words, this is likely to introduce a regression for translators.

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
CreditAttribution: webchick commented
Status: Reviewed & tested by the community » Needs review
tstoeckler’s picture

tstoeckler
he/him
Primary language German
Location Essen, Germany
CreditAttribution: tstoeckler commented
Status: Needs review » Reviewed & tested by the community

I couldn't find where it says that, otherwise I would have posted a link, but we currently don't support format_plural() in watchdog(). You send a plain old string (with replacement variables) to watchdog(). Period.

meba’s picture

meba CreditAttribution: meba commented

Yes, calls to watchdog shouldn't include t(), because they are translated on display. And since format_plural automatically calls t(), we shouldn't format_plural in watchdog.

wojtha’s picture

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

@Dries

I though that using t() or format_plural() in watchdog is not necessary in general and it is not good practice.

If you are not against using format_plural() in watchdog(), there are some places in Drupal core which should use format_plural for watchdog() but don't. This was the only usage of format_plural inside watchdog call in whole core and maybe also in all Drupal contribs (as I tested also Drupal 6 with around 100 most used modules installed).

// locale.inc:555
watchdog('locale', '@count disallowed HTML string(s) in %file', array('@count' => $skips, '%file' => $file->uri), WATCHDOG_WARNING);

// comment.admin.inc:234
watchdog('content', 'Deleted @count comments.', array('@count' => $count));

// node.admin.inc:597
watchdog('content', 'Deleted @count posts.', array('@count' => $count));
wojtha’s picture

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

Changed status accidentally.

@meba, @tstoeckler: Yes, I think too. Format plural is not supported in watchdog. And I don't think it will be useful at all. It is "only" logging tool, not something which appears on a frontpage or on a node edit form.

Dries’s picture

Dries CreditAttribution: Dries commented
Status: Reviewed & tested by the community » Fixed

Thanks for the follow-up comments. That helped me realize I was mistaken. Committed to CVS HEAD.

Eric_A’s picture

Eric_A CreditAttribution: Eric_A commented

The real issue with the first watchdog() call was that it was passed both a translated string *and* an array of replacement variables instead of NULL. It worked because format_plural wasn't passed $args.
The real fix should have been to move the replacement variables to the format_plural() $args and that watchdog should have been passed NULL for $variables.
This would have prevented the regression for translators from RC1.
I cannot bring myself to change the issue status but really, this should have been discussed even if most of us don't care about a watchdog string (freeze) and even if it is a very, very good idea to stay away from format_plural() for watchdog strings.

wojtha’s picture

wojtha CreditAttribution: wojtha commented

@Erik_A no, the real issue is what meba describes in #6: Watchdog strings are (and have to be) translated when they are going to be displayed.

If you store translated string (with tokens replaced with real values - which is what both t() and format_plural() do) in the db, you can't translate it than to the appropriate language when the watchdog message is going to be displayed to the user...

Eric_A’s picture

Eric_A CreditAttribution: Eric_A commented

@#11, Yes, of course, that, too.
So in order to fix the translating bug without a regression for translators is to check $count, set the message that goes with $count and do a watchdog(). Of course this is very ugly and is not the recommended pattern, but my point is that after string freeze ugly fixes need to be discussed. I'll see if I can come with a patch for discussions sake.

Eric_A’s picture

Eric_A CreditAttribution: Eric_A commented

While doing that tiny patch I realized that no strings were changed or added (that is, for the one watchdog() call discussed now). One was removed.
Which makes all the difference in the world, here. Sorry! Carry on, please...

Status: Fixed » Closed (fixed)
Issue tags: -watchdog, -content translation, -format plural

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

Issue tags: +watchdog, +content translation, +format plural

Restoring issue tags, see #2125755: System messages removed all issue tags during D7 upgrade.