Problem/Motivation

If you import from CSV, XML, JSON (non-Drupal sources), it is fairly common to have dates formatted in lots of different ways. Let's provide a mechanism to format them to the required formats provided by Drupal.

Proposed resolution

Code. Do it.
Write tests.

Solution:

  1. Ability to migrate a source in various date formats to a date field
  2. Ability to migrate a source in various date formats to a date range field
  3. Documentation on how to do these migrations on https://www.drupal.org/docs/8/api/migrate-api/migrate-process/migrate-pr...

Remaining tasks

Reviews

User interface changes

API changes

Data model changes

CommentFileSizeAuthor
#108 migrate_date.png56.14 KBsylus
#105 interdiff-2820490-101-105.txt928 bytesshabana.navas
#104 backport_for_8.3-2820490-104.patch7.91 KBshabana.navas
#97 2820490-97.patch8.11 KBshabana.navas
#97 interdiff-92-97.txt1.1 KBshabana.navas
#95 interdiff-87-92.txt3.58 KBManuel Garcia
#94 interdiff-87-92.txt3.58 KBManuel Garcia
#92 2820490-92.patch7.88 KBshabana.navas
#87 interdiff-77-87.txt2.08 KBmpdonadio
#87 2820490-87.patch9.01 KBmpdonadio
#77 interdiff_75-77.txt2.99 KBjofitz
#77 interdiff_68-77.txt5.64 KBjofitz
#77 2820490-77.patch8.83 KBjofitz
#75 interdiff_68-75.txt3.78 KBjofitz
#75 2820490-75.patch7.53 KBjofitz
#68 interdiff-56-68.txt940 bytesmpdonadio
#68 interdiff-66-68.txt1.31 KBmpdonadio
#68 2820490-68.patch7.31 KBmpdonadio
#66 interdiff-56-66.txt1.5 KBmpdonadio
#66 2820490-66.patch7.45 KBmpdonadio
#57 interdiff_45-56.txt3.78 KBheddn
#56 interdiff_45-56.txt1.04 KBheddn
#56 formatdate_process_plugin-2820490-56.patch7.29 KBheddn
#45 interdiff.txt1.52 KBManuel Garcia
#45 formatdate_process_plugin-2820490-45.patch6.19 KBManuel Garcia
#39 interdiff-24-39.txt4.89 KBmpdonadio
#39 2820490-39.patch5.81 KBmpdonadio
#39 2820490-test-only.patch4.89 KBmpdonadio
#24 formatdate_process_plugin-2820490-23.patch4.77 KBManuel Garcia
#22 interdiff.txt633 bytesManuel Garcia
#22 formatdate_process_plugin-2820490-22.patch0 bytesManuel Garcia
#22 2820490-17-19.diff1.21 KBManuel Garcia
#19 formatdate_process_plugin-2820490-19.patch4.78 KBManuel Garcia
#17 interdiff_6-17.txt1.34 KBheddn
#17 formatdate_process_plugin-2820490-17.patch4.68 KBheddn
#6 formatdate_process_plugin-2820490-6.patch4.62 KBheddn
#6 interdiff_4-6.txt456 bytesheddn
#4 formatdate_process_plugin-2820490-2.patch4.71 KBheddn
#2 formatdate_process_plugin-2820490-2.patch4.71 KBheddn
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

heddn created an issue. See original summary.

heddn’s picture

Status: Active » Needs review
FileSize
4.71 KB
heddn’s picture

Version: 8.x-3.x-dev » 8.x-2.x-dev

Bumping this down to v2 for now to get a test pass.

heddn’s picture

FileSize
4.71 KB
Grayside’s picture

  1. +++ b/src/Plugin/migrate/process/FormatDate.php
    @@ -0,0 +1,58 @@
    +    return DateTimePlus::createFromFormat($fromFormat, $value, $timezone, $settings)->format($toFormat);
    

    Shouldn't this use the DateFormatter service so we can accept predefined formats in the Drupal site as well?

  2. +++ b/tests/src/Unit/process/FormatDateTest.php
    @@ -0,0 +1,87 @@
    + * Contains \Drupal\Tests\migrate_plus\Unit\process\SkipOnValueTest.
    

    Copypasta

heddn’s picture

re #5
1) I disagree. There's only a few pre-set formats that the DB will accept the data for a date field. And the chances that those formats are also going to be used to present the data to a public audience are slim. It is a question of audience. Date formats are for people, while the format here is for the DB. The audience for date formatters in the GUI is a reusable date format for non-technical people who aren't writing any code. The audience of a process plugin is a person with some knowledge of software development, who can lookup valid date formats on php.net's website.
2) Fixed.

StevenWill’s picture

I tried to test with a d6 date field using the yml format provided in the comments. I received an error that the value is an array.
////
#0 /var/www/html/modules/migrate_plus/src/Plugin/migrate/process/FormatDate.php(56):
Drupal\Component\Datetime\DateTimePlus::createFromFormat('Y-m-d\\TH:i:s', Array, NULL, Array)

This is the yml entry...
field_release_date:
plugin: format_date
from_format: 'Y-m-d\TH:i:s'
to_format: 'Y-m-d'
source: field_release_date

heddn’s picture

re #7, if you read the IS, this is for dates that come from non-Drupal source. For drupal sources, just migrate the value verbatim into the D8 destination.

RKopacz’s picture

Has this patch been committed? Assuming not, so we must apply to migrate plus if we wish to use, correct?

heddn’s picture

re #9, there is no commit on this yet. It is waiting for someone to review it and mark as tested by the community (RTBC).

RKopacz’s picture

Got it, @heddn, thank you.

Ada Hernandez’s picture

Status: Needs review » Reviewed & tested by the community

I have tested this patch and works good for my migration.

mstrelan’s picture

Unfortunately for me DateTimePlus::createFromFormat() throws an error if my from format is d/m/Y but some of my data does not have leading zeroes. That's probably an issue for Core, but it's worth noting that native PHP's DateTime::createFromFormat() works for d/m/Y or j/m/Y or j/n/Y with or without leading zeroes.

EDIT: Never mind, this can be worked around by disabling format validation.

  field_updated:
    plugin: format_date
    from_format: 'd/m/Y'
    to_format: 'Y-m-d'
    settings:
      validate_format: false
    source: updated
