Patch #32527

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

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

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

0%

Category:Ruby support
Target version:-

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

0001-Fix-ruby-2.7-warning-given-argument-is-nil-this-will.patch Magnifier (1022 Bytes) Seiei Miyagi, 2019-11-28 18:10

0001-Fix-ruby-2.7-warning-given-argument-is-nil-this-will.patch Magnifier (940 Bytes) Seiei Miyagi, 2019-11-29 16:28


Related issues

Related to Redmine - Feature #31500: Ruby 2.7 support Closed

Associated revisions

Revision 19324
Added by Go MAEDA almost 2 years ago

Ruby 2.7: Regexp#match and Regexp#match? with a nil argument are deprecated (#32527, #31500).

History

#1 Updated by Pavel Rosický almost 2 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.

#2 Updated by Go MAEDA almost 2 years ago

#3 Updated by Go MAEDA almost 2 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

#4 Updated by Seiei Miyagi almost 2 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/

#5 Updated by Go MAEDA almost 2 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.

#6 Updated by Seiei Miyagi almost 2 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.

#7 Updated by Go MAEDA almost 2 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?

#8 Updated by Seiei Miyagi almost 2 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.

#9 Updated by Go MAEDA almost 2 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.

#10 Updated by Pavel Rosický almost 2 years ago

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

Also available in: Atom PDF