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

Why Model::update() doesn't return number of updated rows? #1127

Open
farhadi opened this issue Sep 30, 2014 · 6 comments
Open

Why Model::update() doesn't return number of updated rows? #1127

farhadi opened this issue Sep 30, 2014 · 6 comments

Comments

@farhadi
Copy link
Contributor

farhadi commented Sep 30, 2014

In most of cases after a Model::update() we need the number of updated rows. And we have to tweak the underlying resource to extract the number manually.
I have always had this question why this method shouldn't return the number of updated rows out of the box.
If there isn't any technical or logical limitation for this change, I think it's worth a BC break.

@farhadi farhadi changed the title Why model Model::update() doesn't return number of updated rows? Why Model::update() doesn't return number of updated rows? Sep 30, 2014
@mariuswilms
Copy link
Member

A few questions that allow us to decide if this feature is needed:

  • Can you describe your use case/s?
  • What is your current workaround?

@koushki
Copy link
Contributor

koushki commented Sep 30, 2014

I think even this can implement for Model::remove() similarly.

@farhadi
Copy link
Contributor Author

farhadi commented Oct 6, 2014

Use cases:

  1. in an inbox user clicks "mark all as read" and we want to show how many messages have been updated.
  2. in a store when user buys a product we should update the quantity of available products in our database. In this case to prevent race conditions I would do an update with a condition of "quantity > 0" and check affected rows.
  3. in a similar situation as above when I want to decrease user balance I would do an update with a condition of "balance > $price" and check affected rows.

I know I can use FOR UPDATE for 2&3 but I think developer should be able to choose whatever approach he wants based on situations.

Workarounds:

  1. Use a SELECT ROW_COUNT(); after update. which works only on dev branch because in master before each query there is a USE DATABASE.
  2. Override Database::update by either adding a filter or extending the class.

Possible solution:
To keep BC I suggest adding a return option, so to retrieve count we could pass an option like 'return' => 'count'
And as @koushki mentioned, this could be applied to Model::remove as well.

@farhadi
Copy link
Contributor Author

farhadi commented Oct 6, 2014

Anyway, I'm fine with a BC break, because the current returned value is almost useless. I don't know about document databases but in SQL, returned value is almost always true. maybe only in a connection/database failure we will have a false.

@mariuswilms
Copy link
Member

@farhadi Thanks for detailing use case and workaround. This will need further discussion - as that takes time - the next potential integration point would be after 1.0.

@koushki
Copy link
Contributor

koushki commented Oct 6, 2014

@DavidPersson In my opinion, if it will cause break BC it is better that it become fixed in version 1.0

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

No branches or pull requests

3 participants