Skip to content

Commit

Permalink
Merge pull request #930 from rpkamp/simplify-locators
Browse files Browse the repository at this point in the history 
[Data Loader] [Data Locator] [Dependency Injection] Pass root paths to FileSystemLocator during construction
  • Loading branch information
@robfrawley
robfrawley committed Mar 2, 2018
2 parents 3021811 + 485ea32 commit 13cdab6
Show file tree
Hide file tree
Showing 11 changed files with 102 additions and 92 deletions.
5 changes: 1 addition & 4 deletions Binary/Loader/FileSystemLoader.php
View file
Expand Up @@ -37,18 +37,15 @@ class FileSystemLoader implements LoaderInterface
* @param MimeTypeGuesserInterface $mimeGuesser
* @param ExtensionGuesserInterface $extensionGuesser
* @param LocatorInterface $locator
* @param string[] $rootPaths
*/
public function __construct(
MimeTypeGuesserInterface $mimeGuesser,
ExtensionGuesserInterface $extensionGuesser,
LocatorInterface $locator,
array $rootPaths = []
LocatorInterface $locator
) {
$this->mimeTypeGuesser = $mimeGuesser;
$this->extensionGuesser = $extensionGuesser;
$this->locator = $locator;
$this->locator->setOptions(['roots' => $rootPaths]);
}

/**
Expand Down
15 changes: 8 additions & 7 deletions Binary/Locator/FileSystemInsecureLocator.php
View file
Expand Up @@ -17,16 +17,17 @@ class FileSystemInsecureLocator extends FileSystemLocator
* @param string $root
* @param string $path
*
* @return string|false
* @return string|null
*/
protected function generateAbsolutePath($root, $path)
protected function generateAbsolutePath(string $root, string $path): ?string
{
if (false !== mb_strpos($path, '..'.DIRECTORY_SEPARATOR) ||
false !== mb_strpos($path, DIRECTORY_SEPARATOR.'..') ||
false === file_exists($absolute = $root.DIRECTORY_SEPARATOR.$path)) {
return false;
if (false === mb_strpos($path, '..'.DIRECTORY_SEPARATOR) &&
false === mb_strpos($path, DIRECTORY_SEPARATOR.'..') &&
false !== file_exists($absolute = $root.DIRECTORY_SEPARATOR.$path)
) {
return $absolute;
}

return $absolute;
return null;
}
}
71 changes: 33 additions & 38 deletions Binary/Locator/FileSystemLocator.php
View file
Expand Up @@ -13,8 +13,6 @@

use Liip\ImagineBundle\Exception\Binary\Loader\NotLoadableException;
use Liip\ImagineBundle\Exception\InvalidArgumentException;
use Symfony\Component\OptionsResolver\Exception\ExceptionInterface;
use Symfony\Component\OptionsResolver\OptionsResolver;

