Revert #22610 by default and make it opt-in #41412

intrip · 2021-02-11T17:03:44Z

Summary

Change #22610 in order to revert the behaviour by default.
If the option all: true is given applies the new behaviour.

Fixes #41051

activemodel/test/cases/validations/inclusion_validation_test.rb

activemodel/lib/active_model/validations/inclusion.rb

intrip · 2021-02-12T10:06:53Z

activemodel/lib/active_model/validations/clusivity.rb

@@ -24,7 +26,7 @@ def include?(record, value)
          delimiter
        end

-        if value.is_a?(Array)
+        if value.is_a?(Array) && options[:all] == true


The idea behind the all name is that we can introduce also an any option (use .any? instead of .all?) which makes sense for the exclusion validation, check cases such as: #22610 (comment)

intrip · 2021-02-12T11:01:43Z

activemodel/lib/active_model/validations/inclusion.rb

@@ -33,6 +34,8 @@ module HelperMethods
      #   with <tt>Range#cover?</tt>, otherwise with <tt>include?</tt>. When using
      #   a proc or lambda the instance under validation is passed as an argument.
      # * <tt>:within</tt> - A synonym(or alias) for <tt>:in</tt>
+      # * <tt>:all</tt> - If attribute is an <tt>Array</tt> validates the inclusion of


I didn't document this option for validates_exclusion_of (even if is available in the code path) because I couldn't find a valid use case.

intrip · 2021-02-22T10:24:35Z

@rafaelfranca could you take a peek 🙏 ?

hovsater · 2021-03-06T07:32:07Z

Sorry for being out of the loop on this one. I think this looks good. 👍

My initial implementation was actually based around an option (see the pull request description), each_object: true, but was later revamped to just look for array values based on feedback in the pull request. I knew there was a reason I chose to stick with an option to begin with. 🙂

intrip · 2021-04-08T09:52:11Z

Any chance someone from the core team could take a look at this? 🙇

intrip · 2021-05-10T15:30:04Z

bump 🏓

intrip · 2021-06-10T09:35:22Z

@zzak sorry to tag you but you're super active, maybe you can take a look a this PR too? It's been there for a while 😄

zzak · 2021-06-10T10:26:45Z

activemodel/test/cases/validations/inclusion_validation_test.rb

-  def test_validates_inclusion_of_with_array_value
-    Person.validates_inclusion_of :karma, in: %w( abe monkey )
+  def test_validates_inclusion_of_with_array_value_and_all_option_is_true
+    Person.validates_inclusion_of :karma, in: %w( abe monkey ), all: true


Is all required for this test to pass? Or were you modifying an existing test on purpose 🤔

Yes all: true is required, this is basically the purpose of this PR: reverting the default and making this behaviour opt-in

@intrip What happens if we remove all: true?

I'm hesitant to revert the old behavior as this likely already cost some upgrade cycles for folks, to revert that behavior may be confusing. If we kept the current behavior, is there still a way to opt-out?

@zzak
I understand your point, honestly I've just followed #41051 (comment) suggestion.
Changing this to opt-out is dead simple in any case and I don't mind following a more conservative approach.

I will trust @rafaelfranca here, since his original suggestion was to revert to the old behavior and opt-in. 🙏

Just making sure we didn't at least consider the conservative approach first.

zzak · 2021-06-10T10:29:07Z

@intrip I took a quick look, this one tab was open all day.. The approach seems good to me? But I think it's better to wait for someone like @rafaelfranca who has more context here. 🙏

rails-bot · 2021-09-26T10:31:32Z

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.
Thank you for your contributions.

intrip · 2021-09-26T13:28:50Z

still relevant

rails-bot · 2021-12-25T14:07:08Z

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.
Thank you for your contributions.

intrip · 2021-12-27T09:50:17Z

ping

rails-bot · 2022-03-27T10:27:29Z

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.
Thank you for your contributions.

intrip · 2022-03-29T09:46:23Z

pong

Change rails#22610 in order to revert the behaviour by default. If the option `all: true` is given applies the new behaviour.

elliottmason · 2023-03-23T01:42:48Z

Is there another way to concisely express a validation that functions the way validates :value, exclusion: { in: [nil] } did prior to #22610? It's one thing for the behavior to change, but it seems like achieving the previous behavior is basically broken right now without writing a custom validation.

zzak · 2023-03-24T04:46:27Z

@elliottmason Are you able to try this PR in your app? It would be good to know if there are any regressions before this is merged.

elliottmason · 2023-03-27T16:32:12Z

Thanks for the suggestion @zzak I will do that and report my findings.

sdrioux · 2023-07-21T19:30:43Z

@zzak @rafaelfranca this fix has been around for awhile - any chance we could get this merged?

sdrioux · 2023-07-21T19:31:10Z

@elliottmason Did you have any luck with this PR?

xjunior · 2024-01-12T17:34:29Z

We're reaching 6.1 now, and this came up. Are there plans to merge this?

xjunior · 2024-01-12T17:47:32Z

My current solution to this is:

-                exclusion: { in: [nil] }
+                length: { minimum: 0, allow_nil: false, message: "can't be nil" }

rails-bot bot added activemodel docs labels Feb 11, 2021

intrip force-pushed the 41051-add-explicit-option-for-inclusion-all-when-array branch from 511333d to 3aacc11 Compare February 11, 2021 17:06

intrip commented Feb 11, 2021

View reviewed changes

activemodel/test/cases/validations/inclusion_validation_test.rb Outdated Show resolved Hide resolved

intrip commented Feb 11, 2021

View reviewed changes

activemodel/lib/active_model/validations/inclusion.rb Outdated Show resolved Hide resolved

intrip force-pushed the 41051-add-explicit-option-for-inclusion-all-when-array branch 2 times, most recently from 4889b5a to 37b34fe Compare February 12, 2021 09:59

intrip changed the title ~~WIP~~ Revert #22610 by default and makes it opt-in Feb 12, 2021

intrip commented Feb 12, 2021

View reviewed changes

intrip changed the title ~~Revert #22610 by default and makes it opt-in~~ Revert #22610 by default and make it opt-in Feb 12, 2021

intrip force-pushed the 41051-add-explicit-option-for-inclusion-all-when-array branch 4 times, most recently from 4c108a6 to e2c2e20 Compare February 12, 2021 10:13

intrip commented Feb 12, 2021

View reviewed changes

intrip mentioned this pull request Feb 12, 2021

Empty arrays don't pass validate exclusion of nil in Rails 6.1 #41051

Open

intrip force-pushed the 41051-add-explicit-option-for-inclusion-all-when-array branch from 2cbb005 to fb0b09b Compare May 7, 2021 13:09

intrip force-pushed the 41051-add-explicit-option-for-inclusion-all-when-array branch from fb0b09b to e270d5a Compare May 26, 2021 08:49

intrip force-pushed the 41051-add-explicit-option-for-inclusion-all-when-array branch from e270d5a to 56a87db Compare June 9, 2021 09:10

zzak reviewed Jun 10, 2021

View reviewed changes

intrip force-pushed the 41051-add-explicit-option-for-inclusion-all-when-array branch from 56a87db to afa899a Compare June 25, 2021 15:30

zzak requested a review from rafaelfranca June 25, 2021 22:47

intrip force-pushed the 41051-add-explicit-option-for-inclusion-all-when-array branch from afa899a to 650623f Compare June 28, 2021 09:17

zzak added the needs feedback label Jun 28, 2021

zzak removed the docs label Jun 28, 2021

rails-bot bot added the stale label Sep 26, 2021

rails-bot bot removed the stale label Sep 26, 2021

intrip force-pushed the 41051-add-explicit-option-for-inclusion-all-when-array branch from 650623f to 18fe512 Compare September 26, 2021 13:32

rails-bot bot added the stale label Dec 25, 2021

rails-bot bot removed the stale label Dec 27, 2021

rails-bot bot added the stale label Mar 27, 2022

rails-bot bot removed the stale label Mar 29, 2022

Revert rails#22610 by default and make it opt-in

e481fc8

Change rails#22610 in order to revert the behaviour by default. If the option `all: true` is given applies the new behaviour.

intrip force-pushed the 41051-add-explicit-option-for-inclusion-all-when-array branch from 18fe512 to e481fc8 Compare August 24, 2022 21:07

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Revert #22610 by default and make it opt-in #41412

Revert #22610 by default and make it opt-in #41412

intrip commented Feb 11, 2021 •

edited

intrip Feb 12, 2021

intrip Feb 12, 2021

intrip commented Feb 22, 2021

hovsater commented Mar 6, 2021 •

edited

intrip commented Apr 8, 2021

intrip commented May 10, 2021

intrip commented Jun 10, 2021 •

edited

zzak Jun 10, 2021

intrip Jun 10, 2021

zzak Jun 11, 2021

intrip Jun 11, 2021 •

edited

zzak Jun 11, 2021

zzak commented Jun 10, 2021

rails-bot bot commented Sep 26, 2021

intrip commented Sep 26, 2021

rails-bot bot commented Dec 25, 2021

intrip commented Dec 27, 2021

rails-bot bot commented Mar 27, 2022

intrip commented Mar 29, 2022

elliottmason commented Mar 23, 2023

zzak commented Mar 24, 2023

elliottmason commented Mar 27, 2023

sdrioux commented Jul 21, 2023

sdrioux commented Jul 21, 2023

xjunior commented Jan 12, 2024

xjunior commented Jan 12, 2024

Revert #22610 by default and make it opt-in #41412

Are you sure you want to change the base?

Revert #22610 by default and make it opt-in #41412

Conversation

intrip commented Feb 11, 2021 • edited

Summary

intrip Feb 12, 2021

Choose a reason for hiding this comment

intrip Feb 12, 2021

Choose a reason for hiding this comment

intrip commented Feb 22, 2021

hovsater commented Mar 6, 2021 • edited

intrip commented Apr 8, 2021

intrip commented May 10, 2021

intrip commented Jun 10, 2021 • edited

zzak Jun 10, 2021

Choose a reason for hiding this comment

intrip Jun 10, 2021

Choose a reason for hiding this comment

zzak Jun 11, 2021

Choose a reason for hiding this comment

intrip Jun 11, 2021 • edited

Choose a reason for hiding this comment

zzak Jun 11, 2021

Choose a reason for hiding this comment

zzak commented Jun 10, 2021

rails-bot bot commented Sep 26, 2021

intrip commented Sep 26, 2021

rails-bot bot commented Dec 25, 2021

intrip commented Dec 27, 2021

rails-bot bot commented Mar 27, 2022

intrip commented Mar 29, 2022

elliottmason commented Mar 23, 2023

zzak commented Mar 24, 2023

elliottmason commented Mar 27, 2023

sdrioux commented Jul 21, 2023

sdrioux commented Jul 21, 2023

xjunior commented Jan 12, 2024

xjunior commented Jan 12, 2024

intrip commented Feb 11, 2021 •

edited

hovsater commented Mar 6, 2021 •

edited

intrip commented Jun 10, 2021 •

edited

intrip Jun 11, 2021 •

edited