Skip to content

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

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.

Sign up for GitHub

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

Closed
wants to merge 1 commit into from
Closed

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

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
159 changes: 68 additions & 91 deletions system/Model.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -334,37 +334,6 @@ public function find($id)
return $row['data'];
}

//--------------------------------------------------------------------

/**
* Extract a subset of data
*
* @param string|array $key
* @param string|null $value
*
* @return array|null The rows of data.
*/
public function findWhere($key, $value = null)
{
$builder = $this->builder();

if ($this->tempUseSoftDeletes === true)
{
$builder->where($this->deletedField, 0);
}

$rows = $builder->where($key, $value)
->get();

$rows = $rows->getResult($this->tempReturnType);

$rows = $this->trigger('afterFind', ['data' => $rows]);

$this->tempReturnType = $this->returnType;
$this->tempUseSoftDeletes = $this->useSoftDeletes;

return $rows['data'];
}

//--------------------------------------------------------------------

Expand Down Expand Up @@ -612,13 +581,34 @@ public function insert($data, bool $returnID = true)
* Updates a single record in $this->table. If an object is provided,
* it will attempt to convert it into an array.
*
* updates a single item by id
* $model->update(id, data)
*
* updates multiple items by array of ids
* $model->update(array_id, data)
*
* @param int|string $id
* @param array|object $data
*
* @return bool
*/
public function update($id, $data)
public function update($id, $data = null)
{
// because we're going to use "where" for the items to updated
// id became a surrogate for data argument
// not ideal, but lets see where this goes...
if ($data === null)
Copy link
Member

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

@timothymarois timothymarois Jan 30, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

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.

{
$data = $id;
$id = null;
}

// you can not proceed.
if ($id === null && $data === null) return false;
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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


// convert id to array if not already
if ($id !== null && !is_array($id)) $id = [$id];

// If $data is using a custom class with public or protected
// properties representing the table elements, we need to grab
// them as an array.
Expand Down Expand Up @@ -666,10 +656,21 @@ public function update($id, $data)
}

// Must use the set() method to ensure objects get converted to arrays
$result = $this->builder()
->where($this->primaryKey, $id)
->set($data['data'])
->update();

if ($id === null)
{
$result = $this->builder()
->set($data['data'])
->update();
}
else
{
$result = $this->builder()
->whereIn($this->primaryKey, $id)
->set($data['data'])
->update();
}


$this->trigger('afterUpdate', ['id' => $id, 'data' => $originalData, 'result' => $result]);

Expand All @@ -679,18 +680,27 @@ public function update($id, $data)
//--------------------------------------------------------------------

/**
* Deletes a single record from $this->table where $id matches
* Deletes a single or many records from $this->table where $id matches
* the table's primaryKey
*
* @param mixed $id The rows primary key
* deletes multiple items by a where clause
* $model->where(array)->delete()
*
* deletes multiple items by ids
* $model->delete(ids)
*
* deletes a single item by id
* $model->delete(id)
*
* @param mixed $id The rows primary key (values)
* @param bool $purge Allows overriding the soft deletes setting.
*
* @return mixed
* @throws \CodeIgniter\Database\Exceptions\DatabaseException
*/
public function delete($id, $purge = false)
public function delete($id = null, $purge = false)
{
$this->trigger('beforeDelete', ['id' => $id, 'purge' => $purge]);
if ($id !== null && !is_array($id)) $id = [$id];
Copy link
Contributor

Choose a reason for hiding this comment

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

Why to remove the beforeDelete trigger?

Copy link
Member

Choose a reason for hiding this comment

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

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


if ($this->useSoftDeletes && ! $purge)
{
Expand All @@ -701,69 +711,36 @@ public function delete($id, $purge = false)
$set[$this->updatedField] = $this->setDate();
}

$result = $this->builder()
->where($this->primaryKey, $id)
if ($id)
{
$result = $this->builder()
->whereIn($this->primaryKey, $id)
->update($set);
}
else
{
$result = $this->builder()->update($set);
}
}
else
{
$result = $this->builder()
->where($this->primaryKey, $id)
if ($id)
{
$result = $this->builder()
->whereIn($this->primaryKey, $id)
->delete();
}
else
{
$result = $this->builder()->delete();
}
}

$this->trigger('afterDelete', ['id' => $id, 'purge' => $purge, 'result' => $result, 'data' => null]);

return $result;
}

//--------------------------------------------------------------------

/**
* Deletes multiple records from $this->table where the specified
* key/value matches.
*
* @param string|array $key
* @param string|null $value
* @param bool $purge Allows overriding the soft deletes setting.
*
* @return mixed
* @throws \CodeIgniter\Database\Exceptions\DatabaseException
*/
public function deleteWhere($key, $value = null, $purge = false)
{
// Don't let them shoot themselves in the foot...
if (empty($key))
{
throw new DatabaseException('You must provided a valid key to deleteWhere.');
}

$this->trigger('beforeDelete', ['key' => $key, 'value' => $value, 'purge' => $purge]);

if ($this->useSoftDeletes && ! $purge)
{
$set[$this->deletedField] = 1;

if ($this->useTimestamps)
{
$set[$this->updatedField] = $this->setDate();
}

$result = $this->builder()
->where($key, $value)
->update($set);
}
else
{
$result = $this->builder()
->where($key, $value)
->delete();
}

$this->trigger('afterDelete', ['key' => $key, 'value' => $value, 'purge' => $purge, 'result' => $result, 'data' => null]);

return $result;
}

//--------------------------------------------------------------------

Expand Down