String cleanup: make strings consistent in DefaultFetcher::fetch

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.

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.

Looks good to me - and passes all tests for Aggregator...

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().

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".

#5: aggregator-1906692-5-string_cleanup.patch queued for re-testing.

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.

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...

well, this removes the ability of translating the string on runtime (eg when displaying on the Recent log entries page)
what we could do here, is make both string the same.. eg use "due to" or "because of" in both cases

thanks

Consistency++

Committed 02c8071 and pushed to 8.x. Thanks!