Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ManageSieve: make sieve_formattime() parse all time-formatters PHP does. #9655

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

bennet0496
Copy link

@bennet0496 bennet0496 commented Sep 27, 2024

Roundcube's $config['time_format'] understands all PHP time and especially timezone format specifiers listed in the PHP docs; however, sieve_formattime() that used this value via rcmail.env.time_format does not.

Therefore if the config value e.g. is $config['time_format'] = "H:i T" to include the abbreviated timezone name (which works else where, like the message list), then forms using sieve_formattime(), the OOO form, will generate times like "6:00 T" and so on
image
This causes the actual time be set to "Tango Time", i.e. UTC-7
image
Which for users outside of UTC-7 is unexpected behavior.

Therefore I changed sieve_formattime() to understand any formatter that may reasonably (or unreasonably, like microseconds or Unix Time. But never say never) be configured by an admin or user. For the timezone values, I opted to pass these values from PHP to JS via set_env() as in JS these values are extraordinarily hard to reliably to obtain without additional libraries.
image

I also changed some thing at the beginning of sieve_formattime() that looked odd to me. Like per-declaring all variables. And actually used the value that h would have been in the g and h cases. But I'm also happy reverting these changes if they are undesired.

@pabzm
Copy link
Member

pabzm commented Oct 30, 2024

Thank you very much for your contribution!

Your solution is solid, but please don't include the drive-by improvements into the same PR (or at least not into the same commit). It makes it unnecessary hard to comprehend, which changes are related.

Could you correct that?

@bennet0496
Copy link
Author

I removed the drive-by changes to the 12h format. Do you want the fix for that here as another commit or do you want another PR for that?

Also I removed the Unix timestamp, because looking over it, it doesn't quite makes sense here, because of the implicitly included date.

@pabzm
Copy link
Member

pabzm commented Nov 1, 2024

Please open a new PR for the 12h-changes. Thank you!

@pabzm
Copy link
Member

pabzm commented Nov 5, 2024

Great, thanks! Now all that's missing from my point of view is an entry to the Changelog. Would you mind adding one? (Feel free to decline!)

@alecpl Any thoughts on this?

@bennet0496
Copy link
Author

Is the entry ok like this? If not I will revert it :)

@pabzm
Copy link
Member

pabzm commented Nov 5, 2024

Perfect, thank you! 👍

@pabzm pabzm requested review from alecpl and pabzm November 5, 2024 16:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants