Updated the CI model for improved/consistent delete and update use.

[timothymarois]

Sorry, this took a while, but here is the tested version of the new model updates for update() and delete() methods.

Usage:

// UPDATES

// update single item by id
$model->update(string $id, array $data);

// update multiple items by array of ids
$model->update(array $id_array, array $data);

// update multiple items by using where()
$model->where(['tag'=>'php'])->update(array $data);


// DELETES:

// delete a single item
$model->delete(string $id);

// delete multiple items
$model->delete(array $id_array);

// delete multiple items by using where
$model->where(['tag'=>'php'])->delete();

Removed Methods:
findWhere()
deleteWhere()

Referencing:
#865

Any feedback is greatly appreciated. I've tested these myself, and haven't gone into the phpunit test yet due to the mess it currently is. (initial post for this update)

[lonnieezell]

I'd prefer to throw an exception here. It seems like any time this would happen would be something the developer should have caught so we should fail loudly so they have a chance to fix it. Just returning false here would make the bug harder to find and easier to slip through into production.

[timothymarois]

Yeah, that's a good idea, otherwise, it's a silent death with no reason why.

[lonnieezell]

The more I think about the polymorphic aspects of our parameters the less I like it. I know we all had a discussion about this previously and decided to go this route, but splitting it into two methods would be more clear. Maybe updateWith($data) and updateById($id, $data) or something?

What are your thoughts?

[timothymarois]

I do agree it's not the best solution, I'd really like it to be one method for simplicity sake. However, if we think it's doing too much, we could branch it out. I might be in favor of doing something like updateWith since the consistency with delete uses id as its argument. Though we might want to make it (sort of back to the original idea of) updateWhere and deleteWhere keeping the update/delete methods for only primary keys use.

[lonnieezell]

What makes you say the ModelTest is messy?

[timothymarois]

@lonnieezell Sorry I meant messy in a broad sense not specifically to the ModelTest, I have to rework my local environment.

[timothymarois]

@lonnieezell Once I do that, then I'll put up the proper tests and we can work to merge this in.

[jim-parry]

Being picky, but we expect commits to be GPG-signed :)

[natanfelles]

Why to remove the beforeDelete trigger?

[lonnieezell]

That is not intentional, and it should be left in there.

[natanfelles]

PR Usage:

// update multiple items by array of ids 
$model->update(array $id_array, array $data);

Proposed changes

What do you think about to work like the Query Builder updateBatch() in the Model update():

$data = array(
   array(
      'id'    => '1' ,
      'title' => 'My title' ,
      'name'  => 'My Name 2' ,
      'date'  => 'My date 2'
   ),
   array(
      'id'    => '2' ,
      'title' => 'Another title' ,
      'name'  => 'Another Name 2' ,
      'date'  => 'Another date 2'
   )
);

// Query Builder way
$builder->updateBatch($data, 'id');

// Model way
$model->update($data);

Then, would be necessary to check if the first param is array.

If is not array, then is the primaryKey: $model->update($id, $data). This works the same way it is currently.

If is array, we have two use cases:

1. The user was set a where: $model->where($where)->update($data)
2. The user wants an updateBatch.

To know if he wants a updateBatch, we could to check the value of the first key of $data.

If it is an array, then call $builder->updateBatch($data, $this->primaryKey);, otherwise call a single update as is done currently, but not using the primaryKey, because of the where().

Method

The method would be like:

/*
 * @param int|string|array $id Primary Key or an array of update values.
 *                             If is an associative array will be called an update batch.
 * @param array|object     $data
 *
 * @return bool|int True on success, false on fail
 *                  or the number of affected rows as integer if using update batch
 */
public function update($id, $data = null)

Usage:

1. Where with Primary Key in the method:

$model->update(1, ['color' => 'blue']);

2. Where out of the method and 'color' (the first key) is not an array:

$model->where(['id' => 1])->update(['color' => 'blue']);

3. Where with Primary Key in the method, because 0 (the first key) is an array. Then the child array must have the $this->primaryKey ('id' in the example):

$model->update([
    ['id' => 1, 'color' => 'blue'],
    ['id' => 2, 'color' => 'red'],
    ['id' => 3, 'color' => 'yellow'],
]);

* Something similar could be implemented for the $model->insert(), maybe.

[lonnieezell]

@natanfelles I can't go with that. Too many overriding uses of the same method. We're already pushing it on these, honestly. :)

[natanfelles]

@lonnieezell Ok. Thanks! I just looked at the PR and thought I could help with a guess. 👍

[natanfelles]

Sorry for the urge, but I still think this $id_array is not very necessary:

$model->update([1, 2, 3], ['color' => 'blue']);
// UPDATE `{$this->table}` SET `color` = 'blue' WHERE `id` IN ('1', '2', '3')

Does the same that the current PR update multiple and my proposed usage 2:

$model->whereIn('id', [1, 2, 3])->update(['color' => 'blue']);
// UPDATE `{$this->table}` SET `color` = 'blue' WHERE `id` IN ('1', '2', '3')

Then let the method pick up the $primaryKey automatically and enable to update multiple different values at once, with a single query:

$model->update([
    ['id' => 1, 'color' => ''],
    ['id' => 2, 'color' => 'red'],
    ['id' => 3, 'color' => 'yellow'],
]);
/*
UPDATE `{$this->table}` SET `color` = CASE
WHEN `id` = '1' THEN ''
WHEN `id` = '2' THEN 'red'
WHEN `id` = '3' THEN 'yellow'
ELSE `color` END
WHERE `id` IN ('1','2','3')
*/

I returned to this because I was inserting/updating several items in a single query, through the *Batch() methods, and I saw that the validation did not run. So I came back to find this concept more interesting, because we can already do the validation array by array and stop the process if necessary.

* Array or Object

---

But if it is still too many overriding uses of the method, what will be done in relation to the *Batch() methods? I believe that each item should be validated in the Model, since the validation is already prepared in it.

The Color 1 field is required.

[lonnieezell]

Changes never pushed. I'm pushing these changes in a separate commit later tonight.