Feature #32938

Rails 6: Zeitwerk support

Added by Pavel Rosický over 1 year ago. Updated 1 day ago.

Status:NewStart date:
Priority:NormalDue date:
Assignee:-% Done:

0%

Category:Rails support
Target version:5.0.0
Resolution:

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.

0001-Explicitly-require-a-class-called-in-initializers-th.patch Magnifier (4.01 KB) Mizuki ISHIKAWA, 2021-10-19 08:50

0002-Split-multiple-classes-and-modules-that-existed-in-t.patch Magnifier (18.7 KB) Mizuki ISHIKAWA, 2021-10-19 08:50

0003-Move-lib-redmine-core_ext-to-lib-core_ext.patch Magnifier (2.29 KB) Mizuki ISHIKAWA, 2021-10-19 08:50

0004-Remove-require_dependency.patch Magnifier (2.06 KB) Mizuki ISHIKAWA, 2021-10-19 08:50

0006-Change-camelcase-rules-to-keep-special-module-or-cla.patch Magnifier (1.14 KB) Mizuki ISHIKAWA, 2021-10-19 08:50

0005-Fix-modules-where-folder-structure-does-not-meet-Zei.patch Magnifier (82.8 KB) Mizuki ISHIKAWA, 2021-10-19 08:50

0008-Change-autoload-mode-from-classic-to-zeitwerk.patch Magnifier (1.01 KB) Mizuki ISHIKAWA, 2021-10-19 08:50

0007-Move-only-modules-and-classes-that-match-Zeitwerk-ru.patch Magnifier (109 KB) Mizuki ISHIKAWA, 2021-10-19 08:50

0001-Move-DateValidator-to-app-validators.patch Magnifier (1.14 KB) Marius BALTEANU, 2021-10-23 00:50


Related issues

Related to Redmine - Feature #29914: Migrate to Rails 6.1 New
Duplicated by Redmine - Defect #35004: DEPRECATION WARNING during startup Closed

Associated revisions

Revision 21251
Added by Marius BALTEANU 4 days ago

Fix rubocop "Style/CombinableLoops" offense in wiki_page.rb (#29914, #32938).

Revision 21252
Added by Marius BALTEANU 4 days ago

Split multiple classes and modules that existed in the same file (#29914, #32938).

Patch by Mizuki ISHIKAWA.

Revision 21253
Added by Marius BALTEANU 4 days ago

Update .rubocop_todo and fix Style/AndOr offense (#29914, #32938).

Revision 21254
Added by Marius BALTEANU 3 days ago

Revert changes made in r21251 due to a test fail (#29914, #32938).

Revision 21258
Added by Marius BALTEANU about 3 hours ago

Move custom DateValidator from lib/redmine/core_ext/active_record.rb to app/validators/date_validator.rb (#29914, #32938).

Revision 21259
Added by Marius BALTEANU about 3 hours ago

Extract Diff and DiffTable from lib/redmine/unified_diff.rb to their own files (#29914, #32938).

Revision 21260
Added by Marius BALTEANU about 2 hours ago

Extract Shell and DateCalculation classes from lib/redmine/utils.rb to their own files (#29914, #32938).

Revision 21261
Added by Marius BALTEANU about 2 hours ago

Remove extra blank line (#29914, #32938).

History

#1 Updated by Marius BALTEANU over 1 year ago

#2 Updated by Go MAEDA over 1 year ago

  • Category set to Rails support

#3 Updated by Enziin System over 1 year ago

+1 Thank you!

#4 Updated by Marius BALTEANU 7 months ago

Pavel, do you have any working patch for this?

#5 Updated by Pavel Rosický 7 months 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.

#6 Updated by Pavel Rosický 7 months ago

I had to remove some references due to your spam protection :)

#7 Updated by Go MAEDA 6 months ago

  • Duplicated by Defect #35004: DEPRECATION WARNING during startup added

#8 Updated by Go MAEDA 6 months ago

  • Target version set to 5.0.0

#9 Updated by Mizuki ISHIKAWA 6 months 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

#10 Updated by Mizuki ISHIKAWA 5 days ago

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 on

include 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.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/

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#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

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.

#11 Updated by Marius BALTEANU 5 days ago

Thanks Mizuki for working on this. I have as weel some code, I will share it with you as soon I can.

#12 Updated by Marius BALTEANU 4 days 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.

#13 Updated by Marius BALTEANU 1 day ago

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.

Also available in: Atom PDF