Automattic / newspack-custom-content-migrator

Custom migration tasks for launching and migrating Newspack sites on Atomic
5 stars 5 forks source link

feat: creating a class to handle conversions between timezones #377

Closed eddiesshop closed 11 months ago

eddiesshop commented 11 months ago

How to test



Works as follows:

$date = ( new \NewspackCustomContentMigrator\Logic\DateTimeConverter( 'America/New_York' ) )->convert( '11/06/2023 12:00:00', 'GMT' ); // 2023-11-06 17:00:00

// or
$converter = new \NewspackCustomContentMigrator\Logic\DateTimeConverter( 'America/New_York' );
$converter->set_target_timezone( 'UTC' );
$converter->convert( '12/25/2023 00:00:00' ); // 2023-12-25 05:00:00
$converter->convert( '01/01/2024 00:00:00' ); // 2024-01-01 05:00:00
naxoc commented 11 months ago

I'm normally all about helper classes, but i have had a lot of pain in the past with DateTime in PHP, so let me ask a few questions to better understand why this one is necessary :)

PHP's DateTime are mutable, which is good if you can keep track of it. In the convert() function it's possible to pass a DateTime or a string. What it returns is a string, but in the meantime the DateTime passed as an argument has been mutated with the timezone setting. There is an immutable DateTime I suggest using whenever possible to avoid surprises: https://www.php.net/manual/en/class.datetimeimmutable.php

eddiesshop commented 11 months ago

Thanks for the scrutiny @naxoc! I built this class with the intention of reducing this block of code to a one-liner if possible.

This could be used during migrations where he set it up once in the constructor with the timezone we know the publisher is coming from

public function __constructor() {
    $this->converter = new DateTimeConverter( 'America/Los_Angeles' );
    $this->converter->set_target_timezone( 'GMT' );
}

Then, on any instance we'd like to correctly generate the GMT date fields, we could use this

$gmt_created_at = $this->converter->convert( '01/01/2024 09:00' );

In my mind it makes sense because of code reusability, but I know that there are a lot of potential pitfalls (e.g. having to maintain the $common_formats property). Personally I'd rather use the Carbon PHP date package where we could do something like this instead

$date_1 = Carbon::parse( '01/01/2024 05:30' )->shiftTimezone( 'America/Los_Angeles' ); // If date format is unknown
$date_2 = Carbon::createFromFormat( 'Y-m-d H', '1975-05-21 22' )->shiftTimezone( 'America/Los_Angeles' ); // If date format is known

$date_1->setTimezone( 'GMT' )->toDateTimeString(); // returns date-time in GMT

However, if we want to keep external packages to a minimum, I think this class would achieve that.

eddiesshop commented 11 months ago

Thanks @naxoc for finding this bit of code which tells us that as long as the timezone is set on the site, WordPress is already equipped to handle gmt related fields properly. I will close this PR as we wouldn't ever really need it.