Redmine

There're already tons of deprecation warnings on Ruby 2.7. The patch looks good, but you have to use Regex#match? instead of String#match?, because Redmine still supports Ruby 2.3.

Pavel Rosický wrote:

The patch looks good, but you have to use Regex#match? instead of String#match?, because Redmine still supports Ruby 2.3.

$ ruby -v
ruby 2.3.8p459 (2018-10-18 revision 65136) [x86_64-darwin18]
$ ruby test/functional/attachments_controller_test.rb
Run options: --seed 63689

# Running:

..................E

Error:
AttachmentsControllerTest#test_download_text_file:
NoMethodError: undefined method `match?' for "Rails Testing":String
Did you mean?  match
    app/controllers/application_controller.rb:647:in `filename_for_content_disposition'
    app/controllers/attachments_controller.rb:75:in `download'
    lib/redmine/sudo_mode.rb:64:in `sudo_mode'
    test/functional/attachments_controller_test.rb:302:in `test_download_text_file'

bin/rails test test/functional/attachments_controller_test.rb:301

I will update the patch. By the way, in ruby 2.3, both “String # match?” and “Regexp # match?” do not exist.
https://docs.ruby-lang.org/en/2.3.0/Regexp.html

root@0554350dda7e:/all-ruby# bin/ruby-2.3.8 -ve '//.match?("")'
ruby 2.3.8p459 (2018-10-18 revision 65136) [x86_64-linux]
-e:1:in `<main>': undefined method `match?' for //:Regexp (NoMethodError)
Did you mean?  match

I don't know why the build passed on ruby-2.3
https://www.redmine.org/builds/

Seiei Miyagi wrote:

I will update the patch. By the way, in ruby 2.3, both “String # match?” and “Regexp # match?” do not exist.
https://docs.ruby-lang.org/en/2.3.0/Regexp.html

[...]

I don't know why the build passed on ruby-2.3
https://www.redmine.org/builds/

Regexp#match? is defined by ActiveSupport for older versions of Ruby. Please see #28940 for details.

Seiei Miyagi wrote:

Updated the patch.

What do you think about the following code? It is simpler and very slightly efficient.

Index: app/controllers/application_controller.rb
===================================================================
--- app/controllers/application_controller.rb    (リビジョン 19319)
+++ app/controllers/application_controller.rb    (作業コピー)
@@ -644,7 +644,7 @@

   # Returns a string that can be used as filename value in Content-Disposition header
   def filename_for_content_disposition(name)
-    %r{(MSIE|Trident|Edge)}.match?(request.env['HTTP_USER_AGENT']) ? ERB::Util.url_encode(name) : name
+    %r{(MSIE|Trident|Edge)}.match?(request.env['HTTP_USER_AGENT'].to_s) ? ERB::Util.url_encode(name) : name
   end

   def api_request?

Go MAEDA wrote:

Seiei Miyagi wrote:

Updated the patch.

What do you think about the following code? It is simpler and very slightly efficient.

[...]

Looks great!

My code is calling request.env[] twice, thus your code is better.

fyi, the change will be probably reverted in the final release, see https://bugs.ruby-lang.org/issues/13083