[2.3][Enhancement][FrameworkBundle] Move the warmup logic to the CacheWarmerAggregate #6223

vicb · 2012-12-07T10:44:43Z

This is mainly moving logic from the cache clear command to its own class so that it could be called more easily (ie from the WDT).

Feedback ?

fabpot · 2012-12-11T12:43:40Z

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

-            rename($realCacheDir, $oldCacheDir);
+            $oldCacheDir  = $cacheDir.'__old__';
+            rename($cacheDir, $oldCacheDir);
+            $this->getContainer()->get('filesystem')->remove($oldCacheDir);


This can be removed as it is already done after the condition.

Actually, you should remove the other one instead.

vicb · 2012-12-11T13:07:42Z

@fabpot sorry for this messy PR, but I wanted to have some feedback before polishing the code.
Would it be accepted ?

fabpot · 2012-12-11T14:24:30Z

The approach looks good to me. The only thing I'm not sure about is whether the logic belongs to the cache warmer aggregate or to another (new) class (I tend to think that the latter would be better).

vicb · 2012-12-11T15:00:09Z

You are probably right. It would also avoid a BC break (warmers position in the constructor). I'll update after I have time to think about it.

vicb · 2012-12-11T17:23:29Z

@fabpot what about CacheWarmerDumper ?

vicb · 2012-12-11T17:24:59Z

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

