Migrate Drupal 7 date field "todate" value

Great find, @Wim Leers!

Fix looks good on a quick skim. A few nits or things I'm not thrilled about, but they're not the fault of your patch. ;)

We probably want a test that shows the data loss, and to see it work with the fix applied.

Don't have time for a more thorough review right now, but I'll try to keep an eye on this.

Thanks!
-Derek

Good find. This needs a test, which you would think would be easy, since we have a field_date_with_end_time in the d7 text fixture, whose end date isn't actually verified in tests.

Unfortunately, you have opened up a bit of a can of worms, as that field is a timestamp with an end time, which is currently being migrated into a timestamp field in Drupal 8. D8 timestamp fields don't support an end time (they are primitive field types in lib/Core, not the datetime module).
Near as I can tell, nearly all the testing around that field is with respect to making sure it's migrated as a timestamp, not to actually check to see if the end date is migrated.

So, your patch doesn't migrate the one end date that we have in the fixture, and that one field with an end date doesn't appear to have a comparable field in D8 to migrate to correctly anyway. (whew)

So, we need to decide what to do with timestamps with end dates, fix that, then probably add fields to the fixture for timestamp without end date, and datetime with enddate, and then test all of them properly.

Some of which can be relegated to follow-ups. I feel like we can fix datetimes with end date and kick the decision on what to do about timestamps with end dates into a follow up. We will need a new datetime field with end date in the d7 fixture to test migration of. We should also at least take a glance and see if there is a comparable issue with D6 that we can fix easily. D6 is lower priority but if we can kill two birds with one stone we should, and if we identify an issue with d6 that we can't easily fix, we should at least document it.

+++ b/core/modules/datetime/src/Plugin/migrate/field/DateField.php
@@ -19,7 +20,7 @@
- *   destination_module = "datetime"
+ *   destination_module = "datetime_range"

So, we can only specify one, and we can't know which until we get to inspect the field settings. It's unfortunate that the two choices are in separate modules in this case. In the other case where we split field types (textareas to plain text and filtered text) at least plain text was in core and didn't require a module.

At least by requiring datetime_range here, we will implicitly require datetime as well, so we are covered either way.

By the way, while this is an important bug to fix, we don't generally consider the fact that a piece of data isn't migrated to be 'data loss' in the issue escalation sense. The data isn't lost, it's still sitting on your D7 site right where it was. It simply wasn't migrated. There are many bits of data that may not be migrated for one reason or another, including views, text fields with some instances configured as plain text, and some as filtered, translated node revisions, contrib fields like paragraphs, and anything else that doesn't have a provided migration path. Unless we are somehow deleting the original data from the D7 database, it would not be considered data loss.

Yeah, nobody here but us chickens in the datetime.module. ;)

I am updating the "Remaining tasks" in the issue summary.

I closed #3017761: Drupal 7 Date field with end date, to Drupal 8 Date Range field as a duplicate of this issue.

Following on from #6, this add a new field to the drupal7 test fixture, a datetime field with an end date, and tests in the MigrateField and MigrateFieldInstance tests. And updated the field plugin test.

Also removed the warning of data loss from the IS in accord with the use of data loss as explained by mikelutz in #6.

I think the todos here are:
Decide what to do with timestamps with end dates, but do this in a followup.
* destination_module = "datetime_range" This needs some documentation to explain that datetime_range needs to be enabled.

Forgot to make an interdiff. Here it is.

Enable datetime_range in the failing tests.

Entity count fun again and add a sort of arrays before asserting.

Ok, sorting isn't straightforward instead change the expected arrays.

It looks as though there was a random test failure and you queued a new test. Since the test passed, I am setting the status back to NR.

@benjifisher, Yes, I did. Thanks for updating the status.

Did a brief self review which led to updated comments and a easier to read testDefineValueProcessPipeline().

I am working on a migration to Drupal 8.9 and want to test/use this fix. The patch in #22 has conflicts in many of the tests (maybe all). So here is a copy of the patch without any changes to the tests.

So far, testing with Drupal 8.9.1 and the patch in #23 looks good.

Change title because I kept reading it twice.

Rerolled the patch from #22

Fix incorrect field_id number in field_config_instance table that was introduced to the d7 fixture in #3153791: Add comment field for 'et' content type to d7 fixture.

No real comments of substance on the code. Its solid. My lone concern is with the switch from datetime to datetime_range.

