Newsgroups: php.internals
Path: news.php.net
Xref: news.php.net php.internals:114019
MIME-Version: 1.0
References: <CAP-hcNSGz6WQaCaebLdDm26emTrzxBUpyYvO0MF6anVB8t+=mg@mail.gmail.com>
 <CAGBsUrexRQiCuFbecaGGv_uho9m+QeUfO9+uG4A5z2beyZ1Sog@mail.gmail.com>
In-Reply-To: <CAGBsUrexRQiCuFbecaGGv_uho9m+QeUfO9+uG4A5z2beyZ1Sog@mail.gmail.com>
Date: Sat, 10 Apr 2021 17:41:40 -0700
Message-ID: <CAP-hcNSUWtLfiHc_VcGCPC=uneA8BCE=qNLp6p-S8niP_kmaQA@mail.gmail.com>
To: Kamil Tekiela <tekiela246@gmail.com>
Cc: PHP Internals <internals@lists.php.net>
Content-Type: multipart/alternative; boundary="000000000000bcad6305bfa7a93e"
Subject: Re: [PHP-DEV] Use MySQL syntax only for parsing MySQL statements in PDO
From: morozov@tut.by (Sergei Morozov)

--000000000000bcad6305bfa7a93e
Content-Type: text/plain; charset="UTF-8"

Hi,

> it seems to be a cheap hack

To me, it seems to be within the existing architecture. I agree that the
existing architecture has its flaws.

> What about backtick character ` ? What about NO_BACKSLASH_ESCAPES mode?

These are out of scope. I wasn't aware of the NO_BACKSLASH_ESCAPES but it
indeed seems to be impossible to support on the client side.

> Emulated prepares have a lot of issues and this is only one of them.

This specific issue doesn't seem to be related to the emulation. As far as
I understand, PDO parses SQL as well in order to replace positional
placeholders and vise versa depending on the input and the target platform
capabilities.

> If you want you can see the effect your PR has on this test case

I tried running the example on the stock PHP 8.0.3, and regardless of the
emulation mode and NO_BACKSLASH_ESCAPES mode I saw the same error:

> SQLSTATE[HY093]: Invalid parameter number: number of bound variables does
not match number of tokens in

With my patch, I don't expect to see any changes in parsing MySQL queries
so I don't understand how it's relevant. Am I missing something?

On Sat, Apr 10, 2021 at 3:33 PM Kamil Tekiela <tekiela246@gmail.com> wrote:

> This is an interesting solution to the problem, but I am unsure if it's
> the best one. I didn't look into it in detail yet, but at first glance, it
> seems to be a cheap hack to solve a bug. What about backtick character ` ?
> What about NO_BACKSLASH_ESCAPES mode? Any question mark within
> backtick-escaped column name would still cause a problem. In fact, when I
> tested it with NO_BACKSLASH_ESCAPES the PR didn't fix the problem. I
> got a different error, but the problem still exists.
> This is a naive solution that fixes one tiny bug in a single driver with
> no regards for tons of other issues. I am against adding such hacks into
> the PHP source code.
>
> What we need is a full SQL parser that is context-aware. This task should
> be delegated to the PDO drivers. PDO can't do it reliably at the base
> level. The problem is that PDO is never going to be able to reliably parse
> the SQL even if we have a separate parser for each driver. PDO is not the
> server and it doesn't know how the SQL will be parsed at the end of the
> day. At its root, it seems to be an unfixable problem. Emulated prepares
> have a lot of issues and this is only one of them. The solution is to use
> native prepares whenever they are available on the server.
>
> If you want you can see the effect your PR has on this test case:
>
> $pdo = new \PDO("mysql:host=localhost;dbname=test;charset=utf8mb4",
> 'user', 'password', [
>   \PDO::ATTR_ERRMODE => \PDO::ERRMODE_EXCEPTION,
>   \PDO::ATTR_EMULATE_PREPARES => true
> ]);
> $pdo->exec("SET sql_mode = NO_BACKSLASH_ESCAPES;");
> $stmt = $pdo->prepare("SELECT * FROM foo WHERE text='\' and 1=? and
> text='\'");
> $stmt->execute([1]);
>
> I'm sorry but this is a hard pass from me.
>


-- 
Sergei Morozov

--000000000000bcad6305bfa7a93e--