Skip to content

Commit

Permalink
Avoiding entries in error logs due the search for cwebps in common sy…
Browse files Browse the repository at this point in the history 
…stem paths. Fixes #227
  • Loading branch information
@rosell-dk
rosell-dk committed Oct 3, 2019
1 parent d5fca19 commit dfb2a8a
Show file tree
Hide file tree
Showing 2 changed files with 116 additions and 16 deletions.
34 changes: 33 additions & 1 deletion src/Convert/Converters/BaseTraits/WarningLoggerTrait.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,12 @@ abstract protected function logLn($msg, $style = '');
/** @var string|array|null Previous error handler (stored in order to be able pass warnings on) */
private $previousErrorHandler;

/** @var boolean Suppress ALL warnings? (both from log and from bubbling up) */
private $suppressWarnings;

/** @var int Count number of warnings */
private $warningCounter;

/**
* Handle warnings and notices during conversion by logging them and passing them on.
*
Expand Down Expand Up @@ -59,9 +65,13 @@ public function warningHandler($errno, $errstr, $errfile, $errline)
If it was possible to suppress the warnings with @ without suppressing warnings on systems
with error reporting set to E_NONE, I would do that.
*/

$this->warningCounter++;
if ($this->suppressWarnings) {
return;
}

$errorTypes = [
E_WARNING => "Warning",
E_NOTICE => "Notice",
Expand Down Expand Up @@ -119,6 +129,8 @@ public function warningHandler($errno, $errstr, $errfile, $errline)
*/
protected function activateWarningLogger()
{
$this->suppressWarnings = false;
$this->warningCounter = 0;
$this->previousErrorHandler = set_error_handler(
array($this, "warningHandler"),
E_WARNING | E_USER_WARNING | E_NOTICE | E_USER_NOTICE
Expand All @@ -136,4 +148,24 @@ protected function deactivateWarningLogger()
{
restore_error_handler();
}

protected function disableWarningsTemporarily()
{
$this->suppressWarnings = true;
}

protected function reenableWarnings()
{
$this->suppressWarnings = false;
}

protected function getWarningCount()
{
return $this->warningCounter;
}

protected function resetWarningCount()
{
$this->warningCounter = 0;
}
}
98 changes: 83 additions & 15 deletions src/Convert/Converters/Cwebp.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
namespace WebPConvert\Convert\Converters;

use WebPConvert\Convert\Converters\AbstractConverter;
use WebPConvert\Convert\Converters\BaseTraits\WarningLoggerTrait;
use WebPConvert\Convert\Converters\ConverterTraits\EncodingAutoTrait;
use WebPConvert\Convert\Converters\ConverterTraits\ExecTrait;
use WebPConvert\Convert\Exceptions\ConversionFailed\ConverterNotOperational\SystemRequirementsNotMetException;
Expand Down Expand Up @@ -109,7 +110,11 @@ public function checkOperationality()
$this->checkOperationalityExecTrait();

$options = $this->options;
if (!$options['try-supplied-binary-for-os'] && !$options['try-common-system-paths'] && !$options['try-cwebp']) {
if (!$options['try-supplied-binary-for-os'] &&
!$options['try-common-system-paths'] &&
!$options['try-cwebp'] &&
!$option['try-discovering-cwebp']
) {
throw new ConverterNotOperationalException(
'Configured to neither try pure cwebp command, ' .
'nor look for cweb binaries in common system locations and ' .
Expand Down Expand Up @@ -396,10 +401,78 @@ private function getSuppliedBinaryPathForOS()
}
$result[] = $binaryFile;
}

return $result;
}

/**
* A fileExist function that actually works! (it requires exec(), though).
*
* A warning-free fileExists method that works even when open_basedir is in effect.
* For performance reasons, the file_exists method will be tried first. If that issues a warning,
* we fall back to using exec().
*
*/
private function fileExists($path)
{
// There is a challenges here:
// We want to suppress warnings, but at the same time we want to know that it happened.
// We achieve this by registering an error handler (that is done automatically for all converters
// in WarningLoggerTrait).

// Disable warnings. This does not disable the warning counter (in WarningLoggerTrait).
$this->disableWarningsTemporarily();

// Reset warning count. This way we will be able to determine if a warning occured after
// the file_exists() call (by checking warning count)
$this->resetWarningCount();
$found = @file_exists($path);
$this->reenableWarnings();

if ($found) {
return true;
}

if ($this->getWarningCount() == 0) {
// no warnings were issued. So no open_basedir restriction in effect that messes
// things up. We can safely conclude that the file does not exist.
return false;
}

// The path was not found with file_exists, but on the other hand:
// a warning was issued, which probably means we got a open_basedir restriction in effect warning.
// So the file COULD exist.

// Lets try to find out by executing "ls path/to/cwebp"
exec('ls ' . $path, $output, $returnCode);
if (($returnCode == 0) && (isset($output[0]))) {
return true;
}

// We assume that "ls" command is general available!
// As that failed, we can conclude the file does not exist.
return false;
}


/**
* Discover binaries by looking in common system paths.
*
* We try a small set of common system paths, such as "/usr/bin/cwebp", filtering out
* non-existing paths.
*
* @return array Cwebp binaries found in common system locations
*/
private function discoverCwebpsInCommonSystemPaths()
{
$binaries = [];
foreach (self::$cwebpDefaultPaths as $binary) {
if ($this->fileExists($binary)) {
$binaries[] = $binary;
}
}
return $binaries;
}

/**
* Discover installed binaries using "whereis -b cwebp"
*
Expand Down Expand Up @@ -555,24 +628,19 @@ private function getCwebpVersions()
$versions = $this->detectVersions(['cwebp']);
}
if ($this->options['try-discovering-cwebp']) {
$this->logLn(
'Detecting versions of of cwebp discovered using "whereis cwebp" command'
);
$versions = array_replace_recursive($versions, $this->detectVersions($this->discoverBinaries()));
}
if ($this->options['try-common-system-paths']) {
// Brute-force trying common system paths
// Note:
// We used to do a file_exists($binary) check.
// That was not a good idea because it could trigger open_basedir errors. The open_basedir
// restriction does not operate on the exec command. So note to self: Do not do that again.
// BUT. Could we actually disable those errors? ie:
// WarningLoggerTrait::shutUp = true;
// if (@file_exists(...)) {...}
// WarningLoggerTrait::shutUp = false;

$this->logLn(
'Brute force trying version detecting cwebp binaries in common system paths ' .
'(some may not be found, that is to be expected)'
'Detecting versions of cwebp binaries found in common system paths'
);
$versions = array_replace_recursive(
$versions,
$this->detectVersions($this->discoverCwebpsInCommonSystemPaths())
);
$versions = array_replace_recursive($versions, $this->detectVersions(self::$cwebpDefaultPaths));
}
if ($this->options['try-supplied-binary-for-os']) {
$versions = array_merge_recursive(
Expand Down

0 comments on commit dfb2a8a

Please sign in to comment.