Skip to content

[FrameworkBundle] fixes cahe:clear command's warmup #7360

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.

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

Merged
merged 3 commits into from
Mar 14, 2013

Conversation

jfsimon
Copy link
Contributor

@jfsimon jfsimon commented Mar 13, 2013

Solution taken is to replace the last char of the cache directory name to create a temporary cache directory, this way the temporary cache path has the same length than the real one. I tested this on several projects, in dev and prod environments.

Q A
Bug fix? yes
New feature? no
BC breaks? yes
Deprecations? no
Tests pass? yes
Fixed tickets #6203

$content = preg_replace('/'.preg_quote($warmupDir, '/').'/', preg_replace('/_new$/', '', $warmupDir), $content);

file_put_contents(preg_replace($regex, '', $file), $content);
$content = str_replace($this->getTempKernelSuffix(), '', file_get_contents($file));
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is str_replace better than preg_replace? I think it's more readable, but what about performances?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

str_replace is considered faster

@jfsimon
Copy link
Contributor Author

jfsimon commented Mar 13, 2013

@toloco @gergelypolonkai @ghost-x47 @stewe it would be great if you could test this patch on your projects and report result!

@toloco
Copy link

toloco commented Mar 13, 2013

Im sorry but have the same...

Notice: unserialize(): Error at offset 155 of 174227 bytes in /home/tolopalmer/Projects/shareandcoach/app/bootstrap.php.cache line 915

@jfsimon
Copy link
Contributor Author

jfsimon commented Mar 13, 2013

@toloco could you paste the backtrace in a gist? and maybe the concerned file?

@stof
Copy link
Member

stof commented Mar 13, 2013

@jfsimon You probably have the same issue with the name of the temporary kernel class

@jfsimon
Copy link
Contributor Author

jfsimon commented Mar 13, 2013

@stof if you're right, it's a nightmare. It must be possible to write a parser/fixer for serialized objects, don't you think?

@toloco
Copy link

toloco commented Mar 13, 2013

Here you are the gist with the stack and the bootstrap.php.cache file

https://gist.github.com/toloco/5152581

@mpdude
Copy link
Contributor

mpdude commented Mar 13, 2013

@jfsimon Writing such a parser is painting yourself in the corner.

Use a temp kernel class name of the same length as a quick fix.

#7230 could bring a solution because we might be able to inject a different ConfigCache factory during the command that intercepts and substitutes Resources before they get written into the meta file. Not sure if that PR has a chance of being picked though.

@toloco
Copy link

toloco commented Mar 14, 2013

So guys? we are blocked with this problem, can I help you? I can provide more stacks if it's needed

@mpdude
Copy link
Contributor

mpdude commented Mar 14, 2013

@toloco Could you please post the /home/tolopalmer/Projects/shareandcoach/app/cache/dev/appDevUrlMatcher.php.meta file? That's the one that is broken.

@jfsimon
Copy link
Contributor Author

jfsimon commented Mar 14, 2013

@mpdude you can find its content in the gist https://gist.github.com/toloco/5152581 (1st file, 6th line)

@mpdude
Copy link
Contributor

mpdude commented Mar 14, 2013

@toloco That file should contain a serialized set of Resources, it's not in the Gist.

@jfsimon
Copy link
Contributor Author

jfsimon commented Mar 14, 2013

@mpdude it's more visible in the raw file: ttps://gist.github.com/toloco/5152581/raw/48a1a823b5c8e6ba03936a52e8dc0d0ff1888f8a/Error+

@jfsimon
Copy link
Contributor Author

jfsimon commented Mar 14, 2013

@toloco
Copy link

toloco commented Mar 14, 2013

https://gist.github.com/toloco/5160317 here you are the appDevUrlMatcher.php and meta

@jfsimon
Copy link
Contributor Author

jfsimon commented Mar 14, 2013

@toloco I applied @mpdude's solution (have a temp kernel class name of the same length than the real one).
Could you test it to see if it fixes your problem?

@mpdude
Copy link
Contributor

mpdude commented Mar 14, 2013

@jfsimon Thanks!
@toloco If Jean-François' fix does not work, please make sure that the .meta file you posted was the broken one? I was able to unserialize it without problems.

@toloco
Copy link

toloco commented Mar 14, 2013

Man!!!! you are the fucking boss it works!!

@mpdude
Copy link
Contributor

mpdude commented Mar 14, 2013

@jfsimon you just made someone happy.

@jfsimon
Copy link
Contributor Author

jfsimon commented Mar 14, 2013

@toloco @mpdude \o/

fabpot added a commit that referenced this pull request Mar 14, 2013
This PR was merged into the 2.1 branch.

Commits
-------

f2ef6bc [FrameworkBundle] removed BC break
cc3a40e [FrameworkBundle] changed temp kernel name in cache:clear
7d87ecd [FrameworkBundle] fixed cahe:clear command's warmup

Discussion
----------

[FrameworkBundle] fixes cahe:clear command's warmup

Solution taken is to replace the last char of the cache directory name to create a temporary cache directory, this way the temporary cache path has the same length than the real one. I tested this on several projects, in dev and prod environments.

| Q             | A
| ------------- | ---
| Bug fix?      | yes
| New feature?  | no
| BC breaks?    | yes
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | #6203

---------------------------------------------------------------------------

by jfsimon at 2013-03-13T12:32:25Z

@toloco @gergelypolonkai @ghost-x47 @stewe it would be great if you could test this patch on your projects and report result!

---------------------------------------------------------------------------

by toloco at 2013-03-13T12:41:47Z

Im sorry but have the same...

