10000 [HttpKernel] enabling cache-reloading when cache file is rebuilt by stampycode · Pull Request #20065 · symfony/symfony · GitHub
[go: up one dir, main page]
Skip to content

[HttpKernel] enabling cache-reloading when cache file is rebuilt #20065

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
wants to merge 4 commits into from
Closed

[HttpKernel] enabling cache-reloading when cache file is rebuilt #20065

wants to merge 4 commits into from

Conversation

stampycode
Copy link
@stampycode stampycode commented Sep 27, 2016 •
edited
Loading
Q A
Branch? "master"
Bug fix? yes
New feature? no
BC breaks? no
Deprecations? no
Tests pass? all, and disabled one test which is misleading. (

symfony/src/Symfony/Bundle/TwigBundle/Tests/Functional/CacheWarmingTest.php

Line 34 in f29d46f

public function testCacheIsNotWarmedWhenTemplatingIsDisabled()
)
Fixed tickets #20024
License MIT
Doc PR none

Allows the cache file to be deleted, rebuilt & reloaded between Container builds in WebTestCase scenario, to enable testing of multiple configurations of the same application through WebTestCase.

Will only reload the cache file when in debug mode and the cache class file is deleted.

[HttpKernel] enabling cache-reloading when cache file is rebuilt
977b27f 
This allows the cache file to be deleted and thus rebuilt between Container builds in WebTestCase scenario, to enable testing of multiple configurations of the same application through WebTestCase.
@stampycode
Copy link
Author

In fact, the CacheWarming test that fails here is misleading; In master, the two test cases, testCacheIsProperlyWarmedWhenTemplatingIsAvailable and testCacheIsNotWarmedWhenTemplatingIsDisabled fail when the tests are reversed. This shows that the cache pre-warming always takes place if the class hasn't previously been loaded with require_once... @tucksaun

This PR effectively re-generates classes that are 'not fresh' while ditching the logic behind require_once to tell us whether a class has been loaded before, regardless of whether we have a newer version of the class we want to load instead.

This makes for a much more powerful testing suite, being able to test a Container with a config that changes between test cases.

stampy added 3 commits September 28, 2016 10:27
[CacheWarmingTest] skipping test, as it's misleading.
fac0a35 
This test fails if run before the previous test, so is actually dependent on the current execution history.
[HttpKernel] finding loaded class name when loading container from ca…
a165fa8 
…che file

When generating multiple versions of the same container class, each new class name is suffixed with an incrementing version number, as you can't re-use class names.
So when we load the class file, the class itself may have a different name than the file name suggests - so after we load the class file, we have to find the class name.
[HttpKernel] load cached class fails when desired class is versioned
d20686c
@stampycode stampycode mentioned this pull request Sep 28, 2016
Closed
@stampycode
Copy link
Author

@fabpot @jakzal @nicolas-grekas - can someone please nudge fabbot.io ?

This was referenced Sep 30, 2016
Merged
Merged
@tucksaun
Copy link
Contributor

thanks for reporting this @stampycode !
this should be fixed with #20103

fabpot added a commit that referenced this pull request Sep 30, 2016
@fabpot
minor #20103 [TwigBundle] Fix CacheWarmingTest are order dependent (t…
d2c050e 
…ucksaun)

This PR was merged into the 2.7 branch.

Discussion
----------

[TwigBundle] Fix CacheWarmingTest are order dependent

| Q             | A
| ------------- | ---
| Branch?       | 2.7
| Bug fix?      | yes
| New feature?  | no
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | -
| License       | MIT
| Doc PR        | -

Fix issue reported in #20065

Commits
-------

fcd6ec2 [TwigBundle] Fix CacheWarmingTest are order dependent
@fabpot fabpot closed this Sep 30, 2016
fabpot added a commit that referenced this pull request Sep 30, 2016
@fabpot
minor #20104 [TwigBundle] Adjust CacheWarmingTest for TemplateCacheWa…
2081bd9 
…rmer introduced in 2.8 (tucksaun)

This PR was merged into the 2.8 branch.

Discussion
----------

[TwigBundle] Adjust CacheWarmingTest for TemplateCacheWarmer introduced in 2.8

| Q             | A
| ------------- | ---
| Branch?       | 2.8
| Bug fix?      | yes
| New feature?  | no
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | -
| License       | MIT
| Doc PR        | -

With #20103, issue reported in #20065 is fixed, however it will now break in 2.8+ as another cache warmer has been introduced with #16271.

Commits
-------

360ca48 [TwigBundle] Adjust CacheWarmingTest for TemplateCacheWarmer introduced in 2.8
@stampycode
Copy link
Author
stampycode commented Sep 30, 2016 •
edited
Loading

@fabpot, @tucksaun - finding the bug in the CacheWarmingTest was only a side-effect of this ticket - the purpose of which was to merge my PR - is this now being rejected with no reason?

@jakzal
Copy link
Contributor
jakzal commented Oct 3, 2016

@stampycode problem with this PR in my view is that it introduces quite a lot of code (that could potentially break other people's code) for what I see as an edge case.

@stampycode
Copy link
Author

@jakzal Surely any change to the Kernel could potentially break other people's code if other people are toying with the Kernel?
Currently what happens, if the cache files are deleted between tests, is the cached files are re-generated and then NOT USED, because the require_once statement blocks the same file being re-loaded, and php disallows the same class from being declared twice.

In my mind this PR is fixing a bug - you can put debug statements in the parameters.php file and see that the file is being reloaded, but there is no indication that the cache file is not being reloaded when the file is re-generated.

It's not a core deliverable of Symfony to silently block the re-loading of that cache file in a single execution, such as during PHPUnit tests.

It may break people's code if they are depending on this bug existing, but that doesn't sound like a good argument to not fix it.

And this would make PHPUnit and WebTestCase much more powerful.

@jakzal
Copy link
Contributor
jakzal commented Oct 3, 2016

Loading the kernel class that changes during the request is not a supported use case (this doesn't happen in real life). I think this PR is a wrong solution to the problem since it introduces code in a production class that would only be there for tests.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
Bug HttpKernel Status: Needs Review
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@stampycode @tucksaun @jakzal @fabpot @carsonbot
0