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.

Files: 
CommentFileSizeAuthor
d7_watchdog_action_tracker.patch1.46 KBwojtha
PASSED: [[SimpleTest]]: [MySQL] 29,698 pass(es). View

Comments

meba’s picture

Status: Needs review » Reviewed & tested by the community

Sounds fine to me

wojtha’s picture

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

Dries’s picture

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

Status: Reviewed & tested by the community » Needs review
tstoeckler’s picture

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

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

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

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

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

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

@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

@#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

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.