Support MySQL GIPK mode

Add "GIPK" to dictionary

The 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.)

Not sure what the bot is saying here - it will be "needs work" until converted to MR, or converting to MR is just a recommendation?

@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.

@xjm time to fire up the convert patch to MR bot then?

The 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.

Removing "(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.

In this case I think it's fine to add a // cspell:ignore gipk comment to the file which will allow that word to appear in this file only.

Yes, cspell also picks up the "GIPK" from inside the MySQL constant! so easier to just ignore it.

Know it's a task but feels like one of those issues that should have test coverage or a mock test if possible?

@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

This 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.

If it’s one of those that can’t be tested could it be documented in the issue summary?

Apparently 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.

Hmm 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.

Ok 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

@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.

@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.

I'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.

@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.

Yes that was my question, and you answered it.

And yes - DrupalCI is now officially deprecated.

Patch #19 is now an MR

A 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?

@Charlie ChX Negyesi Looks like mysql\Connection popCommittableTransactions was removed in 📌 Refactor transactions Fixed , 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?

I didn't have any specifics , no, I guess this is fine then, sorry for the noise.

So keep coming back to this and I'm on ddev 8.0 but the test is skipped. Any idea?

@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)

ddev is on 8.0.33 but according to the test is needs to be less then 8.0.30 correct?

@smustgrave the test will be skipped on MySQL less than 8.0.30

Believe I was finally able to test with ddev and get a failing test.

Believe the change is good. Sorry for the delay.

I 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!

Automatically closed - issue fixed for 2 weeks with no activity.

mfb → closed merge request !5788