Redmine

Hello. Sorry for long response.
Am I right that your idea is about using propshaft without other tools (without compilation)?
In my work I start from another side - compilation with old good sprockets as digest generator.

And after all I can say that your way can be much simplier because of using tools like webpack or other requires many changes in existed js files of redmine.
For example application.js is just a big helper that in propper way should be splitted to many small libraries.
Another problem is a huge usage of js.erb files that directly call functions from application.js and other js files.

One other problem that we have on our installation - many existed themes and plugins assumed that redmine assets available on root of public directory (not in assets subdirectory), I see a lot of hardcoded pathes in js and css that use absolute and relative paths to the root of redmine hosts.
As result I still have old mechanic with copying assets to root of public directory for backward compability.
Good alternative is usage of old behaviour for plugin_name/assets/* and use new behaviour for some other directories like plugin_name/app/assets/*.

Takashi Kato, this is a very nice improvement, thank you for your hard working on this.

• additionals: https://github.com/alphanodes/additionals
• redmine_issue_dynamic_edit: https://github.com/Ilogeek/redmine_issue_dynamic_edit

Also, I've quickly created a fourth patch (attached) that removes the manually hardcoded rails-ujs library from jquery-3.6.1-ui-1.13.2-ujs-7.1.2.js and uses the one provided by the actionview gem, it's working very well.

Some notes:
1. Task bin/rails redmine:plugins:assets no longer works:

bin/rails aborted!
NoMethodError: undefined method `mirror_assets' for Redmine::PluginLoader:Class

        Redmine::PluginLoader.mirror_assets(name)
                             ^^^^^^^^^^^^^^
/work/lib/tasks/redmine.rake:164:in `block (3 levels) in <top (required)>'

Should we adapt it to generate the assets for a specific plugin or completely drop it.

2. For Redmine core assets, as next improvement after integrating these patches, I would like to move our existing assets to app/assets for the sake of consistency with Rails default structure and to not provide in the public folder two folders, the "source" assets and the actual assets (generated with Asset Pipeline). I'm aware that more changes are required, but I do not see a real problem to include all of them in 6.0.0.

3. Dmitry Lisichkin has a point in #note-3, but at the same time, I think the complexity to keep both behaviours for plugins do not worth it. I'm even in favour to move the plugin assets in "app/assets" to force plugins to use the correct way and also to stick to the Rails structure. The changes to adapt a plugin to this breaking change are minimal.

Takashi Kato, do you have any clue why the following test fails on Redmine CI, but not on local environment?

Failure:
Redmine::AssetPathTest#test_asset_path_size [test/unit/lib/redmine/asset_path_test.rb:31]:
Expected: 2
  Actual: 3

bin/rails test test/unit/lib/redmine/asset_path_test.rb:30

Marius BĂLTEANU wrote in #note-9:

Takashi Kato, do you have any clue why the following test fails on Redmine CI, but not on local environment?

[...]

Can it be .svn directory?

Dmitry Lisichkin wrote in #note-10:

Marius BĂLTEANU wrote in #note-9:

Takashi Kato, do you have any clue why the following test fails on Redmine CI, but not on local environment?

[...]

Can it be .svn directory?

It could be, I've added some test code to see what asset_path contains, now we need to wait for the CI to run.

Fix the failing test in r22641.

Dmitry Lisichkin, thanks for your advise!

Marius BĂLTEANU
Thank you for the detailed review and merge!!

Dmitry Lisichkin
I didn't realize it! Thanks!

• included favicon in assets pipeline and switched to favicon_link_tag (https://apidock.com/rails/ActionView/Helpers/AssetTagHelper/favicon_link_tag)
• updated installation docs
• moved all public assets (javascript, stylesheets, images and themes) from public to app/assets

I'm wondering if it's a good ideea to split our assets from the one owned by outside entities, such as jQuery, jQuery UI or Tribute and move all these to vendor/javascripts and vendor/stylesheets.

Marius BĂLTEANU wrote in #note-18:

I made the following changes:

• included favicon in assets pipeline and switched to favicon_link_tag (https://apidock.com/rails/ActionView/Helpers/AssetTagHelper/favicon_link_tag)
• updated installation docs
• moved all public assets (javascript, stylesheets, images and themes) from public to app/assets

I'm wondering if it's a good ideea to split our assets from the one owned by outside entities, such as jQuery, jQuery UI or Tribute and move all these to vendor/javascripts and vendor/stylesheets.

I've been using a non-standard theme( e.g., https://github.com/farend/redmine_theme_farend_basic , https://github.com/farend/redmine_theme_farend_bleuclair ) with Redmine, and after the changes in r22696, I needed to do the following to keep the theme working:

1. Move the theme from public/themes to app/assets/themes.
2. Change the path where the theme imports Redmine's application.css. (Refer to the commit: https://github.com/farend/redmine_theme_farend_bleuclair/commit/c06330c064d19b02ea3bb6ca7e9a5c37d6c33519)

However, making the change in step 2 causes the theme to break in earlier versions of Redmine, so I'm looking for a way to make the change compatible with both versions.
Additionally, when I added the standard theme to app/assets/themes, it became a target for git staging. I would appreciate a change in .gitignore to prevent this.

diff --git a/.gitignore b/.gitignore
index f25b049a5..f38d49c39 100644
--- a/.gitignore
+++ b/.gitignore
@@ -27,10 +27,10 @@
 /public/assets/*
 /public/dispatch.*
 /public/plugin_assets/*
-/public/themes/*
-!/public/themes/alternate
-!/public/themes/classic
-!/public/themes/README
+/app/assets/themes/*
+!/app/assets/themes/alternate
+!/app/assets/themes/classic
+!/app/assets/themes/README
 /tmp/*
 /tmp/cache/*
 /tmp/pdf/*

Mizuki ISHIKAWA wrote in #note-19:

Marius BĂLTEANU wrote in #note-18:

[...]

I've been using a non-standard theme( e.g., https://github.com/farend/redmine_theme_farend_basic , https://github.com/farend/redmine_theme_farend_bleuclair ) with Redmine, and after the changes in r22696, I needed to do the following to keep the theme working:

1. Move the theme from public/themes to app/assets/themes.
2. Change the path where the theme imports Redmine's application.css. (Refer to the commit: https://github.com/farend/redmine_theme_farend_bleuclair/commit/c06330c064d19b02ea3bb6ca7e9a5c37d6c33519)

Yes, both changes are required. The first change could be avoided if we keep the themes folder in public/themes, but I find very confusing to have the sources and the assets in the same public directory.

However, making the change in step 2 causes the theme to break in earlier versions of Redmine, so I'm looking for a way to make the change compatible with both versions.

Indeed, I don't see right now a way to keep them compatible, but I will investigate more in the following days.

Additionally, when I added the standard theme to app/assets/themes, it became a target for git staging. I would appreciate a change in .gitignore to prevent this.
[...]

I will update the ignore files for git, mercurial and svn, thanks!

Regarding the directory for non-standard themes, what do you think if we move them to themes in the root directory as we have for plugins? The structure will be the following:

app/assets/themes
  +- alternate/
  +- classic/

themes/
  +- <theme name>/
       |
       +- favicon/
       |    +- <favicon file> (e.g. favicon.ico, favicon.png)
       |
       +- javascripts/
       |    +- theme.js
       |
       +- stylesheets/
            +- application.css

In this way, when you install Redmine and you want to customize it, you work only with themes and plugins directories, you don't need to browse the internal structure from app.

Marius BĂLTEANU wrote in #note-20:

Regarding the directory for non-standard themes, what do you think if we move them to themes in the root directory as we have for plugins? The structure will be the following:
[...]

In this way, when you install Redmine and you want to customize it, you work only with themes and plugins directories, you don't need to browse the internal structure from app.

I agree. Having a similar structure to the plugins directory ensures consistency and should make it easier to understand for both new users and existing users.

I found ThemesTest#test_old_theme_compatibility test fails. https://www.redmine.org/builds/logs/build_trunk_mysql_ruby-3.2_1247.html
Probably because the diff in the 0001-Add-asset-path-to-default-to-support-old-style-theme.patch test/fixtures/themes/foo_theme/stylesheets/application.css file was missing when it was committed in r23173.

Error:
ThemesTest#test_old_theme_compatibility:
Errno::ENOENT: No such file or directory @ dir_initialize - /var/lib/redmine/test/fixtures/themes/foo_theme
    lib/redmine/themes.rb:115:in `asset_paths'
    test/integration/lib/redmine/themes_test.rb:115:in `test_old_theme_compatibility'

Mizuki ISHIKAWA wrote in #note-31:

I found ThemesTest#test_old_theme_compatibility test fails. https://www.redmine.org/builds/logs/build_trunk_mysql_ruby-3.2_1247.html
Probably because the diff in the 0001-Add-asset-path-to-default-to-support-old-style-theme.patch test/fixtures/themes/foo_theme/stylesheets/application.css file was missing when it was committed in r23173.

[...]

Fixed in r23175. Thank you.

The current code seems to be warned by rubocop. https://github.com/redmine/redmine/actions/runs/11639710239/job/32416198818

You can fix it with the following diff:

diff --git a/test/integration/lib/redmine/themes_test.rb b/test/integration/lib/redmine/themes_test.rb
index 7dc6b8df0..10cfac660 100644
--- a/test/integration/lib/redmine/themes_test.rb
+++ b/test/integration/lib/redmine/themes_test.rb
@@ -117,9 +117,9 @@ class ThemesTest < Redmine::IntegrationTest
     Rails.application.assets.load_path.clear_cache

     asset = Rails.application.assets.load_path.find('themes/foo_theme/application.css')
-    get "/assets/#{asset.digested_path.to_s}" 
+    get "/assets/#{asset.digested_path}" 

     assert_response :success
-    assert_match %r{url\(\"/assets/application-\w+\.css\"\)}, response.body
+    assert_match %r{url\("/assets/application-\w+\.css"\)}, response.body
   end
 end

Mizuki ISHIKAWA wrote in #note-33:

The current code seems to be warned by rubocop. https://github.com/redmine/redmine/actions/runs/11639710239/job/32416198818

It was already corrected in r23176. Thank you very much.

Redmine does not start in production after r23174.

$ bin/rails s -e production
=> Booting Puma
=> Rails 7.2.2 application starting in production
=> Run `bin/rails server --help` for more startup options
Exiting
/redmine/config/initializers/10-patches.rb:125:in `initialize': undefined local variable or method `manifest_path' for an instance of Propshaft::Assembly (NameError)

      if Rails.application.config.assets.redmine_detect_update && (!manifest_path.exist? || manifest_outdated?)
                                                                    ^^^^^^^^^^^^^
Did you mean?  manifest_outdated?
    from /bundle/ruby/3.3.0/gems/propshaft-1.1.0/lib/propshaft/railtie.rb:43:in `new'
    from /bundle/ruby/3.3.0/gems/propshaft-1.1.0/lib/propshaft/railtie.rb:43:in `block in <class:Railtie>'
    from /bundle/ruby/3.3.0/gems/activesupport-7.2.2/lib/active_support/lazy_load_hooks.rb:94:in `block in execute_hook'

This error appears to be caused by the following change in Propshaft v1.0.0.
https://github.com/rails/propshaft/pull/195

Takashi Kato, do you have any ideea why we load automatically the assets also from plugins/<plugin_name>/app/assets?