Come together with the global Drupal community in Rotterdam, 28 Sept – 1 Oct 2026. Sessions, contribution, connection, and Early Bird savings until 8 June.Problem/Motivation
MySQL 8 has a sql_generate_invisible_primary_key (GIPK) mode which, if enabled, creates an invisible primary key for tables that do not have one. If a Drupal module later calls the addField() method to add a primary key to a table that was created in GIPK mode, the MySQL driver will need to drop the invisible column in the same statement. There is already a clause that drops any already-existing primary key, but that's not quite enough in this mode.
Steps to reproduce
- Use a recent version of MySQL 8
- Set sql_generate_invisible_primary_key=ON either globally or in the session (requires permission to do so)
- Run:
Drupal::database()->schema()->createTable('deleteme', ['fields' => ['foo' => ['type' => 'int']]]); Drupal::database()->schema()->addField('deleteme', 'id', ['type' => 'serial', 'not null' => TRUE], ['primary key' => ['id']]);
This should throw Drupal\Core\Database\DatabaseExceptionWrapper SQLSTATE[HY000]: General error: 4111 Please drop primary key column to be able to drop generated invisible primary key.: ALTER TABLE "deleteme" ADD "id" INT NOT NULL auto_increment, DROP PRIMARY KEY, ADD PRIMARY KEY ("id");
Proposed resolution
Perhaps addField() could catch this exception and add a clause to the statement that drops the invisible column?
Remaining tasks
User interface changes
API changes
Data model changes
Release notes snippet
| Comment | File | Size | Author |
|---|
Issue fork drupal-3399160
Show commands
Start 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:
- 3399160-support-mysql-gipk
changes, plain diff MR !5788
Comments
Comment #2
mfbComment #3
mfbAdd "GIPK" to dictionary
Comment #4
needs-review-queue-bot commentedThe Needs Review Queue Bot tested this issue.
While you are making the above changes, we recommend that you convert this patch to a merge request. Merge requests are preferred over patches. Be sure to hide the old patch files as well. (Converting an issue to a merge request without other contributions to the issue will not receive credit.)
Comment #5
mfbNot sure what the bot is saying here - it will be "needs work" until converted to MR, or converting to MR is just a recommendation?
Comment #6
xjm@mfb We've turned off automatic patch testing and are trying to get core issues converted to MRs, but we also don't want a bunch of credit-gamers rolling into issues, adding noise, and taking down Drupal.org infrastructure with a zillion new MRs at once. We will still commit patches if necessary, but NR and RTBC issues should ideally have an MR rather than a patch. We aren't quite at the point of disallowing all patches, though; thence (strongly) recommended.
Comment #7
mfb@xjm time to fire up the convert patch to MR bot then?
Comment #8
needs-review-queue-bot commentedThe Needs Review Queue Bot tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".
This does not mean that the patch needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.
Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.
Comment #9
mfbRemoving "(GIPK)" from the code comment because the dictionary is constantly changing and thus patch "fuzz" appears, which the CI system interprets as a hard conflict.
Comment #10
longwaveIn this case I think it's fine to add a
// cspell:ignore gipkcomment to the file which will allow that word to appear in this file only.Comment #11
mfbYes, cspell also picks up the "GIPK" from inside the MySQL constant! so easier to just ignore it.
Comment #12
smustgrave commentedKnow it's a task but feels like one of those issues that should have test coverage or a mock test if possible?
Comment #13
mfb@smustgrave not sure we can test this MySQL setting, as setting it requires the SESSION_VARIABLES_ADMIN privilege. We could try and see if the CI environment has this privilege though
Comment #14
mfbThis test fails for me, but passes if I grant the SESSION_VARIABLES_ADMIN privilege. Note this is just a test test :) to be replaced with a simpler/quicker failing test if it passes.
Comment #15
smustgrave commentedIf it’s one of those that can’t be tested could it be documented in the issue summary?
Comment #16
mfbApparently the CI system does have privileges to test this setting, so here's a failing test, which will be skipped if there are not privileges to test it.
Comment #17
mfbHmm that failed to fail :/ Maybe because the CI system is running an old version of MySQL, and the test is being skipped? Let's see if we can find out.
Comment #18
mfbComment #19
mfbOk wow, drupal CI runs on MySQL 8.0.11, which was released way back in April 2018! So, the test will be skipped on any patch I upload, but at least we can run the test locally.
So, here's the patch at #11 combined with the test at #16
Comment #20
smustgrave commented@catch shared this in #needs-review-queue-initative channel as I've not seen one of these before, https://www.drupal.org/about/core/policies/core-change-policies/drupal-d...
Wasn't able to test locally as I'm on ddev which also believe is 8.0.11 but looking at the test believe it covers the scenario (kudos!)
I believe this is good but wonder if submaintainer sign off is also needed.
Comment #21
mfb@smustgrave I'm not sure what you mean re: linking to "Interface and base class method signature changes"
Looking at the ddev docker images, it looks like you might be able to update to their latest mysql image, which is version 8.0.33
Sounds correct that someone should sign off on adding support, and this particular approach.
Comment #22
bradjones1I'm not quite sure this needs any particular sign-off on whether to adopt - it's a feature of MySQL 8 that might be used alongside Drupal and so we should improve compatibility. Is it possible to enable this functionality at runtime or during install? Do we want to venture into that workflow?
I don't see any change to interfaces in the patch at #19?
Also this should probably get converted to an MR at this point.
Comment #23
mfb@bradjones1 are you asking if drupal should enable this MySQL setting for its session? I don't think it should, as this is a "restricted" session variable that requires an extra privilege: SESSION_VARIABLES_ADMIN. Seems best to support it being either on or off, and not try to toggle it (aside from in the test).
I can convert the patch to merge request, which I believe should cause it to run on a more recent version of MySQL - last I looked, it appeared that DrupalCI used an old version of MySQL, while GitLab CI used a more recent version.
Comment #25
bradjones1Yes that was my question, and you answered it.
And yes - DrupalCI is now officially deprecated.
Comment #26
mfbPatch #19 is now an MR
Comment #27
ghost of drupal pastA very wise former core subsystem maintainer taught me to introduce a helper method when some functionality is repeated at least three times.
Currently migrate Sql and mysql\Connection popCommittableTransactions extracts the mysql error code. This adds the third. All three are using subtly different checks which once again shows a helper would be helpful. Also, don't we need to loop the previous exceptions as FinalExceptionSubscriber does?
Comment #28
mfb@Charlie ChX Negyesi Looks like mysql\Connection popCommittableTransactions was removed in #3364706: Refactor transactions, so it would appear this isn't done often enough for a helper method to be useful.
I don't think we need to loop thru previous exceptions; did you have a scenario in mind where that is needed?
Comment #29
ghost of drupal pastI didn't have any specifics , no, I guess this is fine then, sorry for the noise.
Comment #30
smustgrave commentedSo keep coming back to this and I'm on ddev 8.0 but the test is skipped. Any idea?
Comment #31
mfb@smustgrave which reason for skipping are you hitting? Both should be resolvable, either grant the SESSION_VARIABLES_ADMIN privilege or upgrade to MySQL 8.0.30+ (or both)
Comment #32
smustgrave commentedddev is on 8.0.33 but according to the test is needs to be less then 8.0.30 correct?
Comment #33
mfb@smustgrave the test will be skipped on MySQL less than 8.0.30
Comment #34
smustgrave commentedBelieve I was finally able to test with ddev and get a failing test.
Believe the change is good. Sorry for the delay.
Comment #35
longwaveI reproduced the issue locally and the fix looks reasonable. Backported to 10.2.x as it's a borderline bug fix and harmless change unless this mode is enabled - the specific error code is only thrown for this case - which means anyone who wants to use this can do so now instead of waiting for 10.3/11.0.
Committed and pushed 6a0d80e461 to 11.x and 7eb3267ba2 to 10.2.x. Thanks!