Suggested commit message:
Issue #2121299 by andypost, bdone, benjy, penyaskito, chx, claudiu.cristea, damiankloip, gregboggs, InternetDevels, jessehs, jhedstrom, marvil07, mikeryan, pcambra, Xano, YesCT, axoplasm, kgoel, dclavain, jeckman, alvar0hurtad0, Drupali, fastangel, gloob, jhedstrom, joshtaylor, mpgeek, alexpott: Migrate Drupal 6 to Drupal 8
Problem/Motivation
Now the upgrade path has been replaced with the Migration API we need to provide migrations from Drupal 6 to Drupal 8.
Proposed resolution
Provide default migrations from Drupal 6 to Drupal 8 including CCK and a few additional CCK field modules such as date.
How To Review
This patch has 82 migrations that consist of:
- Source - The source which reads the data to be migrated directly from the D6 database. Separate patch for review.
- Migration - The migration YAML file which provides the mappings between the source and destination fields. Separate patch for review.
- Dump - A PHP version of the D6 data for testing. Some migrations share dump files. Separate patch for review
- Test - A test class which uses the Dump and the Migrate API to test the one migration. Separate patch for review.
Process Plugins
Some migrations have process plugins which apply processing to specific fields that are been imported. For example the FileUri process plugin generates the correct stream wrapper string. These and other files can be reviewed here.
MigrateDrupal6Test
This is one monster test that runs every single migration. We squashed a lot of bugs writing this test and it catches many issues which are sometimes disguised when an individual migration test sets something up "just right" in the setUp method. Individual tests are still useful in our opinion because debugging MigrateDrupal6Test is a special ... experience best described here.
Remaining tasks
Review the patch.
User interface changes
n/a
API changes
n/a
Comment | File | Size | Author |
---|---|---|---|
#35 | 2121299.35.patch | 650.34 KB | alexpott |
#35 | 24-35-interdiff.txt | 61.62 KB | alexpott |
#24 | 2121299_24.patch | 645.02 KB | benjy |
#24 | interdiff.txt | 2.48 KB | benjy |
#22 | 2121299_22.patch | 645.04 KB | benjy |
Comments
Comment #0.0
chx CreditAttribution: chx commentedUpdated issue summary.
Comment #1
chx CreditAttribution: chx commented#2125717: Migrate in core: patch #1 has been posted.
Comment #2
chx CreditAttribution: chx commentedWe have split migrate into migrate and migrate_drupal modules so that after the migration from an older Drupal version is done all those migration can be removed from the system as they won't be needed any more.
Comment #3
chx CreditAttribution: chx commentedFiled the meta issue for D6->D8 migrations and began to file the children of that.
Comment #4
benjy CreditAttribution: benjy commented09aadac
Comment #5
dawehnerI wonder whether you could break up the patch (not on an issue base but just in a way that you can actually use dreditor).
For example you could create one for the source, one for the tests and put the migration + dump bits into another one.
Comment #6
chx CreditAttribution: chx commentedSo theoretically -- yes we perhaps could split -- we have unit tests for the sources so those can stand alone but the migrations-dumps-test belong together otherwise we are committing untested code and that's probably unwise. Especially MigrateDrupal6Test tends to break like every second day when this or that API changes a bit. It practically covers almost all CRUD in Drupal 8 right now.
Comment #7
chx CreditAttribution: chx commentedAh, sorry didn't comprehend #5 , just separate files, I will try to do some git magic, yes.
Comment #8
chx CreditAttribution: chx commentedPatch split is rolled by:
the txts are attached in a zip; they don't need to be reviewed.
Comment #10
dawehnerThank you very much! I will find some time this night.
Comment #11
chx CreditAttribution: chx commentedObviously *Test.php won't run or pass; it misses the bases but if it wouldn't the dumps and the migrations aren't there.
Comment #13
chx CreditAttribution: chx commentedAnother note: the "rest" contained a few things that are not in the original. Fixed, hopefully.
Comment #14
chx CreditAttribution: chx commentedComment #15
chx CreditAttribution: chx commentedComment #16
chx CreditAttribution: chx commentedComment #17
dawehnerFirst small bit before the browser crashed (100KB is kind of a limit)
FormBase provides $this->t()
We can use $this->t() here.
This feels like a usecase for form plural, isn't it?
newline
A little bit more context in the exception message would be nice.
Just curious: Aren't there other schemas?
This seems to be a leftover from c&p code.
I guess we can make this testclass abstract?
Comment #18
dawehnerIs there a rule when we use a d6 prefix and when not?
Just in case you want to clean up some of the classes ... the coding standard suggests to add a newline between the new curly braces at the end.
... and a new line between the namespace and the use. This is absolute no requirement to change here, just a suggestion.
There seems to be a little bit c&p going on.
This is damn cool that we support that migration.
Tried to understand that comment. Maybe something like: This works, but we cannot test that, because there is no support for distinct() in FakeSelect, yet.
Another c&p of existing documentation.
it is not really obvious for me, what this plugin is supposed to do :(
I guess we could reduce the amount of code here by using a trait for these two methods but just for the test classes. This would also allow us to kill the use statements.
On top of that there is also no real need to re-specify the namespace.
Comment #19
benjy CreditAttribution: benjy commented#17
#18
ca1fcce
Comment #21
chx CreditAttribution: chx commented> I'm not aware of any other schemas in D6 but we should ask someone who can give us a definite answer.
Am I allowed to be the "someone"? Drupal 6 didn't have wrappers; it only had public and private. Consult https://api.drupal.org/api/drupal/includes%21file.inc/function/file_crea... for example.
Comment #22
benjy CreditAttribution: benjy commentedMigrations needed moving into install as per: https://drupal.org/node/2234799
Comment #24
benjy CreditAttribution: benjy commentedA migration had been changed in the sandbox and it broke the test. interdiff attach shows them changes.
Comment #25
dawehnerNoone is able to review that properly anyway :p
Comment #26
penyaskito+1 to RTBC.
Comment #27
chx CreditAttribution: chx commentedComment #28
penyaskitoComment #29
penyaskitoComment #30
effulgentsia CreditAttribution: effulgentsia commentedSite owners with D6 sites being able to test the D8 beta with their real content would be pretty awesome.
Comment #31
penyaskitoComment #32
jibranThere are a lot of doc issue and @todos missing links every time I tried to review it but I am unable to complete the review. So can we please create a followup doc fix novice issue for this.
Comment #33
webchickSince this patch is a Really Big Deal™, we pinged Dries to see if he wanted to take a crack at reviewing/committing it. If he doesn't get to it by Friday, I can take a look. Not explicitly assigning to him though since it's not like he has a monopoly on reviewing/committing, I just personally won't have time before Friday myself.
Comment #34
alexpottReviewing...
Comment #35
alexpottOk I've looked at every line of this patch - posting some minor tidy ups - yes if we picked every nit this patch would take ages. I've just gone for what glared at me whilst looking at all 645k. I'm happy to commit this is this is green as my amendments are totally minor - especially if one of the migration team does a quick review of the interdiff and +1's.
Comment #36
chx CreditAttribution: chx commentedI read these changes; they are OK (if the bot agrees).
Comment #37
alexpottAdding myself to the suggested commit message due to the patch in #35
Comment #38
alexpottLet's get people testing this!
Committed 66800ac and pushed to 8.x. Thanks!
Comment #40
donquixote CreditAttribution: donquixote commentedWe notice this did not conflict with #2083547: PSR-4: Putting it all together. Great!
However, all the drupal_get_path() will cause trouble when these newly introduced files move to PSR-4.
The solution is to use
dirname(__DIR__) . '/Dumps/XYZ.php'
instead ofdrupal_get_path('module', 'migrate_drupal') . '...'
.This is done here: #2248149: Use __DIR__ instead of drupal_get_path() for local PSR-0/PSR-4 directories.
Comment #41
donquixote CreditAttribution: donquixote commentedFollow-up: #2254157: Refactor Dump classes in migrate_drupal
Comment #45
chx CreditAttribution: chx commented