8000 [Form] `AbstractChoiceLoader` can return invalid results · Issue #44655 · symfony/symfony · GitHub
[go: up one dir, main page]
Skip to content

[Form] AbstractChoiceLoader can return invalid results #44655

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.

Sign up for GitHub

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

Jump to bottom
Closed
cs278 opened this issue Dec 15, 2021 · 1 comment
Closed

[Form] AbstractChoiceLoader can return invalid results #44655

cs278 opened this issue Dec 15, 2021 · 1 comment
Labels
Bug Form Status: Needs Review

Comments

@cs278
Copy link
Contributor
cs278 commented Dec 15, 2021

Symfony version(s) affected

5.1.0, 5.4-dev, 6.1-dev

Description

I've discovered a design issue with AbstractChoiceLoader which manifested itself in a form failing to recognise a valid submitted value.

In our application this causes problems when the (new in 5.1) choice_filter option is used with the expanded => true option, initially I thought this was something specific to us (we actually had a choice_filter option on our type before Symfony added one) but it seems this combination are not used by many/any people as it just flat out doesn't work.

The root cause I believe is the following code:

symfony/src/Symfony/Component/Form/ChoiceList/Loader/AbstractChoiceLoader.php

Lines 34 to 37 in 9870e7e

public function loadChoiceList(callable $value = null): ChoiceListInterface
{
return $this->choiceList ?? ($this->choiceList = new ArrayChoiceList($this->loadChoices(), $value));
}

Observe how the ArrayChoiceList is stored in a property without regard for the supplied $value callback. It appears that $value is sometimes called with null or the value of the choice_filter option which results in inconsistent data.

How to reproduce

This code demonstrates the problem if run from the root directory of a Symfony checkout.

<?php

require 'vendor/autoload.php';

use Symfony\Component\Form\ChoiceList\Loader\CallbackChoiceLoader;

$loader = new CallbackChoiceLoader(function () {
    return [
        'ABC',
        'XYZ',
    ];
});

$loader2 = clone $loader;

dump($loader->loadChoiceList(null)->getValues());
/*
^ array:2 [
  0 => "ABC"
  1 => "XYZ"
]
*/

// Call with a value callback which is not applied
dump($loader->loadChoiceList('strrev')->getValues());

/*
^ array:2 [
  0 => "ABC"
  1 => "XYZ"
]
*/

// Call the cloned object which will apply the callback
dump($loader2->loadChoiceList('strrev')->getValues());

/*
^ array:2 [
  0 => "CBA"
  1 => "ZYX"
]
*/

// Call again and notice the values are still reversed.
dump($loader2->loadChoiceList(null)->getValues());

/*
^ array:2 [
  0 => "CBA"
  1 => "ZYX"
]
*/

Again a more representative example of the problem:

<?php

require 'vendor/autoload.php';

use Symfony\Component\Form\Extension\Core\CoreExtension;
use Symfony\Component\Form\Extension\Core\Type\ChoiceType;
use Symfony\Component\Form\FormFactory;
use Symfony\Component\Form\FormRegistry;
use Symfony\Component\Form\ResolvedFormTypeFactory;

$registry = new FormRegistry([
    new CoreExtension(),
], new ResolvedFormTypeFactory());
$factory = new FormFactory($registry);

$test = function (bool $expanded, ?callable $filter) use ($factory) {
    $builder = $factory->createBuilder();

    $builder->add('choice', ChoiceType::class, [
        'choices' => ['ABC', 'XYZ'],
        'expanded' => $expanded,
        'choice_filter' => $filter,
        'choice_value' => function ($choice) {
            return strrev($choice);
        },
    ]);

    $form = $builder->getForm();

    $form->submit([
        'choice' => 'CBA',
    ]);

    echo 'display: ', $expanded ? 'expanded' : 'collapsed', "\n";
    echo 'filter: ', $filter !== null ? 'yes' : 'no', "\n";

    if (!$form->isValid()) {
        echo "failed...\n\n";
        dump($form->getErrors(true, true)[0]->getCause());
    } else {
        echo "pass\n";
    }

    echo "\n\n";
};


$test(true, null);
$test(false, null);

// Any old callable is sufficient to trigger the problem
$test(true, fn () => true);
$test(false, fn () => true);

Possible Solution

This is one possible solution, with the downside that ArrayChoiceList is going to be constructed more frequently.

