Fix bug #78227 Prepared statements ignore RENAME COLUMN #7495
Conversation
cmb69
left a comment
There was a problem hiding this comment.
Thank you for working on this! I will have a closer look soon, but you can address my comments right away, if you like.
| --CLEAN-- | ||
| <?php | ||
| if (getenv('REDIR_TEST_DIR') === false) putenv('REDIR_TEST_DIR='.__DIR__ . '/../../pdo/tests/'); | ||
| require_once getenv('REDIR_TEST_DIR') . 'pdo_test.inc'; | ||
| $db = PDOTest::factory(); | ||
| $db->exec("drop table bug_78227"); | ||
| ?> |
There was a problem hiding this comment.
| --CLEAN-- | |
| <?php | |
| if (getenv('REDIR_TEST_DIR') === false) putenv('REDIR_TEST_DIR='.__DIR__ . '/../../pdo/tests/'); | |
| require_once getenv('REDIR_TEST_DIR') . 'pdo_test.inc'; | |
| $db = PDOTest::factory(); | |
| $db->exec("drop table bug_78227"); | |
| ?> |
This clean section fails on most CI for SQLite3; I assume an in memory DB is used there. Apparently, there is no need for this cleanup, anyway, if the table name is test instead of the custom bug_78227.
There was a problem hiding this comment.
Thanks.
I fix it.
|
Since this is a bug fix, and I can't see any ABI break, this should probably target an older version; maybe it's too late for PHP-7.4, but PHP-8.0 or at least PHP-8.1 seem to be good candidates. |
| php_strtolower(statement_val, statement_len); | ||
|
|
||
| stmt->select_all = 0; | ||
| if (php_memnstr(statement_val, "select", sizeof("select") - 1, end) && php_memnstr(statement_val, "*", sizeof("*") - 1, end)) { |
There was a problem hiding this comment.
This might not be the best idea. What if we always reset the columns (regardless of the query)? Would that be a real performance issue?
There was a problem hiding this comment.
Thanks for your comment.
I'm sorry to reply you so late.
My English is poor but I will try my best to express my thought clearly.
When sql is just like
select * from test where id = ?It will reset the columns in order to get the latest columns information.
When sql is just like
select foo from test where id = ?it will not reset columns.
There was a problem hiding this comment.
stmt->columns will not change after the first execute, the key of result set will not change too although we change columns name.
There was a problem hiding this comment.
I'm sorry to reply you so late.
No problem! And thank you for the PR!
I think I understand the intention of the code, but select foo from test where name = "*" or even update some_table set selected = 42 where name = "*" would also reset the columns, and generally, it might not be problem to always reset the columns for every execution of the prepared statement (unless that would be a performance bottleneck for some drivers). That appears to be cleaner than trying to detect select statements that need it.
There was a problem hiding this comment.
Oh my god, I did not take that into mind.
There was a problem hiding this comment.
@cmb69 Thanks for your review.
I should fix this from another perspective.
| if (getenv('REDIR_TEST_DIR') === false) putenv('REDIR_TEST_DIR='.__DIR__ . '/../../pdo/tests/'); | ||
| require_once getenv('REDIR_TEST_DIR') . 'pdo_test.inc'; | ||
| $db = PDOTest::factory(); | ||
| @$db->exec("drop table test"); |
There was a problem hiding this comment.
Why is there a @? Please don't silence mysterious warnings. If your intention was to ignore the error when the table doesn't exist then use the SQL syntax IF EXISTS
There was a problem hiding this comment.
IF EXISTS is non-standard to my knowledge, and as such might not be supported by all drivers.
There was a problem hiding this comment.
Thanks for your comment.
This is my mistake, I am so sorry.
My pr is not perfect enough and I will solve it as soon as possible.
There was a problem hiding this comment.
Then a try-catch needs to be used here. If @ was intended then a comment should be put explaining which notice/warning we are ignoring here. Exceptions won't be ignored.
There was a problem hiding this comment.
I am not sure IF EXIST was supported by all database.
|
I don't like this change. This is something that happens extremely rarely. Changing table structure while reading from the table is not something that is usually done. I'd argue that it happens accidentally, rather than on purpose. If we want to fix this, then the correct way would be to re-read the column metadata whenever |
You are right. I didn't think very thoughtful. But it is so diffuclt to detect table structure changing or |
https://bugs.php.net/bug.php?id=78227
I am not sure whether it could solve this bug.
If not, your suggestions would be greatly appreciated.
Thanks.