Posted by Simon Georges on February 3, 2013 at 3:08pm
5 followers
| Project: | Drupal core |
| Version: | 8.x-dev |
| Component: | aggregator.module |
| Category: | task |
| Priority: | normal |
| Assigned: | Unassigned |
| Status: | needs review |
Issue Summary
Hi,
watchdog() and drupal_set_message() are using similar error messages for aggregator but with a small difference, and I'm proposing to use the same strings instead.
All credits goes to Pomliane for finding them.
Comments
#1
Please review the following patch.
#2
Since you're changing the messages to be the same, why not build the message once as a variable and then use the variable, instead of rebuilding the message twice? I.e.:
$errmsg = t('The feed from %site seems to be broken because of error "%error".', array('%site' => $feed->label(), '%error' => $response->getStatusCode() . ' ' . $response->getReasonPhrase()));
watchdog('aggregator', $errmsg, WATCHDOG_WARNING);
drupal_set_message($errmsg);
Less error-prone for the future, and probably saves a few cpu cycles too.
#3
Because a watchdog message shouldn't be translated, as it will be later, during the watchdog call (at least I think).
#4
That's so but, because the exact same message is being used and that message will have already been translated, it seems to me that there's no reason to put it through translation twice. If the substitution values have been translated for the watchdog() call, it's hardly likely that the exact same message will be translated into a different language for the immediately following drupal_set_message(t()) call.
#5
Oh, yeah, you're totally right! Something like that, then?
#6
Looks good to me - and passes all tests for Aggregator...
#7
Even if it will probably not cause problems on actual sites as #4 explains, it is still wrong to put a translated message in watchdog(). Additionally the string parser cannot pick up variables in t(), so this patch would actually make the string untranslatable.
Let's just use duplicate the strings here, it's not the end of the world. For the variable substitutions it might (or might not) make sense to use a $args variable and pass that to both watchdog() and the t() inside drupal_set_message().
#8
In watchdog() api documentation, it's stated that
$variablesparameter should contain, so that's what I did.
The strings in t() call are not variables but placeholders accepted by the
format_string(), as stated in t() api doc and format_string() api doc, so I don't really understand your argument here.#9
Yes, sorry I mixed that up in my head. What I meant was that you shouldn't do
t($message)(i.e. with a $message variable) because that is not translatable. Obviously the patch doesn't do that.I did not know that we explicitly allow setting variables NULL and therefore to bypass translation in watchdog(). It still feels wrong to me (and it is also unnecessary in my opinion), but I checked theme_dblog_message() and it does account for that case, so there really is no technical case to be made here.
My personal feelings shouldn't hold this issue up, but I would still suggest to have another person take a look at this, therefore setting to "needs review".
#10
Sure, I agree, let's see what the rest of the community thinks ;-)
#11
#5: aggregator-1906692-5-string_cleanup.patch queued for re-testing.
#12
There's nothing been heard here, so I re-queued to make sure the patch still passes testing. If it does, I'm going to RTBC it again.
#13
The last submitted patch, aggregator-1906692-5-string_cleanup.patch, failed testing.
#14
Since the original patch was posted, aggregator/aggregator.parser.inc has been replaced by aggregator/Plugin/aggregator/parser/DefaultParser.php. That's why the patch fails testing now.
So I re-rolled the patch with the appropriate changes to .../DefaultParser.php.
Now someone other than me needs to review it...