class FileSystemLocator implements LocatorInterface
{
Expand All @@ -24,20 +22,13 @@ class FileSystemLocator implements LocatorInterface
private $roots = [];

/**
* @param array[] $options
* @param string[] $roots
*/
public function setOptions(array $options = [])
public function __construct(array $roots = [])
{
$resolver = new OptionsResolver();
$resolver->setDefaults(['roots' => []]);

try {
$options = $resolver->resolve($options);
} catch (ExceptionInterface $e) {
throw new InvalidArgumentException(sprintf('Invalid options provided to %s()', __METHOD__), null, $e);
}

$this->roots = array_map([$this, 'sanitizeRootPath'], (array) $options['roots']);
$this->roots = array_map(function (string $root): string {
return $this->sanitizeRootPath($root);
}, $roots);
}

/**
Expand All @@ -47,13 +38,13 @@ public function setOptions(array $options = [])
*
* @return string
*/
public function locate($path)
public function locate(string $path): string
{
if (false !== $absolute = $this->locateUsingRootPlaceholder($path)) {
if (null !== $absolute = $this->locateUsingRootPlaceholder($path)) {
return $this->sanitizeAbsolutePath($absolute);
}

if (false !== $absolute = $this->locateUsingRootPathsSearch($path)) {
if (null !== $absolute = $this->locateUsingRootPathsSearch($path)) {
return $this->sanitizeAbsolutePath($absolute);
}

Expand All @@ -65,62 +56,66 @@ public function locate($path)
* @param string $root
* @param string $path
*
* @return string|false
* @return string|null
*/
protected function generateAbsolutePath($root, $path)
protected function generateAbsolutePath(string $root, string $path): ?string
{
return realpath($root.DIRECTORY_SEPARATOR.$path);
if (false !== $absolute = realpath($root.DIRECTORY_SEPARATOR.$path)) {
return $absolute;
}

return null;
}

/**
* @param string $path
*
* @return bool|string
* @return string|null
*/
private function locateUsingRootPathsSearch($path)
private function locateUsingRootPathsSearch(string $path): ?string
{
foreach ($this->roots as $root) {
if (false !== $absolute = $this->generateAbsolutePath($root, $path)) {
if (null !== $absolute = $this->generateAbsolutePath($root, $path)) {
return $absolute;
}
}

return false;
return null;
}

/**
* @param string $path
*
* @return bool|string
* @return string|null
*/
private function locateUsingRootPlaceholder($path)
private function locateUsingRootPlaceholder(string $path): ?string
{
if (0 !== mb_strpos($path, '@') || 1 !== preg_match('{@(?<name>[^:]+):(?<path>.+)}', $path, $matches)) {
return false;
if (0 !== mb_strpos($path, '@') || 1 !== preg_match('{^@(?<name>[^:]+):(?<path>.+)$}', $path, $match)) {
return null;
}

if (isset($this->roots[$matches['name']])) {
return $this->generateAbsolutePath($this->roots[$matches['name']], $matches['path']);
if (isset($this->roots[$match['name']])) {
return $this->generateAbsolutePath($this->roots[$match['name']], $match['path']);
}

throw new NotLoadableException(sprintf('Invalid root placeholder "%s" for path "%s"',
$matches['name'], $matches['path']));
throw new NotLoadableException(sprintf('Invalid root placeholder "@%s" for path "%s"',
$match['name'], $match['path']));
}

/**
* @param string $root
* @param string $path
*
* @throws InvalidArgumentException
*
* @return string
*/
private function sanitizeRootPath($root)
private function sanitizeRootPath(string $path): string
{
if (!empty($root) && false !== $realRoot = realpath($root)) {
return $realRoot;
if (!empty($path) && false !== $real = realpath($path)) {
return $real;
}

throw new InvalidArgumentException(sprintf('Root image path not resolvable "%s"', $root));
throw new InvalidArgumentException(sprintf('Root image path not resolvable "%s"', $path));
}

/**
Expand All @@ -130,7 +125,7 @@ private function sanitizeRootPath($root)
*
* @return string
*/
private function sanitizeAbsolutePath($path)
private function sanitizeAbsolutePath(string $path): string
{
foreach ($this->roots as $root) {
if (0 === mb_strpos($path, $root)) {
Expand Down
7 changes: 1 addition & 6 deletions Binary/Locator/LocatorInterface.php
View file
Expand Up @@ -13,15 +13,10 @@

interface LocatorInterface
{
/**
* @param array $options[]
*/
public function setOptions(array $options = []);

/**
* @param string $path
*
* @return string
*/
public function locate($path);
public function locate(string $path): string;
}
8 changes: 5 additions & 3 deletions DependencyInjection/Factory/Loader/FileSystemLoaderFactory.php
View file
Expand Up @@ -14,8 +14,8 @@
use Liip\ImagineBundle\Exception\InvalidArgumentException;
use Liip\ImagineBundle\Utility\Framework\SymfonyFramework;
use Symfony\Component\Config\Definition\Builder\ArrayNodeDefinition;
use Symfony\Component\DependencyInjection\ChildDefinition;
use Symfony\Component\DependencyInjection\ContainerBuilder;
use Symfony\Component\DependencyInjection\Reference;

class FileSystemLoaderFactory extends AbstractLoaderFactory
{
Expand All @@ -24,9 +24,11 @@ class FileSystemLoaderFactory extends AbstractLoaderFactory
*/
public function create(ContainerBuilder $container, $loaderName, array $config)
{
$locatorDefinition = new ChildDefinition(sprintf('liip_imagine.binary.locator.%s', $config['locator']));
$locatorDefinition->replaceArgument(0, $this->resolveDataRoots($config['data_root'], $config['bundle_resources'], $container));

$definition = $this->getChildLoaderDefinition();
$definition->replaceArgument(2, new Reference(sprintf('liip_imagine.binary.locator.%s', $config['locator'])));
$definition->replaceArgument(3, $this->resolveDataRoots($config['data_root'], $config['bundle_resources'], $container));
$definition->replaceArgument(2, $locatorDefinition);

return $this->setTaggedLoaderDefinition($loaderName, $definition, $container);
}
Expand Down
3 changes: 2 additions & 1 deletion Resources/config/imagine.xml
View file
Expand Up @@ -176,7 +176,6 @@
<argument type="service" id="liip_imagine.mime_type_guesser" />
<argument type="service" id="liip_imagine.extension_guesser" />
<argument><!-- will be injected by FileSystemLoaderFactory --></argument>
<argument><!-- will be injected by FileSystemLoaderFactory --></argument>
</service>

<service id="liip_imagine.binary.loader.prototype.stream" class="Liip\ImagineBundle\Binary\Loader\StreamLoader">
Expand All @@ -192,10 +191,12 @@
<!-- Data loader locators -->

<service id="liip_imagine.binary.locator.filesystem" class="Liip\ImagineBundle\Binary\Locator\FileSystemLocator" public="false" shared="false">
<argument><!-- will be injected by FilesystemLoaderFactory --></argument>
<tag name="liip_imagine.binary.locator" shared="false" />
</service>

<service id="liip_imagine.binary.locator.filesystem_insecure" class="Liip\ImagineBundle\Binary\Locator\FileSystemInsecureLocator" public="false" shared="false">
<argument><!-- will be injected by FilesystemLoaderFactory --></argument>
<tag name="liip_imagine.binary.locator" shared="false" />
</service>

Expand Down
23 changes: 12 additions & 11 deletions Tests/Binary/Loader/FileSystemLoaderTest.php
View file
Expand Up @@ -84,7 +84,7 @@ public function testLoad($root, $path)
}

/**
* @return array[]
* @return string[][]
*/
public static function provideMultipleRootLoadCases()
{
Expand All @@ -102,12 +102,12 @@ public static function provideMultipleRootLoadCases()
/**
* @dataProvider provideMultipleRootLoadCases
*
* @param string $root
* @param string $path
* @param string[] $roots
* @param string $path
*/
public function testMultipleRootLoadCases($root, $path)
public function testMultipleRootLoadCases($roots, $path)
{
$this->assertValidLoaderFindReturn($this->getFileSystemLoader($root)->find($path));
$this->assertValidLoaderFindReturn($this->getFileSystemLoader($roots)->find($path));
}

public function testAllowsEmptyRootPath()
Expand Down Expand Up @@ -169,11 +169,13 @@ public function testThrowsIfFileDoesNotExist()
}

/**
* @param string[] $roots
*
* @return FileSystemLocator
*/
private function getFileSystemLocator()
private function getFileSystemLocator(array $roots)
{
return new FileSystemLocator();
return new FileSystemLocator($roots);
}

/**
Expand All @@ -185,18 +187,17 @@ private function getDefaultDataRoots()
}

/**
* @param string|array|null $root
* @param array $roots
* @param LocatorInterface|null $locator
*
* @return FileSystemLoader
*/
private function getFileSystemLoader($root = null, LocatorInterface $locator = null)
private function getFileSystemLoader(array $roots = [], LocatorInterface $locator = null)
{
return new FileSystemLoader(
MimeTypeGuesser::getInstance(),
ExtensionGuesser::getInstance(),
null !== $locator ? $locator : $this->getFileSystemLocator(),
null !== $root ? $root : $this->getDefaultDataRoots()
null !== $locator ? $locator : $this->getFileSystemLocator(count($roots) ? $roots : $this->getDefaultDataRoots())
);
}

Expand Down
10 changes: 1 addition & 9 deletions Tests/Binary/Locator/AbstractFileSystemLocatorTest.php
View file
Expand Up @@ -46,18 +46,10 @@ public function testThrowsIfFileDoesNotExist()
$this->getFileSystemLocator(__DIR__)->locate('fileNotExist');
}

public function testThrowsIfInvalidOptionProvided()
{
$this->expectException(\Liip\ImagineBundle\Exception\InvalidArgumentException::class);
$this->expectExceptionMessage('Invalid options provided to');

$this->getFileSystemLocator(__DIR__)->setOptions(['foo' => 'bar']);
}

public function testThrowsIfRootPlaceholderInvalid()
{
$this->expectException(\Liip\ImagineBundle\Exception\Binary\Loader\NotLoadableException::class);
$this->expectExceptionMessage('Invalid root placeholder "invalid-placeholder" for path');
$this->expectExceptionMessage('Invalid root placeholder "@invalid-placeholder" for path');

$this->getFileSystemLocator(__DIR__)->locate('@invalid-placeholder:file.ext');
}
Expand Down
5 changes: 1 addition & 4 deletions Tests/Binary/Locator/FileSystemInsecureLocatorTest.php
View file
Expand Up @@ -86,9 +86,6 @@ public static function provideMultipleRootLoadCases()
*/
protected function getFileSystemLocator($paths)
{
$locator = new FileSystemInsecureLocator();
$locator->setOptions(['roots' => (array) $paths]);

return $locator;
return new FileSystemInsecureLocator((array) $paths);
}
}
5 changes: 1 addition & 4 deletions Tests/Binary/Locator/FileSystemLocatorTest.php
View file
Expand Up @@ -81,9 +81,6 @@ public static function provideMultipleRootLoadCases()
*/
protected function getFileSystemLocator($paths)
{
$locator = new FileSystemLocator();
$locator->setOptions(['roots' => (array) $paths]);

return $locator;
return new FileSystemLocator((array) $paths);
}
}

0 comments on commit 13cdab6

Please sign in to comment.