[HttpKernel][FrameworkBundle] Add RebootableInterface, fix and un-deprecate cache:clear with warmup #23792

nicolas-grekas · 2017-08-04T22:05:38Z

Q	A
Branch?	3.4
Bug fix?	no
New feature?	yes
BC breaks?	no
Deprecations?	yes
Tests pass?	yes
Fixed tickets	#23592, #8500
License	MIT
Doc PR	-

This PR fixes the reasons why we deprecated cache:clear without --no-warmup.

Internally, it requires some cooperation from the Kernel, so that a deprecation is needed.
This allows the same kernel instance to be rebooted under a new cache directory, used during the warmup.
It leverages the new capability to create two independent non colliding containers.

ogizanagi · 2017-08-05T06:19:20Z

src/Symfony/Bundle/FrameworkBundle/Command/CacheClearCommand.php

+        } else {
+            $realKernel->reboot($warmupDir);
+
+            if (!$isRebootable = $realKernel->getCacheDir() === $realCacheDir) {


Should be:

- if (!$isRebootable = $realKernel->getCacheDir() === $realCacheDir) { + if (!$isRebootable = $realKernel->getCacheDir() !== $realCacheDir) {

right?

comments addressed thanks

nicolas-grekas · 2017-08-07T18:35:51Z

rebased and ready

javiereguiluz · 2017-08-07T18:41:29Z

It's hard to review PRs without description (even if the long title explains a bit what this PR does).

For this and every other PR I'd like to have two brief descriptions:

how does this improves end-user life;
what internal changes will this introduce for Symfony (so we can update Symfony Docs if needed).

Thanks!

Koc · 2017-08-08T08:33:14Z

@nicolas-grekas does it fixes #8500 also?

nicolas-grekas · 2017-08-08T08:51:10Z

@Koc looks like so, but only you can confirm :)

Koc · 2017-08-08T09:03:31Z

does no new instance of AppKernel creates on cache clear?

nicolas-grekas · 2017-08-08T09:16:37Z

That's correct, it reuses (reboots) the same instance.

chalasr

should removals in UPGRADE and CHANGELOG files be reverted (already removed in #23825)?

mnapoli · 2017-08-08T20:04:22Z

src/Symfony/Component/HttpKernel/RebootableInterface.php

+ *
+ * @author Nicolas Grekas <p@tchwork.com>
+ */
+interface RebootableInterface


Should this interface be merged into HttpKernel for 4.0? It seems a lot of stuff will rely on the reboot() method with this PR, maybe there's no sense to keep it in a separate interface and still require kernels to implement it?

(note that my comment would apply specifically for 4.0, not 3.x)

Clearly not in HttpKernel, as only Kernel implements it, not HttpKernel.

Yep my bad, Kernel!

But the method is used only in the cache:clear command, not sure that accounts for "a lot" :)

fabpot · 2017-08-18T07:47:28Z

UPGRADE-3.4.md

+   {
+       public function getCacheDir()
+       {
+           return $this->getWarmupDir() ?: 'my-cache-dir';


I don't like the fact that an implementation detail leaks (getWarmupDir() is really an internal detail). I would prefer to not change this method at all. getCacheDir() is public because it's part of HttpKernelInterface to provide an easy way to override the cache dir in the project's kernel. It was never meant to be called directly, and most devs probably use kernel.cache_dir anyway.

Instead, we could add a phpdoc warning explaining what I've just written :)

…clear with warmup (fabpot)" (nicolas-grekas) This PR was merged into the 3.3 branch. Discussion ---------- Revert "feature #21038 [FrameworkBundle] deprecated cache:clear with warmup (fabpot)" This reverts commit 3495b35, reversing changes made to 7f7b897. | Q | A | ------------- | --- | Branch? | 3.3 | Bug fix? | yes | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | #21038 | License | MIT | Doc PR | - Sibling to #23792: there is no need to trigger the deprecation in 3.3 if we un-deprecate in 3.4. Commits ------- 9009970 Revert "feature #21038 [FrameworkBundle] deprecated cache:clear with warmup (fabpot)"

nicolas-grekas · 2017-08-18T10:38:10Z

getWarmupDir is now gone, comments updated
should be ready

nicolas-grekas · 2017-08-18T10:52:22Z

src/Symfony/Component/HttpKernel/Kernel.php

@@ -786,9 +799,6 @@ protected function dumpContainer(ConfigCache $cache, ContainerBuilder $container
            @chmod($dir.$file, 0666 & ~umask());
        }

-        // track changes made to the container directory
-        $container->fileExists(dirname($dir.$file));


Removes a transient test (CacheClearCommandTest::testCacheIsFreshAfterCacheClearedWithWarmup: Meta file "FixtureTestDebugProjectContainer.php.meta" is not fresh)
and allows debugging the container by patching its generated files when needed (I added this code in 3.4. A bad idea.)
If you mess up with the container files, that's your problem :)

fabpot · 2017-08-18T11:44:38Z

src/Symfony/Component/HttpKernel/RebootableInterface.php

+namespace Symfony\Component\HttpKernel;
+
+/**
+ * Rebotable extends the Kernel to allow it to reboot using a temporary cache directory.


typo Rebootable. Anway, I would say Allows the Kernel to be rebooted using ...

fabpot · 2017-08-18T11:45:45Z

src/Symfony/Component/HttpKernel/RebootableInterface.php

+     *
+     * @param string|null $warmupDir
+     */
+    public function reboot($warmupDir);


What's the use case for passing null?

* @param string|null $warmupDir pass `null` to reboot to the regular cache dir

fabpot · 2017-08-18T11:49:49Z

src/Symfony/Component/HttpKernel/RebootableInterface.php

+     * The getCacheDir() method of a rebootable kernel should not be called
+     * while building the container. Use the %kernel.cache_dir% parameter instead.
+     *
+     * @param string|null $warmupDir pass `null` to reboot to the regular cache dir


null without ```

to reboot in the regular cache dir?

…recate cache:clear with warmup

fabpot · 2017-08-18T12:01:17Z

Thank you @nicolas-grekas.

… fix and un-deprecate cache:clear with warmup (nicolas-grekas) This PR was merged into the 3.4 branch. Discussion ---------- [HttpKernel][FrameworkBundle] Add RebootableInterface, fix and un-deprecate cache:clear with warmup | Q | A | ------------- | --- | Branch? | 3.4 | Bug fix? | no | New feature? | yes | BC breaks? | no | Deprecations? | yes | Tests pass? | yes | Fixed tickets | #23592, #8500 | License | MIT | Doc PR | - This PR fixes the reasons why we deprecated cache:clear without --no-warmup. Internally, it requires some cooperation from the Kernel, so that a deprecation is needed. This allows the same kernel instance to be rebooted under a new cache directory, used during the warmup. It leverages the new capability to create two independent non colliding containers. Commits ------- a4fc492 [HttpKernel][FrameworkBundle] Add RebootableInterface, fix and un-deprecate cache:clear with warmup

hkdobrev · 2017-11-21T23:12:11Z

Thank you for this @nicolas-grekas! I think this blog post could be updated mentioning cache:clear with warmup would not be deprecated in 4.0 if you use RebootableInterface. https://symfony.com/blog/new-in-symfony-3-3-deprecated-cache-clear-with-warmup It's coming as a first result in various cache warmup queries on Google search.

nicolas-grekas · 2017-11-22T05:46:58Z

Good idea, ping @javiereguiluz

amici · 2018-11-02T11:44:32Z

The original (and outdated) 3.3 news is still the first to see on google - and kind of very easy to miss the very important notice about the un-deprecation. Could it be a bit more visible? I'd write there, but comments are closed.

javiereguiluz · 2018-11-05T09:35:40Z

@amici I agree with you. Instead of making the warning more visible ... we've decided to remove the original blog post content. See https://symfony.com/blog/new-in-symfony-3-3-deprecated-cache-clear-with-warmup Thanks!

nicolas-grekas added this to the 3.4 milestone Aug 4, 2017

carsonbot added Status: Needs Review FrameworkBundle HttpKernel Feature Deprecation labels Aug 4, 2017

nicolas-grekas force-pushed the cache-clear branch from 7a2f817 to f25709d Compare August 4, 2017 22:09

ogizanagi reviewed Aug 5, 2017

View reviewed changes

Simperfit approved these changes Aug 5, 2017

View reviewed changes

nicolas-grekas force-pushed the cache-clear branch from f25709d to 2f023ba Compare August 5, 2017 09:51

symfony deleted a comment from ogizanagi Aug 5, 2017

nicolas-grekas force-pushed the cache-clear branch 4 times, most recently from 81671eb to f8adb38 Compare August 7, 2017 18:35

nicolas-grekas force-pushed the cache-clear branch from f8adb38 to 13823e0 Compare August 7, 2017 19:04

nicolas-grekas changed the title ~~[FrameworkBundle][HttpKernel] Fix and un-deprecate cache:clear[ --no-warmup] with new RebootableInterface~~ [HttpKernel][FrameworkBundle] Add RebootableInterface, fix and un-deprecate cache:clear with warmup Aug 7, 2017

nicolas-grekas force-pushed the cache-clear branch 2 times, most recently from 9ec551a to a600b3d Compare August 8, 2017 06:34

nicolas-grekas mentioned this pull request Aug 8, 2017

Revert "feature #21038 [FrameworkBundle] deprecated cache:clear with warmup (fabpot)" #23825

Merged

nicolas-grekas force-pushed the cache-clear branch from a600b3d to f2d2e05 Compare August 8, 2017 06:44

chalasr approved these changes Aug 8, 2017

View reviewed changes

ogizanagi approved these changes Aug 8, 2017

View reviewed changes

mnapoli reviewed Aug 8, 2017

View reviewed changes

nicolas-grekas force-pushed the cache-clear branch from f2d2e05 to c200b89 Compare August 17, 2017 09:29

fabpot requested changes Aug 18, 2017

View reviewed changes

nicolas-grekas force-pushed the cache-clear branch 3 times, most recently from 22c419d to 1f08478 Compare August 18, 2017 10:01

nicolas-grekas force-pushed the cache-clear branch 2 times, most recently from 3ae675f to 14b08ec Compare August 18, 2017 10:49

nicolas-grekas commented Aug 18, 2017

View reviewed changes

fabpot requested changes Aug 18, 2017

View reviewed changes

nicolas-grekas force-pushed the cache-clear branch 2 times, most recently from 302592c to 03e6ef6 Compare August 18, 2017 11:49

fabpot requested changes Aug 18, 2017

View reviewed changes

[HttpKernel][FrameworkBundle] Add RebootableInterface, fix and un-dep…

a4fc492

…recate cache:clear with warmup

nicolas-grekas force-pushed the cache-clear branch from 03e6ef6 to a4fc492 Compare August 18, 2017 11:51

fabpot approved these changes Aug 18, 2017

View reviewed changes

fabpot merged commit a4fc492 into symfony:3.4 Aug 18, 2017

nicolas-grekas deleted the cache-clear branch August 18, 2017 15:11

javiereguiluz mentioned this pull request Sep 10, 2017

[FrameworkBundle] Added RebootableInterface symfony/symfony-docs#8379

Closed

This was referenced Oct 18, 2017

Release v3.4.0-BETA1 #24609

Merged

Release v4.0.0-BETA1 #24612

Merged

nicolas-grekas mentioned this pull request Dec 27, 2017

[FrameworkBundle] Make cache:clear "atomic" and consistent with cache:warmup #25117

Merged

grossmannmartin mentioned this pull request May 1, 2020

[project-base] improved clearing cache behavior shopsys/shopsys#1820

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[HttpKernel][FrameworkBundle] Add RebootableInterface, fix and un-deprecate cache:clear with warmup #23792

[HttpKernel][FrameworkBundle] Add RebootableInterface, fix and un-deprecate cache:clear with warmup #23792

nicolas-grekas commented Aug 4, 2017 •

edited

Loading

ogizanagi Aug 5, 2017

nicolas-grekas Aug 5, 2017

nicolas-grekas commented Aug 7, 2017

javiereguiluz commented Aug 7, 2017

Koc commented Aug 8, 2017

nicolas-grekas commented Aug 8, 2017

Koc commented Aug 8, 2017

nicolas-grekas commented Aug 8, 2017

chalasr left a comment

mnapoli Aug 8, 2017

stof Aug 8, 2017

mnapoli Aug 8, 2017

nicolas-grekas Aug 8, 2017 •

edited

Loading

fabpot Aug 18, 2017

nicolas-grekas commented Aug 18, 2017

nicolas-grekas Aug 18, 2017 •

edited

Loading

fabpot Aug 18, 2017

fabpot Aug 18, 2017

nicolas-grekas Aug 18, 2017

fabpot Aug 18, 2017

fabpot Aug 18, 2017

fabpot commented Aug 18, 2017

hkdobrev commented Nov 21, 2017 •

edited

Loading

nicolas-grekas commented Nov 22, 2017

amici commented Nov 2, 2018

javiereguiluz commented Nov 5, 2018

[HttpKernel][FrameworkBundle] Add RebootableInterface, fix and un-deprecate cache:clear with warmup #23792

[HttpKernel][FrameworkBundle] Add RebootableInterface, fix and un-deprecate cache:clear with warmup #23792

Conversation

nicolas-grekas commented Aug 4, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nicolas-grekas commented Aug 7, 2017

javiereguiluz commented Aug 7, 2017

Koc commented Aug 8, 2017

nicolas-grekas commented Aug 8, 2017

Koc commented Aug 8, 2017

nicolas-grekas commented Aug 8, 2017

chalasr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nicolas-grekas Aug 8, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nicolas-grekas commented Aug 18, 2017

nicolas-grekas Aug 18, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fabpot commented Aug 18, 2017

hkdobrev commented Nov 21, 2017 • edited Loading

nicolas-grekas commented Nov 22, 2017

amici commented Nov 2, 2018

javiereguiluz commented Nov 5, 2018

nicolas-grekas commented Aug 4, 2017 •

edited

Loading

nicolas-grekas Aug 8, 2017 •

edited

Loading

nicolas-grekas Aug 18, 2017 •

edited

Loading

hkdobrev commented Nov 21, 2017 •

edited

Loading