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

Fix UTC offsets when retrieving timezone string in WordPress #26604

Merged
merged 1 commit into from
Jun 27, 2023

Conversation

christianwach
Copy link
Member

Overview

Fixes this issue on Lab.

Before

In WordPress, one can choose e.g. UTC+4 as the site's timezone. This results in no value being returned by get_option('timezone_string') which may lead to unexpected results in CiviCRM.

After

When choosing e.g. UTC+4 in WordPress, a deprecated (but still currently valid) Etc/UTC-4 timezone string is used.

Comments

There is a companion PR in the CiviCRM-WordPress repo.

@civibot
Copy link

civibot bot commented Jun 22, 2023

(Standard links)

@eileenmcnaughton
Copy link
Contributor

@christianwach ping me if you need help getting this merged - I'm kinda assuming @kcristiano will merge it

@demeritcowboy
Copy link
Contributor

I also took a quick look just haven't had a chance to run yet but the idea looked good.

@christianwach
Copy link
Member Author

christianwach commented Jun 26, 2023

@eileenmcnaughton Thanks - @kcristiano wants to err on the side of caution and run it in some production contexts before merging. Similar code is actually in a number of relatively high profile WordPress plugins, so I'm confident he'll find it works as expected.

EDIT: This shouldn't be a substitute for people setting a named timezone in their WordPress site settings 😄

@kcristiano
Copy link
Member

Thanks @eileenmcnaughton I'll add merge-ready once I am ready to merge these.

@demeritcowboy
Copy link
Contributor

I did a quick little run. For example for this core PR part of it, creating an activity via command line and leaving the date blank sets the activity date to the wrong time currently, and with the PR sets it according to the configured offset.

@kcristiano
Copy link
Member

Thanks @demeritcowboy I've also done a number of test and r-run I was waiting on one last test on a staging site that has a custom implementation using the API but this looks good to merge.

@kcristiano kcristiano added the merge ready PR will be merged after a few days if there are no objections label Jun 26, 2023
@kcristiano
Copy link
Member

@eileenmcnaughton if you can merge this I can merge civicrm/civicrm-wordpress#298

@demeritcowboy demeritcowboy merged commit d02c3a7 into civicrm:master Jun 27, 2023
@christianwach christianwach deleted the lab-wp-121 branch February 7, 2024 21:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
master merge ready PR will be merged after a few days if there are no objections
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants