Skip to content

[ExpressionLanguage] Revert readonly flag on ConstantNode::$isNullSafe #46840

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

Closed
wants to merge 1 commit into from
Closed

Conversation

fmata
Copy link
Contributor

@fmata fmata commented Jul 4, 2022

Q A
Branch? 6.1
Bug fix? yes
New feature? no
Deprecations? no
License MIT

Since #45795, the new property ConstantNode::$isNullSafe is readonly but when Hydrator of VarExporter component tries to hydrate Symfony\Component\ExpressionLanguage\Compiler\ConstantNode here, we get an "Error: Cannot initialize readonly property Symfony\Component\ExpressionLanguage\Node\ConstantNode::$isNullSafe from scope Symfony\Component\VarExporter\Internal\Hydrator".

My stacktrace :

Error: Cannot initialize readonly property Symfony\Component\ExpressionLanguage\Node\ConstantNode::$isNullSafe from scope Symfony\Component\VarExporter\Internal\Hydrator

╵ /app/vendor/symfony/var-exporter/Internal/Hydrator.php:63
╵ /app/vendor/symfony/var-exporter/Internal/Hydrator.php:43
╵ /app/var/cache/test/pools/system/RT-ma9MMTZ/G/C/sXxDeGNGSoIu4LvAqLrg:139
╵ /app/vendor/symfony/cache/Adapter/PhpFilesAdapter.php:118
╵ /app/vendor/symfony/cache/Traits/AbstractAdapterTrait.php:210
╵ /app/vendor/symfony/cache/Adapter/TraceableAdapter.php:77
╵ /app/vendor/symfony/expression-language/ExpressionLanguage.php:78
╵ /app/vendor/symfony/expression-language/ExpressionLanguage.php:59
╵ /app/vendor/symfony/validator/Constraints/ExpressionValidator.php:45
╵ /app/vendor/symfony/validator/Validator/RecursiveContextualValidator.php:758
╵ /app/vendor/symfony/validator/Validator/RecursiveContextualValidator.php:603
╵ /app/vendor/symfony/validator/Validator/RecursiveContextualValidator.php:517
╵ /app/vendor/symfony/validator/Validator/RecursiveContextualValidator.php:314
╵ /app/vendor/symfony/validator/Validator/RecursiveContextualValidator.php:139
╵ /app/vendor/symfony/form/Extension/Validator/Constraints/FormValidator.php:108
╵ /app/vendor/symfony/validator/Validator/RecursiveContextualValidator.php:758
╵ /app/vendor/symfony/validator/Validator/RecursiveContextualValidator.php:489
╵ /app/vendor/symfony/validator/Validator/RecursiveContextualValidator.php:314
╵ /app/vendor/symfony/validator/Validator/RecursiveContextualValidator.php:139
╵ /app/vendor/symfony/validator/Validator/RecursiveValidator.php:97
╵ /app/vendor/symfony/validator/Validator/TraceableValidator.php:67
╵ /app/vendor/symfony/form/Extension/Validator/EventListener/ValidationListener.php:49
╵ /app/vendor/symfony/event-dispatcher/EventDispatcher.php:230
╵ /app/vendor/symfony/event-dispatcher/EventDispatcher.php:59
╵ /app/vendor/symfony/event-dispatcher/ImmutableEventDispatcher.php:33
╵ /app/vendor/symfony/form/Form.php:642
╵ /app/tests/Infrastructure/UserInterface/Http/Form/MyFormWithExpressionConstraintTest.php:32

Friendly ping @mytuny

@nicolas-grekas
Copy link
Member

This might be a bug in VarExporter. Can you please share a reproducer?

@stof
Copy link
Member

stof commented Jul 4, 2022

Please also open a separate issue about the need to handle readonly properties in VarExporter

@fmata
Copy link
Contributor Author

fmata commented Jul 4, 2022

All deps are on latest tag 6.1.x.

Filtres.php :

class Filtres
{
    /** @var string|null */
    public $recherche;

    /** @var Statut|null */
    public $statut;

    /** @var DateTimeImmutable|null */
    public $dateDeDebut;

    /** @var DateTimeImmutable|null */
    public $dateDeFin;
}

FiltresType.php :

class FiltresType extends AbstractType
{
    public function buildForm(FormBuilderInterface $builder, array $options): void
    {
        $builder->add('recherche', TextType::class, [
            'required' => false,
        ]);

        if ((bool) $options['statut']) {
            $builder
                ->add('statut', ChoiceType::class, [
                    'required' => false,
                    'choices' => ['oui', 'non'],
                    'choice_label' => 'label',
                    'choice_value' => 'value',
                ])
                ->add('dateDeDebut', DateType::class, [
                    'input' => 'datetime_immutable',
                    'widget' => 'single_text',
                    'required' => false,
                ])
                ->add('dateDeFin', DateType::class, [
                    'input' => 'datetime_immutable',
                    'widget' => 'single_text',
                    'required' => false,
                ])
            ;
        }

        $builder->add('submit', SubmitType::class);
    }

    public function configureOptions(OptionsResolver $resolver): void
    {
        $resolver
            ->setRequired('statut')
            ->setAllowedTypes('statut', 'bool')

            ->setRequired('statut_brouillon')
            ->setAllowedTypes('statut', 'bool')
        ;
    }
}

Filtres.yaml :

App\x\Filtres:
    properties:
        dateDeDebut:
            - Type: DateTimeImmutable
        dateDeFin:
            - Type: DateTimeImmutable
            - Expression:
                  expression: this.dateDeDebut === null  || value === null || value >= this.dateDeDebut

FiltresTypeTest.php :

class FiltresTypeTest extends FormTestCase
{
    /**
     * @dataProvider provideValidDataCases
     *
     * @param array<string, array<string>> $submittedData
     */
    public function test_it_hydrates_the_model_with_valid_values(
        FiltresCras $model,
        array       $submittedData,
        FiltresCras $expected,
    ): void
    {
        $form = self::getContainer()->get('form.factory')->create(FiltresType::class, $model, [
            'statut' => true,
            'statut_brouillon' => true,
        ]);

        $form->submit($submittedData);

        self::assertTrue($form->isSynchronized());
        self::assertTrue($form->isValid());

        self::assertEquals($expected, $model);
    }

    public function provideValidDataCases(): iterable
    {
        $model = new Filtres();

        $expected = clone $model;

        yield [
            $model,
            [],
            $expected,
        ];
    }
}

I hope it's enough to have a look, thanks.

@nicolas-grekas
Copy link
Member

Closing in favor of #46841

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants