Skip to content

Comments

Use MySQL syntax for parsing SQL in AbstractMySQLPlatform#7202

Merged
morozov merged 1 commit intodoctrine:4.3.xfrom
morozov:issues/7199
Oct 29, 2025
Merged

Use MySQL syntax for parsing SQL in AbstractMySQLPlatform#7202
morozov merged 1 commit intodoctrine:4.3.xfrom
morozov:issues/7199

Conversation

@morozov
Copy link
Member

@morozov morozov commented Oct 29, 2025 •
edited
Loading

Fixes #7199.

This is a regression caused by #5089 which removed the MySQL57Platform together with the overridden method. I don't remember why in #4397 I overrode the method only in MySQL57Platform but not in AbstractMySQLPlatform (it seems it would have been the right thing to do).

The regression was possible since at that time we didn't have platform-specific integration tests for the SQL parser. Now we have a couple of them.

@morozov morozov added this to the 4.3.5 milestone Oct 29, 2025
@morozov
Copy link
Member Author

morozov commented Oct 29, 2025 •
edited
Loading

@derrabus could you take a look at the PHPStan failure? It fails on 4.3.x as well and seems related to #7187. PHPStan doesn't fail on 4.4.x and above since 4.3.x hasn't been merged up yet.

 ------ ---------------------------------------------------------------------------------------------------
  Line   tests/Logging/MiddlewareTest.php
 ------ ---------------------------------------------------------------------------------------------------
  40     Parameter #1 $record of method Psr\Log\Test\TestLogger::hasInfo() expects array{level: string,
         message: string|Stringable, context: array{exception?: Throwable}}|string, array{message:
         'Connecting with…', context: array{params: array{user: 'admin', password: '<redacted>'}}} given.
         🪪  argument.type
         💡  Type #1 from the union: Array does not have offset 'level'.

At a glance, the test is correct and PHPStan seems to be right, while the TestLogger method signatures are questionable. For example:

/**
 * @psalm-type log_record_array array{level: string, message: string|\Stringable, context: array{exception?: \Throwable}}
 */

    /**
     * @param log_record_array|string $record
     */
    public function hasInfo($record): bool
    {
        return $this->hasRecord($record, 'info');
    }

The method expects the $record array to contain a level, even though the level is hard-coded in the method implementation. Not passing the level as part of the record seems correct.

@morozov morozov marked this pull request as ready for review October 29, 2025 01:21
@morozov morozov mentioned this pull request Oct 29, 2025
Closed
@derrabus
Copy link
Member

derrabus commented Oct 29, 2025 •
edited
Loading

the TestLogger method signatures are questionable.

I agree and I stubled accross a similar issue on the new typing already: php-fig/log-test#17

@sasezaki sasezaki mentioned this pull request Oct 29, 2025
Closed
@morozov morozov requested review from derrabus and greg0ire October 29, 2025 16:51
@morozov morozov merged commit d437442 into doctrine:4.3.x Oct 29, 2025
95 of 96 checks passed
@morozov morozov deleted the issues/7199 branch October 29, 2025 22:22
@sasezaki sasezaki mentioned this pull request Nov 9, 2025
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@greg0ire greg0ire greg0ire approved these changes

@derrabus derrabus Awaiting requested review from derrabus

Assignees

No one assigned

Labels

MySQL Prepared Statements Regression SQL Parser

Projects

None yet

Milestone

4.3.5

Development

Successfully merging this pull request may close these issues.

SQL Parser is initialized with mySQLStringEscaping: false on MySQL platforms

3 participants

@morozov @derrabus @greg0ire