sparklemotion / mechanize

Mechanize is a ruby library that makes automated web interaction easy.
https://www.rubydoc.info/gems/mechanize/
MIT License
4.39k stars 473 forks source link

fix: allow frozen string body argument on Mechanize::Page#initialize #609

Closed nishidayuya closed 1 year ago

nishidayuya commented 1 year ago

Mechanize::Page#initialize calls String#force_encoding, so FrozenError is occurred.

This pull-request fixes it.


On mechanize-2.8.5 (and currently main branch 4c8d3d2263c091b4ffe8e981b6776e5ae7cb80c9 ), I saw following:

$ cat a.rb
# frozen_string_literal: true

require "mechanize"

Mechanize::Page.new(
  URI("https://example.org"),
  nil,
  "page content", # this argument!
  200,
  Mechanize.new,
)
$ ruby a.rb
/home/yuya/.anyenv/envs/rbenv/versions/3.2.2/lib/ruby/gems/3.2.0/gems/mechanize-2.8.5/lib/mechanize/page.rb:46:in `force_encoding': can't modify frozen String: "page content" (FrozenError)
        from /home/yuya/.anyenv/envs/rbenv/versions/3.2.2/lib/ruby/gems/3.2.0/gems/mechanize-2.8.5/lib/mechanize/page.rb:46:in `initialize'
        from a.rb:5:in `new'
        from a.rb:5:in `<main>'
flavorjones commented 1 year ago

Thank you for submitting this! I've kicked off CI.

flavorjones commented 1 year ago

I'm unsure if force_encoding needs to be called at all, I'd like to investigate a bit more.

flavorjones commented 1 year ago

See #610 for an alternative solution.

nishidayuya commented 1 year ago

See #610 for an alternative solution.

I see #610 :eyes: , and I see :tada: . After #610 is merged, this pull-request must be closed.

flavorjones commented 1 year ago

It seems like the call to force_encoding isn't necessary, so I've merged #610 instead. Thank you for reporting this issue!

flavorjones commented 1 year ago

I've cut v2.9.0 with the change to fix the issue reported here!