Skip to content
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

[5.4] Use str_random() for generating file names #16193

Merged
merged 6 commits into from
Nov 2, 2016
Merged

[5.4] Use str_random() for generating file names #16193

Changes from 3 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
2d301e3
Use UUID for generating file names
themsaid Oct 31, 2016
cd1e2ce
import Uuid
themsaid Oct 31, 2016
d4a45de
update docblock
themsaid Oct 31, 2016
be8d323
strip dashes
themsaid Nov 1, 2016
558fc34
use str_random
themsaid Nov 2, 2016
5f55fec
remove the strip dash
themsaid Nov 2, 2016
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
6 changes: 4 additions & 2 deletions src/Illuminate/Http/FileHelpers.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,8 @@

namespace Illuminate\Http;

use Ramsey\Uuid\Uuid;

trait FileHelpers
{
/**
Expand Down Expand Up @@ -35,7 +37,7 @@ public function clientExtension()
}

/**
* Get a filename for the file that is the MD5 hash of the contents.
* Get a filename for the file.
*
* @param string $path
* @return string
Expand All @@ -46,6 +48,6 @@ public function hashName($path = null)
$path = rtrim($path, '/').'/';
}

return $path.md5_file($this->getRealPath()).'.'.$this->guessExtension();
return $path.Uuid::uuid4()->toString().'.'.$this->guessExtension();
Copy link
Contributor

@lucasmichot lucasmichot Oct 31, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

@themsaid this is a great new addition.
Nevertheless md5 is great as it only contains hexadecimals, and allows to structure the filename splitting parts of its name:

0123456789abcdef.jpg => 01/23/45/67/89/ab/cd/ef.jpg

But UUID4 also contains -.

I believe this character could be removed for readability purpose?

str_replace('-', '',  Uuid::uuid4()->toString())...

Also I think the docblock can be updated

Copy link
Member

Choose a reason for hiding this comment

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

It's fine with me to strip dashes.

Copy link
Member

Choose a reason for hiding this comment

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

or just use str_random(32)

Copy link
Contributor

@vlakoff vlakoff Oct 31, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

Or simply:

bin2hex(random_bytes(16))

(as a bonus: real 128 bits entropy, instead of 122 with UUID4 because of reserved bits.)

Copy link
Contributor

Choose a reason for hiding this comment

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

I think a UUID is more likely to be unique than random_bytes, right? UUID's strength is uniqueness, random_bytes's strength is cryptographically secure randomness.

Copy link
Contributor

@vlakoff vlakoff Nov 1, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

ramsey/uuid just uses random_bytes() too, see RandomBytesGenerator.

In fact my code above is just the removal of a bunch of classes, factories, generators, etc. It is an equivalent code without all the winter layers.

}
}