Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly
Appearance settings

added support for glob loaders in Config #21635

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.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Feb 18, 2017
Merged

added support for glob loaders in Config #21635

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions 5 src/Symfony/Bundle/FrameworkBundle/Resources/config/routing.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,11 @@
<argument type="service" id="file_locator" />
</service>

<service id="routing.loader.glob" class="Symfony\Component\Config\Loader\GlobFileLoader" public="false">
<tag name="routing.loader" />
<argument type="service" id="file_locator" />
</service>

<service id="routing.loader.directory" class="Symfony\Component\Routing\Loader\DirectoryLoader" public="false">
<tag name="routing.loader" />
<argument type="service" id="file_locator" />
Expand Down
86 changes: 85 additions & 1 deletion 86 src/Symfony/Component/Config/Loader/FileLoader.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,9 @@
use Symfony\Component\Config\FileLocatorInterface;
use Symfony\Component\Config\Exception\FileLoaderLoadException;
use Symfony\Component\Config\Exception\FileLoaderImportCircularReferenceException;
use Symfony\Component\Config\Exception\FileLocatorFileNotFoundException;
use Symfony\Component\Finder\Finder;
use Symfony\Component\Finder\Glob;

/**
* FileLoader is the abstract class used by all built-in loaders that are file based.
Expand All @@ -32,7 +35,7 @@ abstract class FileLoader extends Loader
*/
protected $locator;

protected $currentDir;
private $currentDir;

/**
* Constructor.
Expand Down Expand Up @@ -78,6 +81,87 @@ public function getLocator()
* @throws FileLoaderImportCircularReferenceException
*/
public function import($resource, $type = null, $ignoreErrors = false, $sourceResource = null)
{
$ret = array();
$ct = 0;
foreach ($this->glob($resource, false, $_, $ignoreErrors) as $resource => $info) {
++$ct;
$ret[] = $this->doImport($resource, $type, $ignoreErrors, $sourceResource);
}

return $ct > 1 ? $ret : isset($ret[0]) ? $ret[0] : null;
Copy link
Member

Choose a reason for hiding this comment

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

drop $ct then:
isset($ret[1]) ? $ret : ($ret ? $ret[0] : null) or
$ret ? (isset($ret[1]) ? $ret : $ret[0]) : null ?

Copy link
Member Author

Choose a reason for hiding this comment

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

which would be less readable

}

/**
* @internal
*/
protected function glob($resource, $recursive, &$prefix = null, $ignoreErrors = false)
{
if (strlen($resource) === $i = strcspn($resource, '*?{[')) {
if (!$recursive) {
$prefix = null;

yield $resource => new \SplFileInfo($resource);

return;
}
$prefix = $resource;
$resource = '';
} elseif (0 === $i) {
$prefix = '.';
$resource = '/'.$resource;
} else {
$prefix = dirname(substr($resource, 0, 1 + $i));
$resource = substr($resource, strlen($prefix));
}

try {
$prefix = $this->locator->locate($prefix, $this->currentDir, true);
} catch (FileLocatorFileNotFoundException $e) {
if (!$ignoreErrors) {
throw $e;
}

return;
}
$prefix = realpath($prefix) ?: $prefix;

if (false === strpos($resource, '/**/') && (defined('GLOB_BRACE') || false === strpos($resource, '{'))) {
foreach (glob($prefix.$resource, defined('GLOB_BRACE') ? GLOB_BRACE : 0) as $path) {
if ($recursive && is_dir($path)) {
$flags = \FilesystemIterator::SKIP_DOTS | \FilesystemIterator::FOLLOW_SYMLINKS;
foreach (new \RecursiveIteratorIterator(new \RecursiveDirectoryIterator($path, $flags)) as $path => $info) {
if ($info->isFile()) {
yield $path => $info;
}
}
} elseif (is_file($path)) {
yield $path => new \SplFileInfo($path);
}
}

return;
}

if (!class_exists(Finder::class)) {
throw new LogicException(sprintf('Extended glob pattern "%s" cannot be used as the Finder component is not installed.', $resource));
}

$finder = new Finder();
$regex = Glob::toRegex($resource);
if ($recursive) {
$regex = substr_replace($regex, '(/|$)', -2, 1);
}

$prefixLen = strlen($prefix);
foreach ($finder->followLinks()->in($prefix) as $path => $info) {
if (preg_match($regex, substr($path, $prefixLen)) && $info->isFile()) {
yield $path => $info;
}
}
}

private function doImport($resource, $type = null, $ignoreErrors = false, $sourceResource = null)
{
try {
$loader = $this->resolve($resource, $type);
Expand Down
36 changes: 36 additions & 0 deletions 36 src/Symfony/Component/Config/Loader/GlobFileLoader.php
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,36 @@
<?php

/*
* This file is part of the Symfony package.
*
* (c) Fabien Potencier <fabien@symfony.com>
*
* For the full copyright and license information, please view the LICENSE
* file that was distributed with this source code.
*/

namespace Symfony\Component\Config\Loader;

/**
* GlobFileLoader loads files from a glob pattern.
*
* @author Fabien Potencier <fabien@symfony.com>
*/
class GlobFileLoader extends FileLoader
{
/**
* {@inheritdoc}
*/
public function load($resource, $type = null)
{
return $this->import($resource, null, true);
}

/**
* {@inheritdoc}
*/
public function supports($resource, $type = null)
{
return 'glob' === $type;
Copy link
Member

Choose a reason for hiding this comment

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

add a check on $resource, like false === strpbrk($resource, '*?{[')?

Copy link
Member Author

Choose a reason for hiding this comment

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

Not needed as we force $type to be glob

Copy link

Choose a reason for hiding this comment

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

@nicolas-grekas could the check be done at some other place, see #22160
Actually just a check if a glob pattern is used but not the correct type (type: glob) is configured to return a meaningful errormessage?

}
}
88 changes: 9 additions & 79 deletions 88 src/Symfony/Component/DependencyInjection/Loader/FileLoader.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,16 +11,12 @@

namespace Symfony\Component\DependencyInjection\Loader;

use Symfony\Component\Config\Exception\FileLocatorFileNotFoundException;
use Symfony\Component\DependencyInjection\ChildDefinition;
use Symfony\Component\DependencyInjection\ContainerBuilder;
use Symfony\Component\DependencyInjection\Definition;
use Symfony\Component\DependencyInjection\Exception\InvalidArgumentException;
use Symfony\Component\DependencyInjection\Exception\LogicException;
use Symfony\Component\Config\Loader\FileLoader as BaseFileLoader;
use Symfony\Component\Config\FileLocatorInterface;
use Symfony\Component\Finder\Finder;
use Symfony\Component\Finder\Glob;

/**
* FileLoader is the abstract class used by all built-in loaders that are file based.
Expand All @@ -44,22 +40,6 @@ public function __construct(ContainerBuilder $container, FileLocatorInterface $l
parent::__construct($locator);
}

/**
* {@inheritdoc}
*/
public function import($resource, $type = null, $ignoreErrors = false, $sourceResource = null)
{
try {
foreach ($this->glob($resource, false) as $path => $info) {
parent::import($path, $type, $ignoreErrors, $sourceResource);
}
} catch (FileLocatorFileNotFoundException $e) {
if (!$ignoreErrors) {
throw $e;
}
}
}

/**
* Registers a set of classes as services using PSR-4 for discovery.
*
Expand Down Expand Up @@ -106,8 +86,12 @@ private function findClasses($namespace, $resource)
{
$classes = array();
$extRegexp = defined('HHVM_VERSION') ? '/\\.(?:php|hh)$/' : '/\\.php$/';
$prefixLen = null;
foreach ($this->glob($resource, true, $prefix) as $path => $info) {
Copy link
Member

Choose a reason for hiding this comment

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

if you call glob outside of the loop, you can compute prefixLen outside of the foreach

Copy link
Member Author

Choose a reason for hiding this comment

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

that does not work

if (null === $prefixLen) {
$prefixLen = strlen($prefix);
}

foreach ($this->glob($resource, true, $prefixLen) as $path => $info) {
if (!preg_match($extRegexp, $path, $m) || !$info->isReadable()) {
continue;
}
Expand All @@ -124,65 +108,11 @@ private function findClasses($namespace, $resource)
}
}

return $classes;
}

private function glob($resource, $recursive, &$prefixLen = null)
{
if (strlen($resource) === $i = strcspn($resource, '*?{[')) {
if (!$recursive) {
yield $resource => new \SplFileInfo($resource);

return;
}
$resourcePrefix = $resource;
$resource = '';
} elseif (0 === $i) {
$resourcePrefix = '.';
$resource = '/'.$resource;
} else {
$resourcePrefix = dirname(substr($resource, 0, 1 + $i));
$resource = substr($resource, strlen($resourcePrefix));
}

$resourcePrefix = $this->locator->locate($resourcePrefix, $this->currentDir, true);
$resourcePrefix = realpath($resourcePrefix) ?: $resourcePrefix;
$prefixLen = strlen($resourcePrefix);

// track directories only for new & removed files
$this->container->fileExists($resourcePrefix, '/^$/');

if (false === strpos($resource, '/**/') && (defined('GLOB_BRACE') || false === strpos($resource, '{'))) {
foreach (glob($resourcePrefix.$resource, defined('GLOB_BRACE') ? GLOB_BRACE : 0) as $path) {
if ($recursive && is_dir($path)) {
$flags = \FilesystemIterator::SKIP_DOTS | \FilesystemIterator::FOLLOW_SYMLINKS;
foreach (new \RecursiveIteratorIterator(new \RecursiveDirectoryIterator($path, $flags)) as $path => $info) {
if ($info->isFile()) {
yield $path => $info;
}
}
} elseif (is_file($path)) {
yield $path => new \SplFileInfo($path);
}
}

return;
if (null !== $prefix) {
// track directories only for new & removed files
$this->container->fileExists($prefix, '/^$/');
}

if (!class_exists(Finder::class)) {
throw new LogicException(sprintf('Extended glob pattern "%s" cannot be used as the Finder component is not installed.', $resource));
}

$finder = new Finder();
$regex = Glob::toRegex($resource);
if ($recursive) {
$regex = substr_replace($regex, '(/|$)', -2, 1);
}

foreach ($finder->followLinks()->in($resourcePrefix) as $path => $info) {
if (preg_match($regex, substr($path, $prefixLen)) && $info->isFile()) {
yield $path => $info;
}
}
return $classes;
}
}
2 changes: 2 additions & 0 deletions 2 src/Symfony/Component/HttpKernel/Kernel.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@
use Symfony\Component\HttpKernel\Config\FileLocator;
use Symfony\Component\HttpKernel\DependencyInjection\MergeExtensionConfigurationPass;
use Symfony\Component\HttpKernel\DependencyInjection\AddClassesToCachePass;
use Symfony\Component\Config\Loader\GlobFileLoader;
use Symfony\Component\Config\Loader\LoaderResolver;
use Symfony\Component\Config\Loader\DelegatingLoader;
use Symfony\Component\Config\ConfigCache;
Expand Down Expand Up @@ -686,6 +687,7 @@ protected function getContainerLoader(ContainerInterface $container)
new YamlFileLoader($container, $locator),
new IniFileLoader($container, $locator),
new PhpFileLoader($container, $locator),
new GlobFileLoader($locator),
new DirectoryLoader($container, $locator),
new ClosureLoader($container),
));
Expand Down
39 changes: 25 additions & 14 deletions 39 src/Symfony/Component/Routing/RouteCollectionBuilder.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -61,21 +61,28 @@ public function __construct(LoaderInterface $loader = null)
*/
public function import($resource, $prefix = '/', $type = null)
Copy link
Member

