Skip to content

Commit

Permalink
Clarify dangers of Rails/LexicallyScopedActionFilter
Browse files Browse the repository at this point in the history
This cop encourages users to define action methods in the same class as
any before_, after_ or around_actions that refer to the action.

However, the examples given don't account for the fact that behaviour
might be defined in the superclass. Indeed, this sort of decoration is
one of the main use cases for action decorators.

If this is the case, then the empty action definitions shown in the docs
will override the desired behaviour, and lead users to introduce bugs.
This change emphasises this in the docs, and gives an example to
encourage users to think about the inheritance chain.
  • Loading branch information
urbanautomaton authored and bbatsov committed Apr 4, 2019
1 parent aefacd5 commit 1f5e9fb
Show file tree
Hide file tree
Showing 2 changed files with 60 additions and 8 deletions.
34 changes: 30 additions & 4 deletions lib/rubocop/cop/rails/lexically_scoped_action_filter.rb
Original file line number Diff line number Diff line change
Expand Up @@ -6,10 +6,13 @@ module Rails
# This cop checks that methods specified in the filter's `only` or
# `except` options are defined within the same class or module.
#
# You can technically specify methods of superclass or methods added
# by mixins on the filter, but these confuse developers. If you
# specify methods that are defined in other classes or modules, you
# should define the filter in that class or module.
# You can technically specify methods of superclass or methods added by
# mixins on the filter, but these can confuse developers. If you specify
# methods that are defined in other classes or modules, you should
# define the filter in that class or module.
#
# If you rely on behaviour defined in the superclass actions, you must
# remember to invoke `super` in the subclass actions.
#
# @example
# # bad
Expand Down Expand Up @@ -56,6 +59,29 @@ module Rails
# # something
# end
# end
#
# @example
# class ContentController < ApplicationController
# def update
# @content.update(content_attributes)
# end
# end
#
# class ArticlesController < ContentController
# before_action :load_article, only: [:update]
#
# # the cop requires this method, but it relies on behaviour defined
# # in the superclass, so needs to invoke `super`
# def update
# super
# end
#
# private
#
# def load_article
# @content = Article.find(params[:article_id])
# end
# end
class LexicallyScopedActionFilter < Cop
MSG = '%<action>s not explicitly defined on the %<type>s.'.freeze

Expand Down
34 changes: 30 additions & 4 deletions manual/cops_rails.md
Original file line number Diff line number Diff line change
Expand Up @@ -1169,10 +1169,13 @@ Enabled | Yes | No | 0.52 | -
This cop checks that methods specified in the filter's `only` or
`except` options are defined within the same class or module.

You can technically specify methods of superclass or methods added
by mixins on the filter, but these confuse developers. If you
specify methods that are defined in other classes or modules, you
should define the filter in that class or module.
You can technically specify methods of superclass or methods added by
mixins on the filter, but these can confuse developers. If you specify
methods that are defined in other classes or modules, you should
define the filter in that class or module.

If you rely on behaviour defined in the superclass actions, you must
remember to invoke `super` in the subclass actions.

### Examples

Expand Down Expand Up @@ -1222,6 +1225,29 @@ module FooMixin
end
end
```
```ruby
class ContentController < ApplicationController
def update
@content.update(content_attributes)
end
end

class ArticlesController < ContentController
before_action :load_article, only: [:update]

# the cop requires this method, but it relies on behaviour defined
# in the superclass, so needs to invoke `super`
def update
super
end

private

def load_article
@content = Article.find(params[:article_id])
end
end
```

### Configurable attributes

Expand Down

0 comments on commit 1f5e9fb

Please sign in to comment.