[SymfonyCache] Prevent event class from being cached by Symfony's class cache

[stollr]

This PR is basing on #243 and @stof 's idea for improvement.

Symfony's class cache (cache//classes.php) compiles classes that are loaded on each request.

But when using EventDispatchingHttpCache there may occure errors, when using Symfony's HttpCache, because EventDispatchingHttpCache loads some classes (Symfony\Component\EventDispatcher\Event) individually via autoload and later when the HttpKernel is bootet in a subrequest and the class cache is loaded, PHP runs into a fatal error because of redeclaration of the class Symfony\Component\EventDispatcher\Event (as you can see in #242).

To fix this issue I have added an compiler pass which autoloads Symfony\Component\EventDispatcher\Event during cache building phase, so that Symfony removes it from its cache class map before the cache is built. This only happens if cache invalidation is used (as it only happens in that case, afaik).

If there are any objections/annotations to this PR, please let me know.

[stollr]

Argh, just realized that this does not work as expected @stof because the classes.php is generated on the first kernel boot, which does not happen when clearing cache via cli. It only works if you remove the cache folder without using app/console cache:clear and then call the web application.

Maybe we have to load classes in the bundle's boot method.

[stollr]

Even this does not work, because the class cache is created before the bundles are booted.

My last idea is to load the class in the FOSHttpCacheBundle constructor. But we do not have access to the service container there to check if it is needed to load the class. This would have an unnecessary performance impact on configurations where other cache backends are used :-(

Any ideas?

[dbu]

we could introduce a constructor argument, defaulting to assume the AppCache is used. or is there a reliable way to discover whether the AppCache is in place? ah - might even be different for web requests and cli :-(
did you check if doing the loading in the constructor works reliably?

i am not sure how relevant the penalty it is, because in a setup where performance really matters, an opcache should be used anyways, which makes the class caches rather irrelevant. afaik the classes.php is only relevant for systems without opcache.

@stof wdyt?

[stollr]

i am not sure how relevant the penalty it is, because in a setup where performance really matters, an opcache should be used anyways, which makes the class caches rather irrelevant.

Yes, that's true ;-)

I've extended the constructor of class EventDispatchingHttpCache now like this:

    public function __construct(HttpKernelInterface $kernel, $cacheDir = null)
    {
        $this->preloadClasses();
        // ....
    }

    // ....
    protected function preloadClasses()
    {
        class_exists('FOS\\HttpCache\\SymfonyCache\\CacheEvent');
    }

As EventDispatchingHttpCache must be extended anyway, it's no big deal to overwrite the preloadClasses method.

If there are no objections I'll commit this solution and hope that it will get pulled.

[dbu]

i see two problems: will the AppCache even be instantiated when running on the command line? and for version 2, we are refactoring this base class into a trait: FriendsOfSymfony/FOSHttpCache#233

with the trait, this solution no longer works. i think we should do it in the bundle class that is instantiated by AppKernel, and have an optional constructor argument to disable the preloading if its not wanted.

[stollr]

No, AppCache is not loaded in app/console. What problem do you see with it?

I have tried to load that class in the bundle constructor. But even there it does not work, because the bundles are loaded after the class cache has been loaded (see Symfony\Component\HttpKernel\Kernel::boot) :-(

[dbu]

I have tried to load that class in the bundle constructor. But even
there it does not work, because the bundles are loaded /after/ the class
cache has been loaded (see |Symfony\Component\HttpKernel\Kernel::boot|)

indeed :-(
well, it makes sense to load that cache as soon as possible, of course,
or it won't help much at all.

so what can we do? i can only see that we document for the user to
change their AppKernel (not AppCache) and add:

public function boot()
{
class_exists(...);

parent::boot();
}

not very nice but i can't think of a different solution. other ideas?

[stollr]

Why not in AppCache?
I would prefer to do this in the bundle as such things could easily be overlooked.

Or we reconsider using my first approach with overwriting the class mapping as I have done in #243 . It is not that nice, but there is no nice solution as we realized :-(

[dbu]

if AppCache is not executed on console, it is not the place to do this. and with the trait it will not be easier than AppKernel - the user needs to be aware of it and do something actively. i can't see how we could work around that without having the user do something.

[stollr]

if AppCache is not executed on console, it is not the place to do this.

Why that? AppCache is not loaded in console, so EventDispatchingHttpCache isn't loaded neither, which means that there won't be any redeclaration errors in console ;-)

[dbu]

but is the cache not also build when the console is run? and when later we have a web request, we get the error?

[stof]

@Naitsirch boot() happens after the classes are loaded, because the class cache is built during the building phase of the container. You should put this logic in the build() method instead

[stollr]

@dbu hm... usually that's not the case, because in console the Kernel is not told to load the class cache. But there may be people who turn that on. Then the error will occure.

@stof But the build() method is not called during web access. Please look at my comment above:

this does not work as expected @stof because the classes.php is generated on the first kernel boot, which does not happen when clearing cache via cli

For better unterstanding:

1. app/console cache:clear clears cache and builds the mapping for the class cache (app/cache/dev/classes.map). In this stage the class cache is not created, yet, and the declared classes are not regarded.
2. on next kernel boot the class cache is loaded, if Kernel::$loadClassCache is true (see Symfony\Component\HttpKernel\Kernel::boot`) regarding the declared classes.

What does that mean:

• Loading the CacheEvent in bundle's build() method won't have any effect, because declared classes are not regarded (see 1.) during build phase.
• Loading it in EventDispatchingHttpCache will fail if the user calls $kernel->loadClassCache(); in app/console

Solutions:

• tell the user to load CacheEvent class in app/console, web/app.php and web/app_dev.php
• use the patch provided in [SymfonyCache] Remove event classes from Symfony's class cache to fix #242 #243
• or maybe there is a better one? :-)

[dbu]

loadClassCache is only triggered in the app.php and app_dev.php by default. but i think we should tell people to overwrite their AppKernel::boot method to prepend the class_exists call. or maybe more straightforward the loadClassCache method. that would fix the problem in a single place. loadClassCache does not yet do anything, it just tells the kernel to load the cache at boot time.

hm, but i think i still did not get it. that classes.map file is only built if loadClassCache was called. but it is only done at AppKernel::boot, so after AppCache::handle started. that handle method however triggers one of our events. so the classes should be properly seen, or not? or do we need a class_exists in case there are no event listeners?

do we try to load the class before the cache is built, so that the cache building sees that this class is needed?

[stollr]

hm, but i think i still did not get it. that classes.map file is only built if loadClassCache was called.

No, the classes.map is build on cache warming. The classes.php is built on kernel boot if loadClassCache was called.

but it is only done at AppKernel::boot, so after AppCache::handle started. that handle method however triggers one of our events. so the classes should be properly seen, or not? or do we need a class_exists in case there are no event listeners?

This is a bit complicated. The CacheEvent is not triggered if the HTTP method is GET. And most requests are getters. So usually the kernel is booted on a GET request, which means that the CacheEvent class is not loaded and so Event is later built into the class cache.

Then later when a POST request is done, the CacheEvent is loaded in EventDispatchingHttpCache::handle and later the class cache is loaded, too, and boom -> class redeclaration error occures.

If the first request after a cache warmup is a POST request, the Event class is not built into the class cache and the problem does not occure.

do we try to load the class before the cache is built, so that the cache building sees that this class is needed?

The class is not loaded if it's a GET request.

Sorry, if I am sometimes a bit unclear.

[stollr]

I have tested adding class_exists('FOS\\HttpCache\\SymfonyCache\\CacheEvent'); to the constructor in `app/AppCache.php and this works, too.

[dbu]

okay, i think i am slowly getting it. no need to apologize, i don't think you are unclear. its just really quite complicated.

would it always work if we do the class_exists() in the handle method of the AppCache, this line in the new trait? https://github.com/FriendsOfSymfony/FOSHttpCache/pull/233/files#diff-c4c7c91d2acdd9555a29b400d019d6b0R78

the problem with the constructor is that we are moving towards a trait, where we won't tell people anymore what constructor they should have.

[stollr]

As far as I see, it would work. But only if $kernel->loadClassCache() is not used in app/console. Otherwise user's will get the same error (as said here).

[dbu]

doing loadClassCache in app/console is not the default behaviour. so i would just add a warning in the documentation about the app/console topic. and if people don't read the doc but do optimizations, they hopefully find the doc when googling about the error... and given how magically that class cache is built, using loadClassCache in the console and app.php sounds like inviting some disaster anyways.

i suggest that we add this trick to the handle method to the master branch of FOSHttpCache in the trait for the cache kernel, and for now also as bugfix in the kernel in this repository. ok?

[stollr]

@dbu I have updated the PR to reflect our latest idea and added some documentation how to handle the redeclaration error if loadClassCache is used in app/console.

I hope this is fine now :-)

Maybe you could pick it and put it into the trait in master?

[stollr]

Can somebody tell me what's wrong with my change in file Resources/doc/features/symfony-http-cache.rst? It seems like the build failed because of an error there.

[dbu]

sure, will port that to the trait. will have some time for this tomorrow. the build failure is because the spell checker is not happy with some of the words. app, dev and php are not in its dictionary, will need to look whether we find a different way of saying it, or put it as `code` that is not checked. "occure" should probably be "occurs"

[stollr]

Okay, the spelling errors are fixed, now. But there is still one problem. I don't know how this could be introduced by my PR:

Problem 1
- Installation request for symfony/symfony 2.8.*@dev -> satisfiable by symfony/symfony[2.8.x-dev].
- symfony/symfony 2.8.x-dev requires symfony/polyfill-intl-icu ~1.0 -> no matching package found.

[dbu]

see FriendsOfSymfony/FOSHttpCache#249

thanks a lot @Naitsirch for digging through this. i hope we manage to update the bundle to 2.0 soon, then the problem will go away here as well.