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

Make context safe for interpolation #15

Merged
merged 1 commit into from May 23, 2019

Conversation

xymbol
Copy link
Contributor

@xymbol xymbol commented Jan 14, 2018

Fixes an issue related to colliding context and i18n reserved keys, which results in application name being rendered on translation lookup failure. The chance of encountering this problem is real as some of these names are relatively common terms.

  • cascade
  • deep_interpolation
  • fallback
  • fallback_in_progress
  • format
  • object
  • raise
  • resolve
  • scope
  • separator
  • throw

Please see: https://github.com/svenfuchs/i18n/blob/405b672ed121d48d2e41140da29f63b350b9899e/lib/i18n.rb#L15.

This change filters out reserved keys from context before translation.

load_translations(users: { show: 'User' })
allow(helper).to receive_message_chain(:controller, :view_assigns).and_return('scope' => 'Foo')

expect(helper.title).to eq('User')
Copy link
Collaborator

Choose a reason for hiding this comment

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

Prefer double-quoted strings unless you need single quotes to avoid extra backslashes for escaping.

stub_rails
stub_controller_and_action(:users, :show)
load_translations(users: { show: 'User' })
allow(helper).to receive_message_chain(:controller, :view_assigns).and_return('scope' => 'Foo')
Copy link
Collaborator

Choose a reason for hiding this comment

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

Line is too long. [99/80]
Prefer double-quoted strings unless you need single quotes to avoid extra backslashes for escaping.

it 'makes context safe to be used as interpolation options' do
stub_rails
stub_controller_and_action(:users, :show)
load_translations(users: { show: 'User' })
Copy link
Collaborator

Choose a reason for hiding this comment

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

Prefer double-quoted strings unless you need single quotes to avoid extra backslashes for escaping.

@@ -51,6 +51,15 @@
expect(helper.title(greeting: 'Hello')).to eq('Hello Caleb')
end

it 'makes context safe to be used as interpolation options' do
Copy link
Collaborator

Choose a reason for hiding this comment

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

Prefer double-quoted strings unless you need single quotes to avoid extra backslashes for escaping.

@@ -59,6 +59,10 @@ def adjusted_action_name(action_name)
action_name
end
end

def safe_context
context.except *I18n::RESERVED_KEYS
Copy link
Collaborator

Choose a reason for hiding this comment

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

Ambiguous splat operator. Parenthesize the method arguments if it's surely a splat operator, or add a whitespace to the right of the * if it should be a multiplication.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.2%) to 95.556% when pulling 211f831 on xymbol:am-safe-context into 52c24d5 on calebthompson:master.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.2%) to 95.556% when pulling be13d7c on xymbol:am-safe-context into 52c24d5 on calebthompson:master.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.4%) to 95.745% when pulling 5b1f9f9 on xymbol:am-safe-context into 52c24d5 on calebthompson:master.

@xymbol
Copy link
Contributor Author

xymbol commented Aug 24, 2018

@calebthompson Hi, just a mention to see if this can be merged in (or not). Thanks!

load_translations(users: { show: 'User' })
allow(helper).to receive_message_chain(:controller, :view_assigns).and_return('scope' => 'Foo')

expect(helper.title).to eq('User')
Copy link
Collaborator

Choose a reason for hiding this comment

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

Style/StringLiterals: Prefer double-quoted strings unless you need single quotes to avoid extra backslashes for escaping.

stub_rails
stub_controller_and_action(:users, :show)
load_translations(users: { show: 'User' })
allow(helper).to receive_message_chain(:controller, :view_assigns).and_return('scope' => 'Foo')
Copy link
Collaborator

Choose a reason for hiding this comment

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

Metrics/LineLength: Line is too long. [99/80]
Style/StringLiterals: Prefer double-quoted strings unless you need single quotes to avoid extra backslashes for escaping.

it 'makes context safe to be passed as interpolation options' do
stub_rails
stub_controller_and_action(:users, :show)
load_translations(users: { show: 'User' })
Copy link
Collaborator

Choose a reason for hiding this comment

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

Style/StringLiterals: Prefer double-quoted strings unless you need single quotes to avoid extra backslashes for escaping.

@@ -51,6 +51,15 @@
expect(helper.title(greeting: 'Hello')).to eq('Hello Caleb')
end

it 'makes context safe to be passed as interpolation options' do
Copy link
Collaborator

Choose a reason for hiding this comment

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

Style/StringLiterals: Prefer double-quoted strings unless you need single quotes to avoid extra backslashes for escaping.

@calebhearth calebhearth merged commit 938262a into calebhearth:master May 23, 2019
@xymbol xymbol deleted the am-safe-context branch May 28, 2019 19:33
calebhearth added a commit that referenced this pull request Aug 20, 2020
* Make context safe for interpolation by filtering out reserved keys for
  interpolation. See [#15].
* Be explicit that we are passing in double splat options (Fixes
  warnings in Ruby 2.7) See [#19]

[#15]: #15
[#19]: https://github.com/calebthompson/title/pull/19/files
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

4 participants