Skip to content

Commit

Permalink
Stop generating code to autoload overrides
Browse files Browse the repository at this point in the history
Up to this PR, when installing Solidus, we're adding code to the host
application to load files matching `app/**/*_decorator*.rb`.

There exist a series of issues about that:

- It goes against current recommendations on the Rails guides, where
overrides are placed under `app/overrides/**/_*override.rb` ( see
https://guides.rubyonrails.org/engines.html#improving-engine-functionality).

- The `_decorator.rb` suffix is misleading, as that kind of overrides
has to be seen as monkey patches and not related to the decorator
pattern (see https://en.wikipedia.org/wiki/Decorator_pattern).

- Although it's something needed a lot of times, monkey patching core
classes should be seen as a last resort after ruling out other
strategies (like configuring a custom class for some behavior). As such,
we should not encourage it.

- It may conflict with the naming used by other gems, like Draper
(https://github.com/drapergem/draper).

For all of that, it's something that we should address in the docs.

This change is backward compatible, as the running code doesn't belong
to the Solidus engine but to the previously generated applications,
which will keep it after upgrading.

The deleted code is, in fact, intended to work with the classic
autoloader (removed on Rails 7). However, it still works because of
https://github.com/rails/rails/blob/296ef7a17221e81881e38b51aa2b014d7a28bac5/activesupport/lib/active_support/dependencies/require_dependency.rb,
which is deprecated.

On a related information, the recommended `app/overrides/` directory is
the same used by `deface` (https://github.com/spree/deface), a common
dependency in Solidus projects. However, both types of overrides can
coexist without interfering with each other (see
solidusio#3010 (comment)).
We decided to tackle the issue upstream on `deface` (see
solidusio#3010 (comment)).

Closes solidusio#3010
  • Loading branch information
waiting-for-dev committed Dec 27, 2021
1 parent d467123 commit 4f14183
Show file tree
Hide file tree
Showing 2 changed files with 34 additions and 11 deletions.
34 changes: 34 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,40 @@

- Fix CSRF forgery protection bypass for Spree::OrdersController#populate [GHSA-h3fg-h5v3-vf8m](https://github.com/solidusio/solidus/security/advisories/GHSA-h3fg-h5v3-vf8m)

**Other important changes**

New Solidus applications won't autoload files matching `app/**/*_decorator*.rb`
pattern anymore. For previous Solidus applications, it's something that will
keep working as the responsible code was added to your `config/application.rb`
when Solidus was installed. That code is intended to work with Rails' classic
autoloader, deprecated on Rails 6 and removed on Rails 7. It keeps working
because of a [compatibility
layer](https://github.com/rails/rails/blob/296ef7a17221e81881e38b51aa2b014d7a28bac5/activesupport/lib/active_support/dependencies/require_dependency.rb)
which is also deprecated. However, it may be eventually removed, so you're
better off updating your `application.rb` file. You should substitute:

```ruby
config.to_prepare do
Dir.glob(Rails.root.join('app/**/*_decorator*.rb')) do |path|
require_dependency(path)
end
```

With:

```ruby
overrides = "#{Rails.root}/app/overrides" # use your actual directory here
Rails.autoloaders.main.ignore(overrides)
config.to_prepare do
Dir.glob("#{overrides}/**/*_decorator*.rb").each do |override|
load override
end
end
```

You might also want to adopt the [current Rails recommendation but
overrides](https://guides.rubyonrails.org/engines.html#improving-engine-functionality).

### Core

- Add configuration option for `migration_path` [#4190](https://github.com/solidusio/solidus/pull/4190) ([SuperGoodSoft](https://github.com/supergoodsoft/))
Expand Down
11 changes: 0 additions & 11 deletions core/lib/generators/solidus/install/install_generator.rb
Original file line number Diff line number Diff line change
Expand Up @@ -103,17 +103,6 @@ def create_overrides_directory
end

def configure_application
application <<-RUBY
# Load application's model / class decorators
initializer 'spree.decorators' do |app|
config.to_prepare do
Dir.glob(Rails.root.join('app/**/*_decorator*.rb')) do |path|
require_dependency(path)
end
end
end
RUBY

if !options[:enforce_available_locales].nil?
application <<-RUBY
# Prevent this deprecation message: https://github.com/svenfuchs/i18n/commit/3b6e56e
Expand Down

0 comments on commit 4f14183

Please sign in to comment.