SQLParserUtils does not support quotes in comments when combined with quoted parameters

[JochemKlingeler]

Bug Report

	Version
doctrine/dbal	v2.10.0
SQL Server	2016
PHP	7.2.24

Summary

SqlParserUtils does not properly replace named parameters when there is an SQL comment with a quote in it, when the query also has "hardcoded" properties.

Pull request #3743 contains a regression test for this problem.

How to reproduce

Execute a query using an initialized Doctrine\DBAL\Connection:

$statement = $connection->executeQuery(
    "SELECT :foo /* some comment with one quote. This one -->' */ FROM :bar WHERE SOME_VALUE = 'hardcoded'",
    [':foo' => 'FOO', ':bar' => 'BAR']
`);

Expected behaviour

I can call fetchAll on $statement and get an array from BAR where SOME_VALUE is 'hardcoded'.

Current behaviour

An exception is thrown:

In DBALException.php line 169:

  An exception occurred while executing 'SELECT ? /* some comment with one quote. This one -->' */ FROM :bar WHERE slug = 'hardcoded'' with params ["FOO"]:

  SQLSTATE[IMSSP]: An error occurred substituting the named parameters.

In PDOConnection.php line 63:
  SQLSTATE[IMSSP]: An error occurred substituting the named parameters.

In PDOConnection.php line 61:
  SQLSTATE[HY093]: Invalid parameter number: mixed named and positional parameters

Context

As long as the query is valid, it should not matter what I put in the comments.
The produced error does not allude to the fact that a comment is at fault here.
For all intents and purposes the query, amount of parameters and the given types seem to be correct and only by removing the comment (or specifically the quote) will you get it to work.

The problem becomes even less visible when there is another comment with two quotes does not seem to produce the error. As long as there are an even amount of quotes (or 0) it will work as expected.

This problem can easily be triggerd by writing a comment like:

--- Don't include softdeleted FOO records

To prevent the programmer from hunting down this problem, either the parser should ignore comments, or throw an exception when this problem occurs.

Workaround

Our my project I added an assertion to (hopefully) help prevent my teammates from falling into the same trap:

    public function executeQuery(
        string $sql,
        ?array $params = [],
        ?array $types = [],
        ?QueryCacheProfile $qcp = null
    ): ResultStatement {
        // Only if there are params will executeQuery run the SQLParserUtils
        if ($params) {
            self::assertSqlHasNoBadQuotesInComments($sql);
        }
        return parent::executeQuery($sql, $params, $types);
    }

    public static function assertSqlHasNoBadQuotesInComments(string $sql): void
    {
        // match comments starting with two dashes (till newline) or anything between /* and */ (including newlines)
        preg_match_all('~(\/\*[\S\s]*?\*\/)|(--.*)~', $sql, $fragments);
        if (empty($fragments[0])) {
            return;
        }
        $quotes = [
            '"', // double quote
            "'", // single quote
            '`', // tick
        ];
        foreach ($fragments[0] as $fragment) {
            foreach ($quotes as $quote) {
                // If an even amount of quotes are used, there is no problem
                if (0 === substr_count($fragment, $quote); % 2) {
                    continue;
                }
                throw new SQLParserUtilsException(
                    'Can not properly parse SQL string if an unequal amount of quotes are used in comments! '
                    . 'Caused by query comment: '
                    . \PHP_EOL
                    . $fragment
                );
            }
        }
    }

[JochemKlingeler]

This relates to #3497

[morozov]

As far as the parser is concerned, this is fixed in #4397.

[github-actions]

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.