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

Only perform regexes on Strings #3616

Merged
Merged
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Cast argument to string before performing regex
Recently we started getting warnings about calling #=~ on things like
booleans and arrays etc.

This commit ensures that we cast the argument to a string before
performing a regex on it to ensure that we aren't relying on what
calling a regex match type method on a boolean returns.
jacquesporveau authored and Dennis Marchand committed May 30, 2020
commit 5d6bdda43787a7bd30e8ec1f653999280dd2b19f
3 changes: 1 addition & 2 deletions core/lib/spree/preferences/preferable.rb
Original file line number Diff line number Diff line change
@@ -158,8 +158,7 @@ def convert_preference_value(value, type)
value.to_i
when :boolean
if !value ||
value == 0 ||
value =~ /\A(f|false|0)\Z/i ||
value.to_s =~ /\A(f|false|0|^)\Z/i ||
Copy link
Member

@aldesantis aldesantis May 22, 2020

Choose a reason for hiding this comment

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

Does it make sense to use this helper instead?

ActiveModel::Type::Boolean.new.cast(value)

Looking at the documentation, it seems to do what we need.

Copy link
Member

Choose a reason for hiding this comment

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

Yes, I prefer that. It's arguably a breaking change though and one that might be weird to use a config option to control. Thoughts?

Copy link
Member

Choose a reason for hiding this comment

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

Hmm, yeah, we may decide to either hold off on this until 3.1 or merge the current version now and migrate to ActiveModel's API in 3.1.

@solidusio/core-team any other thoughts?

Copy link
Member

Choose a reason for hiding this comment

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

I'm fine with merging as is, though I'm curious why you'd want to make the switch the the ActiveModel API in 3.1 rather than 3.0. Wouldn't that be a breaking change that would be better off rolling into 3.0?

Copy link
Member

@aldesantis aldesantis May 26, 2020

Choose a reason for hiding this comment

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

Sorry, that comment didn't make any sense. 😅 I meant 3.0, not 3.1. The problem is that ideally we don't want any changes at all in 3.0: we should just remove all deprecations introduced in previous versions of Solidus. The idea is that, if you're on 2.11 and not seeing any deprecations, you can safely upgrade to 3.0.

I think one approach could be to print a deprecation warning if the value of a configuration option is one of the values that is now being treated as true, but would be treated as false by ActiveModel. Something like the following:

if value.in?(ActiveModel::Type::Boolean::FALSE_VALUES - ['f', 'false', '0'])
  Spree::Deprecation.warn "#{value.inspect} will be treated as false starting in Solidus 3.0."
end

# ...

The check would have to be a bit more complex, since it looks like we currently treat any object that responds to #empty? and returns true as false, while ActiveRecord only seems to do it for empty strings, and even then, they return nil rather than false.

We would do this in 2.11, and then upgrade to the new behavior in 3.0.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, I agree; that would be ideal. I think we're getting away from the original intent of this PR, though. The goal here was to address a deprecation where true is being passed into this class which isn't actually handled by the above deprecation suggestion.

It's more work then I'm sure @jacquesporveau had intended to do here, but maybe we want to leave the existing logic, but wrap it in a conditional deprecation warning that's fired if the existing logic doesn't produce the same value as the ActiveModel version (potentially with a config option to use the new ActiveModel logic instead.) In v3.0 we remove the old logic and move forward with only the ActiveModel version.

@jacquesporveau Apologies for all the back and forth on this one. We're trying to be careful in deciding what belongs in v2.11 since it's our last chance to deprecate things ahead of Solidus v3.0.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sa'll good

(value.respond_to?(:empty?) && value.empty?)
false
else