[pmg-devel] applied: [PATCH pmg-api] dkim: local mail: fix `Date` header formatting under different locales

[Stoiko Ivanov <s.ivanov@proxmox.com>]

Thanks for tackling this so quickly and for the through analysis in the
forum!
adding tests is very welcome!

I applied the patch after some quick positive tests and looking it
through, but caught an issue right after pushing (see comment inline).
To not keep our public branch with the issue I pushed a follow-up for this.

One meta-observation - I think it'd would have helped me if the
new tests were added in a separate commit.

comments/nits inline:
On Mon,  1 Sep 2025 14:12:27 +0200
Christoph Heiss <c.heiss@proxmox.com> wrote:

> This was reported in the community forum to cause problems if the
> (shell) locale was set to something different than 'C' or 'en_US.UTF-8'
> [0].
while first skimming over your patch I asked myself why this is needed, as
I thought we set this somewhere for our daemons (which in case of a
timer-triggered service (as pmgreport/pmgspamreport, wouldn't have helped
either) - seems I was wrong.
>   Date: Mo, 25 Aug 2025 00:01:04 +0200
nit: Tuesday, Wednesday, Thursday, Saturday might be more easily noticable
in this case
> 
> instead of the correct format of:
> 
>   Date: Mon, 25 Aug 2025 00:01:04 +0200
> 
> ..snip..
> +=head3 format_date_header
> +
> +Returns a RFC2822-formatted timestamp for usage in the Date header.
nit: writing RFC5322 might save a few people checking that both agree on
the date-time format (they do ;)
> +
> +sub format_date_header {
> +    # ensure that we always use the right locale for formatting `Date` headers
> +    my $old_locale = setlocale(POSIX::LC_TIME, 'C') // 'C';
seems `setlocale` only returns the old-locale when called without a second
argument (it's mentioned implicitly(I'd even say a bit hidden) in `perldoc
POSIX`, a bit more noticable to me in `man 3 setlocale`):
```
If locale is NULL, the current locale is only queried, not modified
```


> +    my $date = strftime('%a, %d %b %Y %T %z', @_);
> +    setlocale(POSIX::LC_TIME, 'C');
this would not restore the locale to the previously set value (I assume
that was the intention)
setlocale(POSIX::LC_TIME, $old_locale);
would accomplish that
(tested by printing the date a few times in this function

> +
> +    return $date;
> +}
> +
>  1;
> diff --git a/src/tests/Makefile b/src/tests/Makefile
> index dc35796..68f77e4 100644
> --- a/src/tests/Makefile
> +++ b/src/tests/Makefile
> @@ -4,7 +4,8 @@ export PERLLIB = ..
>  
>  all:
>  
> -check:
> +.PHONY: check
nit: thanks for the cleanup, and fine to add these things in the patch
touching the Makefile - but please mention them in the commit-message

>..snip..
>
> +};
> +
> +subtest 'format_date_header works with other locales' => sub {
> +    # also check correctness under some other locale
> +    my $old_locale = setlocale(POSIX::LC_TIME);
here saving the old locale and resetting after the tests seems correct...)

> ..snip..


_______________________________________________
pmg-devel mailing list
pmg-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pmg-devel