Project

General

Profile

Actions

Patch #32527

closed

Fix ruby 2.7 warning: given argument is nil; this will raise a TypeError in the next release

Added by Seiei Miyagi almost 5 years ago. Updated almost 5 years ago.

Status:
Closed
Priority:
Normal
Assignee:
-
Category:
Ruby support
Target version:
-
Start date:
Due date:
% Done:

0%

Estimated time:

Description

the `request.env['HTTP_USER_AGENT']` may `nil`.

e.g. `curl -A '' http://localhost:3000`

Quote from ruby 2.7 preview3 NEWS file:

Regexp#match and Regexp#match? with a nil argument are deprecated and will raise a TypeError in the next release. [Feature \#13083]

https://github.com/ruby/ruby/blob/v2_7_0_preview3/NEWS#core-classes-updates-outstanding-ones-only

cf. https://bugs.ruby-lang.org/issues/13083


Files


Related issues

Related to Redmine - Feature #31500: Ruby 2.7 supportClosedGo MAEDA

Actions
Actions #1

Updated by Pavel Rosický almost 5 years ago

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.

Actions #2

Updated by Go MAEDA almost 5 years ago

Actions #3

Updated by Go MAEDA almost 5 years ago

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

Updated by Seiei Miyagi almost 5 years ago

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/

Actions #5

Updated by Go MAEDA almost 5 years ago

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.

Actions #6

Updated by Seiei Miyagi almost 5 years ago

Go MAEDA wrote:

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.

I fully understand. thank you!

Updated the patch.

Actions #7

Updated by Go MAEDA almost 5 years ago

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?
Actions #8

Updated by Seiei Miyagi almost 5 years ago

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.

Actions #9

Updated by Go MAEDA almost 5 years ago

  • Category set to Ruby support
  • Status changed from New to Closed

Committed the patch in #32527#note-7 as a part of #31500.

Thank you for reporting this issue.

Actions #10

Updated by Pavel Rosický almost 5 years ago

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

Actions

Also available in: Atom PDF