Setting the MySQL transaction isolation level (Fixes #4281) #4339
Conversation
|
Download the artifacts for this pull request: |
tyler36
force-pushed
the
tyler36/drupal-10
branch
from
4c58ac6
to
05a9e3c
Compare
|
Add to Drupal 9 and rebased. |
|
I'm going to take a look too and see if there's any way we can do this with extra config, that would represent what would be done in the real world. I imagine what we have here is what will work out easiest and best though. |
|
here is the change record about the mysql isolation level and the warning if it isn't using READ COMMITTED: https://www.drupal.org/node/3269885 . according to the CR will be introduced in 9.5.0. and i notice it is not only the docs page but the CR also has the recommendations with SET GLOBAL. so the issue is broader (in regards of the drupal documentation and the according issue that we filed). |
There was a problem hiding this comment.
When making a PR, try to change only the things that are relevant to the topic of the PR. If you want to make other changes (if "TRUE" should be "true" due to change in style preferences?) then make those changes in another PR that is style-related.
I'm pretty sure the changes to the go templating here introducing the spaces between members of the go template is unnatural, not sure if it's a syntax error. But that again is something that shouldn't be introduced if not related to the goal of the PR.
Overall, though, since this approach is currently recommended in the Drupal Change Notice, I think this approach will be fine.
| @@ -8,14 +9,14 @@ | |||
| */ | |||
|
|
|||
| $host = "{{ $config.DatabaseHost }}"; | |||
| $port = {{ $config.DatabasePort }}; | |||
There was a problem hiding this comment.
These aren't correct alterations AFAIK. Did your editor do this? These are golang template instructions, I haven't seen the spaces like this before.
| $settings['hash_salt'] = '{{ $config.HashSalt }}'; | ||
|
|
||
| // This will prevent Drupal from setting read-only permissions on sites/default. | ||
| $settings['skip_permissions_hardening'] = TRUE; | ||
| $settings['skip_permissions_hardening'] = true; |
There was a problem hiding this comment.
Is this a change in PHP preferences? If not, let's not change here.
tyler36
force-pushed
the
tyler36/drupal-10
branch
from
05a9e3c
to
b42833d
Compare
|
Sorry, Vscode tried to format on save. I thought the following setting was suppose to prevent that but it doesn't appear to be working. "editor.formatOnSaveMode": "modifications", |
|
Those EVIL editors! Sometimes they can help so much... and other times not so much. |
There was a problem hiding this comment.
I tested this on Drupal 9.4, drupal9.5 and Drupal 10 and it behaved as expected. Thanks! Tried mariadb:10.4 and mysql:8.0
I'm a bit reluctant, as discussed before, I don't think the Drupal recommendations are correct, that the config change should be in the mysql config, not done like this. But I'm good with this because it's the Drupal-recommended way and it's also the easiest for us to do.
When I look at the postgres database requirement I'll be thinking about this.
|
When @rpkoller approves we're good to go. |
There was a problem hiding this comment.
hm i am not really qualified for reviewing that. ;) but i've manually tested and added the snippet to one of my existing sites (removed the cnf file beforehand and restarted). adding the according lines set the transaction isolation level and commenting it out set the level back to REPEATABLE-READ.
the only detail i wonder is, does it have certain performance implications to set the level on the drupal settings level - that way isn't it set on every request? with the cnf solution it would be set once on startup on the database level instead?
but as @rfay said it is still the current Drupal-recommended best practice therefore a plus one from me. but talking of still, have to see who best to ping and ask on the drupal end about the underlaying issue. cuz since yesterday when i've learned searching for the change record the issue isnt just limited to the docs page but also the change record itself. there the recommendation is used as well. maybe the best place might be the d10readiness meetings?
p.s. first added the comment to the general conversation until i found where to approve changes. haven't expected that a comment is required there as well. therefore deleted the comment in the general conversation and readded it here.
|
In the future we have two options:
|
The Problem/Issue/Bug:
This resolves the issue of #4281 which requires MySQL tranaction isolation level setting.
How this PR Solves The Problem:
Add the required line to 'mysql' driver configurations.
Manual Testing Instructions:
Comment out line in a Drupal 10 install and a warning appears on
/admin/reports/statusRelated Issue Link(s):
Fixes #4281
Setting the MySQL transaction isolation level
Suggestion for: (3285800) Setting the MySQL transaction isolation level