Skip to content

Commit

Permalink
Fix Zeitwerk compatibility
Browse files Browse the repository at this point in the history
Add paths to both `autoload_paths` and
`eager_load_paths`. `eager_load_paths` is required to have paths
covered by `zeitwerk:check`.

Ignore and require Paperclip processors, which are cached during
initialization and, thus, cannot be reloaded.

For Rails < 6, we cannot add `lib` to the `eager_load_paths` since
Resque eager loads [1], which then evaluates the engine class a second
time. This time `autoload_paths` is already frozen, resulting in an
error.

Do not invoke `configure!` in `finalize!` to prevent triggering auto
loading during initialization. Instead rely on `to_prepare` in
Pageflow engine to call `configure!`.

REDMINE-19438

[1] https://github.com/resque/resque/blob/v1.27.4/lib/resque/tasks.rb#L45

REDMINE-19439
  • Loading branch information
tf committed Oct 2, 2023
1 parent 9a7a985 commit 5881f7f
Show file tree
Hide file tree
Showing 7 changed files with 108 additions and 45 deletions.
4 changes: 4 additions & 0 deletions config/initializers/paperclip.rb
Original file line number Diff line number Diff line change
@@ -1,3 +1,7 @@
require 'pageflow/paperclip_processors/vtt'
require 'pageflow/paperclip_processors/audio_waveform'
require 'pageflow/paperclip_processors/noop'

Paperclip.interpolates(:pageflow_s3_root) do |_attachment, _style|
Pageflow.config.paperclip_s3_root
end
Expand Down
16 changes: 15 additions & 1 deletion entry_types/paged/lib/pageflow_paged/engine.rb
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,21 @@ module PageflowPaged
class Engine < ::Rails::Engine
isolate_namespace PageflowPaged

config.paths.add('lib', eager_load: true)
if Pageflow::RailsVersion.experimental?
lib = root.join('lib')

config.autoload_paths << lib
config.eager_load_paths << lib

initializer 'pageflow_paged.autoloading' do
Rails.autoloaders.main.ignore(
lib.join('tasks')
)
end
else
config.paths.add('lib', eager_load: true)
end

config.i18n.load_path += Dir[config.root.join('config', 'locales', '**', '*.yml').to_s]

initializer 'pageflow_paged.assets.precompile' do |app|
Expand Down
17 changes: 16 additions & 1 deletion entry_types/scrolled/lib/pageflow_scrolled/engine.rb
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,22 @@ module PageflowScrolled
class Engine < ::Rails::Engine
isolate_namespace PageflowScrolled

config.paths.add('lib', eager_load: true)
if Pageflow::RailsVersion.experimental?
lib = root.join('lib')

config.autoload_paths << lib
config.eager_load_paths << lib

initializer 'pageflow_scrolled.autoloading' do
Rails.autoloaders.main.ignore(
lib.join('generators'),
lib.join('tasks')
)
end
else
config.paths.add('lib', eager_load: true)
end

config.i18n.load_path += Dir[config.root.join('config', 'locales', '**', '*.yml').to_s]

initializer 'pageflow_scrolled.assets.precompile' do |app|
Expand Down
97 changes: 58 additions & 39 deletions lib/pageflow/engine.rb
Original file line number Diff line number Diff line change
Expand Up @@ -41,44 +41,63 @@ module Pageflow
class Engine < ::Rails::Engine
isolate_namespace Pageflow

config.paths.add('app/views/components', autoload: true)
config.paths.add('lib', autoload: true)

def eager_load!
# Manually eager load `lib/pageflow` as the least bad option:
#
# - Autoload paths are not eager loaded in production.
#
# - `lib` cannot be an eager load path since otherwise templates
# in `lib/generators` are also executed.
#
# - `lib/pageflow` cannot be an eager load path since eager load
# paths are automatically used as autoload paths. That way
# `lib/pageflow/admin/something.rb` could be autoloaded via
# `Admin::Something`.
#
# - Using `require` in `lib/pageflow.rb` disables code
# reloading.
#
# - Using `require_dependency` in `lib/pageflow.rb` does not
# activate code reloading either since it requires the
# autoload path to be set up correctly, which only happens
# during initialization.
super

lib_path = config.root.join('lib')
matcher = %r{\A#{Regexp.escape(lib_path.to_s)}/(.*)\.rb\Z}

already_required_files = [
'pageflow/engine',
'pageflow/global_config_api',
'pageflow/news_item_api',
'pageflow/version'
]

Dir.glob("#{lib_path}/pageflow/**/*.rb").sort.each do |file|
logical_path = file.sub(matcher, '\1')
require_dependency(logical_path) unless already_required_files.include?(logical_path)
if Pageflow::RailsVersion.experimental?
config.autoload_paths << root.join('app/views/components')
config.eager_load_paths << root.join('app/views/components')

lib = root.join('lib')

config.autoload_paths << lib
config.eager_load_paths << lib

initializer 'pageflow.autoloading' do
Rails.autoloaders.main.ignore(
lib.join('generators'),
lib.join('tasks'),
lib.join('pageflow/paperclip_processors'),
lib.join('pageflow/version.rb')
)
end
else
config.paths.add('app/views/components', autoload: true)
config.paths.add('lib', autoload: true)

def eager_load!
# Manually eager load `lib/pageflow` as the least bad option:
#
# - Autoload paths are not eager loaded in production.
#
# - `lib` cannot be an eager load path since otherwise templates
# in `lib/generators` are also executed.
#
# - `lib/pageflow` cannot be an eager load path since eager load
# paths are automatically used as autoload paths. That way
# `lib/pageflow/admin/something.rb` could be autoloaded via
# `Admin::Something`.
#
# - Using `require` in `lib/pageflow.rb` disables code
# reloading.
#
# - Using `require_dependency` in `lib/pageflow.rb` does not
# activate code reloading either since it requires the
# autoload path to be set up correctly, which only happens
# during initialization.
super

lib_path = config.root.join('lib')
matcher = %r{\A#{Regexp.escape(lib_path.to_s)}/(.*)\.rb\Z}

already_required_files = [
'pageflow/engine',
'pageflow/global_config_api',
'pageflow/news_item_api',
'pageflow/version'
]

Dir.glob("#{lib_path}/pageflow/**/*.rb").sort.each do |file|
logical_path = file.sub(matcher, '\1')
require_dependency(logical_path) unless already_required_files.include?(logical_path)
end
end
end

Expand Down Expand Up @@ -106,7 +125,7 @@ def eager_load!
end

initializer 'pageflow.factories', after: 'factory_bot.set_factory_paths' do
if Pageflow.configured? && defined?(FactoryBot)
if defined?(FactoryBot)
FactoryBot.definition_file_paths.unshift(Engine.root.join('spec', 'factories'))
end
end
Expand Down
4 changes: 2 additions & 2 deletions lib/pageflow/global_config_api.rb
Original file line number Diff line number Diff line change
Expand Up @@ -87,8 +87,6 @@ def after_global_configure(&block)
# coniguration is now complete.
def finalize!
@finalized = true
configure!
@config.lint!
end

# @api private
Expand All @@ -103,6 +101,8 @@ def configure!
@after_global_configure_blocks.each do |block|
block.call(@config)
end

@config.lint!
end

private
Expand Down
14 changes: 12 additions & 2 deletions spec/pageflow/global_config_api_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ class PageflowModule
config.features.register('some_stuff')
end
pageflow.finalize!
pageflow.configure!

expect(pageflow.config.features.enabled?('some_stuff')).to eq(true)
end
Expand Down Expand Up @@ -62,6 +63,7 @@ class PageflowModule
config.file_types.register(file_type3)
end
pageflow.finalize!
pageflow.configure!

expect(pageflow.config.file_types.count).to eq 3
end
Expand All @@ -88,6 +90,7 @@ class PageflowModule
config.file_types.register(file_type3)
end
pageflow.finalize!
pageflow.configure!

expect(pageflow.config.file_types.count).to eq 2
end
Expand All @@ -111,6 +114,7 @@ class PageflowModule
config.widget_types.register(widget_type3)
end
pageflow.finalize!
pageflow.configure!

expect(pageflow.config.widget_types.count).to eq 3
end
Expand All @@ -134,6 +138,7 @@ class PageflowModule
config.widget_types.register(widget_type3)
end
pageflow.finalize!
pageflow.configure!

expect(pageflow.config.widget_types.count).to eq 2
end
Expand All @@ -151,6 +156,7 @@ class PageflowModule
config.revision_components.register(:global)
end
pageflow.finalize!
pageflow.configure!

expect(pageflow.config.revision_components.sort).to eq([:for_entry_type, :global])
end
Expand All @@ -170,6 +176,7 @@ class PageflowModule
config.hooks.on(:some_event, global_subscriber)
end
pageflow.finalize!
pageflow.configure!

pageflow.config.hooks.invoke(:some_event)

Expand All @@ -178,7 +185,7 @@ class PageflowModule
end
end

describe '#finalize!' do
describe '#configure!' do
it 'yields config to after_configure blocks' do
result = false
pageflow = PageflowModule.new
Expand All @@ -190,6 +197,7 @@ class PageflowModule
result = config.features.enabled?('some_stuff')
end
pageflow.finalize!
pageflow.configure!

expect(result).to eq(true)
end
Expand All @@ -205,6 +213,7 @@ class PageflowModule
result = config.features.enabled?('some_stuff')
end
pageflow.finalize!
pageflow.configure!

expect(result).to eq(true)
end
Expand All @@ -216,9 +225,10 @@ class PageflowModule
config.features.enable_by_default('unknown')
config.features.register('other_stuff')
end
pageflow.finalize!

expect {
pageflow.finalize!
pageflow.configure!
}.to raise_error(/unknown feature/)
end

Expand Down
1 change: 1 addition & 0 deletions spec/pageflow/news_item_api_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ module Pageflow
config.news = news
end
pageflow_module.finalize!
pageflow_module.configure!

expect(news).to have_received(:item).with(:some_news_item, message: '')
end
Expand Down

0 comments on commit 5881f7f

Please sign in to comment.