From 5881f7fad82b0bfc132c4cd1ff0a7cbe04e9f0f1 Mon Sep 17 00:00:00 2001 From: Tim Fischbach Date: Fri, 29 Sep 2023 10:44:38 +0200 Subject: [PATCH 1/3] Fix Zeitwerk compatibility 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 --- config/initializers/paperclip.rb | 4 + .../paged/lib/pageflow_paged/engine.rb | 16 ++- .../scrolled/lib/pageflow_scrolled/engine.rb | 17 +++- lib/pageflow/engine.rb | 97 +++++++++++-------- lib/pageflow/global_config_api.rb | 4 +- spec/pageflow/global_config_api_spec.rb | 14 ++- spec/pageflow/news_item_api_spec.rb | 1 + 7 files changed, 108 insertions(+), 45 deletions(-) diff --git a/config/initializers/paperclip.rb b/config/initializers/paperclip.rb index 1af9717959..f8fd2044a0 100644 --- a/config/initializers/paperclip.rb +++ b/config/initializers/paperclip.rb @@ -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 diff --git a/entry_types/paged/lib/pageflow_paged/engine.rb b/entry_types/paged/lib/pageflow_paged/engine.rb index 4db56d0d04..0c1cfa4efe 100644 --- a/entry_types/paged/lib/pageflow_paged/engine.rb +++ b/entry_types/paged/lib/pageflow_paged/engine.rb @@ -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| diff --git a/entry_types/scrolled/lib/pageflow_scrolled/engine.rb b/entry_types/scrolled/lib/pageflow_scrolled/engine.rb index 54b5148e2a..77db3f8c18 100644 --- a/entry_types/scrolled/lib/pageflow_scrolled/engine.rb +++ b/entry_types/scrolled/lib/pageflow_scrolled/engine.rb @@ -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| diff --git a/lib/pageflow/engine.rb b/lib/pageflow/engine.rb index bedeee0b9f..1290ed38bf 100644 --- a/lib/pageflow/engine.rb +++ b/lib/pageflow/engine.rb @@ -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 @@ -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 diff --git a/lib/pageflow/global_config_api.rb b/lib/pageflow/global_config_api.rb index eb31bd79ef..c8adc18d04 100644 --- a/lib/pageflow/global_config_api.rb +++ b/lib/pageflow/global_config_api.rb @@ -87,8 +87,6 @@ def after_global_configure(&block) # coniguration is now complete. def finalize! @finalized = true - configure! - @config.lint! end # @api private @@ -103,6 +101,8 @@ def configure! @after_global_configure_blocks.each do |block| block.call(@config) end + + @config.lint! end private diff --git a/spec/pageflow/global_config_api_spec.rb b/spec/pageflow/global_config_api_spec.rb index 7f474e352d..7c7be494ba 100644 --- a/spec/pageflow/global_config_api_spec.rb +++ b/spec/pageflow/global_config_api_spec.rb @@ -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 @@ -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 @@ -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 @@ -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 @@ -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 @@ -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 @@ -170,6 +176,7 @@ class PageflowModule config.hooks.on(:some_event, global_subscriber) end pageflow.finalize! + pageflow.configure! pageflow.config.hooks.invoke(:some_event) @@ -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 @@ -190,6 +197,7 @@ class PageflowModule result = config.features.enabled?('some_stuff') end pageflow.finalize! + pageflow.configure! expect(result).to eq(true) end @@ -205,6 +213,7 @@ class PageflowModule result = config.features.enabled?('some_stuff') end pageflow.finalize! + pageflow.configure! expect(result).to eq(true) end @@ -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 diff --git a/spec/pageflow/news_item_api_spec.rb b/spec/pageflow/news_item_api_spec.rb index 765730c333..7f292bfbe7 100644 --- a/spec/pageflow/news_item_api_spec.rb +++ b/spec/pageflow/news_item_api_spec.rb @@ -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 From 4565e981f6ed3167b472ad8957fe4297009c199a Mon Sep 17 00:00:00 2001 From: Tim Fischbach Date: Fri, 29 Sep 2023 11:57:16 +0200 Subject: [PATCH 2/3] Fix pageflow-support dependency REDMINE-19438 --- spec/support/pageflow-support.gemspec | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/spec/support/pageflow-support.gemspec b/spec/support/pageflow-support.gemspec index 527d940acc..a1d1a0bd4c 100644 --- a/spec/support/pageflow-support.gemspec +++ b/spec/support/pageflow-support.gemspec @@ -29,9 +29,9 @@ Gem::Specification.new do |s| s.add_runtime_dependency 'resque-scheduler', '~> 2.5' if Pageflow::RailsVersion.experimental? - s.add_development_dependency 'ar_after_transaction', '~> 0.8.0' + s.add_runtime_dependency 'ar_after_transaction', '~> 0.8.0' else - s.add_development_dependency 'ar_after_transaction', '~> 0.5.0' + s.add_runtime_dependency 'ar_after_transaction', '~> 0.5.0' end s.add_runtime_dependency 'redis', '~> 3.0' From b71adb136f8b74659b5893b3b399c336ddb3a463 Mon Sep 17 00:00:00 2001 From: Tim Fischbach Date: Fri, 29 Sep 2023 12:02:22 +0200 Subject: [PATCH 3/3] Require pageflow/rails_vesion in engines Required for plugin test suites. REDMINE-19438 --- entry_types/paged/lib/pageflow_paged/engine.rb | 2 ++ entry_types/scrolled/lib/pageflow_scrolled/engine.rb | 1 + lib/pageflow/engine.rb | 2 ++ spec/support/pageflow/dummy/app.rb | 1 + spec/support/pageflow/dummy/rails_template.rb | 2 ++ 5 files changed, 8 insertions(+) diff --git a/entry_types/paged/lib/pageflow_paged/engine.rb b/entry_types/paged/lib/pageflow_paged/engine.rb index 0c1cfa4efe..839b2e42a7 100644 --- a/entry_types/paged/lib/pageflow_paged/engine.rb +++ b/entry_types/paged/lib/pageflow_paged/engine.rb @@ -1,3 +1,5 @@ +require 'pageflow/rails_version' + module PageflowPaged # Rails integration class Engine < ::Rails::Engine diff --git a/entry_types/scrolled/lib/pageflow_scrolled/engine.rb b/entry_types/scrolled/lib/pageflow_scrolled/engine.rb index 77db3f8c18..25f49ddad6 100644 --- a/entry_types/scrolled/lib/pageflow_scrolled/engine.rb +++ b/entry_types/scrolled/lib/pageflow_scrolled/engine.rb @@ -1,4 +1,5 @@ require 'rails' +require 'pageflow/rails_version' module PageflowScrolled # Rails integration diff --git a/lib/pageflow/engine.rb b/lib/pageflow/engine.rb index 1290ed38bf..01dc40cbec 100644 --- a/lib/pageflow/engine.rb +++ b/lib/pageflow/engine.rb @@ -31,6 +31,8 @@ require 'pageflow_scrolled' require 'symmetric-encryption' +require 'pageflow/rails_version' + if Gem::Specification.find_all_by_name('pageflow-react', '>= 0.0').any? fail('The pageflow-react gem has been merged into the pageflow gem. ' \ 'See the pageflow changelog for update instructions.') diff --git a/spec/support/pageflow/dummy/app.rb b/spec/support/pageflow/dummy/app.rb index a606619393..91331df8ea 100644 --- a/spec/support/pageflow/dummy/app.rb +++ b/spec/support/pageflow/dummy/app.rb @@ -1,5 +1,6 @@ require 'pageflow/version' require 'pageflow/dummy/exit_on_failure_patch' +require 'pageflow/rails_version' module Pageflow module Dummy diff --git a/spec/support/pageflow/dummy/rails_template.rb b/spec/support/pageflow/dummy/rails_template.rb index 84eb7acf5e..c41586daa6 100644 --- a/spec/support/pageflow/dummy/rails_template.rb +++ b/spec/support/pageflow/dummy/rails_template.rb @@ -1,3 +1,5 @@ +require 'pageflow/rails_version' + def source_paths [File.join(File.expand_path(File.dirname(__FILE__)), 'templates')] end