Determine if binds are simple or named by looking at the $binds array #5138

vlakoff · 2021-09-26T07:58:28Z

To determine if binds are simple or named, rather than trying to deduce it from the SQL string, which is the source of many errors (for example see #5114, and all the issues it links to), simply look at the $binds array. If it's an indexed array, the binds are simple, if it's an associative array, the binds are named.

Note that if #5127 is merged before, this PR will need a slight rebasing before merging.

kenjis · 2021-09-26T08:43:18Z

Coding Standard checking was failed.
Run composer cs-fix, please.

vlakoff · 2021-09-26T09:17:19Z

On a related note, is the test empty($this->bindMarker) really useful?

To trigger it, the user should extend the Query class and assign the bindMarker property an empty value… doesn't seem very likely to happen.

vlakoff · 2021-09-27T08:36:55Z

The PR currently makes an unit test fail:

CodeIgniter\Database\Builder\WhereTest::testWhereValueClosure
Error: Object of class Closure could not be converted to string

/home/runner/work/CodeIgniter4/CodeIgniter4/system/Database/Query.php:325
/home/runner/work/CodeIgniter4/CodeIgniter4/system/Database/Query.php:298
/home/runner/work/CodeIgniter4/CodeIgniter4/system/Database/Query.php:150
/home/runner/work/CodeIgniter4/CodeIgniter4/system/Database/BaseBuilder.php:1411
/home/runner/work/CodeIgniter4/CodeIgniter4/system/Database/BaseBuilder.php:1395
/home/runner/work/CodeIgniter4/CodeIgniter4/system/Database/BaseBuilder.php:676
/home/runner/work/CodeIgniter4/CodeIgniter4/system/Database/BaseBuilder.php:606
/home/runner/work/CodeIgniter4/CodeIgniter4/tests/system/Database/Builder/WhereTest.php:134

See for instance at: https://github.com/codeigniter4/CodeIgniter4/pull/5138/checks?check_run_id=3712623836

I've already spend a lot of time investigating this. It seems to be due to some stale binds data that should be cleaned between query compilations.

kenjis · 2021-09-27T09:25:16Z

$replacersTo[1] is a Closure. It is from $binds['advance_amount']

kenjis · 2021-09-27T12:21:38Z

system/Database/Query.php

        }

-        return strtr($sql, $replacers);
+        return str_replace($replacersFrom, $replacersTo, $sql);


Why do you replace strtr() with str_replace()?

Per my commit diff:

strtr() is purposed for single-character replacements, and less performant when used otherwise.

I noticed a (well-known) issue with str_replace(): the replacements may overlap, so if a binded value contains itself a bind marker (e.g. ['comment' => 'a bind marker looks like :foobar:', 'foobar' => 'bazqux'], that look-alike marker would be erroneously replaced.

Thus, I will revert to strtr(), and we may want to add an unit test so that such regression wouldn't be introduced later.

some refs: answer on Stack Overflow, comment on php.net

PR adding an unit test for that case: #5184.

kenjis · 2021-09-27T12:30:25Z

This passed that test.

--- a/system/Database/Query.php
+++ b/system/Database/Query.php
@@ -11,6 +11,8 @@
 
 namespace CodeIgniter\Database;
 
+use Closure;
+
 /**
  * Query builder
  */
@@ -301,6 +303,8 @@ class Query implements QueryInterface
 
     /**
      * Match bindings
+     *
+     * @param array<string, array{0: int|string|Closure, 1: bool}> $binds
      */
     protected function matchNamedBinds(string $sql, array $binds): string
     {
@@ -308,6 +312,11 @@ class Query implements QueryInterface
         $replacersTo   = [];
 
         foreach ($binds as $placeholder => $value) {
+            // If $value[0] is Closure, it is a sub query. We don't need to replace.
+            if ($value[0] instanceof Closure) {
+                continue;
+            }
+
             // $value[1] contains the boolean whether should be escaped or not
             $escapedValue = $value[1] ? $this->db->escape($value[0]) : $value[0];

vlakoff · 2021-09-27T12:34:37Z

$replacersTo[1] is a Closure. It is from $binds['advance_amount']

Indeed. I have pinpointed the deep cause of this, and have to work on a fix.

Basically, we really shouldn't have a closure here. The current code works because it doesn't look at the binds array but at the SQL string, which doesn't have :placeholders: at that point, so it returns early and doesn't go to the closure.

vlakoff · 2021-09-27T12:37:17Z

This passed that test.

(...)

Thanks. Along the fix for the root cause, maybe such guard should also be added to prevent errors coming from other places.

vlakoff · 2021-10-27T11:23:50Z

#5247 fixes the underlying cause of the error.

Also, I don't think we should add the "skip if closure" guard, as a closure is really not expected here. If ever a closure lands here, it's for sure because of a mistake beforehand, that has to be fixed rather than ignored.

An additional possibility could be to implement support of callbacks as bind values. Something like:

foreach ($binds as $placeholder => $value) {
    if ($value instanceof Closure) {
        $value = '(' . str_replace("\n", ' ', $value($builder)->getCompiledSelect()) . ')';
    }
    str_replace(":{$placeholder}:", $value, $sql);
}

… but unless there is a demonstrated need for such feature, we should better leave it apart.

vlakoff · 2021-11-16T00:59:31Z

On a related note, in this call:

$this->matchSimpleBinds($sql, $binds, $bindCount, $ml);

protected function matchSimpleBinds(string $sql, array $binds, int $bindCount, int $ml): string

It's redundant to pass $bindCount and $ml, as their values can easily be deduced from $binds and $this->bindMarker respectively.

However, it would be a BC break to change the method signature…

vlakoff · 2021-11-16T01:45:21Z

Now that the prerequisite #5247 has been merged, I think this PR should be ready to be merged.

kenjis · 2021-11-16T10:33:53Z

@vlakoff This branch is too old.
Could you rebase it?

https://github.com/codeigniter4/CodeIgniter4/blob/develop/contributing/workflow.md#pushing-your-branch

vlakoff · 2021-11-16T11:24:55Z

Rebased, although there were no merge conflicts.

kenjis

LGTM

This is cleaner.

vlakoff · 2021-11-16T13:08:24Z

This conditional to determine if the array is a list or an associative map may be improved:

if (is_numeric(key(array_slice($binds, 0, 1)))) { ... }

Rather than is_numeric() we should use the stricter is_int(). As a reminder, PHP automatically casts array keys like '1' (string representing an integer) to 1 (integer).

if (is_int(key(array_slice($binds, 0, 1)))) { ... }

lonnieezell

Looks like a good set of changes that simplifies and should have a slight performance boost getting rid of the regex. Nicely done.

I just have one question about array_slice. I realize that was in the original code, but looking at it in context now, I don't think it's necessary, but would love a second opinion on that.

lonnieezell · 2021-11-21T04:15:00Z

system/Database/Query.php

-            $binds     = $this->binds;
+        $binds = is_array($this->binds) ? $this->binds : [$this->binds];
+
+        if (is_int(key(array_slice($binds, 0, 1)))) {


Is array_slice necessary here? Since the array has just been created, the current index should be the first element, so I think just doing is_int(key($binds)) is enough.

Or using array_key_first?

$binds may be a reference to $this->binds, and as PHP doesn't copy arrays until they are modified, the pointer position of $binds will be the same as of $this->binds, which may have been modified beforehand, we can't know.

A benefit of key(array_slice($binds, 0, 1)) over reset($binds) is that with the latter PHP would internally make a copy of the whole array, while the former just creates a tiny array of one item.

Good idea about array_key_first(). As we are targetting PHP 7.3+, that's probably what we want to use.

paulbalandan

Can we add some tests for this?

paulbalandan · 2021-11-21T07:53:15Z

system/Database/Query.php

-            $binds     = $this->binds;
+        $binds = is_array($this->binds) ? $this->binds : [$this->binds];
+
+        if (is_int(key(array_slice($binds, 0, 1)))) {


Or using array_key_first?

paulbalandan · 2021-11-21T07:55:32Z

system/Database/Query.php

-            $bindCount = 1;
-        } else {
-            $binds     = $this->binds;
+        $binds = is_array($this->binds) ? $this->binds : [$this->binds];


Suggested change

$binds = is_array($this->binds) ? $this->binds : [$this->binds];

$binds = (array) $this->binds;

I can't think of any regressions with that change, so why not.

vlakoff · 2021-11-21T15:18:27Z

Can we add some tests for this?

Unit test aren't really my strong point…
(disclaimer: I have an aversion to writing tests, as I always fear there would be redundancies or missing spots)

If you can provide me tests that fail before and pass after this PR, I would happily add them.

vlakoff · 2021-11-22T17:21:45Z

I added tests: vlakoff@d7ad8ae.
I have yet to squash this commit (and apply the other suggestions), but you can already have a glance at these.

testNamedBindsWithBindMarkerElsewhere():
This PR makes this test pass.
testSimpleBindsWithNamedBindPlaceholderElsewhere():
The ! $hasBinds condition in $hasNamedBinds = ! $hasBinds && preg_match('/:(?!=).+:/', $sql) === 1 already made this test pass, but it had the side effect of making the other test fail…
(and in turn, removing that condition would make both $hasBinds and $hasNamedBinds true, which is wrong, and make the other test pass, but only because of the order in the ternary $sql = $hasNamedBinds ? $this->matchNamedBinds(...) : $this->matchSimpleBinds(...))

* Simple binds: matchSimpleBinds() also handles the bind marker, and more elaborately: handles enclosing quotes, checks the number of markers… * Named binds: there just won't be replacements for matchNamedBinds() to make.

It's very unlikely that the bindMarker property would be empty.

Considering that $binds is usually an array, and that only matchSimpleBinds() needs the count.

We are looking for an integer, excluding other numeric values (decimals, etc). As a reminder: for array keys, PHP automatically casts strings representing integers to regular integers.

Same results, but simpler and faster code. Note array_key_first() is available on PHP 7.3+, which is thankfully the minimum PHP version required by CodeIgniter.

vlakoff · 2021-11-23T02:09:25Z

Force-pushed, squashing the unit tests and the code suggestions.

I don't think we could improve the PR even further :)

kenjis · 2021-11-23T02:12:09Z

Coverage is perfect! 👍

$ ./phpunit tests/system/Database/BaseQueryTest.php --coverage-html=tests/coverage/ --path-coverage -d memory_limit=1024m

paulbalandan

I think this is good for me. Thanks for the tests!

vlakoff · 2021-11-23T17:08:27Z

I just thought of a possible regression with this change:

- $binds = is_array($this->binds) ? $this->binds : [$this->binds];
+ $binds = (array) $this->binds;

If the provided binds are not an array or a scalar, but a "stringable object" (for example Laravel's Str). Here is how it is casted:

$value = Str::of('foobar');

// var_dump( $value );
object(Illuminate\Support\Stringable) {
  ["value":protected] => string "foobar"
}

// var_dump( (string) $value );
string "foobar"

// var_dump( (array) $value );
array {
  ["\0*\0value"] => string "foobar"
}

Therefore, the following code would now fail:

$value = Str::of('foobar');

$sql = 'SELECT * FROM posts WHERE title = ?';
$db->query($sql, $value);

kenjis · 2021-11-23T23:03:18Z

@vlakoff It is for all objects.
https://www.php.net/manual/en/language.types.array.php#language.types.array.casting

By the way, is there any possiblity that $this->binds is not an array?

vlakoff · 2021-11-24T01:47:02Z

It is for all objects.

I know it is for all objects, but passing a "stringable object" instead of a string seems to be a valid and possible use case.

By the way, is there any possiblity that $this->binds is not an array?

In method setQuery() we also have if (! is_array($binds)) { $binds = [$binds]; }.
In method setBinds() the parameter $binds is array typed.

So $this->binds should be an array indeed. But I don't know, maybe the user would extend the class and assign to $this->binds in a new method.

Anyway, $this->binds is not expected to be assigned anything else than an array. Doing otherwise should be considered as an error.

kenjis · 2021-11-24T01:57:44Z

setQuery()'s $binds seems to be int|string|array.

And I think the code $binds = (array) $this->binds; is not needed.

As discussed on GitHub PR codeigniter4#5138, $this->binds is not expected to be something else than an array.

vlakoff · 2021-11-24T03:57:52Z

Opened a PR for this: #5379.

This was referenced Sep 26, 2021

Fix query binding with two colons in query #5117

Merged

Bug: Query binding fails when date_format is set with '%H:%i:%s' #5114

Closed

vlakoff force-pushed the db-query-2 branch 2 times, most recently from 4ef0cd9 to eb05138 Compare September 26, 2021 08:31

vlakoff force-pushed the db-query-2 branch from eb05138 to b352200 Compare September 26, 2021 09:07

vlakoff force-pushed the db-query-2 branch 3 times, most recently from e474e6a to 4aa7109 Compare September 26, 2021 09:44

kenjis added the database Issues or pull requests that affect the database layer label Sep 26, 2021

kenjis reviewed Sep 27, 2021

View reviewed changes

vlakoff force-pushed the db-query-2 branch 2 times, most recently from a5897b0 to e77de3e Compare October 8, 2021 09:10

vlakoff mentioned this pull request Oct 27, 2021

Do not inappropriately register bind when the value is a closure #5247

Merged

kenjis added the refactor Pull requests that refactor code label Nov 9, 2021

vlakoff marked this pull request as ready for review November 16, 2021 01:06

vlakoff force-pushed the db-query-2 branch from a416cde to e77de3e Compare November 16, 2021 01:10

vlakoff force-pushed the db-query-2 branch from e77de3e to ecdf459 Compare November 16, 2021 11:23

kenjis approved these changes Nov 16, 2021

View reviewed changes

vlakoff force-pushed the db-query-2 branch 3 times, most recently from 91e28ab to 52d3a32 Compare November 17, 2021 09:45

lonnieezell approved these changes Nov 21, 2021

View reviewed changes

paulbalandan reviewed Nov 21, 2021

View reviewed changes

vlakoff added 6 commits November 23, 2021 02:51

Determine if binds are simple or named by looking at the $binds array

09147bb

Remove tests that are now superfluous

a8e549b

* Simple binds: matchSimpleBinds() also handles the bind marker, and more elaborately: handles enclosing quotes, checks the number of markers… * Named binds: there just won't be replacements for matchNamedBinds() to make.

Remove another superfluous test

d1a9738

It's very unlikely that the bindMarker property would be empty.

Some refinement / optimization

211f713

Considering that $binds is usually an array, and that only matchSimpleBinds() needs the count.

Make the test "array is a list" a bit more robust

2a08bc7

We are looking for an integer, excluding other numeric values (decimals, etc). As a reminder: for array keys, PHP automatically casts strings representing integers to regular integers.

Use array_key_first()

9d92d56

Same results, but simpler and faster code. Note array_key_first() is available on PHP 7.3+, which is thankfully the minimum PHP version required by CodeIgniter.

vlakoff force-pushed the db-query-2 branch from d7ad8ae to 9d92d56 Compare November 23, 2021 02:04

kenjis approved these changes Nov 23, 2021

View reviewed changes

paulbalandan approved these changes Nov 23, 2021

View reviewed changes

kenjis merged commit 445cb65 into codeigniter4:develop Nov 23, 2021

vlakoff added a commit to vlakoff/CodeIgniter4 that referenced this pull request Nov 24, 2021

Remove unneeded cast to array

d22c6f1

As discussed on GitHub PR codeigniter4#5138, $this->binds is not expected to be something else than an array.

vlakoff mentioned this pull request Nov 24, 2021

Remove unneeded cast to array #5379

Merged

vlakoff deleted the db-query-2 branch November 24, 2021 16:14

	$binds = is_array($this->binds) ? $this->binds : [$this->binds];
	$binds = (array) $this->binds;

Determine if binds are simple or named by looking at the $binds array #5138

Determine if binds are simple or named by looking at the $binds array #5138

Uh oh!

Conversation

vlakoff commented Sep 26, 2021

Uh oh!

kenjis commented Sep 26, 2021

Uh oh!

vlakoff commented Sep 26, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vlakoff commented Sep 27, 2021

Uh oh!

kenjis commented Sep 27, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vlakoff Oct 6, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kenjis commented Sep 27, 2021

Uh oh!

vlakoff commented Sep 27, 2021

Uh oh!

vlakoff commented Sep 27, 2021

Uh oh!

vlakoff commented Oct 27, 2021

Uh oh!

vlakoff commented Nov 16, 2021

Uh oh!

vlakoff commented Nov 16, 2021

Uh oh!

kenjis commented Nov 16, 2021

Uh oh!

vlakoff commented Nov 16, 2021

Uh oh!

kenjis left a comment

Choose a reason for hiding this comment

Uh oh!

vlakoff commented Nov 16, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lonnieezell left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vlakoff Nov 21, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

paulbalandan left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vlakoff commented Nov 21, 2021

Uh oh!

vlakoff commented Nov 22, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vlakoff commented Nov 23, 2021

Uh oh!

kenjis commented Nov 23, 2021

Uh oh!

paulbalandan left a comment

Choose a reason for hiding this comment

Uh oh!

vlakoff commented Sep 26, 2021 •

edited

Loading

kenjis commented Sep 27, 2021 •

edited

Loading

vlakoff Oct 6, 2021 •

edited

Loading

vlakoff commented Nov 16, 2021 •

edited

Loading

vlakoff Nov 21, 2021 •

edited

Loading

vlakoff commented Nov 22, 2021 •

edited

Loading