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

Remove deprecated passing string to define callback #27608

Conversation

@kamipo
Copy link
Member

@kamipo kamipo commented Jan 8, 2017

And raise ArgumentError when passing string to define callback.

@kamipo kamipo added the activesupport label Jan 8, 2017
Passing string to define callback is deprecated and will be removed
in Rails 5.1 without replacement.
raise ArgumentError, <<-MSG.squish
Passing string to define callback is not supported. Use proc instead.
MSG
end

This comment has been minimized.

@kaspth

kaspth Jan 8, 2017
Member

I'm not sure we should add this defensive coding. Presumably people have seen the deprecation warnings and acted on them.

This comment has been minimized.

@kamipo

kamipo Jan 8, 2017
Author Member

We still have string eval code for if and unless conditional options. Therefore passing string to define callback works again unless raising an error.

https://github.com/rails/rails/blob/v5.0.1/activesupport/test/callbacks_test.rb#L197-L201

This comment has been minimized.

@kaspth

kaspth Jan 15, 2017
Member

@rafaelfranca @matthewd is the original deprecation warning obtuse enough that we could construe it to mean …also don't use strings in :if and :unless?

This comment has been minimized.

@rafaelfranca

rafaelfranca Jan 18, 2017
Member

In that case we will have to deprecate it now since we didn't before. @kamipo could you also deprecate it in a new commit in this PR?

This comment has been minimized.

@kamipo

kamipo Jan 18, 2017
Author Member

Sure, will do.

@kaspth
Copy link
Member

@kaspth kaspth commented Jan 8, 2017

@rafaelfranca since you removed most of the deprecations. r? @rafaelfranca

@kamipo kamipo force-pushed the kamipo:remove_deprecated_passing_string_to_define_callback branch Jan 16, 2017
@rafaelfranca
Copy link
Member

@rafaelfranca rafaelfranca commented Jan 18, 2017

I should not merge this PR since deprecations removal should only be done by the release manager, but since this was not an easy one I'll merge it. But please don't open new PRs removing deprecation, better to open issues pointing ones that we forgot. I was aware of this one and was going to remove it with the return false callback deprecation.

@kamipo kamipo force-pushed the kamipo:remove_deprecated_passing_string_to_define_callback branch Jan 18, 2017
@kamipo
Copy link
Member Author

@kamipo kamipo commented Jan 18, 2017

Okay. BTW we still have another deprecation for deprecated_false_terminator. Do we remove the deprecation for Rails 5.1?

def display_deprecation_warning_for_false_terminator
ActiveSupport::Deprecation.warn(<<-MSG.squish)
Returning `false` in Active Record and Active Model callbacks will not implicitly halt a callback chain in Rails 5.1.
To explicitly halt the callback chain, please use `throw :abort` instead.
MSG
end

          def display_deprecation_warning_for_false_terminator
            ActiveSupport::Deprecation.warn(<<-MSG.squish)
              Returning `false` in Active Record and Active Model callbacks will not implicitly halt a callback chain in Rails 5.1.
              To explicitly halt the callback chain, please use `throw :abort` instead.
            MSG
          end
@rafaelfranca
Copy link
Member

@rafaelfranca rafaelfranca commented Jan 18, 2017

@kamipo kamipo force-pushed the kamipo:remove_deprecated_passing_string_to_define_callback branch Jan 18, 2017
@kamipo
Copy link
Member Author

@kamipo kamipo commented Jan 19, 2017

I added the deprecation for :if and :unless options.

kamipo added 2 commits Jan 8, 2017
And raise `ArgumentError` when passing string to define callback.
…n `set_callback` and `skip_callback`
@kamipo kamipo force-pushed the kamipo:remove_deprecated_passing_string_to_define_callback branch to 0952552 Feb 4, 2017
@rafaelfranca rafaelfranca merged commit d7bbe07 into rails:master Feb 7, 2017
1 check passed
1 check passed
codeclimate no new or fixed issues
Details
@kamipo kamipo deleted the kamipo:remove_deprecated_passing_string_to_define_callback branch Feb 7, 2017
y-yagi added a commit to y-yagi/rails that referenced this pull request Feb 18, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants