Skip to content

fix: prepared query is executed when using QueryBuilder#6164

Merged
kenjis merged 4 commits intocodeigniter4:developfrom
kenjis:fix-prepared-query-bug
Jun 21, 2022
Merged

fix: prepared query is executed when using QueryBuilder#6164
kenjis merged 4 commits intocodeigniter4:developfrom
kenjis:fix-prepared-query-bug

Conversation

@kenjis
Copy link
Copy Markdown
Member

@kenjis kenjis commented Jun 20, 2022

Description

  • prepared query is executed when using QueryBuilder

How to confirm the bug
The MySQL query log of testExecuteRunsQueryAndReturnsResultObject():

2022-06-20T10:27:28.146476Z	  277 Query	INSERT INTO `user` (`name`, `email`, `country`) VALUES ('a', 'b@example.com', 'x')
2022-06-20T10:27:28.151105Z	  277 Prepare	INSERT INTO `user` (`name`, `email`, `country`) VALUES (?, ?, ?)
2022-06-20T10:27:28.151758Z	  277 Execute	INSERT INTO `user` (`name`, `email`, `country`) VALUES ('foo', 'foo@example.com', 'US')
2022-06-20T10:27:28.177816Z	  277 Execute	INSERT INTO `user` (`name`, `email`, `country`) VALUES ('bar', 'bar@example.com', 'GB')

Checklist:

  • Securely signed commits
  • Component(s) with PHPDoc blocks, only if necessary or adds value
  • Unit testing, with >80% coverage
  • User guide updated
  • Conforms to style guide

@kenjis kenjis added the bug Verified issues on the current code behavior or pull requests that will fix them label Jun 20, 2022
Copy link
Copy Markdown
Member

@MGatner MGatner left a comment

Choose a reason for hiding this comment

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

I'm not very familiar with query pretending but the code logic looks good. Was this bug actually "hiding" because of the PHPStan exemption? Or was that just a coincidence?

@lonnieezell
Copy link
Copy Markdown
Member

The changes seem fine but I guess I'm not understanding how this stops the prepared query from being executed with a query builder call.

@kenjis
Copy link
Copy Markdown
Member Author

kenjis commented Jun 21, 2022

@lonnieezell See the current develop code. Here is the line to execute queries.

$this->resultID = $this->simpleQuery($query->getQuery());

But the line to return when $pretend is true is here.
// If $pretend is true, then we just want to return
// the actual query object here. There won't be
// any results to return.
if ($this->pretend) {
return $query;
}

I have moved this before the simpleQuery().
In the original code, I don't see why the return is so late.

I believe this PR behaves the same as before in Prepared Queries without the bug,
but please point out later if I have missed something.

@kenjis
Copy link
Copy Markdown
Member Author

kenjis commented Jun 21, 2022

Was this bug actually "hiding" because of the PHPStan exemption?

Probably no.

If we remove the the PHPStan exemption, we get the errors.

 ------ -------------------------------------------- 
  Line   system/Database/BaseConnection.php          
 ------ -------------------------------------------- 
  637    Negated boolean expression is always true.  
  649    Negated boolean expression is always true.  
 ------ -------------------------------------------- 

It is true that the if conditions of the lines with errors are not necessary, but probably we don't notice this bug.

@kenjis kenjis merged commit a36882d into codeigniter4:develop Jun 21, 2022
@kenjis kenjis deleted the fix-prepared-query-bug branch June 21, 2022 00:36
@kenjis kenjis added the database Issues or pull requests that affect the database layer label Aug 16, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Verified issues on the current code behavior or pull requests that will fix them database Issues or pull requests that affect the database layer

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants