Deprecate string options in URL helpers #17743

Closed
wants to merge 1 commit into
from

Conversation

Projects
None yet
8 participants
@mrgilman
Contributor

mrgilman commented Nov 24, 2014

Fixes #16958

I pretty much implemented @bronzle's fix in the issue comments, so he should have credit too.

@zzak

This comment has been minimized.

Show comment
Hide comment
@zzak

zzak Nov 24, 2014

Member

I think this would need a changelog

Member

zzak commented Nov 24, 2014

I think this would need a changelog

@sgrif

This comment has been minimized.

Show comment
Hide comment
@sgrif

sgrif Nov 24, 2014

Member

If you want to credit someone else, put both names in the changelog, and at the bottom of the commit message put [your handle & their handle]

Member

sgrif commented Nov 24, 2014

If you want to credit someone else, put both names in the changelog, and at the bottom of the commit message put [your handle & their handle]

@@ -293,6 +293,19 @@ def handle_positional_args(controller_options, inner_options, args, result, path
result.merge!(inner_options)
end
+
+ DEPRECATED_STRING_OPTIONS = %w[controller action].freeze

This comment has been minimized.

@sgrif

sgrif Nov 24, 2014

Member

Should this be a set?

@sgrif

sgrif Nov 24, 2014

Member

Should this be a set?

This comment has been minimized.

@bronzle

bronzle Nov 24, 2014

Contributor

Probably a good idea.

@bronzle

bronzle Nov 24, 2014

Contributor

Probably a good idea.

+ DEPRECATED_STRING_OPTIONS = %w[controller action].freeze
+
+ def deprecate_string_options(options = {})
+ if options && options.keys.any? { |k| DEPRECATED_STRING_OPTIONS.include?(k) }

This comment has been minimized.

@sgrif

sgrif Nov 24, 2014

Member

We can avoid allocating the keys array by changing this to options.each_key.any?

@sgrif

sgrif Nov 24, 2014

Member

We can avoid allocating the keys array by changing this to options.each_key.any?

This comment has been minimized.

@bronzle

bronzle Nov 24, 2014

Contributor

also good idea.

@bronzle

bronzle Nov 24, 2014

Contributor

also good idea.

+
+ def deprecate_string_options(options = {})
+ if options && options.keys.any? { |k| DEPRECATED_STRING_OPTIONS.include?(k) }
+ ActiveSupport::Deprecation.warn('Calling URL Helpers with string keys is deprecated.')

This comment has been minimized.

@sgrif

sgrif Nov 24, 2014

Member

Can we inform the user what changes they need to make to their code?

@sgrif

sgrif Nov 24, 2014

Member

Can we inform the user what changes they need to make to their code?

This comment has been minimized.

@bronzle

bronzle Nov 24, 2014

Contributor

we could do, something more like:

# ...
if options && deprecated_options = options.keys.any? { |k| DEPRECATED_STRING_OPTIONS.include?(k) }
  ActiveSupport::Deprecation.warn("Calling URL Helpers with string keys (#{deprecated_options.join(', ')}) is deprecated. Please call with symbols instead.")
# ...

or something even better

@bronzle

bronzle Nov 24, 2014

Contributor

we could do, something more like:

# ...
if options && deprecated_options = options.keys.any? { |k| DEPRECATED_STRING_OPTIONS.include?(k) }
  ActiveSupport::Deprecation.warn("Calling URL Helpers with string keys (#{deprecated_options.join(', ')}) is deprecated. Please call with symbols instead.")
# ...

or something even better

+ ActiveSupport::Deprecation.warn('Calling URL Helpers with string keys is deprecated.')
+ DEPRECATED_STRING_OPTIONS.each do |option|
+ value = options.delete(option)
+ options[option.to_sym] = value

This comment has been minimized.

@sgrif

sgrif Nov 24, 2014

Member

Is this going to assign keys that weren't previously present?

@sgrif

sgrif Nov 24, 2014

Member

Is this going to assign keys that weren't previously present?

This comment has been minimized.

@sgrif

sgrif Nov 24, 2014

Member

Also if I'm a terrible person and defined my route as 'controller' => 'foo', :action => 'bar', will this change my :action to nil?

@sgrif

sgrif Nov 24, 2014

Member

Also if I'm a terrible person and defined my route as 'controller' => 'foo', :action => 'bar', will this change my :action to nil?

This comment has been minimized.

@bronzle

bronzle Nov 24, 2014

Contributor

regarding assigning items that were not previously there, I originally had: options[options.to_sym] ||= value if value which gives the symbol option precedent as well as only assign when value is not nil (actually might want unless value.nil? instead?)

I think this guards against your second point, but I'm not sure without thinking harder (don't want to)

@bronzle

bronzle Nov 24, 2014

Contributor

regarding assigning items that were not previously there, I originally had: options[options.to_sym] ||= value if value which gives the symbol option precedent as well as only assign when value is not nil (actually might want unless value.nil? instead?)

I think this guards against your second point, but I'm not sure without thinking harder (don't want to)

@mrgilman

This comment has been minimized.

Show comment
Hide comment
@mrgilman

mrgilman Nov 24, 2014

Contributor

Thanks for the feedback! Made the suggested changes and added a changelog entry.

Contributor

mrgilman commented Nov 24, 2014

Thanks for the feedback! Made the suggested changes and added a changelog entry.

@rafaelfranca

This comment has been minimized.

Show comment
Hide comment
@rafaelfranca

rafaelfranca Nov 24, 2014

Member

I pretty much implemented @bronzle's fix in the issue comments, so he should have credit too.

❤️

Member

rafaelfranca commented Nov 24, 2014

