Allow form caching

[tca3]

Hello,

When using Doctrine2's AnnotationBuilder, I would like to be able to cache the form created from an entity since it is quite slow to build it from annotations (I love them nonetheless). However, some calls in the constructors for Zend\Form make it impossible to cache objects in APC since they do not call the constructors when they are unserialized.

I got rid of one call in the constructor of FilterChain but couldn't in Fieldset since it resulted in a failed test. I do not know if this is the right approach (though I think __wakeup() and __sleep() methods are out of the question IMHO) but this seems to be a light fix for this approach. The tests seem to be ok with this, The removal of the call in FilterChain might break BC when I think about it. Any thoughts ?

Another approach could be implementing a setIterator() method on FieldSet and a setFilters() on FilterChain. I do not know what approach is best, I do not know if this method is better than the other, test-wise.

Here's the problematic code :

public function getForm($entity)
{
    $cache = \Zend\Cache\StorageFactory::factory(array(
        'adapter' => 'apc',
        'plugins' => array(
            'exception_handler' => array('throw_exceptions' => false),
        ),
    ));

    $em           = $this->getManager();
    $cacheKey     = get_class($entity);
    $hydrator     = new DoctrineHydrator($em, $this->getRepoName());

    if ($cache->hasItem($cacheKey)) {
        $form = $cache->getItem($cacheKey);
    }
    else {
        $builder      = new AnnotationBuilder($em);
        $form         = $builder->createForm($entity);
        $cache->setItem($cacheKey, $form);
    }
    $form->setHydrator($hydrator);
    $form->bind($entity);

    return $form;
}

[tca3]

Might be a BC break ?

[weierophinney]

Only if you hadn't replaced every call to $this->filters with $this->getFilters(). :)

[tca3]

What if I extended the class and that I try accessing $this->filters directly ?

class FilterChainExtension extends FilterChain
{
    public function __construct()
    {
        parent::__construct();
        $this->filters->getQueue(); // Fatal error
    }
}

Or in any other method for that matter, any call to $this->filters directly would trigger an error "access to non-object"

[weierophinney]

@ThomasCantonnet makes a good point; we should keep the instantiation within the constructor.

[Ocramius]

@ThomasCantonnet needs tests that verify serializability of this thing then...

Caching the entire form feels really dangerous though. It's not the right way in my opinion. Instead, caching annotations would be a solution.

[tca3]

@Ocramius Hmm it seems more logical indeed.

What kind of problem would one might encounter when serializing forms, out of curiosity ?

[macnibblet]

@ThomasCantonnet This is a BC since you introduce a new dependency, i myself wanted to fix this but it's a bit more complicated then one might except.

Secondly you shouldn't cache the entire form but only the form configuration because serializing an entire form will generate a huge amount of bugs where people do not correctly handle waking up.

[tca3]

@macnibblet The dependency on PriorityQueue is already present in both classes ;) I agree with the configuration for forms but I don't necessarily do on the sleep/wake. It should be possible to cache an entire form without them.

[macnibblet]

@ThomasCantonnet What happens when you cache a form with a validator that requires any given resource such as a Zend\Db connection or doctrine ?

[tca3]

@macnibblet I don't know I haven't been in that case; but I reckon that caching a form was a bad idea anyway, I cached the config instead. Feature-wise it would have been interesting though.

[weierophinney]

Cache the iterator in the call on line 619, so that the method call doesn't need to be made multiple times.

[tca3]

Done, I reckon even if caching forms was not necessarily a good idea this PR makes things a bit more robust anyhow. I suppose we need a test ?

[macnibblet]

I think the important part that we must cache is the result of parsing the entity for annotations, thats the blood sucker..

[weierophinney]

@ThomasCantonnet It looks good -- but as @Ocramius notes, we need tests to verify it does what it claims to, and also to ensure we don't accidentally remove the behavior in the future.

[tca3]

@weierophinney I woudn't know where to start when it comes to the testability of a Form's serializability. But I could justify this PR as a simple addition of a helper method for the class. That, I can test :)