Getting the following error when migrating D6->D8, since updating to mysql 5.7.9
SQLSTATE[42000]: Syntax error or access violation: 1171 All parts of a PRIMARY KEY must be NOT NULL; if you need NULL in a key, use UNIQUE instead: CREATE TABLE {migrate_map_5649cac9deb88} ( `sourceid1` VARCHAR(255) CHARACTER SET ascii COLLATE ascii_general_ci DEFAULT NULL, `source_row_status` TINYINT unsigned NOT NULL DEFAULT 0 COMMENT 'Indicates current status of the source row', `rollback_action` TINYINT unsigned NOT NULL DEFAULT 0 COMMENT 'Flag indicating what to do for this item on rollback', `last_imported` INT unsigned NOT NULL DEFAULT 0 COMMENT 'UNIX timestamp of the last time this row was imported', `hash` VARCHAR(64) NULL DEFAULT NULL COMMENT 'Hash of source row data, for detecting changes', PRIMARY KEY (`sourceid1`) ) ENGINE = InnoDB DEFAULT CHARACTER SET utf8mb4 COMMENT 'Mappings from source identifier value(s) to destination…'; Array ( )
| Comment | File | Size | Author |
|---|---|---|---|
| #21 | interdiff.txt | 773 bytes | mikeryan |
| #21 | error_creating-2616282-21.patch | 1.6 KB | mikeryan |
Comments
Comment #2
peter.walter commentedDeclaring key as not null appears to fix.
Comment #3
mikeryanSet to "Needs review" for testing.
Comment #4
mikeryanSo, that test will at least demonstrate the change doesn't break anything with MySQL 5.5 - to demonstrate it fixes things for MySQL 5.7 we need #2616370: Testing MySQL >5.5?.
Comment #5
peter.walter commentedLast patch does not work reliably for new migrations, have moved 'not null' assignment outside of conditional branch.
Comment #6
peter.walter commentedComment #8
webchickComment #9
mikeryanComment #10
mikeryanCode is simple, straight-forward, and minimal risk at this point, and without it migration is totally broken on MySQL 5.7.9 (and I would guess 5.7.*). It'd be ideal to get a full test suite run on 5.7.9 of course (not least because who knows if there's anything else in core that would break?), but I'm perfectly comfortable with this patch.
webchick mentioned the idea of an upgrade path (setting not null on existing map table PKs) - but, if a site is already running 5.7.? then they wouldn't have managed to create any map tables to upgrade...
Comment #11
mikeryanMigrate is not alone: #2388139: Installation error MYSQL Primary key is null with MySQL 5.7 on block_content_revision.
Comment #12
catchWhat happens if you have a site on 5.5 from before this patch lands, then upgrade to 5.7 if we don't provide an upgrade path?
Comment #13
catchComment #14
mikeryanHmm... I would assume (hope) MySQL would fix the columns up on upgrade, because the alternative is it would break the tables. But, I don't know for sure, need to do some research.
Comment #15
mikeryanActually, as near as I can tell the current MySQL behavior is to automatically (and silently) make PK columns NOT NULL by default if it's not specified - the change in behavior is to now error out instead of silently fix it for you. So, any existing map tables already have their PK columns set NOT NULL.
Comment #16
catchOK that's good to know. We should consider a follow-up to validate definitions in the database layer.
Comment #17
catchGiven I made the parent critical, making this RC target too.
Comment #18
chx commentedMay I ask for a few hours to read up on this?
Comment #19
chx commentedMy problem: what if the *source* has a null in one of the ids? It shouldn't, of course, of course, but it's the source, after all.
Mike Ryan says " In D7 the way to deal with that was to turn the NULL into 0/'' as appropriate for the field type in prepareRow".
I can agree on this approach but this needs to be documented on
Comment #20
chx commentedComment #21
mikeryanIn practice, this is rare - at least having NULL being a valid part of a unique ID on the source side (although a novice migration developer might make poor choices when defining source IDs and include a nullable field, that's not so rare).
Yes, this is where it should be documented that source ID fields should never be NULL (and if a field that logically helps identify a source record as unique could be NULL, prepareRow() should rewrite it to 0/''). The attached patch improves the getIds() docblock (a quick edit as I'm at the end of my day, may bikeshedding commence...).
Improving prepareRow() docs should be a separate issue, yes. I don't think this is the place to document the issue at hand - poorly-chosen source keys are something that can be fixed in prepareRow(), yes, but are by no means a primary (or even secondary) purpose for prepareRow().
Comment #22
mikeryanAnd at the last second I recall, we can't fix up source keys in prepareRow(), to fix them up requires #2613878: Use hash for Migration source keys, rather than verbatim values.
Din-din time!
Comment #24
mikeryanWTF? Retest...
Comment #25
alexpottI tested this on MySQL 5.7.9 by running
Drupal\file\Tests\Migrate\d6\MigrateFileTest. It fails without the patch and passes with it.Comment #26
catchCommitted/pushed to 8.0.x, thanks!
Comment #28
chx commentedWhy was this rushed in? The doxygen now is factually incorrect as discussed in the child issue.
Comment #29
mikeryan@chx: Can you point at the issue you're referencing? What doxygen is incorrect? Was there something that said you could have a NULL source key column?
It's important to note that the change has no actual effect for MySQL versions < 5.7.3 - the PK columns are automatically set to NOT NULL even if you don't explicitly set 'not null'.
Comment #30
chx commented> if necessary, use prepareRow() or hook_migrate_prepare_row() to rewrite NULL values
The current architecture does not let you do that; there are no setters for source ID values at this moment.
Comment #31
mikeryanAh, right, should've gone back and fixed that up upon my realization in #22...