Notice: unserialize(): Error at offset 155 of 174227 bytes in /home/tolopalmer/Projects/shareandcoach/app/bootstrap.php.cache line 915

---------------------------------------------------------------------------

by jfsimon at 2013-03-13T12:45:04Z

@toloco could you paste the backtrace in a gist? and maybe the concerned file?

---------------------------------------------------------------------------

by stof at 2013-03-13T13:11:47Z

@jfsimon You probably have the same issue with the name of the temporary kernel class

---------------------------------------------------------------------------

by jfsimon at 2013-03-13T13:36:13Z

@stof if you're right, it's a nightmare. It must be possible to write a parser/fixer for serialized objects, don't you think?

---------------------------------------------------------------------------

by toloco at 2013-03-13T14:22:56Z

Here you are the gist with the stack and the bootstrap.php.cache file

https://gist.github.com/toloco/5152581

---------------------------------------------------------------------------

by mpdude at 2013-03-13T20:08:08Z

@jfsimon Writing such a parser is painting yourself in the corner.

Use a temp kernel class name of the same length as a quick fix.

#7230 could bring a solution because we might be able to inject a different ConfigCache factory during the command that intercepts and substitutes Resources before they get written into the meta file. Not sure if that PR has a chance of being picked though.

---------------------------------------------------------------------------

by toloco at 2013-03-14T08:19:58Z

So guys? we are blocked with this problem, can I help you? I can provide more stacks if it's needed

---------------------------------------------------------------------------

by mpdude at 2013-03-14T10:05:05Z

@toloco Could you please post the /home/tolopalmer/Projects/shareandcoach/app/cache/dev/appDevUrlMatcher.php.meta file? That's the one that is broken.

---------------------------------------------------------------------------

by jfsimon at 2013-03-14T10:15:20Z

@mpdude you can find its content in the gist https://gist.github.com/toloco/5152581 (1st file, 6th line)

---------------------------------------------------------------------------

by mpdude at 2013-03-14T10:24:55Z

@toloco That file should contain a serialized set of Resources, it's not in the Gist.

---------------------------------------------------------------------------

by jfsimon at 2013-03-14T10:33:12Z

@mpdude it's more visible in the raw file: ttps://gist.github.com/toloco/5152581/raw/48a1a823b5c8e6ba03936a52e8dc0d0ff1888f8a/Error+

---------------------------------------------------------------------------

by jfsimon at 2013-03-14T10:33:27Z

sorry: https://gist.github.com/toloco/5152581/raw/48a1a823b5c8e6ba03936a52e8dc0d0ff1888f8a/Error+

---------------------------------------------------------------------------

by toloco at 2013-03-14T10:37:09Z

https://gist.github.com/toloco/5160317 here you are the appDevUrlMatcher.php and meta

---------------------------------------------------------------------------

by jfsimon at 2013-03-14T10:51:46Z

@toloco I applied @mpdude's solution (have a temp kernel class name of the same length than the real one).
Could you test it to see if it fixes your problem?

---------------------------------------------------------------------------

by mpdude at 2013-03-14T10:58:46Z

@jfsimon Thanks!
@toloco If Jean-François' fix does not work, please make sure that the .meta file you posted was the broken one? I was able to unserialize it without problems.

---------------------------------------------------------------------------

by toloco at 2013-03-14T11:02:09Z

Man!!!! you are the fucking boss it works!!

---------------------------------------------------------------------------

by mpdude at 2013-03-14T11:04:30Z

@jfsimon you just made someone happy.

---------------------------------------------------------------------------

by jfsimon at 2013-03-14T11:12:39Z

@toloco @mpdude \o/
@fabpot fabpot merged commit f2ef6bc into symfony:2.1 Mar 14, 2013
@craue
Copy link
Contributor

craue commented Mar 14, 2013

With this, I'm getting an error like

Fatal error: Cannot redeclare class EnhancedProxy_118d51486383c6c253e028cf920e4439dd7c4e33__CG__\MyVendor\MyBundle\Controller\MyController in app/cache/de_/jms_diextra/proxies/MyVendor-MyBundle-Controller-MyController.php on line 28

when clearing my cache.

@sstok
Copy link
Contributor

sstok commented Apr 12, 2013

I think I know what the problem is.

The cache command is started with the current container and static Kernel class name.
In the old situation a temporary Kernel class was created with a "unique" name, now the name is very predictable!
Leading to this problem.

The most simple solution would be to the not use a static _, but single random character (not equal to the last char of the real kernel class) :) the length is still the same but its name is dynamic.

@jfsimon can you test this theory?

@sstok
Copy link
Contributor

sstok commented Apr 13, 2013

I tried making the last char random, still getting the error.

@mpdude
Copy link
Contributor

mpdude commented Apr 14, 2013

What object does that proxy proxy? Which class does it extend? "Cannot redeclare class" rather sounds as if a proxy of the same name as an already used proxy was loaded...

@craue
Copy link
Contributor

craue commented Apr 22, 2013

Is anyone even working on a fix? The current behavior is pretty annoying. It seems to me that there's no progress as noone feels responsible.

@skeud
Copy link

skeud commented Jul 4, 2014

Is anyone working on fixing this issue?

@toloco
Copy link

toloco commented Jul 4, 2014

Man it's closed, what do you think?

@wouterj
Copy link
Member

wouterj commented Jul 4, 2014

@toloco let's keep it friendly here. Not everyone is familiair with the GitHub/git workflow. And if you read the previous comments, you see that there is a new issue raised, of which @skeud is asking for an update.

@skeud
Copy link

skeud commented Jul 4, 2014

Sorry for the too fast copy/paste from the other issue linked to this one. I didn't checked/see that the issue was closed here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants