D6 image field widget settings stored the min and max resolution in one string and if nothing was specified then a simple 0 was stored. In Drupal 8, however, a single 0 causes a form error when trying to submit ImageItem::fieldSettingsForm.
The file directory setting in D6 is not imported at all, but instead hardcoded as public://, which clearly is wrong since the help text for the D8 file directory field says
Do not include preceding or trailing slashes.
Change 0 to '', and public:// to $widget_settings['file_path'] in FieldInstanceSetting::transform.
Add a test for image field settings in MigrateFieldInstanceTest.php
Review patch.
Just trying our very first D8 site and noticed this after running the import (where all files failed to migrate).
Drupal 8.0.0 and Drupal Upgrade Dev snapshot from today. No other modules installed.
Original settings
So:
1) File directory:
This gets mapped to public:// schema correctly, but the schema ends up being used as the directory too.
2) Resolution
This is pulled through as 0. 0 should be blanked out (this causes a validation error on save)
Luckily, small site, so we'll just manually upload, but reporting anyways :)
| Comment | File | Size | Author |
|---|---|---|---|
| #16 | Screenshot.png | 5.06 KB | alan d. |
| #12 | drupal-migrate-drupal-6-image-field-resolution-settings-2621794-12.patch | 2.84 KB | shaundychko |
| #10 | test.patch | 1.23 KB | shaundychko |
| image-settings-d8.png | 43.29 KB | alan d. | |
| image-settings-d6.png | 45.56 KB | alan d. |
Comments
Comment #2
mikeryanMoving to the core issue queue - as a general rule of thumb, if the results of a migration are incorrect, the problem is usually going to be in the core framework, not in the UI.
Comment #3
shaundychkoComment #4
chx commentedComment #5
chx commentedComment #6
shaundychkoAttached is a patch with only a test. It should fail since the d6 widget settings are by default
0, but D8 expects the default to be an empty string. The second patch with both a fix and tests makes0turn into an empty string when migrating.Comment #9
chx commentedComment #10
shaundychkoA patch which intentionally fails is posted here, along with a second patch that includes the fix. There is also a one line comment added to
MigrateFieldTest.phpwhich I noticed was missing while browsing to make this patch.Comment #11
shaundychkoComment #12
shaundychkoJust a bit of code cleanup. Wrap the comment, and remove an extraneous comment.
Comment #14
joelpittetThank you @ShaunDychko that looks great and makes sense the changes made for this migration. Thanks for the test only patch.
Comment #15
shaundychkoJust a note for the committers: @chx mentored and @joelpittet reviewed code during the Vancouver Code sprint, so they should be mentioned in the git commit message.
Comment #16
alan d. commentedNice work all, I feel lazy not even attempting to look into it myself ;)
Should a secondary issue be added about the file schema? The small site only used public images, but I would assume that private storage would also cause issues. The migration completely failed, possibly due to either validation fails on the resolution, but more likely imho, (a windows machine), failed to create the base folder "public://" due to the use of invalid characters for this operating system.
In fact, there was no validation when I added "public://" as part of the file storage directory, and the upload failed (Running 8.0.0)
i.e. I could do this:
and the field is unusable as the file path is invalid and the upload fails.
Comment #17
alan d. commentedComment #18
joelpittet@Alan D. the schema shouldn't be added to the file directory, there is a place for the storage Upload destination under "Field Settings"
Comment #19
alan d. commentedHi Joel, that is the second part of the settings issue I reported, the file schema was set to public (correctly migrated or was simply the site default) and the file destination folder was set to "public://" too. I had not touched these settings.
However... using the latest dev snapshot, I have run the update again with the patch, but no issue seen with the folder settings this time. :)
So rtbtc from a real D6 system perspective regarding the original report.
Still only 48 of the 213 files were imported, better than none, but there is still something fishy going on & have spent 4 hours trying to debug. If I find the reason, I'll create a new thread and ping back.
Comment #20
joelpittet@Alan D. I don't think I'm following. The patch also removes the hardcoded
public://.Comment #21
alan d. commentedSorry missed that line, but as I said above, with the patch no issue with either settings, rtbtc from me as per above :)
There are still issues with both file system (48 files from 248) & domain (67 files from 248) based migrations I'm trying but they are unrelated :(
Comment #22
joelpittet@Alan D. ah maybe a stub a follow-up issue for that and once you can flush out some details we can look into resolving that problem there?
Comment #25
catchCommitted/pushed to 8.1.x and cherry-picked to 8.0.x, thanks!