Early Bird Registration for DrupalCon Portland 2024 is open! Register by 23:59 PST on 31 March 2024, to get $100 off your ticket.
Problem/Motivation
Managed Drupal 7 files need to be migrated to Drupal 8 file entities.
Proposed Resolution
D7's unified file management API (and the fact that files are already entities!) makes this a lot easier than migrating files from Drupal 6 :) We need a simple migration (d7_file) that simply transfers all file_managed rows to Drupal 8 file entities, and copies the actual files to the corresponding Drupal 8 path.
Remaining Tasks
- Write the migration
- Write tests
- Review
- Commit
Comment | File | Size | Author |
---|---|---|---|
#48 | interdiff-2503861-45-48.txt | 3.25 KB | phenaproxima |
#48 | 2503861-48.patch | 10.83 KB | phenaproxima |
#45 | interdiff-2503861-42-45.txt | 1.89 KB | phenaproxima |
#45 | 2503861-45.patch | 10.51 KB | phenaproxima |
#42 | interdiff-2503861-41-42.txt | 0 bytes | phenaproxima |
Comments
Comment #1
phenaproximaInitial patch.
Comment #2
mikeryanProbably some D6 holdover from the user picture handling - better would be to say something like "Every migration that references files by fid must have...".
No biggie, but I'd just as soon leave the straight mapping (thinking ahead to contrib extensions, people wondering why that one field was left unmapped).
D8 has replaced 'timestamp' with 'created and 'changed' (presumably the D6 version needs to also be fixed here).
So, we're putting a test file into the D8 files directory, where the migration is supposed to copy it from a D7 site?
Comment #3
phenaproxima#1) Fixed.
#2) I left it in there, with the comment explaining why it's not migrated, so that people can find out if they're curious.
#3) I didn't fix, because I thought that fields() was supposed to be the list of properties returned by the source. changed and created are mapped in by the process pipeline, but I don't see why the source should need to know about them.
#4) Good point. I have rejiggered the d7_file source plugin to provide a computed property called filepath, which is the absolute path represented by the original URI. It's not thoroughly tested (ideally I'd also test for temporary and private URIs) but it's a start.
Comment #4
mikeryanComment #5
phenaproximaChanging status for testing.
Comment #7
phenaproximaTrying again, basing the test's file operations in its public files directory rather than /tmp.
Comment #8
benjy CreditAttribution: benjy at CodeDrop commentedO, D7 didn't keep track of updated/created.
If we don't need the context of the row then we should do this in initializeIterator(), that way it only runs once per migration.
I know you have plans for this but we should remove these until we have a use for them.
Should this comment be a few lines higher?
This is already set in the migration itself?
I'm not sure about this, surely you can set individual config keys?
As per above.
Missing function comment.
Is this used?
Comment #9
phenaproximaMostly fixed. Didn't bother with #5 through #8, as per discussion on IRC. Regarding #9 -- I don't know if it's used, but I'm following the same pattern as every D6 unit test, which has the exact same structure. When KTBNG lands, we should be able to convert these to kernel tests (which they properly are) and get rid of these stub plugins.
Comment #10
phenaproximaStatus for testbot.
Comment #11
benjy CreditAttribution: benjy at CodeDrop commentedRegarding #9, if it's not used we should just remove it. I don't know what you mean by "exact same structure". It's a dummy class used to make testing easier, we either need it or we don't.
Comment #12
phenaproxima@benjy -- We do need it, because it's the class that actually gets tested. All the other unit tests follow the same pattern, but truth be told I'm not sure we need to do that anymore, now that the tests use SQLite as a backend. I should open a follow-up issue to remove all these dummy classes, if possible; they're tedious to write, and they're all freaking identical anyway.
Comment #13
benjy CreditAttribution: benjy at CodeDrop commentedI was struggling to see how that was the case but I looked closer and found the madness :)
$plugin_class = preg_replace('/^Drupal\\\\(\w+)\\\\Plugin\\\\migrate(\\\\source(\\\\.+)?\\\\)([^\\\\]+)$/', 'Drupal\\Tests\\\$1\\Unit$2Test$4', static::PLUGIN_CLASS);
The attached patch keeps backwards compatibility for the [Class]Test approach but it handles module handler and database so we can start cleaning those classes up!
Comment #14
phenaproximaBrilliant!! If we can do that reflection trick for this one class, we might as well go all the way and remove all of these pointless Test* classes. But let's do that in a follow-up.
Comment #15
benjy CreditAttribution: benjy at CodeDrop commentedOK, the rest looks good. Lets RTBC #9 and then handle the test class clean-up here #2505521: Clean-up un-need test classes in migrate_drupal
Comment #17
phenaproximaBack to RTBC.
It's #9 we want to commit, and it still passes.Comment #18
benjy CreditAttribution: benjy at CodeDrop commented#2505521: Clean-up un-need test classes in migrate_drupal landed so we can remove that class now.
Comment #23
phenaproximaPostponing on #2510072: UTF-8 support in MySQL driver breaks migrate dump files, since tests will not work until that one is committed.
Comment #24
phenaproximaComment #26
phenaproximaBack to RTBC for #18, since @benjy's change was just cleanup and didn't change any of the functionality.
Comment #29
phenaproximaTestbot being weird.
Comment #30
catchIt doesn't look from the patch like it takes into account the possibility of duplicate files in the source table - see #1260938: d6 to d7 update fails on file duplicates '#7061 Integrity constraint violation'.
The test data added there could be re-used here.
Comment #31
phenaproximaDuplicate paths are only an issue in D6; this migration is for D7. (@catch OKed re-RTBCing on IRC.)
Comment #36
phenaproximaBack to RTBC -- @xjm informed me that HEAD broke testbot today due to a bungled commit. But that's all fixed now, so I think we're good.
Comment #37
alexpottNeeds to be a template.. see the CR for this.
Comment #38
alexpottI think the @group has been standardised.
Comment #39
phenaproximaFixed!
Comment #40
mikeryanLooks good to me.
Comment #41
phenaproximaRe-rolled in the wake of #2514168: Streamline migrate_drupal integration tests.
Comment #42
phenaproximaRe-rolled in accordance with #2533886: [meta] Move module-specific migration support into the particular modules supported.
Comment #43
chx CreditAttribution: chx commentedWhile normally I leave RTBCing an issue where benjy is involved to him, this was already RTBC'd by him and only went through non-significant changes so I think I am OK with this.
Comment #44
alexpottWe're missing test coverage of the private and temporary replaces. The private one is important cause security. Also when we test we only test migrating one file - I would have thought we should always test more than one of things like files which are likely to be migrated in their hundreds.
Comment #45
phenaproximaAdded tests of the URI transformation.
Comment #46
alexpottLooks good - thanks. I don;t foresee any problems with concurrent testing from either of the tests being added here. Nice.
Comment #47
webchickSo.. I want to commit this but I found this:
1) There's no doxygen here, but should be.
2) Why are we doing these Reflection shenanigans?
3) I thought this might've been an @inheritdoc situation, but this method doesn't exist in MigrateSqlSourceTestCase. Should it?
Comment #48
phenaproximaRemoved reflection stuff in favor of subclassing.
Comment #49
mikeryanComment #50
webchickGreat, thanks!
Committed and pushed to 8.0.x.