Skip to content

Fix FR #71885 (Allow escaping question mark placeholders) #4217

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 1 commit into from
Jul 22, 2019
Merged

Fix FR #71885 (Allow escaping question mark placeholders) #4217

merged 1 commit into from
Jul 22, 2019

Conversation

mbeccati
Copy link
Contributor

@mbeccati mbeccati commented Jun 1, 2019

@mbeccati mbeccati changed the base branch from master to PHP-7.4 June 1, 2019 13:55
carusogabriel
carusogabriel reviewed Jun 1, 2019
View reviewed changes
carusogabriel
carusogabriel reviewed Jun 1, 2019
View reviewed changes
@nikic nikic added this to the PHP 7.4 milestone Jul 2, 2019
nikic
nikic reviewed Jul 19, 2019
View reviewed changes
Copy link
Member

@nikic nikic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The overall implementation approach of treating ?? as a special binding type seems rather odd to me. I don't really understand why that is necessary. Can ?? be treated as a normal text token and the unescaping applied when copying it?

ext/pdo/pdo_sql_parser.re Outdated
ret = 0;
goto clean_up;
newbuffer_len = inquery_len;
goto rewrite;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this mean we're going to be rewriting placeholders even if they are supported? Or does that get skipped further down?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hopefully 81beba8 should short circuit in case no rewriting is required.

@mbeccati
Copy link
Contributor Author

Can ?? be treated as a normal text token and the unescaping applied when copying it?

@nikic Am I missing some re2c special functionality?

@mbeccati
Copy link
Contributor Author

mbeccati commented Jul 19, 2019 •
edited
Loading

Truth is, the patch is several years old and I can't remember much about the reason I went this route, but I reacall had tried several different approaches and this is what I ended up with, as everything else wasn't quite working. I will try to have a look again during the weekend.

nikic
nikic reviewed Jul 22, 2019
View reviewed changes
ext/pdo/pdo_sql_parser.re
@@ -315,8 +341,13 @@ rewrite:
memcpy(newbuffer, ptr, t);
newbuffer += t;
}
memcpy(newbuffer, plc->quoted, plc->qlen);
newbuffer += plc->qlen;
if (plc->quoted) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is this change necessary?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I will check ;-)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Without, it doesn't work properly. When running the new pdo_pgsql tests, there is a code path that sends a non-quoted "?". I'll try to figure out how and when this afternoon.

nikic
nikic reviewed Jul 22, 2019
View reviewed changes
@cmb69
Copy link
Member

cmb69 commented Jul 22, 2019

ext\pdo_odbc\tests\bug_71885.phpt fails on AppVeyor; AFAIK pdo_odbc is not tested on other CI. This should be investigated.

@mbeccati
Copy link
Contributor Author

@cmb69 yes, I've seen the failure - I had no means of testing pdo_odbc. I guess it is due to the error message not containing a "?", but I could as well be wrong. I was thinking about temporarily removing the generic tests and creating a new PR to better investigate and fix based on the output from AppVeyor.

If you have any better idea, please let me know!

@cmb69
Copy link
Member

cmb69 commented Jul 22, 2019

@mbeccati, I'll try to reproduce the test failure. :)

@cmb69
Copy link
Member

cmb69 commented Jul 22, 2019

Error message is "SQLSTATE[07002]: COUNT field incorrect: 0 [Microsoft][ODBC Driver 17 for SQL Server]Das COUNT-Feld ist nicht korrekt, oder es besteht ein Syntaxfehler (SQLExecDirect[0] at ext\pdo_odbc\odbc_driver.c:245)"

So yes, probably just the question mark missing from the error message. Since other ODBC drivers may yield completely different messages, it might be best to skip the test for PDO_ODBC.

@mbeccati
Copy link
Contributor Author

@cmb69 The test should be skipped on obdc now - I'm not extremely satisfied with it either. Maybe I will try to improve using the newer functionality from debugDumpParams() in the coming days.

@nikic Due to lack of time, I wasn't able to find the code path that leads to the if (plc->quoted) necessary change. I'm happy enough that the PR works as advertised and there are no leaks on pdo pgsql + mysql + sqlite tests. I suppose we can refine in the coming days, if necessary.

nikic
nikic reviewed Jul 22, 2019
View reviewed changes
ext/pdo/tests/bug_71885.phpt Outdated
$dir = getenv('REDIR_TEST_DIR');
if (false == $dir) die('skip no driver');
if (!strncasecmp(getenv('PDOTEST_DSN'), 'pgsql', strlen('pgsql'))) die('skip not relevant for pgsql driver');
if (!strncasecmp(getenv('PDOTEST_DSN'), 'pgsql', strlen('odbc'))) die('skip inconsistent error message with odbc');
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should be odbc for the first string as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@nikic Of course :(

While I have you here. Should I push to 7.4 and master or will you merge the PR?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Feel free to merge :)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@nikic done, thanks!

@php-pulls php-pulls merged commit b19fdc1 into php:PHP-7.4 Jul 22, 2019
@piotr-cz
Copy link

Sorry for misusing this PR, but I can't find any info about this feature either in PHP 7.4 release info or PDO::prepare docs.

Is there any information about it on the php.net site besides the mentioned RFC?

@mbeccati mbeccati deleted the fr71885-new branch February 27, 2020 07:58
@mbeccati
Copy link
Contributor Author

@piotr-cz I'm afraid not yet. TBH I don't know what's the process for docs.

@cmb69
Copy link
Member

cmb69 commented Feb 27, 2020

Only info about this in the PHP manual so far is in the migration guide. Not sure where exactly further info might be provided in the manual proper.

@piotr-cz
Copy link

Thanks @cmb69

In my opinion this feature should be documented at least in

Unfortunately this feature hasn't been acknowledged by the community yet, probably because it's missing in Release Announcement.

@nayabbukhari nayabbukhari mentioned this pull request May 1, 2020
Closed
@cmizzi cmizzi mentioned this pull request Jul 17, 2020
Merged
@stacyharper stacyharper mentioned this pull request Dec 23, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nikic nikic nikic left review comments

+1 more reviewer

@carusogabriel carusogabriel carusogabriel left review comments

Reviewers whose approvals may not affect merge requirements
Assignees
No one assigned
Labels
RFC
Projects
None yet
Milestone
PHP 7.4
Development

Successfully merging this pull request may close these issues.
6 participants
@mbeccati @cmb69 @piotr-cz @nikic @carusogabriel @php-pulls