Do not inappropriately register bind when the value is a closure #5247
Conversation
|
Unless someone figures out realistic unit tests for vlakoff@223ca28 and vlakoff@3e8903a, this PR should be ready. |
|
Also, this change didn't make any test to fail, which points out this precise part of the code isn't covered by unit test. Probably only the string which is normalized afterwards is tested. I'm not planning to write these unit tests, but it someone wants to do so, be my guest :-) |
|
Actually, there is not a single unit test for "whereHaving" currently, contrarily to "whereIn". Maybe someone could be interested in writing these missing unit tests. |
|
Unit testing tests the object behavior, not in the code, not the specific method. |
|
According to line coverage, there are two lines 682,683 in
|
|
The branch coverage for
|
|
@vlakoff Adding this test gets 100% line coverage. public function testWhereAssociateArrayKeyHasEqualValueIsNull()
{
$builder = $this->db->table('user');
$where = [
'id <' => 100,
'col1 =' => null,
];
$builder->where($where);
$expectedSQL = 'SELECT * FROM "user" WHERE "id" < 100 AND "col1" IS NULL';
$this->assertSame($expectedSQL, str_replace("\n", ' ', $builder->getCompiledSelect()));
} |
|
This PR is very good. |
vlakoff
force-pushed
the
db-whereHaving
branch
2 times, most recently
from
172e044 to
23d0522
Compare
|
I have added this unit test. I made some changes starting from the code you provided:
|
|
I tried to think of a use case to test vlakoff/CodeIgniter4@4f702d9, but couldn't come up with one. |
|
Okay, line coverage 100%, Branch coverage 93.75%.
|
Conceptually cleaner, and will be helpful for the next commit.
Such closure just has nothing to do in the $binds array. The code currently works because the binds compiler determines the binds to apply not by looking at the $binds array, but by looking at the query string, so it doesn't reach the closure. If we modify the binds compiler to instead look at the $binds array (which makes more sense), it would attempt to replace this registered named bind in the query string, and trying to replace with a closure would trigger a "cannot cast to string" error.
We can simply merge these two blocks.
Exact same effect: this complicated code was for replacing the last occurrence (there is no built-in function for this), and there is an englobing condition to ensure we replace the substring $op only if it is at the very end of $k. As a bonus, previous code was missing a preg_quote(), but we no longer need it.
vlakoff
force-pushed
the
db-whereHaving
branch
from
77dcdec to
211d405
Compare
If getOperator() found an $op, but it is not at the very end of $k, it does not get removed from $k. Previous code was nevertheless appending $op, so it got repeated a second time in the result, which is obviously wrong.
This space isn't mandatory, and the query string gets normalized later, adding this space, but let's make it cleanly in the first place.
strtr() is faster for single-character replacements.
For this code path: "elseif (preg_match('/\s*(!?=|<>|IS(?:\s+NOT)?)\s*$/i', $k ..."
The above code path "elseif (! $this->hasOperator($k) ..." is covered in testOrWhereInClosure() and testOrWhereNotInClosure().
vlakoff
force-pushed
the
db-whereHaving
branch
from
211d405 to
68ad987
Compare
|
Force-pushed, integrating the small code style changes from reviews. |
kenjis
added
database
|
@vlakoff Thank you! |
Prerequisite for #5138.
The main fix is commit vlakoff@223ca28.