Skip to content
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.

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

Model::delete method #865

Closed
daylightstudio opened this issue Dec 14, 2017 · 15 comments
Closed

Model::delete method #865

daylightstudio opened this issue Dec 14, 2017 · 15 comments

Comments

@daylightstudio
Copy link
Contributor

I'm curious why the Model::delete method is setup to only take an $id and and then there is a separate Model::deleteWhere? If Model::delete was more generic I think it could accommodate more conditions. As it stands now, the Model class needs to be extended to create your own "deleteOrWhere" and "deleteWhereIn" methods.

Thanks for all your hard work!

@lonnieezell
Copy link
Member

That's a fair point. Most of came simply from a base model I had used for years. I would create multiple small methods to handle specialized cases in each model so it was more readable, so the delete and deleteWhere were all that I needed.

With the current model, though, you can access all of the Builder methods in your method chain, as well as the model methods, so it might make more sense to remove deleteWhere and rename delete to deleteOne or something, then allow the builder's normal delete method to be used. But, then that's not consistent with what find and update do, so not sure.... will have to think on that a bit.

@timothymarois
Copy link
Contributor

I think the delete should act like update/where clauses. I've been using the models myself and find it confusing why I can't do delete(['name'=>'thisname']) , Should delete whatever equals that query. That's consistent with the update use, except you don't need to specify a second array/argument. That should be an easy fix, looking first to see if the user put an ID (primary key), or an array of IDs, and then next check if the user puts an array for where clause. This way the delete is used for how everyone would intend it. That's how I would assume it works given the update method.

@lonnieezell
Copy link
Member

@timothymarois I think delete is closer to find/findWhere in usage, honestly. update always requires a single id passed to it.

Looking back over the model methods, it might make sense to make these changes:

  • delete should take either one id, or an array of ids. That is consistent with find.
  • delete should also work like the Query Builder version when no parameters are passed in. This allows for $model->whereIn('status', $statuses)->delete(); flow without the need for a bunch of new custom methods.

I think it would be tricky to reliably determine between an array of ids (which might be strings, also) and would stay consistent with the find/findWhere combo, which has a closer relation to delete than update does, I think.

I'm tempted to say we get rid of findWhere and deleteWhere and allow both find and delete to work. Well - that doesn't quite make sense, actually. The find method is simply a helper method for limit->get.

@timothymarois
Copy link
Contributor

@lonnieezell Yeah You're right, I used a version before which reminded me I had update(array, array) but in the current state of CI4 it's only update(id, array).

As for the findWhere I think it's useless because I always use where()->findAll(), findWhere to me doesn't make much sense. (unless there are other uses which I'm unaware of), I just like the simplicity of stating intention. I know findWhere is merely a helper function to where and get, but really should use the query methods instead. Plus multiple ways of doing the same thing can get confusing in the docs for which way it better than another, should be one straightforward way.

As for delete, I really am for a delete method being used for batch deleting based on where clause. Honestly, this is how I believe update and delete should work for simplicity sake.

// simple update
$db->update($id, $array);
// batch id update
$db->update([1,2,3], $array);
// update based on where (as the first argument states which items it needs to update)
$db->update(['status'=>'pending'], ['status'=>'active']);

// simple delete
$db->delete($id);
// batch id delete
$db->delete([1,2,3]);
// delete based on where (as the argument states which items it needs to delete)
$db->delete(['status'=>'pending']);

These are my thought as to how I think they should work. I know most likely it's too late to really rethink how the model works. But I've always viewed it in this way because it makes the most sense and my intentions for using them. But you can correct me if there are some flaws in that.

@lonnieezell
Copy link
Member

We're not even to a beta-release, yet, so it's not too late :)

I like the simplification of what you've presented. I'm a little concerned that things get confused here. That one method now allows 3 ways to do things. The first two of them make sense and are close in functionality, but adding the third method muddies the usage. While it's simpler in some aspects, doesn't it make it a little less explicit about what it's doing? It's a bit more clear that updateWhere is providing a set of where conditions to find the rows by.

I realize I'm might be getting a little far in the weeds thinking about this one, so I'm definitely interested in other opinions on this. @daylightstudio @jim-parry what's your thoughts?

@jim-parry
Copy link
Contributor

I could be misunderstanding PHP, but aren't
// batch id update
$db->update([1,2,3], $array);
// update based on where (as the first argument states which items it needs to update)
$db->update(['status'=>'pending'], ['status'=>'active']);

both cases of update($array,$array)? How would you tell them apart?

I can see enhancing the existing delete to accept an array of ids.

In the conversation earlier, there was a suggestion of method chaining, eg ->where(...)->delete(). This makes most sense to me, with similar changes to update(). In other words, enhance delete() and update() to work on the current "selection".

@lonnieezell
Copy link
Member

@jim-parry Well, they are 2 different forms of arrays so they could technically be checked for but that seems like it might be easy to miss some edge cases. One form would always have numeric keys while the other may/may not. Seems a bit fragile to me.

@daylightstudio
Copy link
Contributor Author

My opinion on the deleteWhere and the findWhere is that although convenient, I'd expect there to also be deleteOrWhere, findOrWhere... etc.

@jim-parry
Copy link
Contributor

There is no end to it :-/
I would opt on the side if simplicity.

@timothymarois
Copy link
Contributor

timothymarois commented Dec 16, 2017

@jim-parry @lonnieezell I do agree that chaining a where() to delete() would make things convenient and simple in action/clarity. I would be for that since its very self-explanatory. where method should be chained more so than using a findWhere or deleteWhere because the more methods you introduce would mean more documentation and the explanation of what each does and their arguments. But as it stands, I do think we should simplify in some way the current way of doing it cause it can be a bit inconsistent. I find myself referring to the documentation multiple times because there are multiple ways of doing queries in the model. Not sure if this is all getting off topic from the original, but that's my opinions (and after using CI4 in major projects already).

You should be able to do

 $db->where(['status'=>'pending')->delete();

@lonnieezell
Copy link
Member

I think we have all come to more or less the same decision. Sounds good. So, to sum up:

  • remove findWhere, deleteWhere
  • delete should handle a single ID, and array of IDs or nothing. If nothing, will act as the query builder's delete method
  • find should work with a single ID, an array of IDs or nothing. If nothing, will simply be an alias for get().
  • to maintain consistency, update should work as it does, with an ID and an array, or just an array. If just an array it is expected to be the elements to update and would required a where call prior.

@jim-parry
Copy link
Contributor

+1

@timothymarois
Copy link
Contributor

@lonnieezell Agreed. I like the direction.

@timothymarois
Copy link
Contributor

@lonnieezell Also let me know if you need any help, I know we both have been working a lot these days. If I can get my development env set back up for CI, I might be willing to make a few of these changes if you think it will be a while before you can get to them.

@lonnieezell
Copy link
Member

If you’ve got the time I’d say go for it. Knee deep in an outside of work project currently so time is definitely hard to come by for bigger changes right now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants