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

removeAttribute creates a dirty model #152

Closed
zoe-edwards opened this issue Sep 4, 2018 · 7 comments
Closed

removeAttribute creates a dirty model #152

zoe-edwards opened this issue Sep 4, 2018 · 7 comments

Comments

@zoe-edwards
Copy link
Collaborator

zoe-edwards commented Sep 4, 2018

Describe the bug

When using removeAttribute(), followed by save(), it will save the model as it was before.

    $model = new Model;
    $model->oldThing = true;
    $model->save();

    // Later...

    $model = new Model;
    $model->newThing = true;
    $organisation->removeAttribute('oldThing');
    $model->save();

    // $model has oldThing and newThing

    // Later...

    $model = new Model;
    $organisation->removeAttribute('oldThing');
    $model->save();

    // $model has oldThing and newThing

In Eloquent, the model checks to see if it’s dirty before saving:

    // If the model already exists in the database we can just update our record
    // that is already in this database using the current IDs in this "where"
    // clause to only update this model. Otherwise, we'll just insert them.
    if ($this->exists) {
        $saved = $this->isDirty() ?
                    $this->performUpdate($query) : true;
    }

Which would solve the case where removeAttributes() is run and then a save() is performed anyway, such as on an update-type of request, but it wouldn’t solve the adding/removing at the same time problem.

Version info

  • Laravel: 5.6
  • Library: latest
@baopham
Copy link
Owner

baopham commented Sep 4, 2018

Awesome catch! A fix will come in a few days.

@baopham
Copy link
Owner

baopham commented Sep 12, 2018

So essentially after calling removeAttributes, we should expect that the model would have the fresh value (i.e. the attributes should be removed from the instance). The easiest way is to call refresh() but I don't want to do that, which means this is going to take a bit more time than I expected.

@zoe-edwards
Copy link
Collaborator Author

Thanks for replying to the issue @baopham! I’ve worked around it myself by designing the removeAttributes use to only be used as a one-off action which isn’t too bad, but will be very grateful if you solve it!

Yeah, sounds really tricky. I hadn’t thought of using refresh(), but I can see what you mean.

@Tamrael
Copy link

Tamrael commented Nov 9, 2018

So essentially after calling removeAttributes, we should expect that the model would have the fresh value (i.e. the attributes should be removed from the instance). The easiest way is to call refresh() but I don't want to do that, which means this is going to take a bit more time than I expected.

referring to this part of the dynamodb documentation you can just add --return-values ALL_NEW to the query and you will get the item as it appears in dynamodb after the removeAttribute operation. Then one would only need to reapply the previously "dirty" parts of the model to not overwrite changes. This way there won't be a need for additional queries like refresh() would issue

@baopham
Copy link
Owner

baopham commented Nov 9, 2018

This is perfect @Tamrael! Much easier then what I had in mind

@baopham
Copy link
Owner

baopham commented Nov 10, 2018

Fixed in v4.11.1

@zoe-edwards
Copy link
Collaborator Author

Just removed 7 lines of code already from one project 👌

Thank you very much!

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

3 participants