diff --git a/src/Symfony/Component/Form/ChoiceList/Loader/AbstractChoiceLoader.php b/src/Symfony/Component/Form/ChoiceList/Loader/AbstractChoiceLoader.php
index a30af63b1a..00ebc87568 100644
--- a/src/Symfony/Component/Form/ChoiceList/Loader/AbstractChoiceLoader.php
+++ b/src/Symfony/Component/Form/ChoiceList/Loader/AbstractChoiceLoader.php
@@ -20,11 +20,11 @@
 abstract class AbstractChoiceLoader implements ChoiceLoaderInterface
 {
     /**
-     * The loaded choice list.
+     * The stored choices.
      *
-     * @var ArrayChoiceList
+     * @var iterable
      */
-    private $choiceList;
+    private $choices;
 
     /**
      * @final
@@ -33,7 +33,11 @@ abstract class AbstractChoiceLoader implements ChoiceLoaderInterface
      */
     public function loadChoiceList(callable $value = null): ChoiceListInterface
     {
-        return $this->choiceList ?? ($this->choiceList = new ArrayChoiceList($this->loadChoices(), $value));
+        if (null === $this->choices) {
+            $this->choices = $this->loadChoices();
+        }
+
+        return new ArrayChoiceList($this->choices, $value);
     }
 
     /**
@@ -45,10 +49,6 @@ public function loadChoicesForValues(array $values, callable $value = null)
             return [];
         }
 
-        if ($this->choiceList) {
-            return $this->choiceList->getChoicesForValues($values);
-        }
-
         return $this->doLoadChoicesForValues($values, $value);
     }
 
@@ -66,11 +66,7 @@ public function loadValuesForChoices(array $choices, callable $value = null)
             return array_map($value, $choices);
         }
 
-        if ($this->choiceList) {
-            return $this->choiceList->getValuesForChoices($choices);
-        }
-
-        return $this->doLoadValuesForChoices($choices);
+        return $this->loadChoiceList($value)->getValuesForChoices($choices);
     }
 
     abstract protected function loadChoices(): iterable;

Another option would be storing the ArrayChoiceList indexed by the callback. Or more involved add an < 8000 code class="notranslate">ArrayChoiceList::withValue() method allowing the values to be overridden without a rebuild of the whole object.

Seems this simple change fixes the ChoiceType error but doesn't address the underlying issue within AbstractChoiceLoader.

diff --git a/src/Symfony/Component/Form/ChoiceList/Loader/FilterChoiceLoaderDecorator.php b/src/Symfony/Component/Form/ChoiceList/Loader/FilterChoiceLoaderDecorator.php
index a52f3b82e4..6a1122a695 100644
--- a/src/Symfony/Component/Form/ChoiceList/Loader/FilterChoiceLoaderDecorator.php
+++ b/src/Symfony/Component/Form/ChoiceList/Loader/FilterChoiceLoaderDecorator.php
@@ -50,7 +50,7 @@ protected function loadChoices(): iterable
      */
     public function loadChoicesForValues(array $values, callable $value = null): array
     {
-        return array_filter($this->decoratedLoader->loadChoicesForValues($values, $value), $this->filter);
+        return array_filter(parent::loadChoicesForValues($values, $value), $this->filter);
     }
 
     /**
@@ -58,6 +58,6 @@ public function loadChoicesForValues(array $values, callable $value = null): arr
      */
     public function loadValuesForChoices(array $choices, callable $value = null): array
     {
-        return $this->decoratedLoader->loadValuesForChoices(array_filter($choices, $this->filter), $value);
+        return parent::loadValuesForChoices(array_filter($choices, $this->filter), $value);
     }
 }

Additional Context

No response
@cs278 cs278 added the Bug label Dec 15, 2021
@nicolas-grekas
Copy link
Member

I see that you have a possible fix for this. Could you please send a PR (with a test case) so that we can discuss it?

@cs278 cs278 mentioned this issue Dec 20, 2021
Closed
This was referenced Apr 18, 2022
Merged
Closed
xabbuh added a commit that referenced this issue May 20, 2022
@xabbuh
bug #46098 [Form] Fix same choice loader with different choice values…
c6a2331 
… (HeahDude)

This PR was squashed before being merged into the 4.4 branch.

Discussion
----------

[Form] Fix same choice loader with different choice values

| Q             | A
| ------------- | ---
| Branch?       | 4.4
| Bug fix?      | yes
| New feature?  | no <!-- please update src/**/CHANGELOG.md files -->
| Deprecations? | no <!-- please update UPGRADE-*.md and src/**/CHANGELOG.md files -->
| Tickets       | #44655
| License       | MIT
| Doc PR        | ~

It appears that deprecating the caching in the `LazyChoiceList` (cf #18359) was a mistake. The bug went under the radar because in practice every choice field has its own loader instance.
However, optimizations made in #30994 then revealed the flaw (cf #42206) as the loaders were actually shared across many fields.
While working on a fix I ended up implementing something similar to what's proposed in #44655.

I'll send a PR for 5.4 as well.

Commits
-------

65cbf18 [Form] Fix same choice loader with different choice values
@xabbuh xabbuh closed this as completed May 20, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
Bug Form Status: Needs Review
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
4 participants
@cs278 @nicolas-grekas @xabbuh @carsonbot
0