Skip to content

Commit

Permalink
[Process] changed ExecutableFinder to return false instead of throwin…
Browse files Browse the repository at this point in the history
…g an exception when the executable is not found

I've made the change as the executable goal is to find the executable. The fact that it does not find it is
part of the contract and it is not exceptional.
  • Loading branch information
fabpot committed Apr 26, 2011
1 parent f12146d commit e2741ce
Show file tree
Hide file tree
Showing 7 changed files with 23 additions and 60 deletions.
Expand Up @@ -12,7 +12,6 @@
namespace Symfony\Bundle\AsseticBundle\DependencyInjection;

use Symfony\Component\Process\ExecutableFinder;
use Symfony\Component\Process\Exception\ExecutableNotFoundException;
use Symfony\Component\Config\Definition\Builder\TreeBuilder;
use Symfony\Component\Config\Definition\ConfigurationInterface;

Expand Down Expand Up @@ -58,9 +57,9 @@ public function getConfigTreeBuilder()
->booleanNode('use_controller')->defaultValue($this->debug)->end()
->scalarNode('read_from')->defaultValue('%kernel.root_dir%/../web')->end()
->scalarNode('write_to')->defaultValue('%assetic.read_from%')->end()
->scalarNode('java')->defaultValue(function() use ($finder) { try { return $finder->find('java'); } catch(ExecutableNotFoundException $ex) { return null; } })->end()
->scalarNode('node')->defaultValue(function() use ($finder) { try { return $finder->find('node'); } catch(ExecutableNotFoundException $ex) { return null; } })->end()
->scalarNode('sass')->defaultValue(function() use ($finder) { try { return $finder->find('sass'); } catch(ExecutableNotFoundException $ex) { return null; } })->end()
->scalarNode('java')->defaultValue(function() use ($finder) { $java = $finder->find('java'); return $java ? $java : '/usr/bin/java'; })->end()

This comment has been minimized.

Copy link
@stloyd

stloyd Apr 26, 2011

Contributor

Why not use something like (skip unneeded variable assign):

return $finder->find('java') ?: '/usr/bin/java';
->scalarNode('node')->defaultValue(function() use ($finder) { $node = $finder->find('node'); return $node ? $node : '/usr/bin/node'; })->end()
->scalarNode('sass')->defaultValue(function() use ($finder) { $saas = $finder->find('saas'); return $saas ? $saas : '/usr/bin/saas'; })->end()
->end()

// bundles
Expand Down
12 changes: 0 additions & 12 deletions src/Symfony/Component/Process/Exception/Exception.php

This file was deleted.

This file was deleted.

12 changes: 0 additions & 12 deletions src/Symfony/Component/Process/Exception/RuntimeException.php

This file was deleted.

13 changes: 9 additions & 4 deletions src/Symfony/Component/Process/ExecutableFinder.php
Expand Up @@ -11,8 +11,6 @@

namespace Symfony\Component\Process;

use Symfony\Component\Process\Exception\ExecutableNotFoundException;

/**
* Generic executable finder.
*
Expand All @@ -33,6 +31,13 @@ public function addSuffix($suffix)
$this->suffixes[] = $suffix;
}

/**
* Finds an executable by name.
*
* @param string $name The executable name (without the extension)
*
* @return string|false The executable path or false if it cannot be found
*/
public function find($name)
{
$dirs = explode(PATH_SEPARATOR, getenv('PATH') ? getenv('PATH') : getenv('Path'));
Expand All @@ -45,6 +50,6 @@ public function find($name)
}
}

throw new ExecutableNotFoundException($name);
return false;
}
}
}
9 changes: 7 additions & 2 deletions src/Symfony/Component/Process/PhpExecutableFinder.php
Expand Up @@ -28,11 +28,16 @@ public function __construct()
$this->executableFinder = new ExecutableFinder();
}

/**
* Finds The PHP executable.
*
* @return string|false The PHP executable path or false if it cannot be found
*/
public function find()
{
if ($php = getenv('PHP_PATH')) {
if (!is_executable($php)) {
throw new RuntimeException('The defined PHP_PATH environment variable is not a valid PHP executable.');
return false;

This comment has been minimized.

Copy link
@avalanche123

avalanche123 Apr 26, 2011

Contributor

better return null here as in nothing found rather than not found as there is no found state to oppose

}

return $php;
Expand All @@ -53,4 +58,4 @@ public function find()

return $this->executableFinder->find('php');
}
}
}
5 changes: 4 additions & 1 deletion src/Symfony/Component/Process/PhpProcess.php
Expand Up @@ -67,7 +67,10 @@ public function setPhpBinary($php)
public function run($callback = null)
{
if (null === $this->getCommandLine()) {
$this->setCommandLine($this->executableFinder->find());
if (false === $php = $this->executableFinder->find()) {
throw new \RuntimeException('Unable to find the PHP executable.');
}
$this->setCommandLine($php);
}

return parent::run($callback);
Expand Down

4 comments on commit e2741ce

@schmittjoh
Copy link
Contributor

Choose a reason for hiding this comment

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

If you claim that it is not exceptional, could you please show me examples where not finding the executable does not eventually lead to abortion of the normal control flow? I think your changes to the PhpProcess class exemplify quite well, that it is exceptional.

If the problem lies in the naming of the method, then we should rather change "find" to "get" to make the contract stronger, but I cannot come up with any use case where the normal control flow can proceed if the executable does not exist. It's like trying to load a class and if that fails you continue like normal which obviously doesn't work.

@Seldaek
Copy link
Member

Choose a reason for hiding this comment

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

Déjà vu.

I could think of ways where if you don't have an executable you try to resort to some crappy php emulation of whatever you were trying to do with an exec() call.

@schmittjoh
Copy link
Contributor

@schmittjoh schmittjoh commented on e2741ce Apr 26, 2011 via email

Choose a reason for hiding this comment

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

@kriswallsmith
Copy link
Contributor

Choose a reason for hiding this comment

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

A related PR #663.

Please sign in to comment.