Problem/Motivation
The Drupal 7 date module allowed the site builder to configure whether to collect an end date or not:
(This is called 'todate'
in the code.)
For example, on my personal site, I have a project
NodeType
which has a "time range" date
. Example: https://wimleers.com/work/project/ledgrid.
Upon migration, the start value (value
) is migrated, but the end value (value2
) is dropped
- D7
-
- D8 HEAD
That's because this is being migrated to a @FieldType=datetime
field in D8 instead of a @FieldType=daterange
field in D8!
Proposed resolution
Make \Drupal\datetime\Plugin\migrate\field\DateField
detect the todate
setting and:
- expand the
process
pipeline to also transform data for@FieldType=daterange
end_value
property, instead of only thevalue
property. See DateField::defineValueProcessPipeline - dynamically override the statically computed destination field type: return
daterange
instead ofdatetime
when appropriate. See DateField::getFieldType - Throw a MigrateException when a 'todate' value exists and the Datetime Range module is not installed. See DateField::getFieldType.
Remaining tasks
- Make a fail patch.
- Review
- Commit
User interface changes
N/A
API changes
N/A
Data model changes
N/A
Release notes snippet
TBD
Comment | File | Size | Author |
---|---|---|---|
#79 | diff-70-76.txt | 2.17 KB | quietone |
#78 | interdiff-3095237-76-78.txt | 1.04 KB | yogeshmpawar |
#78 | 3095237-78.patch | 33.69 KB | yogeshmpawar |
#76 | 3095237-76.patch | 33.68 KB | yogeshmpawar |
#70 | 3095237-70.patch | 33.67 KB | quietone |
Issue fork drupal-3095237
Show commands
Start within a Git clone of the project using the version control instructions.
Or, if you do not have SSH keys set up on git.drupalcode.org:
Comments
Comment #2
Wim LeersAnd patch! This makes the D7 → D8 migration for all date/time-related fields on
wimleers.com
work perfectly 🥳Comment #3
Wim LeersComment #4
dwwGreat 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
Comment #6
mikelutzGood 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.
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.
Comment #7
Wim LeersExactly :)
That's why I marked this
, not .If the migration loudly complained and warned that an important subset of data did in fact not get migrated, I would agree. Right now it silently omitting data is simply dangerous. It's that danger that prompted me to mark it
.Uh oh 🙈
Comment #8
Wim LeersBeen told that I should put this in
migration system
:)Comment #9
dwwYeah, nobody here but us chickens in the datetime.module. ;)
Comment #10
benjifisherI am updating the "Remaining tasks" in the issue summary.
Comment #12
benjifisherI closed #3017761: Drupal 7 Date field with end date, to Drupal 8 Date Range field as a duplicate of this issue.
Comment #13
quietone CreditAttribution: quietone as a volunteer commentedFollowing 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.Comment #14
quietone CreditAttribution: quietone as a volunteer commentedForgot to make an interdiff. Here it is.
Comment #16
quietone CreditAttribution: quietone as a volunteer commentedEnable datetime_range in the failing tests.
Comment #18
quietone CreditAttribution: quietone as a volunteer commentedEntity count fun again and add a sort of arrays before asserting.
Comment #19
quietone CreditAttribution: quietone as a volunteer commentedOk, sorting isn't straightforward instead change the expected arrays.
Comment #21
benjifisherIt 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.
Comment #22
quietone CreditAttribution: quietone as a volunteer commented@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().
Comment #23
benjifisherI 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.
Comment #24
benjifisherSo far, testing with Drupal 8.9.1 and the patch in #23 looks good.
Comment #25
quietone CreditAttribution: quietone as a volunteer commentedChange title because I kept reading it twice.
Comment #26
quietone CreditAttribution: quietone as a volunteer commentedRerolled the patch from #22
Comment #28
quietone CreditAttribution: quietone as a volunteer commentedFix 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.
Comment #30
heddnNo real comments of substance on the code. Its solid. My lone concern is with the switch from datetime to 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?
This goes with the previous change. We need to handle datetime and range, no?
Comment #31
quietone CreditAttribution: quietone as a volunteer commentedI 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?
Comment #32
gbirch CreditAttribution: gbirch commented#23 continues to work well in 8.9.3. Thanks @benjifisher!
Comment #33
mikelutzI think the test failure in #28 is real, nw for that.
Comment #34
quietone CreditAttribution: quietone as a volunteer commentedNeeds a reroll.
Comment #35
heddnIf 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.
Comment #36
heddnThe 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.
Comment #37
benjifisherI 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 notdatetime_range
. Could we be setting fields to use a type from a module that is not enabled? That seems serious.Comment #39
quietone CreditAttribution: quietone as a volunteer commentedHow about throwing an error in DateField::getFieldType? Tried that in this patch. I haven't looked into changing the destination_module.
Comment #41
quietone CreditAttribution: quietone as a volunteer commentedNow see what happens when datetime is restored in MigrationProvidersExistTest.php and datetime/migrations/state/migrate_drupal.yml.
Comment #42
heddnNit: 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.
Comment #43
ravi.shankar CreditAttribution: ravi.shankar at OpenSense Labs commentedMade changes as per comment #42.
Comment #44
heddnNw for some cspell fixes
Comment #45
ravi.shankar CreditAttribution: ravi.shankar at OpenSense Labs commentedHere I have tried to fix cSpell issue, please review.
Comment #46
heddnI 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?
Comment #47
benjifisherIf 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.
Comment #49
Vishalghyv CreditAttribution: Vishalghyv at Google Summer of Code commentedMade changes as per comment #46
Changed todate - > to date
Comment #50
Vishalghyv CreditAttribution: Vishalghyv at Google Summer of Code commentedThis is weird cspell is flagging todate in some cases and not in some,
Here is a changing of todate to date in all comments
Comment #51
Vishalghyv CreditAttribution: Vishalghyv at Google Summer of Code commentedWhat it is mentioning is in code, Which wasn't flagged in the earlier patches
Comment #52
quietone CreditAttribution: quietone as a volunteer commentedFirst, this needs a reroll. I choose to start again from #45. So that means #46 needs to be done.
Comment #53
quietone CreditAttribution: quietone as a volunteer commentedAnd 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.
Comment #54
quietone CreditAttribution: quietone as a volunteer commentedComment #55
ravi.shankar CreditAttribution: ravi.shankar at OpenSense Labs commentedAdded reroll of patch #53.
Comment #57
quietone CreditAttribution: quietone as a volunteer commented@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.
Comment #59
quietone CreditAttribution: quietone as a volunteer commentedJust a reroll from #53.
Comment #61
quietone CreditAttribution: quietone as a volunteer commentedRearranging the expected values for the failing tests. Need to improve the test to sort the arrays.
Comment #62
quietone CreditAttribution: quietone as a volunteer commentedI'll make a fail patch soon
Comment #63
quietone CreditAttribution: quietone as a volunteer commentedHere 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.
Comment #65
MatroskeenI 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 😇
Comment #66
heddnthis 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.
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.
Comment #67
quietone CreditAttribution: quietone as a volunteer commented@heddn, thanks for the review.
Doing a reroll first
Comment #69
quietone CreditAttribution: quietone as a volunteer commentedAh, need to add a fix for the new test MigrateFieldInstanceOptionTranslationTest.
Comment #70
quietone CreditAttribution: quietone as a volunteer commentedNow the fixes for #66.
1. Reverted. Not even sure why I did that.
2. Comment added.
Comment #71
heddnLGTM.
Comment #72
Volker23 CreditAttribution: Volker23 commentedI 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!
Comment #73
quietone CreditAttribution: quietone as a volunteer commented@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.
Comment #74
alexpottWe need a reroll here.
Comment #75
yogeshmpawarWorking on re-roll.
Comment #76
yogeshmpawarComment #78
yogeshmpawarHope this updated patch will resolve test failures.
Comment #79
quietone CreditAttribution: quietone as a volunteer commentedAdding 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.
Comment #80
quietone CreditAttribution: quietone as a volunteer commentedIn 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.
Comment #81
danflanagan8Checked 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.
Comment #82
alexpottCommitted 4e87f84 and pushed to 9.3.x. Thanks!
Comment #85
sjhuskey CreditAttribution: sjhuskey as a volunteer commentedIt 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?
Comment #86
danflanagan8> 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.
Comment #87
sjhuskey CreditAttribution: sjhuskey as a volunteer commented@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.