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

Do not evaluate :if arguments when :on is not satisfied for transaction callbacks #28063

Merged
merged 1 commit into from Feb 28, 2017

Conversation

Projects
None yet
6 participants
@Erol
Contributor

Erol commented Feb 18, 2017

Summary

This addresses #28019 by prepending the :on action check before the supplied :if argument. The previous behavior appended :on at the end of the list when setting up a callback, allowing :if conditions to be evaluated even if the :on transaction scope is not satisfied.

class Topic < ActiveRecord::Base
  after_commit :do_something, on: [:create, :update], if: :check_something?
end

topic = Topic.create #=> Calls :check_something?
topic.save #=> Calls :check_something?
topic.destroy #=> No longer calls :check_something?
@rails-bot

This comment has been minimized.

Show comment
Hide comment
@rails-bot

rails-bot Feb 18, 2017

r? @rafaelfranca

(@rails-bot has picked a reviewer for you, use r? to override)

rails-bot commented Feb 18, 2017

r? @rafaelfranca

(@rails-bot has picked a reviewer for you, use r? to override)

@Erol Erol changed the title from Do not evaluate given `:if` arguments when the `:on` action check is not satisfied for transaction callbacks to Do not evaluate `:if` arguments when the `:on` action check is not satisfied for transaction callbacks Feb 19, 2017

@Erol Erol changed the title from Do not evaluate `:if` arguments when the `:on` action check is not satisfied for transaction callbacks to Do not evaluate :if arguments when :on is not satisfied for transaction callbacks Feb 19, 2017

@Erol

This comment has been minimized.

Show comment
Hide comment
@Erol

Erol Feb 27, 2017

Contributor

Hi @rafaelfranca, is there anything I can do to move this forward?

Contributor

Erol commented Feb 27, 2017

Hi @rafaelfranca, is there anything I can do to move this forward?

@rafaelfranca rafaelfranca requested review from jeremy and matthewd Feb 28, 2017

@rafaelfranca

This comment has been minimized.

Show comment
Hide comment
@rafaelfranca

rafaelfranca Feb 28, 2017

Member

Hmm, this one is tricky. I'm sure there are people expecting the if to evaluate before the on condition. I think this changes make sense, but I want to hear @matthewd and @jeremy opinions.

Member

rafaelfranca commented Feb 28, 2017

Hmm, this one is tricky. I'm sure there are people expecting the if to evaluate before the on condition. I think this changes make sense, but I want to hear @matthewd and @jeremy opinions.

@niborg

This comment has been minimized.

Show comment
Hide comment
@niborg

niborg Feb 28, 2017

@rafaelfranca surely people shouldn't be putting any model-changing logic in the :if statements? It should only be used to evaluate a boolean expression, so as I understand it nothing would change for those who might expect the 'if' before the 'on'. But for those who expect the 'on' before the 'if', unnecessarily evaluating the 'if' statement could raise an error. (In my case, the 'if' was being unnecessarily evaluated on destroy, when it was specified for an 'on: :update', and during the destroy lifecycle an association that would otherwise be present was not.)

niborg commented Feb 28, 2017

@rafaelfranca surely people shouldn't be putting any model-changing logic in the :if statements? It should only be used to evaluate a boolean expression, so as I understand it nothing would change for those who might expect the 'if' before the 'on'. But for those who expect the 'on' before the 'if', unnecessarily evaluating the 'if' statement could raise an error. (In my case, the 'if' was being unnecessarily evaluated on destroy, when it was specified for an 'on: :update', and during the destroy lifecycle an association that would otherwise be present was not.)

@matthewd

This comment has been minimized.

Show comment
Hide comment
@matthewd

matthewd Feb 28, 2017

Member

Yeah, I think this should be relatively safe: the if condition can't force the overall condition to true when the on doesn't match. Concur with @niborg that anyone mutating the model inside the if deserves what they get.

IOW, it is a behavioural change, but not one that seems likely to affect applications using the API in good faith.

Member

matthewd commented Feb 28, 2017

Yeah, I think this should be relatively safe: the if condition can't force the overall condition to true when the on doesn't match. Concur with @niborg that anyone mutating the model inside the if deserves what they get.

IOW, it is a behavioural change, but not one that seems likely to affect applications using the API in good faith.

@rafaelfranca

This comment has been minimized.

Show comment
Hide comment
@rafaelfranca

rafaelfranca Feb 28, 2017

Member

Seems good. It also matches the validation behavior.

Member

rafaelfranca commented Feb 28, 2017

Seems good. It also matches the validation behavior.

@rafaelfranca rafaelfranca merged commit 6a7bd6e into rails:master Feb 28, 2017

2 checks passed

codeclimate no new or fixed issues
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@Erol Erol deleted the Erol:prioritize-callback-on-action-before-if branch Mar 1, 2017

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