-        }
-
-        // fix meta references to the Kernel
-        foreach ($finder->files()->name('*.meta')->in($warmupDir) as $file) {


@fabpot:

is reusing the finder intended (or a bug) here ?

is handling serialized class only (vs string, ie paths) intended ?

Thanks, too tired to figure out by myslef tonight !

all refs seem to be handled - 3rd party bundles ?

I am mainly talking about the cache folder in string (ie a:9:{i:0;C:46:"Symfony\Component\Config\Resource\FileResource":60:{s:52:"/var/www/sf2.2/app/cache/dev_new/as)

The reuse of the finder is indeed a bug.

…rAggregate class

…mer one Filters are cumulative

vicb · 2012-12-17T10:31:32Z

@schmittjoh would you please have a look why this fails when the cache folder is empty ?

Fatal error: Cannot redeclare class EnhancedProxy_72114c0f8683bcc2a66ceccb136abe5a78f8577e\__CG__\Acme\DemoBundle\Controller\SecuredController in /var/www/sf2.2/app/cache/dev__new__/jms_diextra/proxies/Acme-DemoBundle-Controller-SecuredController.php on line 28

schmittjoh · 2012-12-17T15:42:54Z

It's not something that I can debug soon, but as a pointer it is likely
something that needs to be updated in the ControllerWarmer.

On Mon, Dec 17, 2012 at 11:31 AM, Victor Berchet
notifications@github.comwrote:

@schmittjoh https://github.com/schmittjoh would you please have a look
why this fails when the cache folder is empty ?

Fatal error: Cannot redeclare class EnhancedProxy_72114c0f8683bcc2a66ceccb136abe5a78f8577e__CG__\Acme\DemoBundle\Controller\SecuredController in /var/www/sf2.2/app/cache/dev__new__/jms_diextra/proxies/Acme-DemoBundle-Controller-SecuredController.php on line 28

—
Reply to this email directly or view it on GitHubhttps://github.com//pull/6223#issuecomment-11436754.

vicb · 2012-12-17T15:52:14Z

Thanks, think I got it, need to test... magic values !!!

On 12/17/2012 04:43 PM, Johannes wrote:

It's not something that I can debug soon, but as a pointer it is likely
something that needs to be updated in the ControllerWarmer.

On Mon, Dec 17, 2012 at 11:31 AM, Victor Berchet
notifications@github.comwrote:

@schmittjoh https://github.com/schmittjoh would you please have a
look
why this fails when the cache folder is empty ?

Fatal error: Cannot redeclare class
EnhancedProxy_72114c0f8683bcc2a66ceccb136abe5a78f8577e__CG__\Acme\DemoBundle\Controller\SecuredController
in
/var/www/sf2.2/app/cache/dev__new__/jms_diextra/proxies/Acme-DemoBundle-Controller-SecuredController.php
on line 28

—
Reply to this email directly or view it on
GitHubhttps://github.com//pull/6223#issuecomment-11436754.

—
Reply to this email directly or view it on GitHub
#6223 (comment).

vicb · 2012-12-17T16:09:15Z

BC PR pending.

On 12/17/2012 04:43 PM, Johannes wrote:

It's not something that I can debug soon, but as a pointer it is likely
something that needs to be updated in the ControllerWarmer.

On Mon, Dec 17, 2012 at 11:31 AM, Victor Berchet
notifications@github.comwrote:

@schmittjoh https://github.com/schmittjoh would you please have a
look
why this fails when the cache folder is empty ?

Fatal error: Cannot redeclare class
EnhancedProxy_72114c0f8683bcc2a66ceccb136abe5a78f8577e__CG__\Acme\DemoBundle\Controller\SecuredController
in
/var/www/sf2.2/app/cache/dev__new__/jms_diextra/proxies/Acme-DemoBundle-Controller-SecuredController.php
on line 28

—
Reply to this email directly or view it on
GitHubhttps://github.com//pull/6223#issuecomment-11436754.

—
Reply to this email directly or view it on GitHub
#6223 (comment).

vicb · 2012-12-18T11:55:28Z

@fabpot this is ready to be merged @schmittjoh has merged the PR to the DIXtra

fabpot · 2012-12-18T12:10:47Z

So, you fixed Johannes bundle but what if other bundles out there did the same? What about reverting to using the current suffixes?

…bundles

vicb · 2012-12-18T13:23:35Z

So, you fixed Johannes bundle

I prefer to think that I have fixed the usage of magic values.

May be the PR should not be squashed at it contains different things.

fabpot · 2013-03-20T16:58:59Z

This PR should be updated with the recent changes made to the cache:clear command.

vicb · 2013-03-28T08:41:33Z

It would probably have been smarter to merge this PR before #7360 but it's too late and we have introduced yet a new bug (schmittjoh/JMSDiExtraBundle#96).

@jfsimon you will find the root cause of the bug you have introduced by looking at the comment of this PR (I did introduce the same bug at the start of the PR before making a fix on @schmittjoh repo).

Anyway I don't have a lot of time to work on this now and the work should happen after @jfsimon has fixed his PR. @jfsimon could you please take care of syncing this PR once you're done - ping me if you need help.

(This bug shows one more reason why magic values are evil for the ones who where not already convinced)

/cc @swis @craue

jfsimon · 2013-03-28T14:25:30Z

@vicb I wished I find you on Jabber... wouldn't it be more simple to rebase your PR on master?

jfsimon · 2013-03-28T14:26:53Z

@vicb PS: I have enough time to do it for you.

vicb · 2013-03-28T14:30:23Z

here I am... soon

natebundy · 2013-04-02T15:29:42Z

Hey guys, this is blocking us pretty hard in our current project where we're trying to use @secure to secure a REST API in Symfony2. Is there a way for us to help get this done? I'm willing to help any way I can including getting final code together for a pull request. Thanks!

vicb · 2013-04-02T18:25:46Z

I won't have time to look into this before the end of this week.

This is a bit tricky as the root cause of the issue has not been really identified (according to @jfsimon himself). I am talking about the fix done in #7360, not the "new" issue.

Then we have two options:

revert [FrameworkBundle] fixes cahe:clear command's warmup #7360 and try to better analyze what the issue was (ie reproduce and come with a correct and tested fix). This could lead to a better code but some people might see some defects before the fix is found & deployed.
modify the JMSBundle to fix the new issue (the one that bugs people now). If the fix in [FrameworkBundle] fixes cahe:clear command's warmup #7360 is not the right one (hard to tell without a proper analyze) then the issue might arise in the future.

@fabpot what do you think ?

The current issue is not related to this PR and discussions should better switch to #7505 from now.

fabpot reviewed Dec 11, 2012
View reviewed changes

vicb reviewed Dec 11, 2012
View reviewed changes

vicb added 4 commits December 17, 2012 10:24

[Finder] Add SplFileInfo::putContents()

ddd119f

[HttpKernel] Move the warmup logic from the command to the CacheWarme…

9f82ce0

…rAggregate class

[CacheWarmerAggregate] Create a new Finder instead of reusing the for…

cf5ffd4

…mer one Filters are cumulative

[CacheWarmerAggregate] Fix refs to the kernel

7aa5e31

vicb added 2 commits December 17, 2012 11:37

[Filesystem] Add a fluid interface

5176148

[CacheWarmerAggregate] Leverage the Filesystem fluid interface

d54d21d

vicb mentioned this pull request Dec 17, 2012

[ControllerInjectorsWarmer] Sync with future sf2.2 schmittjoh/JMSDiExtraBundle#78

Merged

[CacheWarmerAggregate] Revert to old suffixes to avoid breaking 3rdp …

47f2712

…bundles

vicb mentioned this pull request Mar 28, 2013

[FrameworkBundle] cache:clear command broken #7505

Closed

vicb closed this Jan 30, 2014

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[2.3][Enhancement][FrameworkBundle] Move the warmup logic to the CacheWarmerAggregate #6223

[2.3][Enhancement][FrameworkBundle] Move the warmup logic to the CacheWarmerAggregate #6223

vicb commented Dec 7, 2012

fabpot Dec 11, 2012

fabpot Dec 11, 2012

vicb commented Dec 11, 2012

fabpot commented Dec 11, 2012

vicb commented Dec 11, 2012

vicb commented Dec 11, 2012

vicb Dec 11, 2012

vicb Dec 11, 2012

fabpot Dec 13, 2012

vicb commented Dec 17, 2012

schmittjoh commented Dec 17, 2012

vicb commented Dec 17, 2012

vicb commented Dec 17, 2012

vicb commented Dec 18, 2012

fabpot commented Dec 18, 2012

vicb commented Dec 18, 2012

fabpot commented Mar 20, 2013

vicb commented Mar 28, 2013

jfsimon commented Mar 28, 2013

jfsimon commented Mar 28, 2013

vicb commented Mar 28, 2013

natebundy commented Apr 2, 2013

vicb commented Apr 2, 2013

[2.3][Enhancement][FrameworkBundle] Move the warmup logic to the CacheWarmerAggregate #6223

[2.3][Enhancement][FrameworkBundle] Move the warmup logic to the CacheWarmerAggregate #6223

Conversation

vicb commented Dec 7, 2012

fabpot Dec 11, 2012

Choose a reason for hiding this comment

fabpot Dec 11, 2012

Choose a reason for hiding this comment

vicb commented Dec 11, 2012

fabpot commented Dec 11, 2012

vicb commented Dec 11, 2012

vicb commented Dec 11, 2012

vicb Dec 11, 2012

Choose a reason for hiding this comment

vicb Dec 11, 2012

Choose a reason for hiding this comment

fabpot Dec 13, 2012

Choose a reason for hiding this comment

vicb commented Dec 17, 2012

schmittjoh commented Dec 17, 2012

vicb commented Dec 17, 2012

vicb commented Dec 17, 2012

vicb commented Dec 18, 2012

fabpot commented Dec 18, 2012

vicb commented Dec 18, 2012

fabpot commented Mar 20, 2013

vicb commented Mar 28, 2013

jfsimon commented Mar 28, 2013

jfsimon commented Mar 28, 2013

vicb commented Mar 28, 2013

natebundy commented Apr 2, 2013

vicb commented Apr 2, 2013