Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
Don't assume that file binary exists on *nix OS#26886
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
| publicstaticfunctionisSupported() | ||
| { | ||
| return'\\' !==DIRECTORY_SEPARATOR &&function_exists('passthru') &&function_exists('escapeshellarg'); | ||
| return'\\' !==DIRECTORY_SEPARATOR &&function_exists('passthru') &&function_exists('escapeshellarg') &&null !==shell_exec('command -v file'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
since the performance penalty for doing this call is high, the result should be cached
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Done.
825f359 to8b1c680Compare| publicstaticfunctionisSupported() | ||
| { | ||
| return'\\' !==DIRECTORY_SEPARATOR &&function_exists('passthru') &&function_exists('escapeshellarg'); | ||
| return'\\' !==DIRECTORY_SEPARATOR &&function_exists('passthru') &&function_exists('escapeshellarg') &&static::hasFileBinary(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
this must beself:: to access a private API. Otherwise, a child class would break when calling this code (as it would try to callChildClass::hasFileBinary)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I've just tried this out. It'd only break whenChildClass overrideshasFileBinary. But yes, there is no reason for us to usestatic:: here, since it's a private method anyway.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
we could cache the full expression, including the function_exists check (and thus not need any new method btw)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Done and done.
| { | ||
| static$exists; | ||
| returnisset($exists) ?$exists : ($exists =null !==shell_exec('command -v file')); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
this should usepassthru as well to run the command. Otherwise, it requires adding shell_exec to the list of existing functions (as these are functions which are likely to get disabled on some hosting)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Done.
ad148d7 to526f888Compareteohhanhui commentedApr 12, 2018
Status: Needs Review |
| publicstaticfunctionisSupported() | ||
| { | ||
| return'\\' !==DIRECTORY_SEPARATOR &&function_exists('passthru') &&function_exists('escapeshellarg'); | ||
| static$supported; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
If you don't mind, here is an equivalent shorter implementation:
+ static $supported = null;++ if (null !== $supported) {+ return $supported;+ }++ if ('\\' === DIRECTORY_SEPARATOR || !function_exists('passthru') || !function_exists('escapeshellarg')) {+ return $supported = false;+ }++ ob_start();+ passthru('command -v file', $return);+ $binPath = trim(ob_get_clean());++ return $supported = 0 === $return && preg_match('#^/[^\0/]#', $binPath);
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
But not as readable...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Less cyclomatic complexity so easier to understand actually to me...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Cyclomatic complexity might not be a good measure for readability:https://web.eecs.umich.edu/~weimerw/p/weimer-tse2010-readability-preprint.pdf
| $binPath =trim(ob_get_clean()); | ||
| if (!preg_match('#^(?:/[^\0/])+#',$binPath)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
why this specific regex? can the NUL char actually happen?
also, it looks like this could be just'#^/[^\0/]#'
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Yes, it's possible to have a NUL character:
ob_start();passthru('find -maxdepth 1 -print0');$output =trim(ob_get_clean());echoord($output[1]);
gives:
0nicolas-grekas commentedApr 13, 2018 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
But your example doesn't use About the proposal I made, I did it because I found it harder to review when ob_* closing functions are called in two different code paths. I made the proposal to make Symfony better, not to bother you. |
teohhanhui commentedApr 13, 2018
Rather not assume that
Got it. I will make the change. |
teohhanhui commentedApr 13, 2018
Removed the |
| return'\\' !==DIRECTORY_SEPARATOR &&function_exists('passthru') &&function_exists('escapeshellarg'); | ||
| static$supported; | ||
| if (isset($supported)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I would useif (null !== $supported) here instead.
Certain lightweight distributions such as Alpine Linux (popular for smaller Docker images) do not include it by default.
teohhanhui commentedApr 17, 2018
Status: Needs Review |
fabpot commentedApr 17, 2018
Thank you@teohhanhui. |
This PR was merged into the 2.7 branch.Discussion----------Don't assume that file binary exists on *nix OS| Q | A| ------------- | ---| Branch? | 2.7| Bug fix? | yes| New feature? | no| BC breaks? | no| Deprecations? | no| Tests pass? | yes| Fixed tickets | N/A| License | MIT| Doc PR | N/ACertain lightweight distributions such as Alpine Linux (popular for smaller Docker images) do not include it by default.Commits-------e2c1f24 Don't assume that file binary exists on *nix OS
…ialization (nicolas-grekas)This PR was merged into the 2.7 branch.Discussion----------[HttpFoundation] Fix perf issue during MimeTypeGuesser intialization| Q | A| ------------- | ---| Branch? | 2.7| Bug fix? | yes| New feature? | no| BC breaks? | no| Deprecations? | no| Tests pass? | yes| Fixed tickets |#27307| License | MIT| Doc PR | -introduced in#26886Commits-------f8e7a18 [HttpFoundation] Fix perf issue during MimeTypeGuesser intialization
Certain lightweight distributions such as Alpine Linux (popular for smaller Docker images) do not include it by default.