Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
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.
Comment | File | Size | Author |
---|---|---|---|
#18 | aggregator-1906692-18-string_cleanup.patch | 1.78 KB | Simon Georges |
#14 | aggregator-1906692-14-string_cleanup.patch | 3.49 KB | jmarkel |
#5 | aggregator-1906692-5-string_cleanup.patch | 2.66 KB | Simon Georges |
#1 | aggregator-1906692-1-string_cleanup.patch | 2.46 KB | Simon Georges |
Comments
Comment #1
Simon Georges CreditAttribution: Simon Georges commentedPlease review the following patch.
Comment #2
jmarkel CreditAttribution: jmarkel commentedSince 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.:
Less error-prone for the future, and probably saves a few cpu cycles too.
Comment #3
Simon Georges CreditAttribution: Simon Georges commentedBecause a watchdog message shouldn't be translated, as it will be later, during the watchdog call (at least I think).
Comment #4
jmarkel CreditAttribution: jmarkel commentedThat'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.
Comment #5
Simon Georges CreditAttribution: Simon Georges commentedOh, yeah, you're totally right! Something like that, then?
Comment #6
jmarkel CreditAttribution: jmarkel commentedLooks good to me - and passes all tests for Aggregator...
Comment #7
tstoecklerEven 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().
Comment #8
Simon Georges CreditAttribution: Simon Georges commentedIn watchdog() api documentation, it's stated that
$variables
parameter 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.Comment #9
tstoecklerYes, 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".
Comment #10
Simon Georges CreditAttribution: Simon Georges commentedSure, I agree, let's see what the rest of the community thinks ;-)
Comment #11
jmarkel CreditAttribution: jmarkel commented#5: aggregator-1906692-5-string_cleanup.patch queued for re-testing.
Comment #12
jmarkel CreditAttribution: jmarkel commentedThere'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.
Comment #14
jmarkel CreditAttribution: jmarkel commentedSince 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...
Comment #15
Simon Georges CreditAttribution: Simon Georges commentedPatch still applies.
Has someone with a deeper core knowledge an objection to this being committed?
Comment #16
ParisLiakos CreditAttribution: ParisLiakos commentedwell, 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
Comment #17
Simon Georges CreditAttribution: Simon Georges commentedThat was the first patch of the issue ;-)
Comment #18
Simon Georges CreditAttribution: Simon Georges commentedRe-roll of the 1st patch of the issue on the current 8.x-dev version.
Comment #19
ParisLiakos CreditAttribution: ParisLiakos commentedthanks
Comment #20
alexpottConsistency++
Committed 02c8071 and pushed to 8.x. Thanks!
Comment #21
Simon Georges CreditAttribution: Simon Georges commentedThanks! Now, let's see if we'll find others.