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.

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

Closed
wants to merge 12 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion src/Symfony/Component/ClassLoader/ClassCollectionLoader.php
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@
*/

namespace Symfony\Component\ClassLoader;
use Symfony\Component\Filesystem\Filesystem;
Copy link
Member

Choose a reason for hiding this comment

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

this introduces a dependency to the Filesystem component

Copy link
Contributor Author

Choose a reason for hiding this comment

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

only feasible way, the other is to duplicate the function tempnam2 code into each component, did not think it was the DRY way

Copy link
Member

Choose a reason for hiding this comment

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

well, it could be fine (let's wait to see what @fabpot thinks about it), but it needs to be reflected in the composer.json file


/**
* ClassCollectionLoader.
Expand Down Expand Up @@ -229,7 +230,8 @@ private static function compressCode($code)
*/
private static function writeCacheFile($file, $content)
{
$tmpFile = tempnam(dirname($file), basename($file));
$tmpFile = Filesystem::tempnam2(dirname($file), basename($file));

if (false !== @file_put_contents($tmpFile, $content) && @rename($tmpFile, $file)) {
@chmod($file, 0666 & ~umask());

Expand Down
3 changes: 2 additions & 1 deletion src/Symfony/Component/ClassLoader/composer.json
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,8 @@
"php": ">=5.3.3"
},
"require-dev": {
"symfony/finder": "~2.0"
"symfony/finder": "~2.0",
"symfony/filesystem": "~2.4"
},
"autoload": {
"psr-0": { "Symfony\\Component\\ClassLoader\\": "" }
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@
use Symfony\Component\DependencyInjection\SimpleXMLElement;
use Symfony\Component\DependencyInjection\Exception\InvalidArgumentException;
use Symfony\Component\DependencyInjection\Exception\RuntimeException;
use Symfony\Component\Filesystem\Filesystem;

/**
* XmlFileLoader loads XML files service definitions.
Expand Down Expand Up @@ -309,7 +310,7 @@ public function validateSchema(\DOMDocument $dom)
foreach ($schemaLocations as $namespace => $location) {
$parts = explode('/', $location);
if (0 === stripos($location, 'phar://')) {
$tmpfile = tempnam(sys_get_temp_dir(), 'sf2');
$tmpfile = Filesystem::tempnam2(sys_get_temp_dir(), 'sf2');
if ($tmpfile) {
copy($location, $tmpfile);
$tmpfiles[] = $tmpfile;
Expand Down
3 changes: 2 additions & 1 deletion
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,8 @@
}
],
"require": {
"php": ">=5.3.3"
"php": ">=5.3.3",
"symfony/filesystem": "~2.4"
},
"require-dev": {
"symfony/yaml": "~2.0",
Expand Down
35 changes: 34 additions & 1 deletion src/Symfony/Component/Filesystem/Filesystem.php
Original file line number Diff line number Diff line change
Expand Up @@ -448,7 +448,7 @@ public function dumpFile($filename, $content, $mode = 0666)
throw new IOException(sprintf('Unable to write to the "%s" directory.', $dir), 0, null, $dir);
}

$tmpFile = tempnam($dir, basename($filename));
$tmpFile = self::tempnam2($dir, basename($filename));

if (false === @file_put_contents($tmpFile, $content)) {
throw new IOException(sprintf('Failed to write file "%s".', $filename), 0, null, $filename);
Expand All @@ -471,4 +471,37 @@ private function toIterator($files)

return $files;
}

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

Choose a reason for hiding this comment

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

@GromNaN I tried to look on alternatives http://www.php.net/manual/en/function.function-exists.php#108018

but the problem is GAE does not specify how to fetch functions that are permanently disabled, the logic also could become so complex that is going to get worst. Any further ideas? Worst case scenario we would have a specific class for GAE.

Copy link
Member

Choose a reason for hiding this comment

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

Or we could rely on environment variable.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

that is a good idea! @fabpot ?

{
if (!getenv('DISABLE_FUNCTIONS')) {
return tempnam($dir, $prefix);
}

if ($dir[strlen($dir) - 1] == '/') {
Copy link
Member

Choose a reason for hiding this comment

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

You can do $dir = realpath($dir);. That will remove the trailing slash and avoid repeating the computation several times.

Copy link

Choose a reason for hiding this comment

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

realpath olnly works on filesystem. see https://bugs.php.net/bug.php?id=52769

$trailing_slash = "";
} else {
$trailing_slash = "/";
}
Copy link
Member

Choose a reason for hiding this comment

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

This ugly as hell. Why not just use $dir = rtrim($dir, '/')?


/* 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.

if (!is_dir(realpath($dir)) || filetype(realpath($dir)) != "dir") {
// The specified dir is not actually a dir
return false;
}
if (!is_writable($dir)) {
// The directory will not let us create a file there
return false;
}

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

$filename = $dir.$trailing_slash.$prefix.$seed.$postfix;
} while (file_exists($filename));
touch($filename);

return $filename;
}
}
3 changes: 2 additions & 1 deletion src/Symfony/Component/HttpKernel/CacheWarmer/CacheWarmer.php
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@
*/

namespace Symfony\Component\HttpKernel\CacheWarmer;
use Symfony\Component\Filesystem\Filesystem;

/**
* Abstract cache warmer that knows how to write a file to the cache.
Expand All @@ -20,7 +21,7 @@ abstract class CacheWarmer implements CacheWarmerInterface
{
protected function writeCacheFile($file, $content)
{
$tmpFile = tempnam(dirname($file), basename($file));
$tmpFile = Filesystem::tempnam2(dirname($file), basename($file));
if (false !== @file_put_contents($tmpFile, $content) && @rename($tmpFile, $file)) {
@chmod($file, 0666 & ~umask());

Expand Down
3 changes: 2 additions & 1 deletion src/Symfony/Component/HttpKernel/HttpCache/Store.php
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@

namespace Symfony\Component\HttpKernel\HttpCache;

use Symfony\Component\Filesystem\Filesystem;
use Symfony\Component\HttpFoundation\Request;
use Symfony\Component\HttpFoundation\Response;

Expand Down Expand Up @@ -343,7 +344,7 @@ private function save($key, $data)
return false;
}

$tmpFile = tempnam(dirname($path), basename($path));
$tmpFile = Filesystem::tempnam2(dirname($path), basename($path));
if (false === $fp = @fopen($tmpFile, 'wb')) {
return false;
}
Expand Down
3 changes: 2 additions & 1 deletion src/Symfony/Component/HttpKernel/composer.json
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,8 @@
"symfony/process": "~2.0",
"symfony/routing": "~2.2",
"symfony/stopwatch": "~2.2",
"symfony/templating": "~2.2"
"symfony/templating": "~2.2",
"symfony/filesystem": "~2.4"
},
"suggest": {
"symfony/browser-kit": "",
Expand Down
3 changes: 2 additions & 1 deletion src/Symfony/Component/Translation/Loader/XliffFileLoader.php
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@

namespace Symfony\Component\Translation\Loader;

use Symfony\Component\Filesystem\Filesystem;
use Symfony\Component\Translation\MessageCatalogue;
use Symfony\Component\Translation\Exception\InvalidResourceException;
use Symfony\Component\Translation\Exception\NotFoundResourceException;
Expand Down Expand Up @@ -111,7 +112,7 @@ private function parseFile($file)
$location = str_replace('\\', '/', __DIR__).'/schema/dic/xliff-core/xml.xsd';
$parts = explode('/', $location);
if (0 === stripos($location, 'phar://')) {
$tmpfile = tempnam(sys_get_temp_dir(), 'sf2');
$tmpfile = Filesystem::tempnam2(sys_get_temp_dir(), 'sf2');
if ($tmpfile) {
copy($location, $tmpfile);
$parts = explode('/', str_replace('\\', '/', $tmpfile));
Expand Down
3 changes: 2 additions & 1 deletion src/Symfony/Component/Translation/composer.json
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,8 @@
},
"require-dev": {
"symfony/config": "~2.0",
"symfony/yaml": "~2.2"
"symfony/yaml": "~2.2",
"symfony/filesystem": "~2.4"
},
"suggest": {
"symfony/config": "",
Expand Down
0