Choose a reason for hiding this comment

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

in case of a glob, this is missing tracking of the directory prefixing the pattern - see "$this->container->fileExists($resourcePrefix, '/^$/');" in DI.
Should we do it inline here? Would be like a leak but do we have a better choice?

Copy link
Member Author

Choose a reason for hiding this comment

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

Not about the "leak", as routing resources are tracked in a different file, so we cannot reused the same logic.

Copy link
Member Author

Choose a reason for hiding this comment

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

By the way, we have the same issue with the container.

Copy link
Member Author

Choose a reason for hiding this comment

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

I think there is nothing to fix here. Calling import does not imply that you were using a glob. However, the previous implementation made this assumption and always added the direction as a resource, even for non-globs, I don't think this is something we want.

{
/** @var RouteCollection $collection */
$collection = $this->load($resource, $type);
/** @var RouteCollection[] $collection */
$collections = $this->load($resource, $type);

// create a builder from the RouteCollection
$builder = $this->createBuilder();
foreach ($collection->all() as $name => $route) {
$builder->addRoute($route, $name);
}

foreach ($collection->getResources() as $resource) {
$builder->addResource($resource);
}
foreach ($collections as $collection) {
if (null === $collection) {
continue;
}

// mount into this builder
$this->mount($prefix, $builder);
foreach ($collection->all() as $name => $route) {
$builder->addRoute($route, $name);
}

foreach ($collection->getResources() as $resource) {
$builder->addResource($resource);
}

// mount into this builder
$this->mount($prefix, $builder);
}

return $builder;
}
Expand Down Expand Up @@ -201,7 +208,7 @@ public function setRequirement($key, $regex)
}

/**
* Sets an opiton that will be added to all embedded routes (unless that
* Sets an option that will be added to all embedded routes (unless that
* option is already set).
*
* @param string $key
Expand Down Expand Up @@ -345,7 +352,7 @@ private function generateRouteName(Route $route)
* @param mixed $resource A resource
* @param string|null $type The resource type or null if unknown
*
* @return RouteCollection
* @return RouteCollection[]
*
* @throws FileLoaderLoadException If no loader is found
*/
Expand All @@ -356,7 +363,9 @@ private function load($resource, $type = null)
}

if ($this->loader->supports($resource, $type)) {
return $this->loader->load($resource, $type);
$collections = $this->loader->load($resource, $type);

return is_array($collections) ? $collections : array($collections);
}

if (null === $resolver = $this->loader->getResolver()) {
Expand All @@ -367,6 +376,8 @@ private function load($resource, $type = null)
throw new FileLoaderLoadException($resource);
}

return $loader->load($resource, $type);
$collections = $loader->load($resource, $type);

return is_array($collections) ? $collections : array($collections);
}
}
Morty Proxy This is a proxified and sanitized view of the page, visit original site.