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

Don't leak Object constants in core_ext/module/qualified_const #17845

Merged
merged 1 commit into from Dec 16, 2015

Conversation

Projects
None yet
4 participants
@gsamokovarov
Contributor

gsamokovarov commented Nov 29, 2014

Bumped on this one recently.

Actually, I think we can remove this altogether for Rails 5. Since Ruby 2, Object.const_get actually works for qualified constant names and doesn't raises an exception for absolute paths anymore.

>> Object.const_get '::String'
=> String
>> RUBY_VERSION
=> "2.1.4"
>> Object.const_get '::ActiveSupport::OrderedOptions'
=> ActiveSupport::OrderedOptions

I grepped the code and its not used anywhere but its test.

♡  ag "qualified_const_get"
activesupport/lib/active_support/core_ext/module/qualified_const.rb
26:# Object.const_get('::String') raises NameError and so does qualified_const_get.
39:  def qualified_const_get(path)
52:    mod = mod_name.empty? ? self : qualified_const_get(mod_name)

activesupport/test/core_ext/module/qualified_const_test.rb
59:  test "qualified_const_get" do
60:    assert_equal false, Object.qualified_const_get("QualifiedConstTestMod::X")
61:    assert_equal false, QualifiedConstTestMod.qualified_const_get("X")
62:    assert_equal 1, QualifiedConstTestMod.qualified_const_get("M::X")
63:    assert_equal 1, QualifiedConstTestMod.qualified_const_get("N::X")
64:    assert_equal 2, QualifiedConstTestMod.qualified_const_get("M::C::X")
66:    assert_raise(NameError) { QualifiedConstTestMod.qualified_const_get("M::C::Y")}
95:    assert_raise_with_message(NameError, "wrong constant name ::X") { Object.qualified_const_get("::X")}
96:    assert_raise_with_message(NameError, "wrong constant name ::X") { Object.qualified_const_get("::X::Y")}

guides/source/3_2_release_notes.md
508:* Defined new methods `Module#qualified_const_defined?`, `Module#qualified_const_get` and `Module#qualified_const_set` that are analogous to the corresponding methods in the standard API, but accept qualified constant names.

guides/source/active_support_core_extensions.md
748:The new methods are `qualified_const_defined?`, `qualified_const_get`, and
754:Object.qualified_const_get("Math::PI")            # => 3.141592653589793
761:Math.qualified_const_get("E") # => 2.718281828459045
1704:  mod = mod_name.empty? ? self : qualified_const_get(mod_name)
@rafaelfranca

This comment has been minimized.

Show comment
Hide comment
@rafaelfranca

rafaelfranca Nov 29, 2014

Member

That class was public. So we would have to deprecate all the code.

Member

rafaelfranca commented Nov 29, 2014

That class was public. So we would have to deprecate all the code.

@gsamokovarov

This comment has been minimized.

Show comment
Hide comment
@gsamokovarov

gsamokovarov Nov 29, 2014

Contributor

Tried doing that and the whole test suite light up with deprecation warnings.

Module#qualified_const_defined? is used throughout the autoloading code and new Module#const_defined? doesn't have the same semantics. Its only Module#const_get that behaves similarly to Module#qualified_const_get.

Maybe we can implement Module#qualified_const_get in terms of native Module#const_get for speed, but other than that, we won't be able to touch the other methods.

Contributor

gsamokovarov commented Nov 29, 2014

Tried doing that and the whole test suite light up with deprecation warnings.

Module#qualified_const_defined? is used throughout the autoloading code and new Module#const_defined? doesn't have the same semantics. Its only Module#const_get that behaves similarly to Module#qualified_const_get.

Maybe we can implement Module#qualified_const_get in terms of native Module#const_get for speed, but other than that, we won't be able to touch the other methods.

@rafaelfranca

This comment has been minimized.

Show comment
Hide comment
@rafaelfranca
Member

rafaelfranca commented Jan 5, 2015

cc @fxn

@fxn

This comment has been minimized.

Show comment
Hide comment
@fxn

fxn Jan 5, 2015

Member

ACK!

I am in an immersion working on the contrib app these days, gonna get back here soon.

Member

fxn commented Jan 5, 2015

ACK!

I am in an immersion working on the contrib app these days, gonna get back here soon.

fxn added a commit that referenced this pull request Jan 29, 2015

let dependencies use Module#const_defined?
Module#const_defined? accepts constant paths in modern Ruby, we no longer
need our qualified_* extensions.

References #17845.
@fxn

This comment has been minimized.

Show comment
Hide comment
@fxn

fxn Jan 29, 2015

Member

I have removed internal usage. I think we should just deprecate these extensions for later removal.

Member

fxn commented Jan 29, 2015

I have removed internal usage. I think we should just deprecate these extensions for later removal.

@gsamokovarov

This comment has been minimized.

Show comment
Hide comment
@gsamokovarov

gsamokovarov Jan 29, 2015

Contributor

@fxn I can try to issue a deprecation warning only once, so we don't flood logs with them. Should I do it in this PR?

Contributor

gsamokovarov commented Jan 29, 2015

@fxn I can try to issue a deprecation warning only once, so we don't flood logs with them. Should I do it in this PR?

@fxn

This comment has been minimized.

Show comment
Hide comment
@fxn

fxn Jan 29, 2015

Member

Hmmm.... in principle we could warn always, that's how we normally do it. @rafaelfranca WDYT?

Member

fxn commented Jan 29, 2015

Hmmm.... in principle we could warn always, that's how we normally do it. @rafaelfranca WDYT?

@sgrif

This comment has been minimized.

Show comment
Hide comment
@sgrif

sgrif Nov 23, 2015

Member

@gsamokovarov Are you still interested in working on this? I agree that we should always warn, not warn once.

Member

sgrif commented Nov 23, 2015

@gsamokovarov Are you still interested in working on this? I agree that we should always warn, not warn once.

@sgrif sgrif added this to the 5.0.0 milestone Nov 23, 2015

@gsamokovarov

This comment has been minimized.

Show comment
Hide comment
@gsamokovarov

gsamokovarov Nov 24, 2015

Contributor

@sgrif Sure! Deprecated the qualified_const_ methods, fixed deprecations in the tests and added a changelog entry.

Contributor

gsamokovarov commented Nov 24, 2015

@sgrif Sure! Deprecated the qualified_const_ methods, fixed deprecations in the tests and added a changelog entry.

rafaelfranca added a commit that referenced this pull request Dec 16, 2015

Merge pull request #17845 from gsamokovarov/qualified-const
Don't leak Object constants in core_ext/module/qualified_const

@rafaelfranca rafaelfranca merged commit e134e63 into rails:master Dec 16, 2015

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@rafaelfranca rafaelfranca modified the milestones: 5.0.0 [temp], 5.0.0 Dec 30, 2015

y-yagi added a commit to y-yagi/rails that referenced this pull request May 23, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment