Problem/Motivation

The method Drupal\Core\Database\Driver\mysql\Connection::__construct() cannot be called without the second parameter being set. We have in the method the following code:

if (stripos($connection_options['init_commands']['sql_mode'], $mode) !== FALSE) {

Were $connection_options is the second parameter. If it is not set the following error is given: "TypeError: stripos(): Argument #1 ($haystack) must be of type string, array given".

Proposed resolution

The value of $connection_options['init_commands']['sql_mode'] needs to be set.

Remaining tasks

TBD

User interface changes

None

API changes

None

Data model changes

None

Release notes snippet

TBD

CommentFileSizeAuthor
#25 interdiff_24_25.txt735 bytesgidarai
#25 3251084-25-test-only-should-fail.patch942 bytesgidarai
#25 3251084-25.patch2.5 KBgidarai
#24 3251084-24.patch2.61 KBgidarai
#24 3251084-24-test-only-shoot-fail.patch940 bytesgidarai

Issue fork drupal-3251084

Command icon 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:

About issue forks
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

daffie created an issue. See original summary.

daffie’s picture

daffie CreditAttribution: daffie commented
Related issues: +#3218978: MySQL driver allows settings.php to remove ANSI_QUOTES from sql_mode, but doesn't work when it is

The code that created the problem was added in #3218978: MySQL driver allows settings.php to remove ANSI_QUOTES from sql_mode, but doesn't work when it is. That code is present in D9.2 and D9.3. Nobody has complained about it, therefor keeping the priority on "normal".

mondrake’s picture

mondrake
Location 🇮🇹
CreditAttribution: mondrake as a volunteer commented
Status: Postponed » Active
Issue tags: +Needs issue summary update

#3129043: Move core database drivers to modules of their own landed.

beatrizrodrigues’s picture

beatrizrodrigues
She/Her
CreditAttribution: beatrizrodrigues at CI&T commented
Assigned: Unassigned » beatrizrodrigues

Will work on that :)

beatrizrodrigues opened merge request !1610

beatrizrodrigues’s picture

beatrizrodrigues
She/Her
CreditAttribution: beatrizrodrigues at CI&T commented
Assigned: beatrizrodrigues » Unassigned
Status: Active » Needs work

I did a MR. Hope it fits. Leaving in NW because still needs issue summary update

daffie’s picture

daffie CreditAttribution: daffie commented
Issue summary: View changes
Issue tags: -Needs issue summary update

There is an unresolved thread on the MR.

beatrizrodrigues’s picture

beatrizrodrigues
She/Her
CreditAttribution: beatrizrodrigues at CI&T commented
Status: Needs work » Needs review

Thanks @daffie I think know the right solution is implemented.

daffie’s picture

daffie CreditAttribution: daffie commented
Status: Needs review » Needs work
Issue tags: +Needs tests

The fix is good. Now we need only a test.

daffie’s picture

daffie CreditAttribution: daffie commented
Issue tags: +Bug Smash Initiative
joachim’s picture

joachim CreditAttribution: joachim as a volunteer commented

For a test, simply remove the 2nd parameter in testDeprecationConnection() -- it's only here as a workaround for this bug:

  public function testDeprecationConnection() {
    $this->expectDeprecation('\Drupal\Core\Database\Driver\mysql\Connection is deprecated in drupal:9.4.0 and is removed from drupal:11.0.0. The MySQL database driver has been moved to the mysql module. See https://www.drupal.org/node/3129492');
    // @todo https://www.drupal.org/project/drupal/issues/3251084 Remove setting
    // the $options parameter.
    $options['init_commands']['sql_mode'] = '';
    $connection = new Connection($this->createMock(StubPDO::class), $options);
    $this->assertInstanceOf(Connection::class, $connection);
  }

mr.baileys made their first commit to this issue’s fork.

mr.baileys’s picture

mr.baileys
Primary language Dutch
Location 🇧🇪 (Ghent)
CreditAttribution: mr.baileys commented
Status: Needs work » Needs review
Issue tags: -Needs tests

Thank you @joachim, I updated \Drupal\KernelTests\Core\Database\MysqlDriverLegacyTest::testDeprecationConnection() and removed the code that was in place to work around the bug being fixed in this issue.

joachim’s picture

joachim CreditAttribution: joachim at Factorial GmbH commented
Status: Needs review » Reviewed & tested by the community

LGTM!

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Acro Commerce, Thunder commented
Status: Reviewed & tested by the community » Needs work

Added a comment to the MR that needs work.

Version: 9.4.x-dev » 9.5.x-dev

Drupal 9.4.0-alpha1 was released on May 6, 2022, which means new developments and disruptive changes should now be targeted for the 9.5.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Tauany Bueno’s picture

Tauany Bueno CreditAttribution: Tauany Bueno at CI&T commented
Assigned: Unassigned » Tauany Bueno

hi! i'll work on this :)

Tauany Bueno’s picture

Tauany Bueno CreditAttribution: Tauany Bueno at CI&T commented
Assigned: Tauany Bueno » Unassigned
Status: Needs work » Needs review

Hello!
I fixed the issues pointed out regarding ANSI_QUOTES and the if inside the foreach.
Since I'm new to Drupal, if this doesn't fix the problem I'd be more than happy to learn other solutions and approaches to the issue :)
Changing the status to needs review.

RenatoCostaDev’s picture

RenatoCostaDev CreditAttribution: RenatoCostaDev at CI&T commented
Assigned: Unassigned » RenatoCostaDev

"I'm reviewing this issue"

RenatoCostaDev’s picture

RenatoCostaDev CreditAttribution: RenatoCostaDev at CI&T commented
Assigned: RenatoCostaDev » Unassigned
Status: Needs review » Reviewed & tested by the community

I saw it and seems right to me.

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch at Third and Grove commented
Status: Reviewed & tested by the community » Needs review

Left a question on the MR

daffie’s picture

daffie CreditAttribution: daffie commented
Status: Needs review » Needs work
Issue tags: +Needs tests

Needs work for the 2 unresolved threads on the MR.

Version: 9.5.x-dev » 10.1.x-dev

Drupal 9.5.0-beta2 and Drupal 10.0.0-beta2 were released on September 29, 2022, which means new developments and disruptive changes should now be targeted for the 10.1.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

gidarai’s picture

gidarai CreditAttribution: gidarai at Finalist commented
Assigned: Unassigned » gidarai
Status: Needs work » Needs review
Issue tags: -Needs tests
FileSize
3251084-24-test-only-shoot-fail.patch940 bytes
3251084-24.patch2.61 KB

Created test and rerolled old code changes.
I submitted 2 patches, one with only the test file where the test will fail and another patch with the actual fix where the test succeeds.
Please review.

gidarai’s picture

gidarai CreditAttribution: gidarai at Finalist commented
FileSize
3251084-25.patch2.5 KB
3251084-25-test-only-should-fail.patch942 bytes
interdiff_24_25.txt735 bytes

After review on local-machine from daffie i have made some changes and also have fixed the corrupt patch that failed to apply.

Status: Needs review » Needs work

The last submitted patch, 25: 3251084-25-test-only-should-fail.patch, failed testing. View results

daffie’s picture

daffie CreditAttribution: daffie at Finalist commented
Status: Needs work » Reviewed & tested by the community

Both the fix and the added test look good to me.
For me it is RTBC.

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch at Third and Grove commented

Looks like we still need this follow-up suggested by @alexpott:

Also we should file a follow-up to remove 'ANSI_QUOTES' it is pointless - it is covered by 'ANSI' and our default sql_mode is "SET sql_mode = 'ANSI,TRADITIONAL'"

daffie’s picture

daffie CreditAttribution: daffie at Finalist commented

Also we should file a follow-up to remove 'ANSI_QUOTES' it is pointless - it is covered by 'ANSI' and our default sql_mode is "SET sql_mode = 'ANSI,TRADITIONAL'"

I think that will be fixed in #3261236: ANSI and TRADITIONAL SQL mode implementations vary between mysql and mariadb.

  • catch committed 1b7265e6 on 10.1.x 
    Issue #3251084 by gidarai, beatrizrodrigues, Tauany Bueno, mr.baileys,...
catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch at Third and Grove commented
Status: Reviewed & tested by the community » Fixed

Ahh thanks for finding the existing issue.

Committed 1b7265e and pushed to 10.1.x. Thanks!

Status: Fixed » Closed (fixed)

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