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

Rename Paranoia#destroyed? to Paranoia#paranoia_destroyed? #177

Closed
wants to merge 3 commits into from

Conversation

huoxito
Copy link
Contributor

@huoxito huoxito commented Oct 25, 2014

There was a change in ActiveRecord#update_columns on latest rails
so now it checks for destroyed? instead of persisted?. Paranoia
can't override destoyed? like this anymore otherwise
update_columns will always raise:

    ActiveRecord::ActiveRecordError: cannot update a destroyed record

see rails/rails@0f99aa6

@radar this is a breaking change so not sure how you want to handle it. Couldn't think of a fix other than renaming the method. I'm not sure why destroyed? was overridden here in the first place but if tests are missing a scenario this could break something else too. Let you know if I find anything new.

There was a change in ActiveRecord#update_columns on latest rails
so now it checks for `destroyed?` instead of `persisted?`. Paranoia
can't override `destoyed?` like this anymore otherwise
`update_columns` will always raise:

        ActiveRecord::ActiveRecordError: cannot update a destroyed record

see rails/rails@0f99aa6
@radar
Copy link
Collaborator

radar commented Oct 25, 2014

It was being overridden here so that when you soft-deleted a record, destroyed? would return true. I'm sad that were probably going to lose that behaviour. I will need to take a look at this patch closer later on.

Thanks mate :)

@jhawthorn
Copy link
Collaborator

@huoxito @JDutil I'm getting around to this, but it is a pretty major change and I want to be sure nothing else is affected by this change.

This will probably become 2.1.0, and we'd probably do well to add a warning in the next 2.0.x release.

@radar
Copy link
Collaborator

radar commented Jan 19, 2015

I agree that overriding core methods isn't the way to go and I'm fine with switching it to #paranoia_destroyed? with a deprecation in the 2.0.x releases. Please remember to update the README too.

@JDutil
Copy link

JDutil commented Jan 21, 2015

@jhawthorn anything @huoxito or I can do to move this along?

@jhawthorn jhawthorn added this to the 2.1.0 milestone Jan 22, 2015
@jhawthorn
Copy link
Collaborator

@JDutil @huoxito This has been merged into the rails4 branch, which now tracks the 2.1.x version. I've just pushed up version 2.1.0.pre. I'd love for you guys to give this a test and code review.

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

Successfully merging this pull request may close these issues.

4 participants