[5.4] Use str_random() for generating file names #16193
Conversation
|
Looks like you're missing a namespace import? |
|
Fixed, sorry. Was submitting to 5.3 first and changed branch looks like I dropped this import. |
| @@ -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(); | |||
There was a problem hiding this comment.
@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
There was a problem hiding this comment.
It's fine with me to strip dashes.
There was a problem hiding this comment.
or just use str_random(32)
There was a problem hiding this comment.
Or simply:
bin2hex(random_bytes(16))(as a bonus: real 128 bits entropy, instead of 122 with UUID4 because of reserved bits.)
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
|
Isn't |
|
And I have one note about UUIDv4 uniqueness. I had an experience couple of years ago with this package in high load advertisement tracking system. All clicks were marked with UUIDv4, after half of a year I found that there were ~3% of ID duplications. After that I'm always trying to use UUIDv5 when I want to make really unique value, but it require to generate namespace UUID first. |
| @@ -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(); | |||
There was a problem hiding this comment.
or just use str_random(32)
I thought about this, but maybe it's not a good idea because of inconsistencies on case-sensitive vs. case-insensitive filesystems. |
Just lowercase the output of the function? |
|
The result would be unevenly distributed (bias towards letters), still it would have more entropy than my above solution, so it might be better, technically. Just as a reminder, these kinds of discussions are endless ;) |
|
Just use str_random, and we're done. No need to f**k about with uuids. |
|
Graham does make a good point 😄 |
themsaid
changed the title
|
I had an example in mind for the lowercase need. If you do path splitting as lucasmichot said above: If you share some data between windows and linux servers, you're very likely to encounter issues. I think we should really apply a |
|
Just write your own file storing procedure if you need something different, it's not hard. This methods exists really only for convenience. |
|
Refs the subsequent 5811bc3. 😄 |
|
Documentation needs to be updated: https://laravel.com/docs/master/filesystem#file-uploads. |
In reference to laravel/ideas#161