In Drupal 7, a date field has a granularity setting that the date attributes to collect: year, month, day, hour, minute, and/or second.

In Drupal 8 there is no longer the granularity setting, instead a datetime field has a datetime_type setting that can be "datetime" for date + time or "date" for just dates.

Expected behavior: When migrating from D7, it seems like date-only fields should have their datetime_type set to "date", and date+time fields should get "datetime".

Actual behavior: Both get datetime_type == "datetime".

This concept was raised elsewhere but hasn't been addressed for upgrades from Drupal 7.

CommentFileSizeAuthor
#44 2894068-44.patch17.14 KBalexpott
#44 42-44-interdiff.txt859 bytesalexpott
#42 2894068-42.patch16.86 KBheddn
#33 2894068-33.patch16.86 KBJo Fitzgerald
#31 2894068-31.patch0 bytesJo Fitzgerald
#31 interdiff-29-31.txt661 bytesJo Fitzgerald
#29 2894068-29.patch16.86 KBJo Fitzgerald
#29 interdiff-26-29.txt659 bytesJo Fitzgerald
#26 datetime_type_is_not-2894068-26.patch16.89 KBYogesh Pawar
#19 2894068-19.patch16.87 KBJo Fitzgerald
#17 2894068-17.patch16.86 KBJo Fitzgerald
#17 interdiff-10-17.txt1.55 KBJo Fitzgerald
#15 2894068-10-test-only.patch16.03 KBJo Fitzgerald
#11 datetime_type-2894068-10.patch16.85 KBdavidsickmiller
#9 datetime_type-2894068-9.patch16.83 KBdavidsickmiller
#4 datetime_type-2894068-4.patch10.42 KBdavidsickmiller
#3 datetime_type-2894068-3.patch9.78 KBdavidsickmiller
#2 datetime_type-2894068-2.patch9.7 KBdavidsickmiller
Members fund testing for the Drupal project. Drupal Association Learn more

Comments

davidsickmiller created an issue. See original summary.

davidsickmiller’s picture

The attached patch adds a test demonstrating the problem and a fix for the problem.

davidsickmiller’s picture

Re-rolled for latest 8.4.x...

davidsickmiller’s picture

Missed that MigrateUpgrade7Test needed changes too

davidsickmiller’s picture

Status: Active » Needs review
heddn’s picture

Assigned: Unassigned » quietone

Assigning to vicky to review this week.

quietone’s picture

Status: Needs review » Needs work

@davidsickmiller, thanks for patch!

Can we add the D7 field type 'Date (ISO format)' field and thus cover all the date fields?

