Add from/to timezone settings for the "FormatDate process plugin"

For backwards compatibility, I think I'd like to see the 'timezone' setting map to both to/from. And this now an API change now it is in the code base, so I think this should at least start with 8.4.x

I am actually wondering now if not converting to UTC for FormatDate::transform() is a bug, since that is the implicit TZ for field storage.

1. +++ b/core/modules/migrate/src/Plugin/migrate/process/FormatDate.php
   @@ -15,7 +15,9 @@
   - * - timezone: String identifying the required time zone, see
   

   Please don't remove this parameter. It should remain for backwards compatibility. And the comment updated accordingly.

2. +++ b/core/modules/migrate/tests/src/Unit/process/FormatDateTest.php
   @@ -111,12 +111,13 @@ public function datesDataProvider() {
   -          'timezone' => 'America/Managua',
   

   Please don't remove test coverage either. Add to the test coverage.

3. +++ b/sites/default/default.settings.php
   @@ -86,7 +86,7 @@
   + $databases = array();
   

   Stray changes.

4. +++ b/sites/default/default.settings.php
   @@ -643,7 +643,6 @@
   -# $config['system.file']['path']['temporary'] = '/tmp';
   

   Stray changes.

I like the new to/from config options. But the previous timezone should be kept for BC and should map both to/from when set.

+++ b/core/modules/migrate/tests/src/Unit/process/FormatDateTest.php
@@ -111,12 +111,13 @@ public function datesDataProvider() {
       'timezone' => [
         'configuration' => [
-          'from_format' => 'Y-m-d\TH:i:sO',
-          'to_format' => 'Y-m-d\TH:i:s',
+          'from_format' => 'Y-m-d h:i:s',
+          'to_format' => 'Y-m-d h:i:s',
           'timezone' => 'America/Managua',
+          'to_timezone' => 'UTC',
         ],
-        'value' => '2004-12-19T10:19:42-0600',
-        'expected' => '2004-12-19T10:19:42',
+        'value' => '2004-12-19 10:19:42',
+        'expected' => '2004-12-19 04:19:42',
       ],

This test case also need to remain unchanged. We just need to add a new data set to the provider to test different from/to time zones.

1. +++ b/core/modules/migrate/src/Plugin/migrate/process/FormatDate.php
   @@ -16,6 +16,9 @@
     * - timezone: String identifying the required time zone, see
   

   This is deprecated and should be noted as such and explain what it does if it is used.

2. +++ b/core/modules/migrate/src/Plugin/migrate/process/FormatDate.php
   @@ -92,13 +96,15 @@ public function transform($value, MigrateExecutableInterface $migrate_executable
        $timezone = isset($this->configuration['timezone']) ? $this->configuration['timezone'] : NULL;
   

   we still need to read this config option. It should be used as the to/from if no to/from is set. Remember, folks are already using this code base and have configuration setup to use this process plugin in its previous state. We need to support them using this, while still allowing new functionality for setting to/from as distinct parts.

3. +++ b/core/modules/migrate/tests/src/Unit/process/FormatDateTest.php
   +++ b/core/modules/migrate/tests/src/Unit/process/FormatDateTest.php
   @@ -118,6 +118,16 @@ public function datesDataProvider() {
   

   Tests will likely fail, since we aren't using the timezone config option any more.

I like the new to/from config options. But the previous timezone should be kept for BC and should map both to/from when set.

Hm, as far as I know, migration is experimental. I think this allows to break BC, if needed. Otherwise, we will have bloated codebase from the beginning.

#14, Migrate is experimental, but currently considered beta. These should retain BC whenever possible.

We should also trigger a warning when the deprecated method is used. See https://www.drupal.org/node/2856615.

I don't think we need to modify the existing tests at all, they should still continue to work as previously. What we should do is add more tests for a divergent TZ.

Migrate is experimental, but currently considered beta. These should retain BC whenever possible.

Okay, makes sense.

So, the next steps are:

• Add BC layer (with deprecation notice)
• Add tests to cover new functionality

Anything else?

re #18, that sounds about right. I'd also add that this will need a draft CR.

Added few tests
Added deprecation info
Set default timezone during the installation process to make valid expected values.

1. +++ b/core/modules/migrate/src/Plugin/migrate/process/FormatDate.php
   @@ -15,7 +15,12 @@
   + * - timezone: (deprecated) String identifying the required time zone, see
   + *   DateTimePlus::__construct().
   + *   The timezone parameter is deprecated since version 8.4.0
   

   Please format message accordingly to the deprecation policy, see https://www.drupal.org/node/2856615

2. +++ b/core/modules/migrate/src/Plugin/migrate/process/FormatDate.php
   @@ -92,13 +98,15 @@ public function transform($value, MigrateExecutableInterface $migrate_executable
        $timezone = isset($this->configuration['timezone']) ? $this->configuration['timezone'] : NULL;
   

   Using $this->configuration['timezone'] must trigger deprecation error, see https://www.drupal.org/node/2856615, however cannot find an example in core. Anyone to help?

Also deprecation policy mentions

A unit test proving the deprecation notice will be triggered when the deprecated code is called.

Any examples of such kind of unit test?

Answering my own questions.

Good examples are Drupal\Component\Render\FormattableMarkup and unit test Drupal\Tests\Component\Render\FormattableMarkupTest

Changes since last patch:

1. Amend deprecation messages to comply with deprecation policy
2. Add testing of deprecation error using Symfony PHPUnit-Bridge

However, deprecation policy states:

Tests which exercise deprecated code can be annotated with @group legacy in order to avoid deprecation notices during testing, but a follow-up issue should be filed in order to fix such tests to not exercise deprecated code.

Should we keep testing of timezone configuration key?

Very close. Just a couple small things. The testing for legacy that was added here seems sufficient to me.

1. +++ b/core/modules/migrate/src/Plugin/migrate/process/FormatDate.php
   @@ -15,7 +15,13 @@
   + *   in Drupal 8.4.0 and will be removed before Drupal 9.0.0, use from_timezone
   

   Nit: this should read use to and from. Not just from.

2. +++ b/core/modules/migrate/src/Plugin/migrate/process/FormatDate.php
   @@ -91,14 +98,19 @@ public function transform($value, MigrateExecutableInterface $migrate_executable
   +      @trigger_error('Configuration key "timezone" is deprecated in 8.4.0 and will be removed before Drupal 9.0.0, use "from_timezone" instead. See https://www.drupal.org/node/2885746', E_USER_DEPRECATED);
   

   Same applies here.

Looks good.

+++ b/core/modules/migrate/tests/src/Unit/process/FormatDateTest.php
@@ -15,6 +15,14 @@
+  public function setUp() {
+    parent::setUp();
+    date_default_timezone_set('UTC');
+  }

Why is this needed? We set a non-UTC zone in the test bootstrap (#2498619: Unit tests should use a default timezone other that UTC) to catch problems; running as UTC can mask subtle bugs.

#30, everything run through phpunit should have 'Australia/Sydney' as the default TZ since we set it in the bootstrap; some of the other test bases may set their own (need to double check this). Were you seeing something other that this? Or different results when run locally vs on testbot? If so, we may have a larger problem to address.

Actually, comments by @mpdonadio uncovered interesting thing:

Old code wasn't doing any TZ conversion at all, because it was using the same tz

New patch is coming soon

+++ b/core/modules/migrate/src/Plugin/migrate/process/FormatDate.php
@@ -91,14 +98,19 @@ public function transform($value, MigrateExecutableInterface $migrate_executable
-      $transformed = DateTimePlus::createFromFormat($fromFormat, $value, $timezone, $settings)->format($toFormat);
+      $transformed = DateTimePlus::createFromFormat($fromFormat, $value, $from_timezone, $settings)->format($toFormat, ['timezone' => $to_timezone]);
     }

It's probably not accurate to say that no conversion is being done. The $timezone from the setting is being passed to the constructor, which is used when parsing the $value (and should match the TZ if output in the $toFormat) unless an explicit offset is used in $value. This is working as expected, but I want to add an additional test case.

DELETED

Blerg. Please hold off on posting patches. There is also a very subtle edge case bug here that I need to talk to @heddn about tomorrow.

Work in progress.

1. +++ b/core/modules/migrate/tests/src/Unit/process/FormatDateTest.php
   @@ -15,11 +15,14 @@
   +  public function testLocalTimeZone() {
   +    // @todo The 'Australia/Sydney' time zone is set in core/tests/bootstrap.php
   +    // This assertion checks that the assumptions about local time in the rest
   +    // of the test methods are accurate. This should be removed when
   +    // @link https://www.drupal.org/node/2889803 @endlink is committed.
   +    $this->assertEquals('Australia/Sydney', date_default_timezone_get());
      }
   

   This double checks the assumption that we are indeed 'Australia/Sydney' per the phpunit bootstrap. I haven't updated the new test cases to account for this, so tests will fail. Will post that next.

2. +++ b/core/modules/migrate/tests/src/Unit/process/FormatDateTest.php
   @@ -74,14 +77,14 @@ public function testMigrateExceptionBadFormat() {
   -      'from_format' => 'Y-m-d\TH:i:sO',
   -      'to_format' => 'Y-m-d\TH:i:s',
   +      'from_format' => 'Y-m-d\TH:i:s',
   +      'to_format' => 'Y-m-d\TH:i:s e',
          'timezone' => 'America/Managua',
        ];
        $this->plugin = new FormatDate($configuration, 'test_format_date', []);
   -    $actual = $this->plugin->transform('2004-12-19T10:19:42-0600', $this->migrateExecutable, $this->row, 'field_date');
   +    $actual = $this->plugin->transform('2004-12-19T10:19:42', $this->migrateExecutable, $this->row, 'field_date');
    
   -    $this->assertEquals('2004-12-19T10:19:42', $actual);
   +    $this->assertEquals('2004-12-19T10:19:42 America/Managua', $actual);
   

   This is a really subtle quirk. -0600 is an offset, not a time zone; it just happens to be the offset for 'America/Managua'. We missed this in the original patch. Still digging into this a bit to make sure we haven't really uncovered a problem with an edge case in DateTimePlus.

On these two, look at the fails in FormatDateTest::testDeprecatedTimezoneConfigurationKey(). It's easier to do

vendor/bin/phpunit --debug core/modules/migrate/tests/src/Unit/process/FormatDateTest.php

so you can see the expected/actual better. Not sure what is going on, esp when you compare against https://3v4l.org/0j6hK which behaves as I would expect, other than DateTime::createFromFormat() ignore the $timezone when it is part of the format.

Not sure what is going on, esp when you compare against https://3v4l.org/0j6hK which behaves as I would expect, other than DateTime::createFromFormat() ignore the $timezone when it is part of the format.

I've checked everything couple times. And Drupal's code on top of DateTime behaves exactly as PHP internals.

\DateTimePlus::createFromFormat() ignores passed timezone, if passed time specifies a timezone (i.e. format contains). However, as you noticed -0600 is not a timezone, but an offset. So, when using a legacy timezone code the expected output should be 2004-12-19T10:19:42-06:00 -06:00.

Drupal can try to fix this quirk in PHP behavior by setting a timezone if it's passed. However, it should be done in DateTimePlus, not here (follow up issue?). Also, interesting question would be: what to do if offset doesn't match the timezone.

Setting this to needs review.

This issue is soft blocked on #2889814: DateTimePlus doesn't apply the correct time zone when converting a string with an offset right now. In that issue we either need to decide on whether to make the code match the advertised API docs, or update the API docs to match what the code actually does. Right now, I am leaning towards the later.

+++ b/core/modules/migrate/tests/src/Unit/process/FormatDateTest.php
@@ -57,6 +68,26 @@ public function testMigrateExceptionBadFormat() {
+  public function testDeprecatedTimezoneConfigurationKey() {
+    $configuration = [
+      'from_format' => 'Y-m-d\TH:i:sO',
+      'to_format' => 'c e',
+      'timezone' => 'America/Managua',
+    ];
+    $this->plugin = new FormatDate($configuration, 'test_format_date', []);
+    $actual = $this->plugin->transform('2004-12-19T10:19:42-0600', $this->migrateExecutable, $this->row, 'field_date');
+
+    $this->assertEquals('2004-12-19T10:19:42-06:00 -06:00', $actual);
+  }

The problem with this is that it only partially tests the $timezone parameter. The FormatDate docs refer to the DateTimePlus docs, which leads us to the issue described above. At a minimum, I think the above method needs to be converted to use a provider to test various combinations of passing in an offset, a proper time zone, and not passing in anything for $timezone. Then once we resolve #2889814: DateTimePlus doesn't apply the correct time zone when converting a string with an offset we can either adjust the code and/or update the docs to be a bit more specific about how TZ conversion is handled.

I agree with not blocking this issue with the #2889814: DateTimePlus doesn't apply the correct time zone when converting a string with an offset, we have too much examples of such approach in core issue queue already (sadly).

At a minimum, I think the above method needs to be converted to use a provider to test various combinations of passing in an offset, a proper time zone, and not passing in anything for $timezone.

We cover passing a proper timezone and not passing a timezone at all. I can add more tests with passing timezones that do not match offset in time value.

Tested and appears to be working correct.

I'm confirming with the other datetime maintainer that we are OK with a doc fix for the DateTimePlus quirk (to align all of the docs to be consistent with how \DateTime really works).

So, I think we should

- Update testDeprecatedTimezoneConfigurationKey to use a provider to cover the different cases of zone name, offset, etc, to prevent any regressions.
- Double check the new parameters for the same thing
- Update the docs on this plugin to mention the quirk with the from timezone being ignored with the offset; the most popular date format will likely be the ISO one w/ the offset. This will prevent confusion.

Somewhat novice tasks in #40. phpunit data provider, double checking parameters, updating docs. Yup. That all sounds do-able.

We've waited 5 months for a novice that has never appeared so I'm jumping in.

Patch in #42 no longer applies. Re-rolled (before addressing @mpdonadio's comments in #46).

Added data provider.

Some feedback to ponder.

1. +++ b/core/modules/migrate/src/Plugin/migrate/process/FormatDate.php
   @@ -94,14 +113,24 @@ public function transform($value, MigrateExecutableInterface $migrate_executable
   +      @trigger_error('Configuration key "timezone" is deprecated in 8.4.0 and will be removed before Drupal 9.0.0, use "from_timezone" and "to_timezone" instead. See https://www.drupal.org/node/2885746', E_USER_DEPRECATED);
   

   Nit. Can we get the point release updated here? Or just say in 8.4.x; I'm not recalling how we normally do that in core.

2. +++ b/core/modules/migrate/src/Plugin/migrate/process/FormatDate.php
   @@ -94,14 +113,24 @@ public function transform($value, MigrateExecutableInterface $migrate_executable
   +      $to_timezone = NULL;
   

   Let's check if to_timezone is set and use it before defaulting to NULL.

3. +++ b/core/modules/migrate/tests/src/Unit/process/FormatDateTest.php
   @@ -15,6 +15,17 @@
   +    // @link https://www.drupal.org/node/2889803 @endlink is committed.
   

   This is already fixed, so maybe we should remove this already?

4. +++ b/core/modules/migrate/tests/src/Unit/process/FormatDateTest.php
   @@ -57,6 +68,40 @@ public function testMigrateExceptionBadFormat() {
   +   * @expectedDeprecation Configuration key "timezone" is deprecated in 8.4.0 and will be removed before Drupal 9.0.0, use "from_timezone" and "to_timezone" instead. See https://www.drupal.org/node/2885746
   

   I don't see any other examples of @expectedDeprecation in core. Are we using $this->expectedException() instead?

I don't see any other examples of @expectedDeprecation in core. Are we using $this->expectedException() instead?

Just double checked, and yes `$this->expectedException()` is the preferred now.

Addressed points 1-3 in #51, but I disagree about point 4; @expectedDeprecation appears a few times in the codebase. If we were to use $this->setExpectedException() what would be the Exception Name?

Removed a couple of lines from the patch included in error.

Took a brief look and it really nice to have the timezone here. Thanks everyone.
Points #51-2,3,4 have been addressed.

In #51-1 heddn queried if this should be deprecated in 8.4.0 or 8.4.x. The text hasn't changed, is this intended or an oversight?

I had misunderstood #51-1 and removed any mention of 8.4.0. I have now replaced it with 8.4.x (and in the @trigger_error too).

All feedback addressed. Ready to go.

Re-tested. Appears to have been a testbot fault. Back to RTBC.

Re-tested. Appears to have been a testbot fault (again). Back to RTBC.

Non-related testbot failure.

Added review credit for @heddn

Committed 5b62f5d and pushed to 8.5.x

published change record

Since Migrate is still a beta experimental, is this 8.4.x eligible?

@mpdonadio, I understand that the answer is no. A similar question was asked in #2796393: Migration process plugin not working with multiple source IDs, comment #38. In the next comment xjm responded,

The next patch release window is February 7. As indicated there, is for critical issues only. This is not a critical. It will be included in 8.5.0 on March 7.

.

where 'there' is this link, https://www.drupal.org/core/release-cycle-overview#current-development-c...