Only perform regexes on Strings #3616

jacquesporveau · 2020-05-08T22:46:26Z

Description
We were getting warnings from Ruby 2.7

warning: deprecated Object#=~ is called on TrueClass; it always returns
nil

In the case that true was passed in as a preference here we would try to
perform a regex on it and get nil from value =~ //.

This meant that the conditional would not be met and this case would not
return false but instead return true.

This is the correct behaviour but it's pretty sketchy. We should not be
relying on what calling regex matching methods on things that aren't
strings return here.

Ref #3611

Checklist:

I have followed Pull Request guidelines
I have added a detailed description into each commit message
I have updated Guides and README accordingly to this change (if needed)
I have added tests to cover this change (if needed)
I have attached screenshots to this PR for visual changes (if needed)

jarednorman

I really don't like to see class checks, but I'm not sure I have an immediately better suggestion... unless maybe we added a short-circuit for the case when value is already boolean to the start of this conditional?

if [true, false].include?(value)
  value
elsif # ...the exist conditional

aldesantis · 2020-05-11T07:34:02Z

I like @jarednorman's idea of a shortcircuit, I think it's slightly better than checking the class. I haven't seen any examples in the wild, but I imagine there are cases where users have passed something that is not a String but still implements #=~ — we should still play nice in that case.

The only problem I see with the shortcircuit approach is that we will still call =~ on all other objects that are neither true nor false, so if the user passes an object that is neither a string nor a boolean, we'll still get that warning. Since that's not a supported use case and would be the user's mistake though, I'm not terribly worried about it.

jacquesporveau · 2020-05-11T17:03:11Z

Sounds like there is a learning opportunity here for me. I'd like to try and understand why we don't like to check classes. Could you give me a run down? Maybe that will help me get on the same page as far as looking for another solution.

jarednorman · 2020-05-11T18:16:34Z

There's often a desire in Ruby to embrace duck-typing. This usually means preferring to check an object responds to the method in question before calling it and not worrying about whether the object is of a particular class. Our code should just care that the responds to the methods we want to call ("quacks like a duck").

(Edit: grammar)

softr8 · 2020-05-12T17:09:15Z

It looks like bad idea, but what about:

when :boolean
  ( !value || value.to_s =~ /\A(f|false|0|^)\Z/i ) ? false : true

it checks for '0', 'f', 'false' and ''

jarednorman · 2020-05-13T21:43:27Z

It does look bad, but I'm having trouble thinking of a reason that we shouldn't do it.

jacquesporveau · 2020-05-14T03:01:19Z

Sry I've been inactive. I'll give that a go and see if we need to add anymore tests shortly.

core/lib/spree/preferences/preferable.rb

jacquesporveau · 2020-05-20T17:45:41Z

I think this is what we are looking for. Couldn't just paste in Edwin's solution because apparently we also supported empty arrays here too so needed to cover that as well.

aldesantis · 2020-05-22T09:50:10Z

core/lib/spree/preferences/preferable.rb

@@ -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 ||


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.

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?

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?

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?

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.

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.

gmacdougall

I'm good with this as is.

aldesantis

Discussed in the core team meeting, we decided to hold off on the ActiveModel API for the time being.

aldesantis · 2020-05-28T09:20:31Z

@jacquesporveau looks like CI is not running here, could you take a look? 🙏

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 · 2020-05-30T05:30:46Z

Looks like I had to authorize circle to have my code ran by CI. Weird.

Anyways it looks like everything but ci/circleci: postgres_rails_master_activestorage is passing which seems to be a common theme looking at the last couple of PRs. Seems like the CI workflow might be having a version problem or something like that.

I am not going to look into why that CI check is failing.

aldesantis · 2020-06-01T08:10:59Z

No worries, it's not related to the changes. Thanks @jacquesporveau!

jarednorman reviewed May 11, 2020

View reviewed changes

jacquesporveau force-pushed the jacquesporveau/ruby-warnings branch from 53a2754 to 86321c1 Compare May 20, 2020 17:38

houndci-bot reviewed May 20, 2020

View reviewed changes

core/lib/spree/preferences/preferable.rb Outdated Show resolved Hide resolved

core/lib/spree/preferences/preferable.rb Outdated Show resolved Hide resolved

jacquesporveau force-pushed the jacquesporveau/ruby-warnings branch from 86321c1 to 427b490 Compare May 20, 2020 17:44

aldesantis reviewed May 22, 2020

View reviewed changes

gmacdougall approved these changes May 27, 2020

View reviewed changes

aldesantis approved these changes May 27, 2020

View reviewed changes

jarednorman approved these changes May 27, 2020

View reviewed changes

jacquesporveau force-pushed the jacquesporveau/ruby-warnings branch from 427b490 to 08f8038 Compare May 30, 2020 00:22

jacquesporveau force-pushed the jacquesporveau/ruby-warnings branch from 08f8038 to 5d6bdda Compare May 30, 2020 05:01

aldesantis approved these changes Jun 1, 2020

View reviewed changes

aldesantis merged commit 8263d90 into solidusio:master Jun 1, 2020

jacquesporveau deleted the jacquesporveau/ruby-warnings branch June 1, 2020 16:31

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Only perform regexes on Strings #3616

Only perform regexes on Strings #3616

jacquesporveau commented May 8, 2020 •

edited

Loading

jarednorman left a comment

aldesantis commented May 11, 2020

jacquesporveau commented May 11, 2020

jarednorman commented May 11, 2020 •

edited

Loading

softr8 commented May 12, 2020

jarednorman commented May 13, 2020

jacquesporveau commented May 14, 2020

jacquesporveau commented May 20, 2020

aldesantis May 22, 2020 •

edited

Loading

jarednorman May 22, 2020

aldesantis May 25, 2020

jarednorman May 25, 2020

aldesantis May 26, 2020 •

edited

Loading

jarednorman May 26, 2020

jacquesporveau May 27, 2020

gmacdougall left a comment

aldesantis left a comment

aldesantis commented May 28, 2020

jacquesporveau commented May 30, 2020

aldesantis commented Jun 1, 2020

Only perform regexes on Strings #3616

Only perform regexes on Strings #3616

Conversation

jacquesporveau commented May 8, 2020 • edited Loading

jarednorman left a comment

Choose a reason for hiding this comment

aldesantis commented May 11, 2020

jacquesporveau commented May 11, 2020

jarednorman commented May 11, 2020 • edited Loading

softr8 commented May 12, 2020

jarednorman commented May 13, 2020

jacquesporveau commented May 14, 2020

jacquesporveau commented May 20, 2020

aldesantis May 22, 2020 • edited Loading

Choose a reason for hiding this comment

jarednorman May 22, 2020

Choose a reason for hiding this comment

aldesantis May 25, 2020

Choose a reason for hiding this comment

jarednorman May 25, 2020

Choose a reason for hiding this comment

aldesantis May 26, 2020 • edited Loading

Choose a reason for hiding this comment

jarednorman May 26, 2020

Choose a reason for hiding this comment

jacquesporveau May 27, 2020

Choose a reason for hiding this comment

gmacdougall left a comment

Choose a reason for hiding this comment

aldesantis left a comment

Choose a reason for hiding this comment

aldesantis commented May 28, 2020

jacquesporveau commented May 30, 2020

aldesantis commented Jun 1, 2020

jacquesporveau commented May 8, 2020 •

edited

Loading

jarednorman commented May 11, 2020 •

edited

Loading

aldesantis May 22, 2020 •

edited

Loading

aldesantis May 26, 2020 •

edited

Loading