feat: Add incrementMany() and decrementMany() methods to BaseBuilder and other driver specific builders#10140
feat: Add incrementMany() and decrementMany() methods to BaseBuilder and other driver specific builders#10140patel-vansh wants to merge 12 commits intocodeigniter4:4.8from
incrementMany() and decrementMany() methods to BaseBuilder and other driver specific builders#10140Conversation
paulbalandan
left a comment
There was a problem hiding this comment.
Have you considered adding a separate incrementAll (or other wording) that will take care of the array, so to minimize the BC break?
PS. The random test failure seems legitimate. I'll deal with that later.
|
I also think that |
|
Yeah, that would avoid BC. I guess Maybe the current increment method calls incrementAll internally? So, there's no duplication of any logic. |
I see why not. |
incrementAll() and decrementAll() methods to BaseBuilder and other driver specific builders
|
Not sure why PHPStan analysis have failed. |
|
You apparently fixed (or renamed) the affected code. Just run |
michalsn
left a comment
There was a problem hiding this comment.
Supporting a more convenient API would be nice:
incrementAll(['one', 'two'], 2)
incrementAll(['one' => 2, 'two' => 3])What do you think?
Something like:
public function incrementAll(array $columns, int $value = 1): bool
{
$fields = [];
if (array_is_list($columns)) {
foreach ($columns as $column) {
$column = $this->db->protectIdentifiers($column);
$fields[$column] = "{$column} + {$value}";
}
} else {
foreach ($columns as $column => $value) {
if (! is_int($value)) {
throw new TypeError(...);
}
$column = $this->db->protectIdentifiers($column);
$fields[$column] = "{$column} + {$value}";
}
}
// ...
}|
|
|
I chose
|
incrementAll() and decrementAll() methods to BaseBuilder and other driver specific buildersincrementMany() and decrementMany() methods to BaseBuilder and other driver specific builders
| Query Builder | ||
| ------------- | ||
|
|
||
| - Added new ``incrementMany()`` and ``decrementMany()`` methods to ``CodeIgniter\Database\BaseBuilder`` for performing bulk increment/decrement operations. |
There was a problem hiding this comment.
You have removed the public methods for SQLSRV and Postgre. I think it needs to be displayed in the BC section. I can't write a suggestion well, let them correct me.
The Postgre::increment() method ... has been removed. Added new incrementMany() instead.
There was a problem hiding this comment.
increment() and decrement() are still public through inheritance from BaseBuilder. People overriding those methods in their own subclass can still do so. People calling parent::increment() from a subclass still get working behavior.
| $fields = []; | ||
|
|
||
| foreach ($columns as $col => $val) { | ||
| if (! is_int($val)) { |
There was a problem hiding this comment.
You forgot to test negative values. The is_int() check will work correctly and the data will be changed incorrectly: field + -1. It is necessary to prohibit these values.
The comment applies to all new methods
There was a problem hiding this comment.
Actually, negative values were allowed in earlier behaviour. increment() method allowed negative values which would actually subtract from the field instead of adding. So, I tried to retain that behaviour. If everyone agrees to change that, I would happily change this to disallow negative values in both incrementMany() and decrementMany() method
There was a problem hiding this comment.
I see no problem with allowing negative values. This may be an actively used feature.
|
I was looking at the PostgreSQL Builder's failing test cases and found that the current implementation ( So, here's my suggestion. Instead of |
sounds reasonable to me. I do not see BC break here. |
…ns as input Co-authored-by: Copilot <copilot@github.com>
Co-authored-by: Copilot <copilot@github.com>
d1c5747 to
3127b88
Compare
Description
This PR adds two new functions in
BaseBuilder.php—incrementMany()anddecrementMany(). These two functions enable devs to increment/decrement multiple rows at once atomically.Also, the
increment()anddecrement()methods now internally callincrementMany()anddecrementMany()respectively to prevent duplicating the logic.Checklist: