PDO_MySQL not properly quoting PDO_PARAM_LOB binary data

[lcobucci]

The prepared statement emulation layer is handling binary content in a way that creates warnings in MySQL.

When analysing the query logs, we saw that the content sent to the server is missing 0x5C characters when the using emulated prepares.

This introduces a minimal test case that reproduces the issue to aid the solution.

More info: doctrine/dbal#6522 (comment)

[lcobucci]

@mbeccati as promised 👍

[mbeccati]

@cmb69 @SakiTakamachi Even though it's a bug fix, I've changed the target to 8.4 as I'm not fully comfortable in making this change across all versions. What do you think?

[cmb69]

[…] as I'm not fully comfortable in making this change across all versions.

In my opinion, this is valid reason to target master only. If nobody complains about the behavioral change in PHP 8.4, but somebody request to backport to an earlier version, we can still consider to do it.

[lcobucci]

I agree with the concern and the approach.

The main worry here is having unexpected side-effects with the introduction of the _binary introducer (e.g. apps using PARAM_LOB to bind resources with non-binary data).

There were some PRs that suggested the introduction of a PARAM_BINARY, which could contain the impact of a BC-break.

[SakiTakamachi]

This is a bug fix of sorts, so I'd vote for it to be merged into 8.4.

[mbeccati]

Since the bug fix mainly affects PDO::quote() I thought it was a good idea to cover that too. The test il passing locally and on most CIs, aside from circleci on arm, with a failure that's baffling me.

========DIFF========
001- string(12) "_binary'%s'"
001+ string(13) "_binary'á\'"
========DONE========
FAIL MySQL PDO->quote(), properly handle binary data [ext/pdo_mysql/tests/pdo_mysql_quote_binary.phpt] 

I've checked and both use the same docker image sha256, so I'd expect the configuration and the quoted string to be the same, but on arm we have one extra byte (a backslash?).

I'll try to add a test on the query result and perhaps use %d for the failing var_dump.

[NattyNarwhal]

FWIW, I do have two PRs about better binary support in PDO I haven’t touched in a while (GH-10343 for PDO_DBLIB specifically, and GH-11674 for a general PARAM_BINARY).

[mbeccati]

@NattyNarwhal Well spotted: #10343 is pretty much the same fix as this, for DBLIB. Perhaps it would be worth combining the tests and making them part of the PDO common suite, so that we can verify that PARAM_LOB properly deals with binary data.

#11462 would be the PR for PARAM_BINARY. I will look have a look in the next few days, but I have a feeliing it is material for discussion for an RFC targeting 8.5.

[NattyNarwhal]

I wasn't sure if overriding the semantics of PARAM_LOB was right, so that's why I opened the PARAM_BINARY issue and PR.

It's been a while since I've thought of this topic, but I think it's worth discussing in an RFC on the internals mailing list. There's the issues I brought up on those previous threads, but there's likely other ones worth discussing.

[lcobucci]

#11462 would be the PR for PARAM_BINARY. I will look have a look in the next few days, but I have a feeliing it is material for discussion for an RFC targeting 8.5.

If we agree that PARAM_LOG should make sure to properly handle binary data when using that emulation layer, I'd say that PARAM_BINARY isn't necessary.

[NattyNarwhal]

Yeah, though my problem with overriding the semantics of PARAM_LOB was with i.e. CLOB types, which are (large) strings, and thus not binaries, but with LOB semantics. Likewise, a VARBINARY is not a BLOB, and wouldn't fit having LOB semantics. But it seems drivers are a bit mixed on how seriously they take LOBs, so it might not matter anyways.

[lcobucci]

Absolutely!

That's what I meant in #15949 (comment) 😄