8000 [WIP] remove tempname usage by cordoval · Pull Request #9442 · symfony/symfony · GitHub
[go: up one dir, main page]
Skip to content

[WIP] remove tempname usage #9442

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 12 commits into from
Closed

[WIP] remove tempname usage #9442

wants to merge 12 commits into from

Conversation

cordoval
Copy link
Contributor
@cordoval cordoval commented Nov 5, 2013
Q A
Bug Fix? n
New Feature? n
BC Breaks? n
Deprecations? n
Tests Pass? n
Fixed Tickets #9238
License MIT
Doc PR

from the list:

  • escapeshellarg() and escapeshellcmd() (?)
  • exec() (used by the Console, so not relevant)
  • highlight_file() (only used for debugging purpose, so not relevant)
  • proc_close(), proc_open() and proc_terminate() (used by the Process component, no way around it)
  • shell_exec() (used by the Console, so not relevant)
  • symlink() (used by the Filesystem and Finder) - used only in assets:install which is useless to do in GAE ready only FS
  • tempnam() - addressed in this PR

@cordoval cordoval restored the feature/tempnam-remove branch November 5, 2013 01:43
@cordoval
Copy link
Contributor Author
cordoval commented Nov 5, 2013

@stof i know this is probably wrong and stupid, but i was just experimenting and wanted to know if travis throws us some green by chance

@cordoval
Copy link
Contributor Author
cordoval commented Nov 5, 2013

@stof is travis build of symfony broken on master?

inoryy
inoryy reviewed Nov 5, 2013
View reviewed changes
src/Symfony/Component/Filesystem/Filesystem.php
@@ -471,4 +471,34 @@ private function toIterator($files)

return $files;
}

/** source: http://www.deltascripts.com/board/viewtopic.php?id=7843 */
private static function tempnam2($dir, $prefix, $postfix='')
Copy link
Contributor

Choose a reason for hiding this comment

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

private?

Copy link
Member

Choose a reason for hiding this comment

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

You should use tempnam when available, it must be more performant than your implementation.

@stof
Copy link
Member
stof commented Nov 5, 2013

@cordoval no they are not. See the error: Filesystem::tempnam2 is private but you are calling it outside Filesystem

