Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with
or
.
Download ZIP

Loading…

skip_before_filter strange behavior when using if and only together #9703

Closed
ginter opened this Issue · 6 comments

5 participants

@ginter

I'm running into an issue when using skip_before_filter with both the "if" and "only" options.

before_filter :test
skip_before_filter :test, only: [:show], if: -> { false }

When running the above, I expect "test" to be called when hitting the show action, but this isn't the case.

before_filter :test
skip_before_filter :test, only: [:random], if: -> { false }

When running the above, I expect "test" to be called regardless of the conditional when hitting the show action, but this isn't the case. It is called in this case when the conditional evaluates to false but it is not called if the conditional evaluates to true.

@senny
Owner

I agree that this seems strange. I'll take a look and report back. Please format your code using ``` (I edited your message).

@imanel

The problem is that both only and if are treated separately - if either of them will be fulfilled then skip will be applied. So in case of code:

before_filter :test
skip_before_filter :test, only: [:show], if: -> { false }

it is treated as code:

before_filter :test
skip_before_filter :test, only: [:show]
skip_before_filter :test, if: -> { false }

Unfortunately documentation is not explaining that so it required deeper testing to find that out. To confirm that you need to check output of ActiveSupport::Callbacks.skip_callback.

@ginter

Is that intended behavior or would it be worth trying to fix/change?

@korny

The problem is that skip_before_filter behaves differently than before_filter in this regard. This just confused a coworker and me for almost an hour, and we're not the only ones. It should be clearly documented as a strange behavior, because I guess you can't just change/fix it now that people are depending on the way it used to work…

Since skip_before_filter is used a lot for security requirements, the unexpected behavior can lead to very ugly bugs; thinking of:

skip_before_filter :login_required, only: :show, if: :trusted_origin?

:scream:

@korny

By the way, the easiest way to work around this seems to be to repeat the before_filter with the inverse condition after the skipping.

Wrong:

before_filter :test
skip_before_filter :test, only: [:show], if: :skip_test?

Correct:

before_filter :test
skip_before_filter :test, only: [:show]
before_filter      :test, only: [:show], unless: :skip_test?
@rafaelfranca
Owner

Closed by #18404

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.