Feature #32938
closedRails 6: Zeitwerk support
0%
Description
https://github.com/fxn/zeitwerk
DEPRECATION WARNING: Initialization autoloaded the constants ActiveRecord::Acts, ActiveRecord::Acts::Tree, Redmine::I18n, Redmine::Helpers, Redmine::Helpers::URL , Redmine::SafeAttributes, Redmine::SubclassFactory, CustomField, Redmine::Utils, Redmine::Configuration, Redmine::Scm::Adapters::CommandFailed, IssueRelation, Redmine::WikiFormatting::Macros, Redmine::Pagination, Redmine::SudoMode, ApplicationHelper , Redmine::WikiFormatting::Textile, Redmine::WikiFormatting::Textile::Formatter, Redmine::WikiFormatting::Textile::Helper, Redmine::WikiFormatting::HtmlParser , Redmine::WikiFormatting::Textile::HtmlParser, Redmine::WikiFormatting::Markdown, Redmine::WikiFormatting::Markdown::Formatter, Redmine::WikiFormatting::Markdown::HTML, Redmine::WikiFormatting::Markdown::Helper, Redmine::WikiFormatting::Markdown::HtmlParser, Redmine::Views::ApiTemplateHandler, and Setting. Being able to do this is deprecated. Autoloading during initialization is going to be an error condition in future versions of Rails. Reloading does not reboot the application, and therefore code executed during initialization does not run again. So, if you reload ActiveRecord::Acts, for example, the expected changes won't be reflected in that stale Module object. `config.autoloader` is set to `classic`. These autoloaded constants would have been unloaded if `config.autoloader` had been set to `:zeitwerk`. Please, check the "Autoloading and Reloading Constants" guide for solutions.
Files
Related issues
Updated by Marius BĂLTEANU over 4 years ago
- Related to Feature #29914: Migrate to Rails 6.1 with Zeitwerk autoloading added
Updated by Marius BĂLTEANU over 3 years ago
Pavel, do you have any working patch for this?
Updated by Pavel Rosický over 3 years ago
sry, I don't. This is a pretty heavy change and Redmine has a lot of technical debt in this area. But I have a few hints based on my findings:
Zeitwerk depends on Rails conventions, especially the folder structure.
1/ initializers
/master/config/initializers/10-patches.rb
depends on
include Redmine::I18n
before lib/redmine.rb is loaded, so it triggers autoload. Split them into modules, require explicitly.
2/ there're multiple module definitions in a single file (many places), that don't match the filename and the expected folder structure.
3/ Ruby / Rails patches are global from the pre Ruby 2.0 era. They're patched non-deterministically in some random file
module ActiveRecord class Base def self.human_attribute_name(attr, options = {}) ... end end end
or a require chain like this
/master/lib/redmine/core_ext/string.rb
this issue can be avoided by using manual require at startup time, but you may consider using refinements instead
https://www.cloudbees.com/blog/ruby-refinements/
4/ require_dependency should be removed
/master/app/models/principal.rb#L221
here's a Rails guide and there's also an example of how to deal with STI
https://edgeguides.rubyonrails.org/autoloading_and_reloading_constants.html
5/ autoloads based on #constantize
/master/lib/redmine/wiki_formatting.rb#L42
6/ a folder structure violations, a few examples
/master/lib/diff.rb#L3
module RedmineDiff class Diff end end
should be
class Diff end
or
/master/lib/redmine/diff.rb
module Redmine class Diff end end
7/ module names should use camel-case, upper-case is reserved for constants
/master/lib/redmine/helpers/url.rb#L24
/master/lib/redmine/version.rb#L7
I think now it's pretty clear this is an extremely breaking change. Redmine is a part 1, but even if we fix all these issues, it's impossible to keep compatibility for plugins, but it's well, unavoidable...
Zeitwerk mode can be enabled by putting
config.load_defaults 6.1
into config/application.rb
and tested by
rails zeitwerk:check
Zeitwerk mode will become mandatory in Rails 7.
I think that decisions like module names and folder structure should be discussed with your core team. It's hard to accept a small PR without a confirmation it fixes anything and on the other hand, a big PR might include changes you won't be eventually willing to accept. I would recommend you to investigate these problems yourself, split them into smaller tasks and comment them with your expectations. The community can help you solve it this way, but this task is too big with many open questions.
Updated by Pavel Rosický over 3 years ago
I had to remove some references due to your spam protection :)
Updated by Go MAEDA over 3 years ago
- Has duplicate Defect #35004: DEPRECATION WARNING during startup added
Updated by Mizuki ISHIKAWA over 3 years ago
I haven't tried it, it seems that problem 7 can be avoided by writing config/initializers/inflations.rb.
https://github.com/fxn/zeitwerk#inflection
Updated by Mizuki ISHIKAWA almost 3 years ago
- File 0001-Explicitly-require-a-class-called-in-initializers-th.patch 0001-Explicitly-require-a-class-called-in-initializers-th.patch added
- File 0002-Split-multiple-classes-and-modules-that-existed-in-t.patch 0002-Split-multiple-classes-and-modules-that-existed-in-t.patch added
- File 0003-Move-lib-redmine-core_ext-to-lib-core_ext.patch 0003-Move-lib-redmine-core_ext-to-lib-core_ext.patch added
- File 0004-Remove-require_dependency.patch 0004-Remove-require_dependency.patch added
- File 0005-Fix-modules-where-folder-structure-does-not-meet-Zei.patch 0005-Fix-modules-where-folder-structure-does-not-meet-Zei.patch added
- File 0006-Change-camelcase-rules-to-keep-special-module-or-cla.patch 0006-Change-camelcase-rules-to-keep-special-module-or-cla.patch added
- File 0007-Move-only-modules-and-classes-that-match-Zeitwerk-ru.patch 0007-Move-only-modules-and-classes-that-match-Zeitwerk-ru.patch added
- File 0008-Change-autoload-mode-from-classic-to-zeitwerk.patch 0008-Change-autoload-mode-from-classic-to-zeitwerk.patch added
I've created a draft patches to discuss Zeitwerk support.
Rails 7.0 is going to remove the old loading methods, so we need to think about how to support the new loading methods. https://weblog.rubyonrails.org/2021/9/15/Rails-7-0-alpha-1-released/ (Zeitwerk Exclusively section)
Description of changes. These patches have been split up as much as possible based on the description in #32938#note-5.
1/ initializers
/master/config/initializers/10-patches.rb
depends oninclude Redmine::I18n
before lib/redmine.rb is loaded, so it triggers autoload. Split them into modules, require explicitly.
0001-Explicitly-require-a-class-called-in-initializers-th.patch
There are two ways to work around this, either by enclosing it in Rails.application.reloader.to_prepare or by explicitly writing "require" before using it.
The simpler way is to enclose it in Rails.application.reloader.to_prepare, but the impact on the plugin is so great that we've decided to use the "require" method in 0001-Explicitly-require-a-class-called-in-initializers-th.patch.
Rails.application.reloader.to_prepare do # Autoload classes and modules needed at boot time here. end
2/ there're multiple module definitions in a single file (many places), that don't match the filename and the expected folder structure.
0002-Split-multiple-classes-and-modules-that-existed-in-t.patch
Previously, AnonymousUser was defined in app/models/user.rb, but since it is not correct according to Zeitwerk rules that the class name and file name do not match, I created a new app/models/anonymous_user.rb and split it.
We have also made similar changes to several other classes and modules.
3/ Ruby / Rails patches are global from the pre Ruby 2.0 era. They're patched non-deterministically in some random file
module ActiveRecord
class Base
def self.human_attribute_name(attr, options = {})
...
end
end
end
or a require chain like this
/master/lib/redmine/core_ext/string.rbthis issue can be avoided by using manual require at startup time, but you may consider using refinements instead
https://www.cloudbees.com/blog/ruby-refinements/
0003-Move-lib-redmine-core_ext-to-lib-core_ext.patch
It seemed difficult to fit this into the Zeitwerk rules, so I moved the directory.
4/ require_dependency should be removed
/master/app/models/principal.rb#L221here's a Rails guide and there's also an example of how to deal with STI
https://edgeguides.rubyonrails.org/autoloading_and_reloading_constants.html
0004-Remove-require_dependency.patch
5/ autoloads based on #constantize
/master/lib/redmine/wiki_formatting.rb#L42
This seems to have been resolved by other changes without a direct fix.
6/ a folder structure violations, a few examples
/master/lib/diff.rb#L3
...
0005-Fix-modules-where-folder-structure-does-not-meet-Zei.patch
7/ module names should use camel-case, upper-case is reserved for constants
/master/lib/redmine/helpers/url.rb#L24
/master/lib/redmine/version.rb#L7
0006-Change-camelcase-rules-to-keep-special-module-or-cla.patch
The effect of changing the class name and module name seemed to be significant, so I overwrote the movement of the camelize method without changing the name.
8 / other
0007-Move-only-modules-and-classes-that-match-Zeitwerk-ru.patch
0008-Change-autoload-mode-from-classic-to-zeitwerk.patch
When I tried to pass everything in the lib directory to autoload_paths, I found it very difficult to make the structure of the plugins and generators directories conform to Zeitwerk's rules. (Relation: 6/ a folder structure violations)
So I changed only the lib/redmine directory to lib/autoloads/redmine directory where I could apply autoloading, and set only the files under lib/autoloads directory as `config.autoload_paths += %W(#{config.root}/lib/autoloads)
`.
I already know that applying these patches will cause multiple problems, mainly related to development.
- probrem-1. The directory change from lib/redmine to lib/autoloads/redmine will cause many patches to fail to apply, and the changelog will be hard to read.
- probrem-2. I changed redmine.rb to "require" many files directly, but autoload doesn't work for files loaded here. Even if you rewrite ApplicationHelper during development, the changes will not be reflected until you restart `rails s`.
I'm sure there are many other problems with it, so feedback and new patches would be greatly appreciated.
Updated by Marius BĂLTEANU almost 3 years ago
Thanks Mizuki for working on this. I have as weel some code, I will share it with you as soon I can.
Updated by Marius BĂLTEANU almost 3 years ago
I've committed for now the second patch which is safe regardless the type of autoloader. Also, I've fixed some Rubocop offenses and I've updated the .rubocop_todo.yml
to match the new files.
I will continue with more fixes in order to decrease the number of changes required for zeitwerk
.
Updated by Marius BĂLTEANU almost 3 years ago
- File 0001-Move-DateValidator-to-app-validators.patch 0001-Move-DateValidator-to-app-validators.patch added
For DateValidator
, I propose to move it from lib/redmine/core_ext/active_record.rb
to app/validators/date_validator.rb
. It will work on both classic
and zeitwerk
and I think it make more sense to stay in the app. Also, other Rails projects do this. Unfortunately, I didn't find a recommended by Rails team.
Updated by Mizuki ISHIKAWA almost 3 years ago
Marius BALTEANU wrote:
For
DateValidator
, I propose to move it fromlib/redmine/core_ext/active_record.rb
toapp/validators/date_validator.rb
. It will work on bothclassic
andzeitwerk
and I think it make more sense to stay in the app. Also, other Rails projects do this. Unfortunately, I didn't find a recommended by Rails team.
I also think app/validators/date_validator.rb
would be better.
Thank you for your work on this issue.
Updated by Marius BĂLTEANU almost 3 years ago
Thanks Mizuki! I've already committed the change.
In this moment, we have only one issue left that makesbin/rails zeitwerk:check
to fail, is lib/diff.rb
. This class is very confusing because we already have the Diff
class extracted from the unified_diff.rb
. I think it's better to rename the class or even better, to move it to lib/redmine/string_array_diff/
so in the end we will have:
lib/redmine/string_array_diff/diff.rb
lib/redmine/string_array_diff/diffable.rb
What do you think about this? Once we fix this, we can discuss the remaining deprecation warnings and hopefully we can close this issue.
Any feedback/help is appreciated.
Updated by Mizuki ISHIKAWA almost 3 years ago
- File split-lib-diff.patch split-lib-diff.patch added
Marius BALTEANU wrote:
What do you think about this?
I think the directory structure and naming is good.
In which file do you plan to include the Diffable for Array and String? Am I correct in interpreting the changes as in the attached patch?
1. Change the module structure to match the change in directory structure. (RedmineDiff::Diff => Redmine::StringArrayDiff::Diff, RedmineDiff::Diffable => Redmine::StringArrayDiff::Diffable)
2. Keep lib/diff.rb to maintain the behavior of expanding Arrays and Strings when require 'diff'
is executed in app/models/wiki_page.rb, etc.
Updated by Marius BĂLTEANU almost 3 years ago
Mizuki ISHIKAWA wrote:
Marius BALTEANU wrote:
What do you think about this?
I think the directory structure and naming is good.
In which file do you plan to include the Diffable for Array and String? Am I correct in interpreting the changes as in the attached patch?1. Change the module structure to match the change in directory structure. (RedmineDiff::Diff => Redmine::StringArrayDiff::Diff, RedmineDiff::Diffable => Redmine::StringArrayDiff::Diffable)
2. Keep lib/diff.rb to maintain the behavior of expanding Arrays and Strings whenrequire 'diff'
is executed in app/models/wiki_page.rb, etc.
I think we can move everything to lib/redmine/string_array_diff
and update the require in app/modeles/wiki_page.rb
and lib/redmine/helpers/diff.rb
as I did in attached patch. It works for you? In my CI, all tests pass.
Updated by Mizuki ISHIKAWA almost 3 years ago
Marius BALTEANU wrote:
I think we can move everything to
lib/redmine/string_array_diff
and update the require inapp/modeles/wiki_page.rb
andlib/redmine/helpers/diff.rb
as I did in attached patch. It works for you? In my CI, all tests pass.
Thanks for attaching the patch for clarity.
It looks good and actually worked fine in my environment.
Updated by Takashi Kato almost 3 years ago
- File alt-0001-Extract-Redmine-PluginLoader-from-Redmine-Plugin.patch alt-0001-Extract-Redmine-PluginLoader-from-Redmine-Plugin.patch added
- File alt-0002-Move-preparation-code-to-redmine-preparation.rb.patch alt-0002-Move-preparation-code-to-redmine-preparation.rb.patch added
- File alt-0003-Add-core-extensions-in-prepare-method.patch alt-0003-Add-core-extensions-in-prepare-method.patch added
- File alt-0004-Load-core-plugins-with-require.patch alt-0004-Load-core-plugins-with-require.patch added
- File alt-0005-Use-zeitwerk.patch alt-0005-Use-zeitwerk.patch added
I made alternative patches to reduce require
at initialization time and to increase autoloading coverage.
- I cut out
Redmine::PluginLoader
fromRedmine::Plugin
and use PluginLoader at initialization time. Now, Zeitwerk autoloadsRedmine::Plugin
and other classes and modules in it. - I extracted the preparation code(building menu, setting permissions, queries) in
/lib/redmine.rb
toRedmine::Preparation
and it's now inRails.application.config. to_prepare
block. It reduces the use ofrequire
during initialization. - When setting the autoloading paths for Zeitwerk, I used the
namespace
option. It allows that Zeitwerk autoloads libraries under thelib/redmine
directory without moving them to another directory. - To simplify patching, I've left the core plugins and
RedCloth3
out of the autoload for now. I will fix them later.
Updated by Takashi Kato almost 3 years ago
The above patches are appliable in current trunk(r21261).
Updated by Marius BĂLTEANU almost 3 years ago
Takashi Kato wrote:
The above patches are appliable in current trunk(r21261).
Thanks Takashi for the patches, they look good to me. I've already committed some changes related to require
from patch 0004, I've renamed CoreExtensions
to CoreExt
(r21267) and moved lib/diff.rb
to lib/redmine/string_array_diff/diff
(r21262).
Applying them on top of the changes committed today fix all the warnings related to zeitwerk:
➜ redmine git:(feature/32938_zeitwerk) ✗ bin/rails zeitwerk:check Hold on, I am eager loading the application. W, [2021-10-28T01:34:34.662994 #61216] WARN -- : Creating scope :system. Overwriting existing method Enumeration.system. W, [2021-10-28T01:34:34.858254 #61216] WARN -- : Creating scope :sorted. Overwriting existing method Group.sorted. W, [2021-10-28T01:34:34.929581 #61216] WARN -- : Creating scope :sorted. Overwriting existing method User.sorted. All is good!
Updated by Mizuki ISHIKAWA almost 3 years ago
Takashi Kato wrote:
The above patches are appliable in current trunk(r21261).
I've applied the patch #32938#note-19 and it looks great.
I think this patch supports Zeitwerk in a better way than the patch I posted.
I've tried to run some plugins, and they work well as long as the plugin side is adapted to Zeitwerk's rules.
Updated by Marius BĂLTEANU over 2 years ago
- Status changed from New to Closed
- Assignee set to Marius BĂLTEANU
- Target version deleted (
5.0.0) - Resolution set to Fixed
Patches committed, thanks everyone for working on this!
Updated by Marius BĂLTEANU over 2 years ago
- Related to Defect #36218: Plugin assets are not copied correctly in trunk r21289 added
Updated by Go MAEDA over 2 years ago
- Related to Defect #36245: ActiveSupport::Reloader.to_prepare not working in trunk 21287 added
Updated by Go MAEDA over 2 years ago
- Related to Defect #36287: rake redmine:plugins:assets fails added
Updated by Go MAEDA over 2 years ago
- Related to Defect #15958: AnonymousUser class not found by plugin added
Updated by Go MAEDA over 2 years ago
- Related to Defect #36835: Redmine::Plugin.assets_directory is not working after r21283 added
Updated by Go MAEDA over 2 years ago
- Status changed from Closed to Reopened
Reopening this issue to handle #36835.
Updated by Marius BĂLTEANU over 2 years ago
- Status changed from Reopened to Closed