Not setting the strict option of the Choice constraint to true is deprecated since Symfony 3.4 and will throw an exception in 4.0

Naively setting $strict = TRUE on our Choice subclass.

So let's see if typed data offers us a way out...

That's not going to work :( Because \Drupal\Core\Field\Plugin\Field\FieldType\BooleanItem::getSettableValues does not return boolean values but getCastedValue()
on it's main property would.

We need the early return before the type safety.

One thought is do we need to bring

        if ($constraint->callback) {
            if (!\is_callable($choices = array($this->context->getObject(), $constraint->callback))
                && !\is_callable($choices = array($this->context->getClassName(), $constraint->callback))
                && !\is_callable($choices = $constraint->callback)
            ) {
                throw new ConstraintDefinitionException('The Choice constraint expects a valid callback');
            }
            $choices = \call_user_func($choices);
        } else {
            $choices = $constraint->choices;
        }

out of the parent implementation so we can do the same type juggling for callbacks. I think we probably do.

Addressing #10

Bumping to critical since it's a hard Drupal 9 blocker.

1. +++ b/core/lib/Drupal/Core/Validation/Plugin/Validation/Constraint/AllowedValuesConstraintValidator.php
   @@ -47,9 +49,20 @@ public function __construct(AccountInterface $current_user) {
   +    if ($constraint->callback) {
   +      if (!\is_callable($choices = [$this->context->getObject(), $constraint->callback])
   +        && !\is_callable($choices = [$this->context->getClassName(), $constraint->callback])
   +        && !\is_callable($choices = $constraint->callback)
   +      ) {
   +        throw new ConstraintDefinitionException('The AllowedValuesConstraint constraint expects a valid callback');
   +      }
   +      $allowed_values = \call_user_func($choices);
   +      // parent::validate() does not need to invoke the callback again.
   +      $constraint->callback = NULL;
   +    }
   

   It would be good to have test coverage of this move and it's ability to affect types. This change is not required by core but very much might be needed by contrib / custom.

2. +++ b/core/lib/Drupal/Core/Validation/Plugin/Validation/Constraint/AllowedValuesConstraintValidator.php
   @@ -69,6 +82,22 @@ public function validate($value, Constraint $constraint) {
   +      // Make the types match for strict checking. We can't use typed data here
   +      // because types are not enforced everywhere. For example,
   +      // \Drupal\Core\Field\Plugin\Field\FieldType\BooleanItem::getSettableValues()
   +      // uses 0 and 1 to represent possible Boolean values whilst
   +      // \Drupal\Core\TypedData\Plugin\DataType\BooleanData::getCastedValue()
   +      // will return a proper typed Boolean. Therefore assume that
   +      // $allowed_values contains values of the same type and cast $value to
   +      // match.
   +      settype($value, gettype(reset($allowed_values)));
   

   This code is not the prettiest but actually solving the "TypedData not being properly typed" issue is of way bigger scope.

Here's a test the proves the callback takes precedence to the allowed values and our type coercing works.

+++ b/core/lib/Drupal/Core/Validation/Plugin/Validation/Constraint/AllowedValuesConstraintValidator.php
@@ -69,6 +82,22 @@ public function validate($value, Constraint $constraint) {
 
+    if (isset($allowed_values)) {
+      $constraint->choices = $allowed_values;
+      // Make the types match for strict checking. We can't use typed data here
+      // because types are not enforced everywhere. For example,
+      // \Drupal\Core\Field\Plugin\Field\FieldType\BooleanItem::getSettableValues()
+      // uses 0 and 1 to represent possible Boolean values whilst
+      // \Drupal\Core\TypedData\Plugin\DataType\BooleanData::getCastedValue()
+      // will return a proper typed Boolean. Therefore assume that
+      // $allowed_values contains values of the same type and cast $value to
+      // match.
+      settype($value, gettype(reset($allowed_values)));
+    }
+    elseif ($typed_data instanceof PrimitiveInterface) {
+      $value = $typed_data->getCastedValue();
+    }
+

OK this is really tricky but the comment explains what's going on well enough.

My only question is why not copy over the Symfony 3 non-strict logic that's been deprecated - would that be more/worse code to maintain than what we're adding here?

@catch I think it is worth doing this because in_array() without strict checking is really hard to predict how it works - there are just so many gotchas. Also this is moving in the direction we should be going with TypedData being typed and using PHP 7's primitive types. So it's a stepping stone. It also keeps us in-line with Symfony so when we doing get round to strciter typing in TypedData we've not got so far to move.

@larowlan we never really agreed a loosening of coding standards for tests - so fixed. One day we'll open a policy issue about that :)

While reviewing the code I was wondering whether we could reuse the existing parent's code by setting the callback, but yeah it is not. I added a comment about this.
On top of that I adopted the tests to not add additional deprecated assertion calls.

I don't think there's anything else left to do here.

I've been reviewing this along with other issues in the constraints system for symfony 4. I've been testing it in conjunction with some other patches around the constraint system, and it is working well. The approach makes since given the, ahem, 'constraints' we are facing.

+1 on RTBC

Technically it is a BC break. Existing code may rely on non-strict validation.
https://www.drupal.org/pift-ci-job/1200662

If we desperately need this we should at least provide a change record to explain how custom code is supposed to be updated.

Besides Double Field this also affects core List (float) field.
#3033120: [Regression] Options select widget does not work when allowed values are a mix of strings and integers

+++ b/core/lib/Drupal/Core/Validation/Plugin/Validation/Constraint/AllowedValuesConstraintValidator.php
@@ -69,6 +86,22 @@ public function validate($value, Constraint $constraint) {
+      // Make the types match for strict checking. We can't use typed data here
+      // because types are not enforced everywhere. For example,
+      // \Drupal\Core\Field\Plugin\Field\FieldType\BooleanItem::getSettableValues()
+      // uses 0 and 1 to represent possible Boolean values whilst
+      // \Drupal\Core\TypedData\Plugin\DataType\BooleanData::getCastedValue()
+      // will return a proper typed Boolean. Therefore assume that
+      // $allowed_values contains values of the same type and cast $value to
+      // match.
+      settype($value, gettype(reset($allowed_values)));
+    }

Floats can't be array keys, so list_float typecasts keys to a mix of integers or strings. If a float options allowed value list is [0.0, 1.5, 2.0], then the allowed values list is [(int)0, "1.5", (int)2] if we check a value of "1.5" it is being typecast to gettype(0) = (int)1.5 = 1 before being validated and fails.

I rolled this back based on discussion today with @mikelutz. For one the solution to the problem is not trivial. Second, it looks like a redo of the patch and not really a followup. Better to not have -dev include known regressions that we cannot fix quickly. @mikelutz will bring the fix back here from #3033120: [Regression] Options select widget does not work when allowed values are a mix of strings and integers.

Merging in some work in the other issue against the new 8.7.x with the revert. Testbot fodder...

bugfix in my approach..

Okay, so this should work with some cleanup. Basically default AllowedValueConstraint::strict to FALSE, set it to true for the core use cases and handle the typing for Booleans and Integer lists. If we get through there, and strict still hasn't been set to true, then check if the validator passes for non-strict but not strict, and if it does, throw a deprecation error, typecast so it works with strict and send it on its way.

This way we preserve non-strict behavior behind a deprecation error, and take care of the core use cases.

I'll try to clean up the patch and the direct testing tomorrow.

Here's another approach - kinda going back to #8 but doing an additional couple of twists. Doing the callback ourselves so we can later set the type of the allowed values using the typed data we've been provided. This patch includes the new tests added by @mikelutz to prove what we broke in #20 that was committed and then reverted.

@mikelutz pointed out I was miss a use - now I've confirmed in debug that when dealing with ListFloatItem we are casting all the values to floats properly.

For those wondering why #33 is green - it's because all the things were cast to string because the instanceof PrimitiveInterface always returned false.

So the patch in #34 broke

    // The parent implementation ignores values that are not set, but makes
    // sure some choices are available firstly. However, we want to support
    // empty choices for undefined values; for instance, if a term reference
    // field points to an empty vocabulary.
    if (!isset($value)) {
      return;
    }

because NULL was being cast to an empty string. So to fix we need to do the casting after this check. Which funnily enough makes the patch a little bit nicer in my opinion.

+++ b/core/lib/Drupal/Core/Validation/Plugin/Validation/Constraint/AllowedValuesConstraintValidator.php
@@ -69,6 +73,34 @@ public function validate($value, Constraint $constraint) {
+    // Get the value with the proper datatype in order to make strict
+    // comparisons using in_array().
+    if ($typed_data instanceof PrimitiveInterface) {
+      $value = $typed_data->getCastedValue();
+    }

So one thought I have is are we not always dealing with a PrimitiveInterface type at this point and what would it mean if we're not.

Uploading a patch to find out what's happening in HEAD.

+++ b/core/lib/Drupal/Core/Validation/Plugin/Validation/Constraint/AllowedValuesConstraintValidator.php
@@ -69,6 +73,35 @@ public function validate($value, Constraint $constraint) {
+    foreach ($constraint->choices as &$choice) {
+      settype($choice, gettype($value));
+    }

I suppose we should evaluate gettype($value) outside of the loop.

Good idea @mikelutz. I tried to make this bit of code more elegant with array_map / array_walk but nothing looked nicer and as easier to grok as the foreach to me.

I had ran a speed test earlier, and foreach ran the fastest anyway. array_map was fine when I was only doing it for ListFloatItem and was casting to float, but I think the foreach is cleaner than trying to deal with getting the type in the closure.

Looks good me to me as well. Thanks for working out a better solution.

Superb, thanks for the additional test coverage :)