Per #8 Drupal core starting with 10.2 does not require that migrate IDs not include spaces. Since this has never effected migrations except when needing to roll back, we probably don't need to update the minimum Drupal version in the composer file, especially since 10.1 is already EOL.
Revert #3379184: Document that source IDs can only contain alphanumeric characters or underscores
Original description:
In the parent issue, a change was implemented to disallow spaces in source IDs. However, this change was not documented in an explicit change record, and existing migrations which have source IDs containing spaces will break.
There is no documentation regarding exactly how to fix migrations affected by this issue. Can the CSVs continue to use column names with spaces while the migration configuration needs the spaces deleted? Or do both the original CSV and the migrate configuration need to remove spaces?
E.g., would a CSV that has a "Some Column" column name properly map to a "SomeColumn" ID? Or would that not work?
Since this change leads to a minor version upgrade breaking previously-working migrations (a change which maybe should've waited for a major version increment), it's pretty important to get this properly documented.
Additionally, if this update completely prohibits the use of CSVs that includes spaces, it would be preferable to have a code solution to allow spaces in CSV column names and simply remove spaces before mapping columns to source IDs, since updating how CSV headers are exported from a particular source may not be trivial or possible. In these cases manual edits to CSV headers would be needed.
| Comment | File | Size | Author |
|---|---|---|---|
| #6 | migrate_source_csv-ids-3413894-2.patch | 1.25 KB | b2f |
| #5 | migrate_source_csv-ids-3413894.patch | 919 bytes | b2f |
Show commandsStart 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:
changes
Comments
Comment #2
b2f commentedTotally agree ! And I don't even understand why there is such a heavy restriction on IDs titles since they are not even stored afaik ? The cell value is stored, not the header label, this requirement needs clarification IMO.
Apparently this comes from a problem in \Drupal\migrate\Plugin\migrate\id_map\Sql::getSourceIdsHash https://www.drupal.org/project/migrate_source_csv/issues/3379184 ...
Comment #3
caesius commentedComment #4
b2f commentedIt looks like the only problem is a warning on rollbacks in such case where you have spaces in the header.
This warning seems totally non blocking (hence does not justify the constraint in ids) and perhaphs should be tackled in Sql.php ?
Comment #5
b2f commentedComment #6
b2f commentedThere you go, as I noticed the warning also is symptom of not actually doing the rollback (only in migrate_map, the actual content is not rollbacked), I patch Sql.php in core to map the values to correct IDs.
https://www.drupal.org/project/drupal/issues/3422603
Comment #7
b2f commentedBranch migrate_source_csv-3413894 ready for merge once core is updated (hopefully).
Comment #8
godotislateFix for #3422603: Fixing source IDs with spaces in Sql.php has been committed to 10.2.x-dev, so it should be out in the next 10.2 release. I think this issue then can be converted to be a revert of #3379184: Document that source IDs can only contain alphanumeric characters or underscores and this commit: https://git.drupalcode.org/project/migrate_source_csv/-/commit/91b1488b8...
If it's possible, a test should be added that rolling back with CSV columns containing spaces doesn't emit warnings like
[warning] Undefined array key "Some Header with Space" Sql.php:232. I'm not sure whether a core version constraint should be added as well.Comment #10
godotislatePushed MR with commit to revert. Haven't added the test I mentioned, so leaving at Needs work.
Comment #11
caesius commentedComment #12
heddnCould we do something less drastic and not remove all the test coverage, but rather adjust it to handle spaces too? Same with the exception?
Comment #13
godotislateThere's no longer any restriction on source column names. See https://git.drupalcode.org/project/drupal/-/blob/f45ae4c1201426086212343...
ETA: Not sure the test I mentioned before is actually necessary, since core has test coverage, but it's a nice to have.
Comment #14
godotislateResolved merge conflict and rebased. As mentioned in #13, core has test coverage to make sure spaces and special characters in source names work (see https://git.drupalcode.org/project/drupal/-/blob/11.x/core/modules/migrate/tests/src/Unit/MigrateSqlIdMapTest.php?ref_type=heads#L644), so I don't think it's necessary to duplicate here.
Moving to NR.
Comment #16
heddnThanks for your contributions.
Comment #18
gaëlgComment #19
m4oliveiIt looks like this was the only change after the 3.7 release. Would love to see a quick tag for this.
Comment #20
heddnhttps://www.drupal.org/project/migrate_source_csv/releases/8.x-3.8
Comment #21
m4oliveiAMAZING!
Thanks @heddn. I'm working on a project where patching is a bit of a pain, so this was very helpful!