I pretty much implemented @bronzle's fix in the issue comments, so he should have credit too.

❤️

+ ActiveSupport::Deprecation.warn(msg)
+ DEPRECATED_STRING_OPTIONS.each do |option|
+ value = options.delete(option)
+ options[option.to_sym] ||= value if value

This comment has been minimized.

@rafaelfranca

rafaelfranca Nov 24, 2014

Member

We have a pretty complex logic here without test. For example, why were are using ||=? Why we need to check if value is present?

I'd add tests for these cases to make sure we don't have regressions on this code.

@rafaelfranca

rafaelfranca Nov 24, 2014

Member

We have a pretty complex logic here without test. For example, why were are using ||=? Why we need to check if value is present?

I'd add tests for these cases to make sure we don't have regressions on this code.

@sgrif

This comment has been minimized.

Show comment
Hide comment
@sgrif

sgrif Nov 24, 2014

Member

This looks good to me

Member

sgrif commented Nov 24, 2014

This looks good to me

+ DEPRECATED_STRING_OPTIONS = Set.new(%w[controller action])
+
+ def deprecate_string_options(options = {})
+ if options && options.each_key.any? { |k| DEPRECATED_STRING_OPTIONS.include?(k) }

This comment has been minimized.

@rafaelfranca

rafaelfranca Nov 24, 2014

Member

Maybe it would be better to write

if options && (deprecated_string_options = options & DEPRECATED_STRING_OPTIONS)

and we use the deprecated_string_options variable to print the correct message and to iterate on the each loop.

This way to deleting an option that doesn't existe there.

@rafaelfranca

rafaelfranca Nov 24, 2014

Member

Maybe it would be better to write

if options && (deprecated_string_options = options & DEPRECATED_STRING_OPTIONS)

and we use the deprecated_string_options variable to print the correct message and to iterate on the each loop.

This way to deleting an option that doesn't existe there.

This comment has been minimized.

@sgrif

sgrif Nov 24, 2014

Member

That would need to be on the keys specifically. I think that'd be more readable on two lines, as well. :)

@sgrif

sgrif Nov 24, 2014

Member

That would need to be on the keys specifically. I think that'd be more readable on two lines, as well. :)

@rafaelfranca rafaelfranca self-assigned this Nov 24, 2014

@rafaelfranca rafaelfranca added this to the 4.2.0 milestone Nov 24, 2014

@@ -293,6 +293,22 @@ def handle_positional_args(controller_options, inner_options, args, result, path
result.merge!(inner_options)
end
+
+ DEPRECATED_STRING_OPTIONS = Set.new(%w[controller action])

This comment has been minimized.

@rafaelfranca

rafaelfranca Nov 25, 2014

Member

Does it need to be a set now?

@rafaelfranca

rafaelfranca Nov 25, 2014

Member

Does it need to be a set now?

This comment has been minimized.

@sgrif

sgrif Nov 25, 2014

Member

Nope. I'm about to merge, will fix as I merge.

@sgrif

sgrif Nov 25, 2014

Member

Nope. I'm about to merge, will fix as I merge.

actionpack/CHANGELOG.md
+
+ Use symbols instead. Fixes #16958.
+
+ *Byron Bischoff & Melanie Gilman*

This comment has been minimized.

@tgxworld

tgxworld Nov 25, 2014

Contributor

The format for this should be *Byron Bischoff*, *Melanie Gilman* 😸

@tgxworld

tgxworld Nov 25, 2014

Contributor

The format for this should be *Byron Bischoff*, *Melanie Gilman* 😸

actionpack/CHANGELOG.md
@@ -1,3 +1,9 @@
+* Deprecate use of string keys in URL helpers.
+
+ Use symbols instead. Fixes #16958.

This comment has been minimized.

@tgxworld

tgxworld Nov 25, 2014

Contributor

Could we also move Fixes #16958. to a new line? You may look at the other CHANGELOG entries for reference 😄

@tgxworld

tgxworld Nov 25, 2014

Contributor

Could we also move Fixes #16958. to a new line? You may look at the other CHANGELOG entries for reference 😄

Deprecate string options in URL helpers
Fixes #16958

[Byron Bischoff & Melanie Gilman]
@sgrif

This comment has been minimized.

Show comment
Hide comment
@sgrif

sgrif Nov 25, 2014

Member

Gah... We tried to update the changelog format at the same time. This was merged.

Member

sgrif commented Nov 25, 2014

Gah... We tried to update the changelog format at the same time. This was merged.

@sgrif sgrif closed this Nov 25, 2014

zzak added a commit to zzak/rails that referenced this pull request Nov 25, 2014

sgrif added a commit that referenced this pull request Nov 25, 2014

sachin004 added a commit to sachin004/rails that referenced this pull request Dec 13, 2014

@heaven

This comment has been minimized.

Show comment
Hide comment
@heaven

heaven May 15, 2015

Something is wrong with this:

admin_dashboard_path(params)

Where params is an instance of ActionController::Parameters, produces:

DEPRECATION WARNING: Calling URL helpers with string keys controller, action is deprecated. ...
/dashboard

heaven commented May 15, 2015

Something is wrong with this:

admin_dashboard_path(params)

Where params is an instance of ActionController::Parameters, produces:

DEPRECATION WARNING: Calling URL helpers with string keys controller, action is deprecated. ...
/dashboard
@agis

This comment has been minimized.

Show comment
Hide comment
@agis

agis Sep 17, 2015

Contributor

@heaven Indeed. Follow the discussion in #21648.

Contributor

agis commented Sep 17, 2015

@heaven Indeed. Follow the discussion in #21648.

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