Use MySQL syntax only for parsing MySQL statements

[morozov]

In order to support both the standard and MySQL string literal syntaxes, an alternative parser for MySQL is introduced.

The choice of syntax is made based on the newly introduced statement flag which, as far as I understand, makes this change unacceptable for backporting. It would be nice to identify some other way to detect the syntax using the existing statement properties.

[morozov]

Tagging @cmb69 as the author of #5190 and the reviewer of #80340.

[cmb69]

These numbers (3 and 29) designate how many bits are to be used for these fields (this is called a bitfield). mysql_compat only needs a single bit, and _reserved needs to be decremented, so:

Suggested change

	unsigned mysql_compat:3;
	unsigned _reserved:29;
	unsigned mysql_compat:1;
	unsigned _reserved:28;

[cmb69]

Thanks for the PR!

The choice of syntax is made based on the newly introduced statement flag which, as far as I understand, makes this change unacceptable for backporting.

This might be acceptable.

Anyhow, a different approach could be to introduce something like pdo_parse_params_ex(…, mysql_syntax), and to call that from mysql_handle_preparer() instead of pdo_parse_params(). And let pdo_parse_params() call pdo_parse_params_ex() with 0 as last argument.

[morozov]

Anyhow, a different approach could be to introduce something like pdo_parse_params_ex(…, mysql_syntax), and to call that from mysql_handle_preparer() instead of pdo_parse_params(). And let pdo_parse_params() call pdo_parse_params_ex() with 0 as last argument.

This would be harder since MySQL statements are also parsed from the non-MySQL code in case when prepared statements are emulated:

php-src/ext/pdo/pdo_stmt.c

Lines 412 to 413 in 74fe917

	ret = pdo_parse_params(stmt, stmt->query_string, stmt->query_stringlen,
	&stmt->active_query_string, &stmt->active_query_stringlen);

[cmb69]

Oh, right. Thanks! So the new flag on the driver appears to be the way to go. We should consider reverting it's meaning, though, to not inadvertantly introduce issues with non-bundled drivers, which may also support MySQL backslash escapes. In mean, in case we like to target a stable branch (not sure, if we should target a stable branch, though).

And IIRC, newer MySQL version also support standard conforming escaping, so it may make sense to add a new PDO attribute, so that users can choose that syntax variant for MySQL. This would likely be for the "master" branch only.

[morozov]

We should consider reverting it's meaning, though, to not inadvertantly introduce issues with non-bundled drivers, which may also support MySQL backslash escapes.

As far as I understand, you're saying that instead of the MySQL driver saying "I'm MySQL", the other drivers should say "I'm not MySQL" thereby explicitly opting out of the old behavior. While I agree this is better from the BC perspective, I'm not sure I'm qualified to implement it since it requires changes in many more places that won't be covered by the tests PHP runs on CI.

Furthermore, it would have to change back to "I'm MySQL" in a release that allows BC breaks meaning more code and API changes.

In mean, in case we like to target a stable branch (not sure, if we should target a stable branch, though).

I'm fine if we don't. From the BC perspective, it still qualifies for the 8.0 release but I'm not sure if the schedule allows for it.

And IIRC, newer MySQL version also support standard conforming escaping, so it may make sense to add a new PDO attribute, so that users can choose that syntax variant for MySQL. This would likely be for the "master" branch only.

I can see that MySQL 5.6 support the standard ('') escaping. Do people use even older MySQL versions nowadays? I'm not sure it justifies adding yet another switch.

[KalleZ]

@morozov This is way too late for PHP 8.0, the final release is only just a few weeks away and we have to work on stability, but it is very plausible for PHP 8.1 however (master)

[morozov]

@KalleZ, this is absolutely fine with me.

[cmb69]

Okay, if we're targeting the "master" branch only, there's less need to worry about inadvertant breaks. Still, even if I like, we cannot enforce MySQL users to switch to the standard escaping mechanism; that would break far too much code.

Maybe we should bring this discussion to the internals mailing list?

[morozov]

[...] we cannot enforce MySQL users to switch to the standard escaping mechanism; that would break far too much code.

Why would we need to? The queries coming from pdo_mysql will be parsed using both the MySQL and ANSI syntaxes. Are you talking about the users of the non-bundled PDO-based drivers that rely on the MySQL syntax being parsed by default? What are those drivers?

[sartor]

Just hit in same situation with https://bugs.php.net/bug.php?id=80355
My script to reproduce bug:

$pdo = new \PDO('pgsql:host=localhost;dbname=postgres', 'postgres', 'postgres');
$pdo->prepare("SELECT '\', '--' WHERE 'q' = :e")->execute([':e' => 'q']);

[morozov]

Maybe we should bring this discussion to the internals mailing list?

@cmb69 so what exactly do you think we should discuss on the mailing list?

[mbeccati]

Pls see #6852 as an alternative approach. I was planning to RFC it as soon as I had time ;-)

[morozov]

Closing in favor of #6852 as a better alternative from the API/extensibility standpoint.