1. +++ b/core/modules/datetime/src/Plugin/migrate/field/DateField.php
   @@ -19,7 +20,7 @@
   - *   destination_module = "datetime"
   + *   destination_module = "datetime_range"
   

   We can't really have 2 destination modules, but in reality, that's what we want. Do we need to fork the field migration off so we have one for date and one for range?

2. +++ b/core/modules/migrate/tests/src/Kernel/Plugin/MigrationProvidersExistTest.php
   @@ -126,7 +126,7 @@ public function testFieldProvidersExist() {
   -        'destination_module' => 'datetime',
   +        'destination_module' => 'datetime_range',
   

   This goes with the previous change. We need to handle datetime and range, no?

I don't see how to fork that when they both use the same field_types. Change FieldDiscovery to be able to select \Drupal\datetime\Plugin\migrate\field\DateField or a new field plugin in datetime_range?

#23 continues to work well in 8.9.3. Thanks @benjifisher!

I think the test failure in #28 is real, nw for that.

Needs a reroll.

+++ b/core/modules/datetime/src/Plugin/migrate/field/DateField.php
@@ -19,7 +20,7 @@
+ *   destination_module = "datetime_range"

If we say the destination is datetime_range, but leave the plugin in datetime, that is going to confuse things. But we can't move it either, because of sites that didn't know they need datetime_range and don't have it installed. Not sure what to suggest/do here.

The destination module is used on field plugins to display the nice review form in migrate drupal ui. Its what lets you know if all your data will migrate. There is no good way to know until we start migrating entities that use the field if we have range data. Because it isn't flagged on the source.

However, we could have datime_range alter the field plugin to set the destination to datetime_range (if it is enabled). And if while running a migration we run into range data, complain loudly with exceptions.

And if while running a migration we run into range data, complain loudly with exceptions.

I think you meant to add "and datetime_range is not enabled".

I am setting the status back to NW based on #35 and #36.

We discussed this issue at #3175894: [meeting] Migrate Meeting 2020-10-15. I think #35 is copied from there. Here is some of what I said:

It seems to me that there are bigger problems if datetime is enabled but not datetime_range. Could we be setting fields to use a type from a module that is not enabled? That seems serious.

How about throwing an error in DateField::getFieldType? Tried that in this patch. I haven't looked into changing the destination_module.

Now see what happens when datetime is restored in MigrationProvidersExistTest.php and datetime/migrations/state/migrate_drupal.yml.

+++ b/core/modules/datetime/src/Plugin/migrate/field/DateField.php
@@ -96,4 +105,24 @@ public function defineValueProcessPipeline(MigrationInterface $migration, $field
+        throw new MigrateException(sprintf("Can't migrate field '%s' with 'todate' settings. Enable to datetime_range module. See https://www.drupal.org/docs/8/upgrade/known-issues-when-upgrading-from-drupal-6-or-7-to-drupal-8#datetime", $row->get('field_name')));

Nit: I think we want to say, "Enable the datetime_range module." Also needs tests fixes if we do adjust.

Otherwise, I think this is looking really good. Tagging novice as this seems relatively easy fix.

Made changes as per comment #42.

Nw for some cspell fixes

Here I have tried to fix cSpell issue, please review.

+++ b/core/modules/datetime/src/Plugin/migrate/field/DateField.php
@@ -89,6 +89,7 @@ public function defineValueProcessPipeline(MigrationInterface $migration, $field
+    // cSpell:enableCompoundWords

+++ b/core/modules/datetime/tests/src/Unit/Plugin/migrate/field/DateFieldTest.php
@@ -37,6 +37,7 @@ public function testDefineValueProcessPipeline($data, $from_format, $to_format)
+    // cSpell:enableCompoundWords

I don't see where we use this anywhere else in core. Can we not use the machine names todate, etc. And instead use the words "to date", etc in the code comments? I think it will convey the same info and not require us to introduce this feature?

If I understand correctly, #46 is asking that we change the comments instead of telling the spell checker to ignore them That is what the patch in #45 did.

Made changes as per comment #46
Changed todate - > to date

This is weird cspell is flagging todate in some cases and not in some,
Here is a changing of todate to date in all comments

What it is mentioning is in code, Which wasn't flagged in the earlier patches

First, this needs a reroll. I choose to start again from #45. So that means #46 needs to be done.

And now, address #46.

I actually disagree with heddn because the setting really is called 'todate' not 'to date' and I think changing it is just confusing. Additionally, 'todate' is used in an exception message where, again, using the correct setting name will be helpful to the developer. For those reasons I have added a 'cspell ignore' at the top of the DateField.php and DateFieldtest.php

@Vishalghyv, I haven't looked at the errors from the two patches you submitted. But the simplest thing to do is to run the same checks (spelling, coding standards) that drupalci does locally before you submit a patch. It is very useful and I recommend it. See Running core development checks (>= 9.2.x). Cheers.

Added reroll of patch #53.

@ravi.shankar, rerolling the migration issues that change either of the test fixtures, drupal6.php or drupal7.php, can be tricky. Looking at the test results, the tests are failing because the source database, drupal7,php, could not be imported. This is something that can, and should, be checked locally before uploading the patch.

There are two ways that can be done, a) use the script db-tools.php or b) run a migration test that loads the database. For the first, have a look at Generating database fixtures for migration tests. And for the second run, say d6/MigrateSystemConfigurationTest.php, with phpunit.

Just a reroll from #53.

Rearranging the expected values for the failing tests. Need to improve the test to sort the arrays.

I'll make a fail patch soon

Here is the fail patch and a diff with the success patch. The success patch is the same as #61 so there is no interdiff needed.

I have Drupal 9.2.2 installed and none of the previous patches applied. I did a re-roll because I thought I need this patch for my custom migration. Turns out, it wasn't necessary but I'd like to publish it here to save someone's time.

If the test is failing, it should be safe to apply anyway because the merge conflict was only in the text fixture.
And thanks for the work done here 😇

1. +++ b/core/modules/datetime/migrations/state/datetime.migrate_drupal.yml
   @@ -2,4 +2,4 @@ finished:
   -    date: datetime
   +    date: datetime_range
   

   this modifying to the migration state seems wrong. date should normally get migrated into datetime. it is only in certain conditions that it goes into datetime_range.

2. +++ b/core/modules/datetime/src/Plugin/migrate/field/DateField.php
   @@ -92,6 +95,14 @@ public function defineValueProcessPipeline(MigrationInterface $migration, $field
   +    // so set the end value. If the datetime_range module is not enabled on the
   +    // destination then end_value is ignored.
   

   Do we want to trigger any warning/error on this condition?

   Ah, I see we do trigger an exception later. Maybe we should clarify this comment.

@heddn, thanks for the review.

Doing a reroll first

Ah, need to add a fix for the new test MigrateFieldInstanceOptionTranslationTest.

Now the fixes for #66.

1. Reverted. Not even sure why I did that.
2. Comment added.

LGTM.

I have tried the patch to update an existing migration for the core datetime field. Is it correct, that the patch only supports initial migrations? After adding another patch (https://www.drupal.org/project/drupal/issues/3118262) that supports updating migrations using EntityConfig destination, i got this error:

Cannot change the field type for an existing field storage. The field storage node.field_date has the type datetime. (/docroot/core/modules/field/src/Entity/FieldStorageConfig.php:375)

Any hint how i could solve this is highly appreciated (in case im reporting in the right location).

Thanks!

@Volker23, I am not fully following you but I will try to answer. This patch will work for the initial migration as well as updates/incrementals. If it is not working for you on incremental or upgrade then provide the steps to reproduce the problem and the results. For the second issue you mention, it is best to report your test results in that issue. Thx.

We need a reroll here.

Working on re-roll.

Hope this updated patch will resolve test failures.

Adding diff for the reroll in #76.
The change in #78 was needed because #2976098: MigrateExecutable should add details for the migration & destination property to exceptions that cause a row failure was comitted.

In general, the migration of 'things' from earlier versions was not considered bugs, they were tasks. And the lack of the source data does not affect the functionality of the destination site. Changing to task.

Checked the re-roll and it looks good. The only changes we in the expected messages in a couple tests, both related to the issue that @quietone linked to above. Setting back to RTBC.

Committed 4e87f84 and pushed to 9.3.x. Thanks!

It sounds like this patch would resolve my problem, which is the same as the original poster's issue.

However, when I try to apply this patch to my site (version 9.4.1), I receive the message "Could not apply patch! Skipping. The error was: Cannot apply patch." When I do composer update -vvv, I get "patch: unrecognized option `--no-backup-if-mismatch'".

What can be done to make this patch available to users of 9.4.1?

> What can be done to make this patch available to users of 9.4.1?

This fix was committed in 9.3.x, so it should already be fixed in 9.4.1, no patch required.

@danflanagan8: Thanks for that information. It's what I had hoped, but I'm still having the problem that the original poster had, and I'm baffled about resolving it.