Add watchdog logged for bulk generated aliases
stennie - March 19, 2008 - 16:02
| Project: | Pathauto |
| Version: | 6.x-2.x-dev |
| Component: | Code |
| Category: | feature request |
| Priority: | normal |
| Assigned: | Unassigned |
| Status: | patch (code needs review) |
Description
I find it useful to have bulk updates noted in the watchdog logs, particularly when scheduling via cron (eg. per http://drupal.org/node/236304 ).
Attached is a patch against current cvs head which adds the watchdog logging.
Cheers,
Stephen
| Attachment | Size |
|---|---|
| pathauto_bulk_watchdog.patch | 3.41 KB |
#1
+1
re-rolled the patch against latest cvs head.
#2
#3
That patch is fine for 5.x but not for 6.x. Can you create one for 6.x as well/first?
#4
Hi Greggles
Here is a patch for 6.x
Thanks
Audrey
#5
#6
This won't do:
watchdog('pathauto', $update_msg);.watchdog()should be fed the "raw", untranslated string and then handle the translation when displaying the message in the logs. I'm not sure how to handle plurals withinwatchdog()though, so this might have to be looked into.Also, I'd vote for waiting with committing this for 6.x-2.x.
#7
That was my concern as well about this being non-trivial for the new watchdog system.
And, yes, let's postpone this to 6.x-2.x.
#8
re-worked with the new watchdog system.
#9
#10
<?php$update_msg = 'Bulk generation of index aliases completed, %aliases generated.';
$aliases = format_plural($count, 'one alias', '@count aliases');
?>
is a clear regression of the current code, where "one/@count alias(es)" was a part of the string. First of all, it's splitting one string into two strings, meaning that translators will have to translate some of it out of context. I could easily imagine how "alias" may have to be put differently, depending on what it's an alias of. This would not be possible with the previous patch. Second, it's handing a variable to
t()instead of a string.I'm still not sure how to handle plurals within the watchdog system though, but the way the patch currently does it, is not the way.
#11
The Drupal 6 watchdog system does not allow other callbacks but t() with watchdog. This might mean that you need to reformulate your text, like 'Bulk generation of index aliases completed (%aliases)' or 'Bulk generation of index aliases completed: %aliases' (I'd prefer the first) and then make %aliases the 'one alias' and '@count aliases'. Thus this breaks the sentence flow, so should be easier to translate.
Drupal 7 should allow arbitrary translation callbacks, such as the Drupal 6 menu system does. We unfortunately did not end up there in Drupal 6, so there are some dirty tricks to be used with plurals in Drupal 6 watchdog.
#12
Reworked the patch with suggestion 1 from Gabor.
The idea to break out the strings, was inspired from core modules, for example user.module:
$output = t('There is currently %members and %visitors online.',array('%members' => format_plural($authenticated_count, '1 user', '@count users'),
'%visitors' => format_plural($anonymous_count, '1 guest', '@count guests')));
#13
Ah well. I bow before Gábor's knowledge of the t() system, though I think it's an ugly hack, but if there's no elegant way of doing it for 6.x... *sigh*
Anyway, there's still a small issue with the patch: most of the
$update_msg's (all except one, actually) aren't rounded off by a full stop ("."). I believe they should be, unless there's a reason (e.g., a convention for watchdog messages that I'm not aware of) that the messages shouldn't be treated with proper English grammar. And even if so, the first instance shouldn't have it then. Consistency is King (or something).#14
Okay, patch includes full stops.
#15
For some reason, I missed this in my last comment. Sorry to keep you tossing patch versions about. :/ If you don't get around to it first, I'll fix the patch myself when I get around to test, but... the
$update_msginstances really ought to be replaced with the string itself, as a PO/T extractor won't know what to work with, if it's given a variable instead of a string.#16
While a little annoying, it is actually quite helpful for me, as I learn things I would not otherwise know about.
Also, it is a pleasure to work with module maintainers who are on top of the issue queue.
#17
I think Greg will testify that I'm one of the Drupal devs who pick on the most nits (but really, I'm just trying aspiring to reach the level of Morbus, chx, et al). Greg has been hit by my nit-picking quite a few times by now. Both before and after he decided to make me a co-maintainer. :p
Anyway, the code looks good now. I can't find anything else to pick on off-hand, but I'd like to comment on the loveliness of
drupal_set_message(t(andwatchdog('pathauto',aligning perfectly. It's beautiful. :DAnd just as I was about to send this off, I noticed an an "
alias" which should probably read "$alias". (pathauto_taxonomy.inc, 2nd hunk) ;)#18
okay here it is...