Redmine

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.

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.

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.

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 when require '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.

I made alternative patches to reduce require at initialization time and to increase autoloading coverage.

• I cut out Redmine::PluginLoader from Redmine::Plugin and use PluginLoader at initialization time. Now, Zeitwerk autoloads Redmine::Plugin and other classes and modules in it.
• I extracted the preparation code(building menu, setting permissions, queries) in /lib/redmine.rb to Redmine::Preparation and it's now in Rails.application.config. to_prepare block. It reduces the use of require during initialization.
• When setting the autoloading paths for Zeitwerk, I used the namespace option. It allows that Zeitwerk autoloads libraries under the lib/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.

Patches committed, thanks everyone for working on this!

Reopening this issue to handle #36835.