Don't rely on false return from callbacks to halt execution

[jrafanie]

We're already using throw :abort, so there's no code for us to change.

This setting is removed in rails 5.1 but will temporarily get rid of this
annoying warning:

DEPRECATION WARNING: 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.
(called from block (3 levels) in <top (required)> at
/home/travis/build/ManageIQ/manageiq/spec/models/miq_task_spec.rb:297)

[miq-bot]

Checked commit jrafanie@a8d88d0 with ruby 2.3.3, rubocop 0.52.1, haml-lint 0.20.0, and yamllint 1.10.0
1 file checked, 0 offenses detected
Everything looks fine. 🍰

[kbrock]

This looks good.

ok, so this is complaining that while check_active and check_associations use raise :abort for the fail cases (good future behavior), the methods also return true for the success case - which is basically a NOP.

Looks like 8 models are using raise :abort (or raise something else) in their before_* :check_* callbacks. I am not able to find any of those callbacks that are still using the legacy return false behavior.

This change (on the rails side) makes me nervous since callbacks have cared about the return value for so long.

[kbrock]

LGTM

[jrafanie]

This change (on the rails side) makes me nervous since callbacks have cared about the return value for so long.

Me too but like you, I didn't see anything relying on the return of true or false.