Albert Volkman’s picture

RTBC +1

Albert Volkman’s picture

Actually, I take that back. For some reason the value coming in for the first argument of FormatDate::transform is an array. I have to alter this patch to pass $value['value'] to DateTimePlus::createFromFormat. Of course, it's entirely possible that my configuration is incorrect-

  field_publish_date:
    plugin: format_date
    from_format: 'Y-m-d\Tg:i:s'
    to_format: Y-m-d
    settings:
      validate_format: false
    source: field_published_date
damondt’s picture

Shouldn't you test if $value is null in FormatDate::transform (and in that case just return null) so that it doesn't throw an exception for empty values?

heddn’s picture

Status: Reviewed & tested by the community » Needs review
FileSize
4.68 KB
1.34 KB

Good idea.

mikeryan’s picture

Project: Migrate Plus » Drupal core
Version: 8.x-2.x-dev » 8.2.x-dev
Component: Plugins » migration system
Status: Needs review » Needs work
Issue tags: +Needs reroll, +Needs change record

Per https://www.drupal.org/node/2566779#comment-11784111, moving to the core issue queue.

Needs to be rerolled for core namespaces.

Manuel Garcia’s picture

Status: Needs work » Needs review
Issue tags: -Needs reroll
FileSize
4.78 KB

Putting the files in the right place and addressing the namespaces.

heddn’s picture

Status: Needs review » Needs work
+++ b/core/modules/migrate/src/Plugin/migrate/process/FormatDate.php
@@ -0,0 +1,58 @@
+/**

+++ b/core/modules/migrate/tests/src/Unit/process/FormatDateTest.php
@@ -0,0 +1,83 @@
+ * @group migrate_plus
+ * @coversDefaultClass \Drupal\migrate_plus\Plugin\migrate\process\FormatDate

Wrong group/namespace.

And can we get an interdiff? It would make the review easier.

The last submitted patch, 19: formatdate_process_plugin-2820490-19.patch, failed testing.

Manuel Garcia’s picture

The last submitted patch, 22: 2820490-17-19.diff, failed testing.

Manuel Garcia’s picture

Oops, here's the good patch, interdiff on #22 is good.

The last submitted patch, 22: formatdate_process_plugin-2820490-22.patch, failed testing.

heddn’s picture

Status: Needs review » Reviewed & tested by the community
Issue tags: -Needs change record

Simple re-roll from migrate_plus => migrate module. It has tests and was already RTBC in the other namespace so I think this can be RTBCed here too. I'm simply RTBCing the re-roll.

I've also added a draft change record.

heddn’s picture

Category: Feature request » Task

Changing to a task since this is necessary for #2566779: Migration D6 > D8 of CCK date fields

tstoeckler’s picture

Status: Reviewed & tested by the community » Needs review
+++ b/core/modules/migrate/src/Plugin/migrate/process/FormatDate.php
@@ -0,0 +1,58 @@
+    return $value ? DateTimePlus::createFromFormat($fromFormat, $value, $timezone, $settings)->format($toFormat) : $value;

Should we not catch a possible exception thrown here and re-throw a MigrateException? Possibly a SkipRowException or something like that?

I.e. if somehow an invalid value ends up here.

heddn’s picture

+++ b/core/modules/migrate/src/Plugin/migrate/process/FormatDate.php
@@ -0,0 +1,58 @@
+    return $value ? DateTimePlus::createFromFormat($fromFormat, $value, $timezone, $settings)->format($toFormat) : $value;

There isn't a chance of an exception getting caught, because ::format() already catches Exception and adds to an errors array. So it is impossible to catch such exception.

mikeryan’s picture

You can follow the format_date plugin with a skip_on_empty to cause a skip - and when #2655154: Optionally log messages for skip_on_empty and skip_row_if_not_set lands, you'll be able to generate a message as well.

heddn’s picture

@tstoeckler back to RTBC?

tstoeckler’s picture

But DateTimePlus::createFromFormat() can throw an exception already. See https://api.drupal.org/api/drupal/core%21lib%21Drupal%21Component%21Date...

So I think that one should be caught?

heddn’s picture

Good catch. I didn't notice the createFromFormat; only the format in the calling code. Can we add a test to see if this only happens on a malformed format or is it also possible with a malformed time string? It isn't really clear what causes a FALSE to be returned from createFromFormat.

If it is a malformed format, I'd really like for a loud failure to occur. But if that is lumped in with a malformed data string, well then, I guess we don't have a lot of options. We need to support malformed time strings.

The complexities here are that createFromFormat is throwing an Exception. Which is really nasty. I really hate to catch Exception, it is so generic.

Albert Volkman’s picture

heddn’s picture

re #34 it is almost certainly due to your configuration.

mpdonadio’s picture

+++ b/core/modules/migrate/src/Plugin/migrate/process/FormatDate.php
@@ -0,0 +1,58 @@
+    return $value ? DateTimePlus::createFromFormat($fromFormat, $value, $timezone, $settings)->format($toFormat) : $value;

DateTimePlus::createFromFormat() will throw an \Exception when the $value can't be validated against the $fromFormat and turned into a proper \DateTime object. Think of it as a parsing-regex (eg, preg w/ matches). It is mostly unforgiving, but with some of the format placeholders there can be ambiguity. Since characters that aren't placeholders get interpreted as literals (like for separators), I am unsure is is possible to have an invalid format, just a value that won't match against a particular format.

I'm making an issue to change this to \InvalidArgumentException. It will have to be against 8.3.x, but with the chance to apply to 8.2.x b/c it is a small disruption and also would help this (discussed in IRC w/ @xjm).

Albert Volkman’s picture

@heddn I posted my configuration. Do you see anything wrong with it?

heddn’s picture

re: #37, please open a new support request for it. But I think the source data needs some preprocessing first. You can use the extract process plugin to retrieve the 'value'.

re: #36, that is great info. So can we ignore throwing the exception then by adding the following snippet to the migration? Or do we still need to try/catch the thing?

settings:
      validate_format: false
mpdonadio’s picture

You still need to check for the exception. One can be thrown before that setting is used. See the demo.

I would do something like this. Also adjusted a few small things while I was in there. This comes up green locally.

mpdonadio’s picture

+++ b/core/modules/migrate/src/Plugin/migrate/process/FormatDate.php
@@ -0,0 +1,77 @@
+    $timezone = isset($this->configuration['timezone']) ? $this->configuration['timezone'] : NULL;
+    $settings = isset($this->configuration['settings']) ? $this->configuration['settings'] : [];

Two things about these lines. I think the docblock should be updated to show these as an example, at a minimum the one for timezone.

Also, neither of these lines have test coverage. I don't think settings is really worth it, especially since I think you should almost always be calling that with validate_format==TRUE. Given the number of bugs that have been uncovered and probably still exist with timezones in Drupal 8, I think the timezone setting should have explicit test coverage. It may also be needed once one of the TZ related issues gets ironed out (I referenced a few).

The last submitted patch, 39: 2820490-test-only.patch, failed testing.

mikeryan’s picture

Assigned: Unassigned » mikeryan
mikeryan’s picture

Assigned: mikeryan » Unassigned
Status: Needs review » Needs work
  1. +++ b/core/modules/migrate/src/Plugin/migrate/process/FormatDate.php
    @@ -0,0 +1,77 @@
    + * This plugin returns date storage formats from \DateTime::createFromFormat.
    

    The plugin doesn't *return* formats, it *applies* formats to the source data.

  2. +++ b/core/modules/migrate/src/Plugin/migrate/process/FormatDate.php
    @@ -0,0 +1,77 @@
    + * Example usage for date only fields (DATETIME_DATE_STORAGE_FORMAT):
    

    Before the examples, we should explicitly describe the configuration options - from_format, to_format, timezone, and (as mpdonadio suggests) settings. With a link to php.net documentation on date/time format strings. Please do the same in the change record as well.

  3. +++ b/core/modules/migrate/src/Plugin/migrate/process/FormatDate.php
    @@ -0,0 +1,77 @@
    +    } catch (\Exception $e) {
    

    Coding standard: catch on separate line.

Manuel Garcia’s picture

Assigned: Unassigned » Manuel Garcia
Manuel Garcia’s picture

Status: Needs work » Needs review
FileSize
6.19 KB
1.52 KB

Addressing #43

Manuel Garcia’s picture

Assigned: Manuel Garcia » Unassigned
mikeryan’s picture

Status: Needs review » Needs work

Thanks! But there's also mpdonadio's request in #40, I agree:

Given the number of bugs that have been uncovered and probably still exist with timezones in Drupal 8, I think the timezone setting should have explicit test coverage.

Manuel Garcia’s picture

Issue tags: +Needs tests
RKopacz’s picture

Using the patch in #45, not working for me. Have a CSV file, the yml works correctly without the date field. But when I add

field_date:
    plugin: format_date
    from_format: 'm/d/Y H:i:s'
    to_format: 'Y-m-d\TH:i:s'
    source: date

As per the doxygen comments in FormatDate.php which was installed after applying the patch. 'date' is the column label on the CSV column for the date field.

The import fails. The field in the CSV is configured as follows:

03/01/2006 18:00:00

For March 1, 2006 at 6pm, which seems to be correct. I applied the patch to core 8.2.3

Can anyone spot what I am doing wrong?

mikeryan’s picture

@RKopacz: The configuration should be indented 2 spaces rather than 4, but that shouldn't make a difference in practice.

Exactly what does "the import fails" mean? If there were error messages, please post them here (drush mmsg [migration-id]). If the field doesn't appear to be properly populated on the site, can you look at the SQL table node__field_date and see whether the field was populated at all, and if so how it's formatted?

Thanks.

RKopacz’s picture

@mikeryan thank you for your quick response. I didnt' notice that the pasted text had four spaces. The code in fact has two spaces.

When I run the migration, I get this:

Processed 124 items (0 created, 0 updated, 124 failed, 0 ignored) - done with 'meetings'

That drush command was very helpful, thank you for that! It returned a set of repetitive messages, which all look like this.

87f84924539b4548a3fe8a041f6f8e949efe2b29fa3b429674d82f9f125435a6 1 Format date plugin could not transform "12/03/2014 18:00:00" using the format "m/d/Y".

I thought it might have to do with the destination format, so that seems to confirm it. But is that is not the destination format, what is?

mikeryan’s picture

Actually, it seems like something to do with from_format - in your .yml you have it as 'm/d/Y H:i:s', but the error message reports trying to use "m/d/Y". Is it possible that you edited your .yml from an original value of 'm/d/Y' but have not reloaded (via a config import or config_devel module) your changes?

RKopacz’s picture

@mikeryan yes, I just saw that before I received your message. I am now rolling back the DB to start fresh, and retrying. I will let you know how it works out.

RKopacz’s picture

Ha @mikeryan, when I rolled back the DB and reinstalled the migration module for the yml, it worked! Thank you for your quick feedback, those drush commands are like gold. Very helpful.

My last Everest on this is to now pull in a speaker content type using the entity_lookup plugin. Will be experimenting shortly with that. It seems as though entity_lookup docs are focused on taxonomies, not nodes. If you have any insights, they are welcome, although I realize they are not appropriate for this thread. I will be doing them in a group where the dated node will be dependent on the speaker node.

Thank you again!

heddn’s picture

Assigned: Unassigned » heddn

Assigning to work on the test coverage.

heddn’s picture

heddn’s picture

FileSize
3.78 KB

Bad interdiff. Better now.

marysmech’s picture

I tried patch from #56 for date field migration from D6 to D8. When I use format from plugin example:

field_date:
    plugin: format_date
    from_format: 'd-m-Y'
    to_format: 'd/m/Y'
    source: field_valid

I get error: DateTime::createFromFormat() expects parameter 2 to be string, array given DateTimePlus.php:219

I have been debugging the problem and second parameter ($value) is array of value and delta e.g.

Array
(
    [value] => 2016-05-25T00:00:00
    [delta] => 0
)

Am I doing something wrong? How should I pass field value? I have been trying source: 'field_valid/value' but then is $value empty.

adriancid’s picture

With patch #56 and #45 I always have this error:

[error] Drupal\Component\Plugin\Exception\PluginNotFoundException: The "format_date" plugin does not exist.

marysmech’s picture

I had same problem. For me problem was that I have Drupal installed via composer and after patch apply format_dateplugin was added to wrong directory (to web/core/core/modules/... instead of web/core/modules/migrate/...).

adriancid’s picture

I found that if you apply the patch #56 and reinstall the migrate module the plugin works

mikeryan’s picture

Assigned: Unassigned » mikeryan
phenaproxima’s picture

Status: Needs review » Needs work
+++ b/core/modules/migrate/src/Plugin/migrate/process/FormatDate.php
@@ -0,0 +1,98 @@
+      throw new MigrateException(sprintf('Format date plugin could not transform "%s" using the format "%s". Error: %s', $value, $fromFormat, $e->getMessage()));

Let's pass $e to the MigrateException so that the calling code has access to the previous exception (and can examine its backtrace or other relevant data).

mpdonadio’s picture

#2830079: Change DateTimePlus to throw more specific exceptions landed, so we may want to adjust the try/catch to use the more specific exception.

Manuel Garcia’s picture

+++ b/core/modules/migrate/src/Plugin/migrate/process/FormatDate.php
@@ -0,0 +1,98 @@
+    // @todo Update the exception message we throw based on
+    //   https://www.drupal.org/node/2830079

Like @mpdonadio mentioned, #2830079: Change DateTimePlus to throw more specific exceptions has landed.

mpdonadio’s picture

Status: Needs work » Needs review
FileSize
7.45 KB
1.5 KB

Updated for #63 and #64/#65.

Don't think it is worth having different message strings since we display $e->getMessage(), which will further explain the problem.

Status: Needs review » Needs work

The last submitted patch, 66: 2820490-66.patch, failed testing.

mpdonadio’s picture

Status: Needs work » Needs review
FileSize
7.31 KB
1.31 KB
940 bytes

Ah. I did that against 8.3.x, and #2830079: Change DateTimePlus to throw more specific exceptions was only applied to that and not 8.2.x :/

Here is a proper version against 8.2.x that keeps the @todo and the single catch, and addresses #63. FormatDateTest passes locally.

Attached two interdiffs to make it easier to see the change.

Status: Needs review » Needs work

The last submitted patch, 68: 2820490-68.patch, failed testing.

mpdonadio’s picture

Status: Needs work » Needs review

Appears to be a random failure. Opened #2843024: Random fail in LanguageUILanguageNegotiationTest.

josephdpurcell’s picture

josephdpurcell’s picture

Issue summary: View changes

Updating the description to reflect the needs of the duplicated tickets 2830296 and 2813539 as I understand them--please revise if needed.

quietone’s picture

Status: Needs review » Needs work

Only comments about coding standards, didn't review the code.

  1. +++ b/core/modules/migrate/src/Plugin/migrate/process/FormatDate.php
    @@ -0,0 +1,98 @@
    + * This plugin applies date storage formats from \DateTime::createFromFormat.
    

    DateTimePlus?

    And an additional explanatory paragraph would be useful.

  2. +++ b/core/modules/migrate/src/Plugin/migrate/process/FormatDate.php
    @@ -0,0 +1,98 @@
    + * @MigrateProcessPlugin(
    ...
    +  *
    

    extra space before asterisk.

  3. +++ b/core/modules/migrate/src/Plugin/migrate/process/FormatDate.php
    @@ -0,0 +1,98 @@
    + *
    

    Extra line.

  4. +++ b/core/modules/migrate/src/Plugin/migrate/process/FormatDate.php
    @@ -0,0 +1,98 @@
    + * @MigrateProcessPlugin(
    

    Not sure if this matters, but this is at the end of the docbloc in the current documented process plugins. See ArrayBuild.php

  5. +++ b/core/modules/migrate/src/Plugin/migrate/process/FormatDate.php
    @@ -0,0 +1,98 @@
    + *   - timezone: @see Drupal\Component\Datetime\DateTimePlus::__construct()
    + *   - settings: @see Drupal\Component\Datetime\DateTimePlus::__construct()
    

    This needs some text. The @see information will be moved to an @see section and no test will appear here.

  6. +++ b/core/modules/migrate/src/Plugin/migrate/process/FormatDate.php
    @@ -0,0 +1,98 @@
    + *     field_date:
    

    The indentation of the examples should be 2 spaces.

  7. +++ b/core/modules/migrate/src/Plugin/migrate/process/FormatDate.php
    @@ -0,0 +1,98 @@
    + * Example usage for date only fields (DATETIME_DATE_STORAGE_FORMAT):
    
  8. Needs to show a real world example following each code example.

  9. +++ b/core/modules/migrate/tests/src/Unit/process/FormatDateTest.php
    @@ -0,0 +1,116 @@
    +  public function testTransform($configuration, $value, $expected) {
    

    Doc bloc missing argument documentation

  10. +++ b/core/modules/migrate/src/Plugin/migrate/process/FormatDate.php
    @@ -0,0 +1,98 @@
    +    // Attempt to transformed the supplied date using the defined input format.
    
  11. s/Attempt to transformed/Attempts to transform/

  12. +++ b/core/modules/migrate/src/Plugin/migrate/process/FormatDate.php
    @@ -0,0 +1,98 @@
    +
    

    There are two lines at the end of the file.

  13. +++ b/core/modules/migrate/tests/src/Unit/process/FormatDateTest.php
    @@ -0,0 +1,116 @@
    + * @group migrate
    + * @coversDefaultClass Drupal\migrate\Plugin\migrate\process\FormatDate
    

    Is there supposed to be a blank line between these?

mpdonadio’s picture

#73-1, no it should reference \DateTime::createFromFormat directly. DateTimePlus::createFromFormat() doesn't define the formats, and is a fancy wrapper around \DateTime::createFromFormat(). I am not sure what an explanatory paragraph could potentially say, but a @see http://php.net/manual/en/datetime.createfromformat.php would probably be beneficial here.

jofitz’s picture

Assigned: mikeryan » Unassigned
Status: Needs work » Needs review
FileSize
7.53 KB
3.78 KB
  1. @todo
  2. Remove additional space.
  3. Removed extra line.
  4. Moved to end of docbloc.
  5. @todo
  6. Corrected indentation.
  7. @todo
  8. Added argument documentation.
  9. Text correction.
  10. Removed extra line.
  11. Added missing line.
Manuel Garcia’s picture

Status: Needs review » Needs work

Changes on #75 look good to me.

Setting to needs work for the outstanding issues on #73.1, 5 and 7.

jofitz’s picture

Status: Needs work » Needs review
FileSize
8.83 KB
5.64 KB
2.99 KB

#73.1 Corrected /DateTime to /DateTimePlus and expanded upon the explanatory sentence.
#73.5 Added text to docbloc.
#73.7 Added a real world example to each example.

Manuel Garcia’s picture

Status: Needs review » Reviewed & tested by the community

Thanks @Jo! Changes on #77 look good.

Assuming #77 comes back green, I'm taking the leap and setting it again to RTBC since we now have tests, we're taking advantage of #2830079: Change DateTimePlus to throw more specific exceptions being in, and code styling issues have been addressed.

mpdonadio’s picture

Status: Reviewed & tested by the community » Needs work
+++ b/core/modules/migrate/src/Plugin/migrate/process/FormatDate.php
@@ -0,0 +1,129 @@
+ * Available configuration keys
+ *   - from_format: the source format string as accepted by date()
+ *       http://php.net/manual/en/function.date.php
+ *   - to_format: the format string as accepted by date() to apply on source.

The docs here for the formats should be referencing \DateTime::createFromFormat with a @see http://php.net/manual/datetime.createfromformat.php. There are some subtle differences between the two that come into play when you are parsing versus displaying.

+++ b/core/modules/migrate/src/Plugin/migrate/process/FormatDate.php
@@ -0,0 +1,129 @@
+ * This plugin converts date/datetime strings from one format into another by
+ * applying date storage formats from \DateTimePlus::createFromFormat.

Still not sure if I agree with this comment here. \DateTimePlus::createFromFormat() is what is being called but the format strings aren't defined or documented there (they have a @see to \DateTime::createFromFormat). When we fix the first point above, this may become moot.

josephdpurcell’s picture

The #2813539: Drupal 8 Date Range Migration ticket was moved into this one. Looking at the #77 code I assume this patch isn't for supporting a date time range migration, just date time migrations.

How would a migration to a date time range field work with #77?

Perhaps #77 does in fact support date time ranges by using subfields like @jcisio suggested? For example:

process:
  field_date_range/value:
    plugin: format_date
    from_format: 'm/d/Y'
    to_format: 'Y-m-d'
    source: 'date_start'
  field_date_range/end_value:
    plugin: format_date
    from_format: 'm/d/Y'
    to_format: 'Y-m-d'
    source: 'date_end'

If that is the case, a date time range example somewheres in codes or a test would be useful, or also at https://www.drupal.org/docs/8/api/migrate-api/migrate-process/migrate-pr....

heddn’s picture

Version: 8.2.x-dev » 8.3.x-dev

Two super small nits. And let's just move this to 8.3 an handle the exception properly.

  1. +++ b/core/modules/migrate/src/Plugin/migrate/process/FormatDate.php
    @@ -0,0 +1,129 @@
    + *       http://php.net/manual/en/function.date.php
    

    Let's use @link doxygen.

    https://www.drupal.org/docs/develop/coding-standards/api-documentation-a...

  2. +++ b/core/modules/migrate/src/Plugin/migrate/process/FormatDate.php
    @@ -0,0 +1,129 @@
    +      throw new MigrateException(sprintf('Format date plugin could not transform "%s" using the format "%s". Error: %s', $value, $fromFormat, $e->getMessage()), $e->getCode(), $e);
    

    $e->getCode() is not used and probably isn't necessary.

  3. Lastly, since migrate is now in beta, let's just adopt this date handling into 8.3. And do the work handled by #2830079: Change DateTimePlus to throw more specific exceptions that introduced InvalidArgumentException. That way we'll get rid of an @todo at the same time.
heddn’s picture

Hmm, date_range is going to be different depending on if the destination or source fields has a cardinality of >1. If it is >1, then it would follow a pattern that uses the iterator plugin:
https://gist.github.com/heddn/00f80f1ac388cd579bb8d142a4fe560f

If it is cardinality = 1, then you'd just do normal process plugin mapping that utilizes sub fields.

https://www.mtech-llc.com/blog/charlotte-leon/migration-csv-data-paragraphs is a blog post that demonstrates the differences in functionality between these types of cardinalities.

mpdonadio’s picture

#81-1, good idea, but still think this should refer to http://php.net/manual/datetime.createfromformat.php.

#81-2, since the type signature of MigrateException::__construct is

public function __construct($message = NULL, $code = 0, \Exception $previous = NULL, $level = MigrationInterface::MESSAGE_ERROR, $status = MigrateIdMapInterface::STATUS_FAILED) { }

not sure how you can not pass the code and still have the previous exception get used?

#81-3, if anyone wants to tackle this, you can refer to https://www.drupal.org/files/issues/interdiff-56-66.txt

Version: 8.3.x-dev » 8.4.x-dev

Drupal 8.3.0-alpha1 will be released the week of January 30, 2017, which means new developments and disruptive changes should now be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

mikeryan’s picture

osopolar’s picture

As it took me some time to get it right, here my migration configuration for date fields in a drupal 6 to drupal 8 migration:

process:
  field_date:
    -
     plugin: extract
     source: field_date
     index:
       - '0'
       - value
    -
     plugin: format_date
     from_format: 'Y-m-d H:i:s'
     to_format: 'Y-m-d\TH:i:s'
     timezone: 'GMT'
mpdonadio’s picture

Status: Needs work » Needs review
FileSize
9.01 KB
2.08 KB

Think this addresses all outstanding feedback except #81-2, which I don't think is possible. Test passes locally.

heddn’s picture

Status: Needs review » Reviewed & tested by the community

All feedback addressed. Since this was already rtbc, and I haven't contributed in the 3 months since then I could probably flip the switch. Doing that now.

alexpott’s picture

Status: Reviewed & tested by the community » Needs review
+++ b/core/modules/migrate/src/Plugin/migrate/process/FormatDate.php
@@ -0,0 +1,131 @@
+ *     settings:

There's no test coverage of setting settings in the migration. To be fair validate_format is not tested anywhere in the code base as far as I can see. Can we can an issue created to add test coverage for it.

I'm undecided as to whether we should add test coverage here. We are allowing migration builders to use it... what do you think @mpdonadio?

quietone’s picture

Status: Needs review » Needs work

So good to see this happening.

I am only reviewing the documentation not any of the code. The doc content is fine but the structure/layout should be the same as the other process plugins. They are currently being documented and this one looks and read differently than the others. Let's make this one similar. See #2776179: [meta] Add process plugin documentation to the codebase.

  1. +++ b/core/modules/migrate/src/Plugin/migrate/process/FormatDate.php
    @@ -0,0 +1,131 @@
    + * This plugin converts date/datetime strings from one format into another by
    + * applying date storage formats from \DateTime::createFromFormat.
    

    Summaries are supposed to be one line and begin with a verb. How about 'Converts date/datetime format.

    Adding an explanatory paragraph may be useful. Not sure.

  2. +++ b/core/modules/migrate/src/Plugin/migrate/process/FormatDate.php
    @@ -0,0 +1,131 @@
    + *   - to_format: the format string as accepted by date() to apply on source.
    

    Can we made this simpler, it took two reads to process this. 'The destination format.'

  3. +++ b/core/modules/migrate/src/Plugin/migrate/process/FormatDate.php
    @@ -0,0 +1,131 @@
    + *   - from_format: the source format string as accepted by
    

    All of these should be moved left two spaces and capitalize the first character of the explanation.

  4. +++ b/core/modules/migrate/src/Plugin/migrate/process/FormatDate.php
    @@ -0,0 +1,131 @@
    + * The format_date process plugin (containing the formats of the source datetime
    

    Precede all the examples with a single Example line: 'Example:'

  5. +++ b/core/modules/migrate/src/Plugin/migrate/process/FormatDate.php
    @@ -0,0 +1,131 @@
    + * The source value is a date string in a particular format:
    ...
    + * The source value is a datetime string in a particular format:
    ...
    + * source: 01/05/1955 10:43:22
    + * @endcode
    + * The destination value is the datetime string in a different format:
    + * @code
    + * destination: 1955-01-05T10:43:22
    + * @endcode
    ...
    + * @code
    

    Move this to the end of the example and make it simpler. 'If the source value was '01/05/1955 10:43:22' the transformed value would be 1955-01-05T10:43:22'. And the same for the other examples.

  6. +++ b/core/modules/migrate/src/Plugin/migrate/process/FormatDate.php
    @@ -0,0 +1,131 @@
    + *
    

    Add an @see for the interface. "* @see \Drupal\migrate\Plugin\MigrateProcessInterface"

mpdonadio’s picture

Assigned: Unassigned » mpdonadio

Assigning to myself to ponder #89; anyone feel free to fix the stuff in #90.

The setting we are talking about handles this:

      // Functions that parse date is forgiving, it might create a date that
      // is not exactly a match for the provided value, so test for that by
      // re-creating the date/time formatted string and comparing it to the input. For
      // instance, an input value of '11' using a format of Y (4 digits) gets
      // created as '0011' instead of '2011'.

We have coverage for it in core in DateTimePlusTest::testInvalidDates; the setting defaults to TRUE and the test checks for the exception. We don't have coverage when it is FALSE.

But, this setting on DateTimePlus is really one that I really think shouldn't be there, and that code needs to be refactored anyway. I don't see what you would pass in false. I'm half-tempted to say that we should remove the settings. That is the only setting on DTP.

shabana.navas’s picture

Status: Needs work » Needs review
FileSize
7.88 KB

Added new patch with changes applied from recommendations in #90.

Status: Needs review » Needs work

The last submitted patch, 92: 2820490-92.patch, failed testing.

Manuel Garcia’s picture

Status: Needs work » Needs review
FileSize
3.58 KB

@shabana.navas thanks for working on this! If I may suggest a couple of things to help your patches get reviewed:

  1. Describe what you did / fixed.
  2. Include an interdiff so as to make it easier to review your changes.
Manuel Garcia’s picture

FileSize
3.58 KB

Oops! did the interdiff the wrong way around. Here is the good one.

heddn’s picture

Status: Needs review » Needs work
  1. +++ b/core/modules/migrate/src/Plugin/migrate/process/FormatDate.php
    @@ -9,30 +9,20 @@
    + * Converts date/datetime format.
    

    there's probably space to say from what, to what in the 80 characters. I think more is better.

  2. +++ b/core/modules/migrate/src/Plugin/migrate/process/FormatDate.php
    @@ -9,30 +9,20 @@
    - * Example usage for date only fields (DATETIME_DATE_STORAGE_FORMAT):
    
    @@ -42,17 +32,9 @@
    - * Example usage for datetime fields (DATETIME_DATETIME_STORAGE_FORMAT):
    

    I intentionally mention these formats because when I was migrating into the destination, I found it very useful to know what the required destination format was in D8. Let's explicitly mention what the required destination formats are in the description if we remove the constants. I'm all for #90.5, but let's still slip in what the destination format should be.

    DATETIME_DATE_STORAGE_FORMAT
    DATETIME_DATETIME_STORAGE_FORMAT

The first is a small nit. The second feels more important to me.

shabana.navas’s picture

Status: Needs work » Needs review
FileSize
1.1 KB
8.11 KB

Added changes mentioned in #96 by Lucas. The summary now has just a little more detail and added a little bit of explanation for the examples.

heddn’s picture

Status: Needs review » Reviewed & tested by the community
Related issues: +#2865829: DateTimePlus test of validate_format = FALSE

Looking good to me now. Thanks for the interdiff on that last patch.

I discussed #91/#89 with @mpdonadio in IRC and we agreed to open a follow-up to handle the testing and changes for validate_format = FALSE.

I've opened #2865829: DateTimePlus test of validate_format = FALSE as a follow-up.

With that, I think this is good to go again.

alexpott’s picture

Version: 8.4.x-dev » 8.3.x-dev
Status: Reviewed & tested by the community » Patch (to be ported)

Updating issue credit. I'm crediting people whose feedback had material effect on the patch - I hope I have not missing anything. Thanks to everyone else who tried the patch out on their migrations.

alexpott’s picture

Committed 2c2a7a8 and pushed to 8.4.x. Thanks!

As this is an experimental module and an addition setting to patch to ported and will add to 8.3.x once 8.3.0 is released.

Fixed the following on commit.

  1. +++ b/core/modules/migrate/src/Plugin/migrate/process/FormatDate.php
    @@ -0,0 +1,113 @@
    + * @link http://php.net/manual/datetime.createfromformat.php
    + * \DateTime::createFromFormat. @endlink
    

    This should follow the indentation rules. Also @link can be longer than 80 chars.

  2. +++ b/core/modules/migrate/src/Plugin/migrate/process/FormatDate.php
    @@ -0,0 +1,113 @@
    + * - timezone: String identifying the required time zone.
    ...
    + * - settings: keyed array of settings.
    + * @see \Drupal\Component\Datetime\DateTimePlus::__construct()
    

    We should have one @see for this and all the @see's should be together and the end of the doc block. We should just have See DateTimePlus::__construct() in the text here.

Here's the diff:

diff --git a/core/modules/migrate/src/Plugin/migrate/process/FormatDate.php b/core/modules/migrate/src/Plugin/migrate/process/FormatDate.php
index b73ee3a..b197f17 100644
--- a/core/modules/migrate/src/Plugin/migrate/process/FormatDate.php
+++ b/core/modules/migrate/src/Plugin/migrate/process/FormatDate.php
@@ -13,13 +13,11 @@
  *
  * Available configuration keys
  * - from_format: The source format string as accepted by
- * @link http://php.net/manual/datetime.createfromformat.php
- * \DateTime::createFromFormat. @endlink
+ *   @link http://php.net/manual/datetime.createfromformat.php \DateTime::createFromFormat. @endlink
  * - to_format: The destination format.
- * - timezone: String identifying the required time zone.
- * @see \Drupal\Component\Datetime\DateTimePlus::__construct()
- * - settings: keyed array of settings.
- * @see \Drupal\Component\Datetime\DateTimePlus::__construct()
+ * - timezone: String identifying the required time zone, see
+ *   DateTimePlus::__construct().
+ * - settings: keyed array of settings, see DateTimePlus::__construct().
  *
  * Examples:
  *
@@ -65,6 +63,8 @@
  * If the source value was '2004-12-19T10:19:42-0600' the transformed value
  * would be 2004-12-19T10:19:42.
  *
+ * @see \DateTime::createFromFormat()
+ * @see \Drupal\Component\Datetime\DateTimePlus::__construct()
  * @see \Drupal\migrate\Plugin\MigrateProcessInterface
  *
  * @MigrateProcessPlugin(

  • alexpott committed 2c2a7a8 on 8.4.x
    Issue #2820490 by mpdonadio, heddn, Manuel Garcia, Jo Fitzgerald,...
heddn’s picture

Assigned: mpdonadio » Unassigned

To backport, take the patch that was committed in #101 and replace the exception catching logic to only look for Exception. The patch in #101 should apply against 8.3. But DateTimePlus switches its signature for throwing exceptions.

heddn’s picture

Issue tags: +Novice

Tagging novice for the backport. See #103 for the steps necessary.

shabana.navas’s picture

Status: Patch (to be ported) » Needs review
FileSize
7.91 KB

Backport patch for 8.3.x with change as mentioned in #102.

shabana.navas’s picture

FileSize
928 bytes

Forgot interdiff.

heddn’s picture

Status: Needs review » Reviewed & tested by the community
Issue tags: -Novice

And the backport is now RTBC. The exception is swapped out.

Manuel Garcia’s picture

Excellent, thanks @shabana.navas
+1 to the RTBC

sylus’s picture

FileSize
56.14 KB

This was also mentioned in comment #60 but this patch simply doesn't work with composer + cweagans/composer-patches. Has anyone else tried this? All of my other core patches apply perfectly and this one says it does to but ends up with a structure similar to below:

heddn’s picture

Gábor Hojtsy’s picture

Status: Reviewed & tested by the community » Needs work

#2830079: Change DateTimePlus to throw more specific exceptions was in 8.3.x already so why would we need to catch the more generic exception in 8.3.x?

heddn’s picture

Status: Needs work » Reviewed & tested by the community

If that's the case then I don't see why #101 cannot get directly committed to 8.3.x.

Looks like I misread

As this is an experimental module and an addition setting to patch to ported and will add to 8.3.x once 8.3.0 is released.

in #100. This can now be directly committed to 8.3 then.

Gábor Hojtsy’s picture

Status: Reviewed & tested by the community » Fixed

Verified once again that the right exception is thrown in 8.3.x. And indeed. Cherry-picked.

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.

ozin’s picture

In current implementation the source format and destination format use the same timezone which comes from the settings, but we could have case when we need different timezone for destination format, for instance we have date in America/Denver timezone but we need to convert it to UTC. I think make sense to add timezone setting for the destination date:

  field_end_time:
    plugin: format_date
    source: EXPDATE
    from_format: 'n/j/Y G:i:s'
    to_format: 'Y-m-d\TH:i:s'
    timezone: 'America/Denver'
    to_timezone: 'UTC'
    settings:
      validate_format: false
      $transformed = DateTimePlus::createFromFormat($fromFormat, $value, $timezone, $settings)->format($toFormat, ['timezone' => $to_timezone]);

Does it make sense, if so then I will provide patch?

Manuel Garcia’s picture

@ozin I could see that use case.

I say go for it - though perhaps create a follow up issue to this one, since technically it would be a feature request I believe.
Also, you'll have to provide test coverage for it on core/modules/migrate/tests/src/Unit/process/FormatDateTest.php.

josephdpurcell’s picture

Off hand, I don't think it is possible to use the "from_format" and "to_format" to specify the timezone conversion for you. As such, the proposal from #115 I think sounds great!

Two ideas on that:

1. Perhaps have the keys be "from_timezone" and "to_timezone" to be consistent.

2. Allow the string values for these keys to be anything that could pass into a DateTimeZone constructor http://php.net/manual/en/datetimezone.construct.php

mikeryan’s picture

The format_date process plugin was committed to core and this issue closed back in April. Any proposed extensions to it should be opened as a new "Feature request" issue.

ozin’s picture

I've creae new issue https://www.drupal.org/node/2883892. This weekend we have Drupal Camp in Kyiv(http://camp17.drupal.ua) I hope on codesprints we will implement this feature.

samirjusic’s picture

As a continuation of comment #86 and for anyone looking for a way to migrate date ranges from D7 to D8, the following snippet works on 8.3.4:

field_event_start_end_date/value: 
    -
     plugin: extract
     source: field_event_date
     index:
       - '0'
       - value
    -
     plugin: format_date
     from_format: 'Y-m-d H:i:s'
     to_format: 'Y-m-d\TH:i:s'
     timezone: 'GMT'
  field_event_start_end_date/end_value: 
    -
     plugin: extract
     source: field_event_date
     index:
       - '0'
       - value2
    -
     plugin: format_date
     from_format: 'Y-m-d H:i:s'
     to_format: 'Y-m-d\TH:i:s'
     timezone: 'GMT'

Some notes:

  • field_event_date is a D7 date field with start/end dates enabled on it
  • field_event_start_end_date is a D8 daterange field (available after enabling the Datetime Range module)

The issue I had was with the date format. It seems that D8 requires Y-m-d\TH:i:s format. D7 date field I had kept the date in a slightly different format (Y-m-d H:i:s). When transferred as-is to D8, the end result is that the D8 date field would never show in views or template files (since, I presume, it was wrongly formatted). The above snippet makes migrating D8 date ranges (or simple dates) from D7 to D8 work fine.

Isaiah Nixon’s picture

For anyone who is having trouble migrating D7 dates that don't use the date range ability, simply use the first half of samirjusic's code above. The extract function is still necessary. Also, even if the date field in D7 is displayed as mm/dd/yyyy you will still need to use the 'Y-m-d H:i:s' for the import because that's how D7 always stores it in the databases.

taggartj’s picture

Hello all I am creating a module https://www.drupal.org/project/migration_mapper
which when done will make it easy for devs to create custom migrations easily through a ui,
basicly you choose what entity type you want to migrate to upload a csv or paste raw json
and map your data to your entity then export a migration plus config to then run,
and am just now (at the time of this comment) trying to handle datetime fields
see http://cgit.drupalcode.org/migration_mapper/commit/?id=dfdda33
line "// THIS IS SCARRY !!! as every one has a different format."

if anyone can share some knowledge on the best way to handle that array aka:

$return = [
   'plugin' => 'format_date',
    'from_timezone' => 'TODO',
    'source' => $field_name, 
    'to_timezone' => 'UTC',
    ];
// need to add  a lot of date formatters here 

would it be fare to just put ?
from_format: 'WHATEVER YOUR PHP DATE FORMAT IS IN '
to_format: 'Y-m-d\TH:i:s'

ps Sorry to hijack this closed issue.

docans’s picture

I have tried all of the code above but i cant seem to be able to migrate my date field from drupal 7 to drupal 8. Any idea what i did wrong.

here is my code

langcode: en
status: true
dependencies: {  }
id: remark
class: Drupal\migrate\Plugin\Migration
field_plugin_method: null
cck_plugin_method: null
migration_tags: null
migration_group: null
label: 'Student Article'
source:
  plugin: d7_node
  node_type: student_article_page
process:
  type:
    plugin: default_value
    default_value: student_article
  nid: tnid
  vid: vid
  langcode:
    plugin: default_value
    source: language
    default_value: und
  title: title
  uid: node_uid
  status: status
  created: created
  changed: changed
  promote: promote
  sticky: sticky
  revision_uid: revision_uid
  revision_log: log
  revision_timestamp: timestamp
  comment_node_student_article_page/0/status: comment
  body:
    plugin: get
    source: body
  field_student_article_date/value:
    -
      plugin: extract
      source: field_article_date
      index:
        - '0'
        - value
    -
      plugin: format_date
      from_format: 'Y-m-d H:i:s'
      to_format: 'Y-m-d\TH:i:s'
      timezone: 'GMT'
destination:
  plugin: 'entity:node'
migration_dependencies:
  required:
    - upgrade_d7_user

Can someone please tell me why the body field is able to migrate but the "field_student_article_date" is not able to migrate

docans’s picture

Version: 8.3.x-dev » 8.4.2
heddn’s picture

Version: 8.4.2 » 8.3.x-dev

Please open a new support request. This issue is already closed for some time.

TMWagner’s picture

Seems it would be better to re-open this and keep the thread together. The issue was obviously never resolved.
It is still an issue with D8.6.2

Very generally, the repro for me was migrating from a Drupal 7 site to a brand new install of Drupal 7 (literally brand new install via Composer).

The "template" code generated by running "migrate-upgrade --configure-only" resulted in the following

field_news_date:
    plugin: sub_process
    source: field_news_date
    process:
      value:
        plugin: format_date
        from_format: 'Y-m-d H:i:s'
        to_format: 'Y-m-d\TH:i:s'
        source: value

That fails-

Replacing that with:
field_date: field_news_date

Fails -

The code that works is:

field_date:
    plugin: iterator
    source: field_news_date
    process:
      value:
        plugin: substr
        source: value
        start: 0
        length: 10
joshua.boltz’s picture

I've found the same results, where the format_date process plugin is not working.
When i try to use it like documented in past comments in this issue, i am presented with these warnings:

[warning] DateTime::createFromFormat() expects parameter 2 to be string, array given DateTimePlus.php:246

And the migration fails.

The code that works in what's described in #126

Wim Leers’s picture

FYI: For automatically migrating D7 date fields with the todate setting enabled: they result in data loss when using migrate_drupal today. #3095237: Migrate Drupal 7 date field "todate" value fixes that.

joey-santiago’s picture

I'm migrating a source like this

<field_release_date>2014-09-09 00:00:00</field_release_date>

with this:

field_release_date:
    -
      plugin: skip_on_empty
      method: process
      source: release_date
    -
      plugin: format_date
      from_format: 'Y-m-d H:i:s'
      to_format: 'Y-m-d'
      source: release_date

Given my settings on the d8 side for field_release_date don't use time for the date.

quietone’s picture

Publish change record