Skip to content
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.

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

Fix query binding with two colons in query #5117

Merged
merged 4 commits into from
Sep 24, 2021

Conversation

kenjis
Copy link
Member

@kenjis kenjis commented Sep 23, 2021

Description
Fixes #5114

Checklist:

  • Securely signed commits
  • Component(s) with PHPdocs
  • Unit testing, with >80% coverage
  • User guide updated
  • Conforms to style guide

Copy link
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.

This looks good to me but Database is my "blind spot" so I'd like @paulbalandan or @samsonasik to take a look.

@codeigniter4 codeigniter4 deleted a comment from kenjis Sep 24, 2021
Copy link
Member

@paulbalandan paulbalandan left a comment

Choose a reason for hiding this comment

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

I overlooked a detail in the later part. Sorry for the noise.

@paulbalandan
Copy link
Member

Thank you, @kenjis

@paulbalandan paulbalandan merged commit 194a484 into codeigniter4:develop Sep 24, 2021
@kenjis kenjis deleted the fix-5114 branch September 24, 2021 05:20
@vlakoff
Copy link
Contributor

vlakoff commented Sep 24, 2021

I have checked the differences between the two codes, and I don't see where is the functional difference.

The change in code is that a ! $hasBinds condition has been added when determining $hasNamedBinds,
but nevertheless the early return is triggered when both ! $hasNamedBinds && ! $hasBinds.

So, if $hasBinds is true, there can't be early return, and if $hasBinds is false, $hasNamedBinds gets determined as previously.

@kenjis
Copy link
Member Author

kenjis commented Sep 24, 2021

What are the two codes?

$hasBinds means the query contains bindings, that is ?.
$hasNamedBinds means the query contains named bindings, that is something like:name:.
They are exclusive. We cannot use both at a time.
So I added ! $hasBinds condition when determining $hasNamedBinds.

! $hasNamedBinds && ! $hasBinds means the query does not have :name: nor ?.
That is no bindings at all. So we have nothing to process bindings.
So we go early return.

@vlakoff
Copy link
Contributor

vlakoff commented Sep 24, 2021

I understood, what your change influences is actually a line that is a bit below in the code:

$sql = $hasNamedBinds ? $this->matchNamedBinds($sql, $binds) : $this->matchSimpleBinds($sql, $binds, $bindCount, $ml);

@vlakoff
Copy link
Contributor

vlakoff commented Sep 26, 2021

Though, the code would fail with named binds, simply if there is a "?" somewhere in the supplied SQL string.

As a quick example:

SELECT * FROM songs WHERE band = :band: AND title = 'Do You Love Me?'

hasBinds would be erroneously true, which in turn would make hasNamedBinds erroneously false.

@vlakoff
Copy link
Contributor

vlakoff commented Sep 26, 2021

Refs #5138.

@kenjis kenjis added the bug Verified issues on the current code behavior or pull requests that will fix them label Feb 19, 2022
@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.

Bug: Query binding fails when date_format is set with '%H:%i:%s'
5 participants