[Form] Caching resolved $options is dangerous #6456

vicb · 2012-12-21T13:53:44Z

In a custom FormType, I have:

<?php

class TypePuissanceFiscaleType extends AbstractType
{
    public function setDefaultOptions(OptionsResolverInterface $resolver)
    {
        $query_builder = function(Options $options) {
            $typeVehicule = $options['type_vehicule_id'];
            return function(EntityRepository $repository) use ($typeVehicule) {
                return $repository->createQueryBuilder('tpf')
                    ->where('tpf.typeVehicule = :typeVehicule')
                    ->setParameter('typeVehicule', $typeVehicule);
            };
        };

        $resolver->setDefaults(array(
            'data_class' => '<class>',
            'class' => '<Bundle>:TypePuissanceFiscale',
            'type_vehicule_id' => null,
            'query_builder' => $query_builder
        ));
    }

    public function getParent()
    {
        return 'entity';
    }

    public function getName()
    {
        return 'typepuissancefiscale';
    }
}

The qb is supposed to return a different value according to the value of $options['type_vehicule_id'].

<?php
$form->add('first', 'typepuissancefiscale', array('type_vehicule_id' => 1);
$form->add('first', 'typepuissancefiscale', array('type_vehicule_id' => 2);

However due to resolved options being cached by form types, the options would be resolved only once and re-used for the second form. This means that both forms would use the same query builder which is different from what I expect.

@bschussek

It is very dangerous and should be caught at least in dev mode (if it implies overhead) rather than silently trigerring a wrong behavior.
Do you have any idea of how to fix this (not on how to avoid using the options to pass parameters).

/cc @yohannpoli

The text was updated successfully, but these errors were encountered:

stof · 2012-12-21T14:07:31Z

Actually, the issue is that the QueryBuilder is not cached properly. The DQL string is not the only part that can change, which mean the way the cache key is computed is wrong. It need to depend on the query params, query hints, ORM filters and so on

@stof

@stof are you working on this ? any ETA ?

vicb · 2012-12-21T14:16:50Z

@stof Seems to be a lot better by disabling the cache ! Thanks for the hint.

stof · 2012-12-21T14:34:33Z

@vicb no I'm not. For now, I would suggest reverting b604eb7 as it is breaking things by generating wrong cache keys

vicb · 2012-12-21T14:54:29Z

@bschussek what do you think about reverting the linked commit ?
@bschussek @stof @vicb please update this issue when working on the issue. /cc @fabpot

sstok · 2012-12-22T10:13:55Z

An easy fix would be the include parameters in the cache key as they are most likely to change.
At least it would fix this problem.

vicb · 2012-12-22T10:33:09Z

That's about what stof Saïd.

----- Reply message -----
De : "Sebastiaan Stok" notifications@github.com
Pour : "symfony/symfony" symfony@noreply.github.com
Cc : "Victor Berchet" victor@suumit.com
Objet : [symfony] [Form] Caching resolved $options is dangerous (#6456)
Date : sam., déc. 22, 2012 11:14
An easy fix would be the include parameters in the cache key as they are most likely to change.

At least it would fix this problem.

—

Reply to this email directly or view it on GitHub.

sstok · 2012-12-22T10:38:33Z

I know 😄 I was just suggesting it as a quick fix.

vicb · 2012-12-22T14:07:59Z

@stof suggested a better quick fix (reverting) but the real fix should also be quick if anybody has time these days.

gquemener · 2012-12-27T09:47:00Z

What d'you think? Is it valuable enough for a PR?

vicb · 2012-12-27T10:05:46Z

Good start, I would also use other variables mentionned by stof and I would not modify param values.

gquemener · 2012-12-27T10:13:37Z

Something like that (but with the parameters) would be great, wouldn't it? https://github.com/doctrine/doctrine2/blob/master/lib/Doctrine/ORM/Query.php#L639-L649

sstok · 2012-12-27T13:10:54Z

@gquemener 👍

Doctrine actually has multiple cache types, DQL ->SQL is just one of theme.

Edit:

https://github.com/doctrine/doctrine2/blob/master/lib/Doctrine/ORM/AbstractQuery.php#L782
https://github.com/doctrine/doctrine2/blob/master/lib/Doctrine/ORM/AbstractQuery.php#L487
https://github.com/doctrine/dbal/blob/master/lib/Doctrine/DBAL/Cache/QueryCacheProfile.php

Maybe its best if we can use the information and cache key generation of Doctrine instead of making something our self?

gquemener · 2012-12-27T13:28:59Z

IMO, an existing cache-id generation mechanim should be used.
Problem with https://github.com/doctrine/doctrine2/blob/master/lib/Doctrine/ORM/AbstractQuery.php#L782 is that it requires that a ResultCacheDriver has been configured in Doctrine (which is not always the case I think).

stof · 2012-12-27T13:30:05Z

@sstok We cannot as this cache does not correspond to any Doctrine cache level: it is caching the hydrated results based on the original DQL query, which means it has to take all settings into account whereas each Doctrine cache only uses a few of them (the query cache does not need to take parameters into account for instance as they are not part of the DQL -> SQL conversion, and the result cache does not need to handle DQL specific settings as it works with the SQL).

gquemener · 2012-12-27T14:07:41Z

What about this one? https://github.com/gquemener/symfony/blob/4fa4a9e2229723baf3b6bc6b942893f9fe7b9f4f/src/Symfony/Bridge/Doctrine/Form/Type/EntityType.php#L69-L79

weaverryan · 2012-12-28T22:50:12Z

+1, I'm hitting this bug as well :/.

8000

If you're looking for a workaround, you need to find a way to somehow change the DQL between the different uses of this. For example, in one use I order by a name property on the query builder, and in another instance, I order by a different property. This may not work for everyone, but if you can, it's the most straightforward workaround.

Ping Mr @bschussek! :) This is a bug in a security maintenance release, so we should either revert sha: b604eb7 as @stof suggested or perhaps have an opt-in for this feature (some option to turn it on) - and actually fix it on a non-maintenance release.

Thanks!

gquemener · 2012-12-29T08:44:41Z

As a quickfix in your project you can just avoid to use placeholders/parameters in your dql and it'll work (well if you're not using different hints, filters, ...).

What do you mean @weaverryan when you say "change the DQL between the different uses of this"? The real issue here is about generating a valid unique cache id for a given query builder, right?

fabpot · 2012-12-29T10:54:45Z

@bschussek I'm going to revert b604eb7 (any objections?). Another solution should happen in master.

vicb · 2012-12-29T22:32:58Z

@fabpot it would be great to open a new issue while reverting so that the optimization don't get lost.

fabpot · 2012-12-29T22:36:31Z

reverted.

bicpi · 2013-01-03T21:07:03Z

+1 - I'm hitting on this, too.

@gquemener: Comment by @weaverryan describes a workaround so that you get different cache IDs until this bug is fixed: Different DQL = Different cache ID

// first usage
'query_builder' => function(EntityRepository $er) use ($client) {
    return $er->getByTypeQueryBuilder(Address::TYPE_CONTACT);
},
// ...

// second usage
'query_builder' => function(EntityRepository $er) use ($client) {
    return $er->getByTypeQueryBuilder(Address::TYPE_INVOICE);
        // Workaround: change DQL => triggers new cache ID
        ->andWhere('1=1');
},

Instead of "AND 1=1" you could also add something else which does not change the result (e.g. add another meaningless ORDER BY if your DQL does already manages the order for your view).

vicb · 2013-01-04T06:42:39Z

@fabpot what about closing this issue & create and enhancement request ?

fabpot · 2013-01-04T07:54:34Z

@vicb that would help for sure. thanks.

vicb · 2013-01-04T19:33:45Z

thanks

Do you expect me to do this ?

ianfp · 2013-01-18T16:40:14Z

+1 from me, too. I filed issue 6787, which as @Tobion says is probably a dup.

ianfp · 2013-02-16T17:52:00Z

Any updates on this? This bug is a major nuisance. Or at least some way to disable the caching until there is a fix? The workaround suggested by @weaverryan works, but it's a pretty ugly kludge.

vicb · 2013-02-16T17:58:11Z

If @fabpot reverted it as indicated in a previous comment, the easy fix its to update sf.

wouterj · 2015-02-19T12:54:08Z

The commit is reverted, so I think this can be closed?

Tobion mentioned this issue Jan 18, 2013

Wrong choices appear in "entity" field type added by event listener #6787

Closed

webmozart mentioned this issue Oct 16, 2014

[2.7][Form] Refactored choice lists to support dynamic labels, values and attributes #12148

Closed

3 tasks

Tobion closed this as completed Feb 19, 2015

webmozart mentioned this issue Mar 25, 2015

[Form] Refactored choice lists to support dynamic label, value, index and attribute generation #14050

Merged

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Uh oh!

[Form] Caching resolved $options is dangerous #6456

[Form] Caching resolved $options is dangerous #6456

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

[Form] Caching resolved $options is dangerous #6456

[Form] Caching resolved $options is dangerous #6456

Comments

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!