After running a migration I found that for field_date_with_end_time only the start date is migrated, not the end date. Is this a case for the daterange type?

  1. +++ b/core/modules/field/tests/src/Kernel/Migrate/d7/MigrateFieldTest.php
    @@ -118,6 +119,12 @@ public function testFields() {
    +    $this->assertIdentical('datetime', $field->getSetting('datetime_type'));
    ...
    +    $this->assertIdentical('date', $field->getSetting('datetime_type'));
    

    assertIdentical is deprecated. Should now be assertSame

  2. +++ b/core/modules/field/tests/src/Kernel/Migrate/d7/MigrateFieldTest.php
    @@ -118,6 +119,12 @@ public function testFields() {
    +    $field = FieldStorageConfig::load('node.field_date');
    ...
    +    $field = FieldStorageConfig::load('node.field_date_without_time');
    

    Why is field_date_with_end_time not tested here

quietone’s picture

Assigned: quietone » Unassigned
davidsickmiller’s picture

Status: Needs work » Needs review
FileSize
16.83 KB

Can we add the D7 field type 'Date (ISO format)' field and thus cover all the date fields?

OK, I've added separate field_date_without_time and field_datetime_without_time fields.

After running a migration I found that for field_date_with_end_time only the start date is migrated, not the end date. Is this a case for the daterange type?

Maybe! Do you think this is a regression caused by the code I added to FieldSettings.php? If not, could we handle it in a separate issue?

assertIdentical is deprecated. Should now be assertSame

Fixed!

Why is field_date_with_end_time not tested here

Fixed!

Status: Needs review » Needs work

The last submitted patch, 9: datetime_type-2894068-9.patch, failed testing. View results

davidsickmiller’s picture

Status: Needs review » Needs work

The last submitted patch, 11: datetime_type-2894068-10.patch, failed testing. View results

heddn’s picture

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

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

Jo Fitzgerald’s picture

All looks good to me (ignoring the testbot failure) and ready for RTBC. Have run re-tests for 8.4.x and a test against 8.5.x, but not expecting any surprises in the results.

Uploading a test-only version of the patch in #11 (for completeness as much as anything).

If all the tests come back as expected then this can be set to RTBC.

Status: Needs review » Needs work

The last submitted patch, 15: 2894068-10-test-only.patch, failed testing. View results

Jo Fitzgerald’s picture

Status: Needs work » Needs review
FileSize
1.55 KB
16.86 KB

The d7_field_settings plugin was not catching fields of type "datestamp" (e.g. field_date_with_end_time) and updating the datetime_type setting when appropriate so I have corrected that.

Even so, the datetime_type setting for field_date_with_end_time should not be set because the hour/minute granularities are not 0 so I have also updated the expected to Null in the test.

So we're not quite ready for RTBC, but I think this should take us a step closer.

heddn’s picture

Status: Needs review » Needs work

Back to NW for the remaining tasks. What are those?

Jo Fitzgerald’s picture

Status: Needs work » Needs review
FileSize
16.87 KB

The only remaining task was to understand why field_date_with_end_time did not have a datetime_type setting. This is because it is a datestamp field which becomes a timestamp field (which does not have a datetime_type setting).

Re-rolled the patch (because the patch in #17 no longer applies) and added a comment to explain why we are asserting that the datetime_type setting for field_date_with_end_time is null (I thought this was better than simply ignoring it).

heddn’s picture

Status: Needs review » Needs work

Great test coverage, comments in #19 makes sense. Reviewed the IS again, do we actually need to collect any settings about 'year, month, day, hour, minute, and/or second' or just collect the type? Back to NW for that, either update the IS to reflect what is going on in this issue or update the patch. And if we do need to collect the settings, should that move to a follow-up? Or take place here? If it is simple, let's do it here. If it isn't, punt to a follow-up.

Jo Fitzgerald’s picture

Issue summary: View changes
Status: Needs work » Needs review

The only datetime granularity options in D8 are datetime_type of "datetime" (fields including date & time) and "date" (fields including only date & no time) - there is not the same granularity as D7. I have edited to IS to reflect this.

heddn’s picture

Status: Needs review » Reviewed & tested by the community

Ah yes, all that is stored on the field formatter now and is in contrib as core doesn't support more fine grain settings. I should have remembered. Great. This is good to go then.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 19: 2894068-19.patch, failed testing. View results

Jo Fitzgerald’s picture

Issue tags: +Needs reroll
Yogesh Pawar’s picture

Assigned: Unassigned » Yogesh Pawar
Yogesh Pawar’s picture

Assigned: Yogesh Pawar » Unassigned
Status: Needs work » Needs review
Issue tags: -Needs reroll
FileSize
16.89 KB

Rerolled the patch #19 & because it's failed to apply on 8.5.x branch.

heddn’s picture

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

This is a bug, so moving it back to 8.4. And added a test run as well for good measure.

Status: Needs review » Needs work

The last submitted patch, 26: datetime_type_is_not-2894068-26.patch, failed testing. View results

Jo Fitzgerald’s picture

Status: Needs work » Needs review
FileSize
659 bytes
16.86 KB

This patch only increases the number of field_storage_config by 2 so correcting that.

Status: Needs review » Needs work

The last submitted patch, 29: 2894068-29.patch, failed testing. View results

Jo Fitzgerald’s picture

Status: Needs work » Needs review
FileSize
661 bytes
0 bytes

This patch only increases the number of field_config by 2 so correcting that. We'll get there in the end...

Status: Needs review » Needs work

The last submitted patch, 31: 2894068-31.patch, failed testing. View results

Jo Fitzgerald’s picture

Status: Needs work » Needs review
FileSize
16.86 KB

Ooops, empty patch file! This is what I meant to upload.

heddn’s picture

Status: Needs review » Reviewed & tested by the community

Straight re-roll here, so back to RTBC.

alexpott’s picture

Crediting @heddn and @quietone as reviewers.

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Committed and pushed a151cef151 to 8.5.x and d975b8df0f to 8.4.x. Thanks!

  • alexpott committed a151cef on 8.5.x
    Issue #2894068 by Jo Fitzgerald, davidsickmiller, Yogesh Pawar, heddn,...

  • alexpott committed d975b8d on 8.4.x
    Issue #2894068 by Jo Fitzgerald, davidsickmiller, Yogesh Pawar, heddn,...

  • xjm committed 5c9932a on 8.4.x
    Revert "Issue #2894068 by Jo Fitzgerald, davidsickmiller, Yogesh Pawar,...

  • xjm committed a22c5d3 on 8.5.x
    Revert "Issue #2894068 by Jo Fitzgerald, davidsickmiller, Yogesh Pawar,...
xjm’s picture

Status: Fixed » Needs work
heddn’s picture

alexpott’s picture

Status: Needs review » Needs work

This is a db-other-mysql issue. If I run Drupal\Tests\block\Kernel\Migrate\d7\MigrateBlockTest locally using postgres the test fails...

alexpott’s picture

So dates in D7 had some fun stuff for different DBs :) See date.install in D7 - http://cgit.drupalcode.org/date/tree/date.install

Yaazkal’s picture

Hi, I'm not sure if there is a bug on this or if it is a migrate_plus module issue.

Trying to migrate from Drupal 7.56 to Drupal 8.4.0

On the Drupal 8 site I have installed: migrate, migrate_plus, migrate_tools, migrate_upgrade.

After applying #44 patch, I ran:

drush migrate-upgrade --legacy-db-url=mysql://user:password@server/db --legacy-root=http://example.com --configure-only

drush mi --all

Content was migrated and date field is migrated as only date (the way I want it), but I can't see any date when editing a migrated node. I mean, the field is there but seems to be empty.

So I rollback the migration and ran drush cex to check the migration configuration. I have a content type "Norma" (Norma translates as "norm", for law content creation) and this content type has a date field "fecha" which is the spanish word for date.

So, in migrate_plus.migration.upgrade_d7_node_norma.yml file I can see:

field_fecha:
    plugin: iterator
    source: field_fecha
    process:
      value:
        plugin: format_date
        from_format: 'Y-m-d\TH:i:s'
        to_format: 'Y-m-d\TH:i:s'
        source: value

So I changed to_format: 'Y-m-d\TH:i:s' to be like to_format: 'Y-m-d' and then drush cim and ran again the upgrade, now the dates are correctly shown when editing a migrated node but (and this is the weird part) I get this error (when editing or creating a node of type "norma"): Notice: Undefined index: main-menu in menu_ui_form_node_form_alter() (line 267 of core/modules/menu_ui/menu_ui.module).

So, I went and edit the content type "norma" to asign a menu to see if it fixes and it does... except that the date field "fecha" has now disapeared from the content type, I don't see it listed even in /admin/reports/fields

Not sure if I'm doing it wrong or where to look at.

Regards.

Yaazkal’s picture

Hi, I can confirm that Notice: Undefined index: main-menu in menu_ui_form_node_form_alter() (line 267 of core/modules/menu_ui/menu_ui.module) is not related to this issue (sorry for the noise) but is related to https://www.drupal.org/node/2592829 because even if I don't apply #44 I get the error. This happens because I have selected main-menu as an available menu for the content type that I'm migrating (on D7) and in D8 that menu is now 'main', so 'main-menu' not exists from the D8 perspective. Updating the other issue.

Yaazkal’s picture

Anyway, It's still the need to change the to_format: 'Y-m-d\TH:i:s' to to_format: 'Y-m-d' in the configuration. I made it as I described before: exporting the configuration drush cex, changing the yaml files and then importing the configuration changes drush cim but people can do that on a custom module too. Will be great if the to_format is generated correctly from the beginning.

Regards!

heddn’s picture

Assigned: Unassigned » heddn

Assigning to myself to review in the next week.

heddn’s picture

Assigned: heddn » Unassigned
Status: Needs review » Reviewed & tested by the community

This looks good now. It was already RTBC, just failing on postgress. That is now accommodated in the re-rolled patch. It isn't clear if #45 was with or without this patch. But I can only imagine it was without. Because if it were with the patch, then it would correctly map the fields.

catch’s picture

Status: Reviewed & tested by the community » Fixed

Committed/pushed to 8.5.x and cherry-picked to 8.4.x. Thanks!

  • catch committed c5fba61 on 8.5.x
    Issue #2894068 by Jo Fitzgerald, davidsickmiller, alexpott, heddn,...

  • catch committed 1ade761 on 8.4.x
    Issue #2894068 by Jo Fitzgerald, davidsickmiller, alexpott, heddn,...

Status: Fixed » Closed (fixed)

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