stof
stof reviewed Nov 5, 2013
View reviewed changes
src/Symfony/Component/Translation/composer.json
@@ -20,7 +20,8 @@
},
"require-dev": {
"symfony/config": "~2.0",
"symfony/yaml": "~2.2"
"symfony/yaml": "~2.2",
"symfony/filesystem": "~2.3"
Copy link
Member

Choose a reason for hiding this comment

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

same constraint issue here

@cordoval
Copy link
Contributor Author
cordoval commented Nov 8, 2013

@stof also i am thinking we should add a session handler (custom) for Google Cloud SQL since that is what Google recommends. From then on we can forget about memcache current supported handler

@cordoval
Copy link
Contributor Author
cordoval commented Nov 9, 2013

@stof i think more is a question of whether or not the files under app/cache have absolute paths to files and work on them. I think for instance the files under:

/Users/cordoval/Sites/project/app/cache/prod/templates.php

has:

'FrameworkBundle:Form:attributes.html.php|bootstrap' => '/Users/cordoval/Sites/project/vendor/symfony/symfony/src/Symfony/Bundle/FrameworkBundle/Resources/views/Form/attributes.html.php',

which is totally dependent on my FS, it needs to have the path from the GAE FS.
If we can make it so that the files are all relative to the root folder then GAE will not notice and will use files.

Do you see other problems? I think the tempnam and all those usages are fine since they are done back in the development system. The problem that i see is perhaps making the development system think that the filesystem starts at the root directory of the project somehow.

The other thing is sessions, sessions can be handled via Memcache however GAE says it can be flushed at anytime.

?

@stof
Copy link
Member
stof commented Nov 9, 2013

@cordoval you should not copy your local cache to AppEngine. The cache is tied to the platform.

@cordoval
Copy link
Contributor Author
cordoval commented Nov 9, 2013

#9483

@cordoval
Copy link
Contributor Author

@stof i have resolved most of the problems but now this. I am thinking if you said that the cache/prod/classes.php gets regenerated even if uploaded i wonder how to avoid that. I guess putting it into the memcache also?
If so i have configured the memcache however this seems to be somewhere else. so there is a need to adapt that code to push things into memcache?

Fatal error: Uncaught exception 'RuntimeException' with message 'Failed to write cache file
 "/base/data/home/apps/s~cordoval/x/app/cache/prod/classes.php".' in 
/base/data/home/apps/s~cordoval/x/vendor/symfony/symfony/src/Symfony/Component/
ClassLoader/ClassCollectionLoader.php:241 Stack trace: #0
/base/data/home/apps/s~cordoval/x/vendor/symfony/symfony/src/Symfony/Component/
ClassLoader/ClassCollectionLoader.php(123):
Symfony\Component\ClassLoader\ClassCollectionLoader::writeCacheFile('/base/data/home...',
'doLoadClassCache('classes', '.php') #3 /base/data/home/apps/s~cordova in 
base/data/home/apps/s~cordoval/x/vendor/symfony/symfony/src/Symfony/Component/
ClassLoader/ClassCollectionLoader.php on line 241

it is a bit tricky, that class gets generated after i actually use/test the site on the browser on local. Then i move to other problems that i am solving

@stof
Copy link
Member
stof commented Nov 11, 2013

@cordoval Remove the call to $kernel->loadClasses() in your front controller and it won't get generated anymore

@cordoval
Copy link
Contributor Author

@stof yeah but i found other issues, we need to load twig templates paths from Memcache, so we need another implementation of TemplatePathsCacheWarmer and also another implementation of the locator injected into 8000 TemplateLocator's method locate. If you agree we can start working on this. 👶

fabpot
fabpot reviewed Dec 17, 2013
View reviewed changes
src/Symfony/Component/Filesystem/Filesystem.php

do {
$seed = substr(md5(microtime().posix_getpid()), 0, 8);
$filename = $dir . $trailing_slash . $prefix . $seed . $postfix;
Copy link
Member

Choose a reason for hiding this comment

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

not spaces around dots.

romainneutron
romainneutron reviewed Mar 13, 2014
View reviewed changes
src/Symfony/Component/Filesystem/Filesystem.php
}

do {
$seed = substr(md5(microtime().posix_getpid()), 0, 8);
Copy link
Contributor

Choose a reason for hiding this comment

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

posix_getpid() requires posix extension that is not available on Windows platform. You can use getmypid instead

@ihsw
Copy link
ihsw commented Mar 19, 2014

@cordoval Can this be used in a production environment? I'm interested in running Symfony on GAE.

@jakzal jakzal mentioned this pull request Mar 22, 2014
Closed
fabpot
fabpot reviewed Mar 26, 2014
View reviewed changes
src/Symfony/Component/Filesystem/Filesystem.php
}

/* The PHP function is_dir returns true on files that have no extension.
The file type function will tell you correctly what the file is */
Copy link
Member

Choose a reason for hiding this comment

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

This is wrong (at least in modern PHP versions). is_dir() always behave correctly, even when a file has no extension.

@fabpot
Copy link
Member
fabpot commented Jun 3, 2014

Closing this PR as I've talked with the GAE guys and they are going to implement tempnam.

@fabpot fabpot closed this Jun 3, 2014
@cordoval
Copy link
Contributor Author
cordoval commented Jun 3, 2014

oh nice to know this is good news!

@cordoval cordoval deleted the feature/tempnam-remove branch June 3, 2014 21:30
@jakzal jakzal mentioned this pull request Jun 10, 2014
Closed
@patrickmatsumura
Copy link

Any news on this? tempnam is still listed under "permanently disabled functions".

@sjlangley
Copy link

Will be available in App Engine release 1.9.18.

@fabpot
Copy link
Member
fabpot commented Jan 12, 2015

@sjlangley Thanks for the head up, that's a great news. Combined with the work we've done to make Symfony work on other Cloud platform like Heroku, it should be much easier to host a Symfony website on Google App Engine.

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
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
10 participants
@cordoval @stof @ihsw @fabpot @patrickmatsumura @sjlangley @romainneutron @inoryy @GromNaN @mvrhov
0