Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fixed "can't modify frozen String" crash (bsc#1125006) #421

Merged
merged 2 commits into from Feb 19, 2019

Conversation

lslezak
Copy link
Member

@lslezak lslezak commented Feb 18, 2019

  • The String returned from gettext is frozen and cannot be modified
  • Lazy initialization to make testing easier
  • 3.2.17

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.04%) to 63.659% when pulling 9f4a0d6 on frozen_string_crash into 5fa9ea5 on SLE-12-SP3.

@dgdavid
Copy link
Member

dgdavid commented Feb 18, 2019

Nice catch, @lslezak

The String returned from gettext is frozen and cannot be modified

I am still wondering how do you realized about that? I did the below unsuccessful dummy test

require "yast"
require "yast/i18n"

class SimpleRegistrationTest
  include Yast::I18n

  def initialize
    textdomain "registration"
  end
end
irb(main):001:0> test = SimpleRegistrationTest.new
=> #<SimpleRegistrationTest:0x000000000138bc28 @my_textdomain=["registration"]>                      
irb(main):002:0> out = test._("<p>The addons listed below are registered but not installed: </p>")         
=> "<p>The addons listed below are registered but not installed: </p>"                                                        
irb(main):003:0> out.frozen?
=> false

Another question, means that that could we get more can't modify frozen String crashes from now on those places where the << operator has been used to append text to a previous translation?

Thank you!

Copy link
Member

@dgdavid dgdavid left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 👍, just a grammar nitpicking.

allow(subject).to receive(:_), &:freeze
end

context "when the there is an registered but not installed product" do
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

NP: "When the there is an registered but not installed product"

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, I edited the text, but forgot some leftovers... thanks!

@lslezak
Copy link
Member Author

lslezak commented Feb 19, 2019

I am still wondering how do you realized about that? I did the below unsuccessful dummy test

It depends whether the translation for the message is available or not.

Translation Available

> rpm -q yast2-trans-cs
yast2-trans-cs-84.87.20180514.157a0650d-lp150.1.1.noarch
> LC_ALL=cs_CZ.UTF-8 ruby -ryast -e 'include Yast::I18n; textdomain "base"; puts _("&Yes"); puts _("&Yes").frozen?'
&Ano
true

So the result is a frozen translated string.

Translation Not Available

> rpm -q yast2-trans-es
package yast2-trans-es is not installed
> LC_ALL=es_ES.UTF-8 ruby -ryast -e 'include Yast::I18n; textdomain "base"; puts _("&Yes"); puts _("&Yes").frozen?'
&Yes
false

In this case you'll get the original untranslated and not frozen string.

The problem is that when running the unit tests in Travis, Jenkins or in OBS no translation is installed and the tests run in English locale. However, on users system the translations are usually available. That means it's hard to test it.

I'll open the topic at yast-devel@, a possible solution would be to change

- found ? Translation._(str) : str
+ found ? Translation._(str) : str.freeze

at https://github.com/yast/yast-ruby-bindings/blob/4652448963160abc234d1199f06bf10f08989cf5/src/ruby/yast/i18n.rb#L63 so it behaves consistently.

@lslezak lslezak merged commit e545d5f into SLE-12-SP3 Feb 19, 2019
@lslezak lslezak deleted the frozen_string_crash branch February 19, 2019 09:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants