8000 Class existence resource by fabpot · Pull Request #20121 · symfony/symfony · GitHub
[go: up one dir, main page]

Skip to content

Class existence resource #20121

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 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 2 commits into from
Oct 5, 2016
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
Original file line number Diff line number Diff line change
Expand Up @@ -11,9 +11,13 @@

namespace Symfony\Bundle\TwigBundle\DependencyInjection\Compiler;

use Symfony\Component\Config\Resource\ClassExistenceResource;
use Symfony\Component\DependencyInjection\Compiler\CompilerPassInterface;
use Symfony\Component\DependencyInjection\ContainerBuilder;
use Symfony\Component\DependencyInjection\Reference;
use Symfony\Component\ExpressionLanguage\ExpressionLanguage;
use Symfony\Component\Stopwatch\Stopwatch;
use Symfony\Component\Yaml\Parser as YamlParser;

/**
* @author Jean-François Simon <jeanfrancois.simon@sensiolabs.com>
Expand Down Expand Up @@ -72,15 +76,18 @@ public function process(ContainerBuilder $container)
$container->getDefinition('twig.extension.assets')->addTag('twig.extension');
}

if (class_exists('Symfony\Component\Yaml\Parser')) {
$container->addResource(new ClassExistenceResource(YamlParser::class));
if (class_exists(YamlParser::class)) {
Copy link
Member
@javiereguiluz javiereguiluz Oct 2, 2016

Choose a reason for hiding this comment

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

Related to this comment, just asking if we could replace this:

if (class_exists(YamlParser::class)) {
    $container->getDefinition('twig.extension.yaml')->addTag('twig.extension');
}

by this?

if (!class_exists(YamlParser::class)) {
    $container->removeDefinition('twig.extension.yaml');
}

Copy link
Member

Choose a reason for hiding this comment

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

The difference is that if one uses the twig.extension.yaml directly, the proposed version will break.
I think the current approach is a more open 10000 thus a better practice, even if in this very case using the extension directly is so unlikely.

$container->getDefinition('twig.extension.yaml')->addTag('twig.extension');
}

if (class_exists('Symfony\Component\Stopwatch\Stopwatch')) {
$container->addResource(new ClassExistenceResource(Stopwatch::class));
if (class_exists(Stopwatch::class)) {
$container->getDefinition('twig.extension.debug.stopwatch')->addTag('twig.extension');
}

if (class_exists('Symfony\Component\ExpressionLanguage\ExpressionLanguage')) {
$container->addResource(new ClassExistenceResource(ExpressionLanguage::class));
if (class_exists(ExpressionLanguage::class)) {
$container->getDefinition('twig.extension.expression')->addTag('twig.extension');
}
}
Expand Down
2 changes: 1 addition & 1 deletion src/Symfony/Bundle/TwigBundle/composer.json
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@
"symfony/stopwatch": "~2.8|~3.0",
"symfony/dependency-injection": "~2.8|~3.0",
"symfony/expression-language": "~2.8|~3.0",
"symfony/config": "~2.8|~3.0",
"symfony/config": "~3.2",
"symfony/finder": "~2.8|~3.0",
"symfony/routing": "~2.8|~3.0",
"symfony/templating": "~2.8|~3.0",
Expand Down
75 changes: 75 additions & 0 deletions src/Symfony/Component/Config/Resource/ClassExistenceResource.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,75 @@
<?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\Resource;

/**
* ClassExistenceResource represents a class existence.
* Freshness is only evaluated against resource existence.
*
* The resource must be a fully-qualified class name.
*
* @author Fabien Potencier <fabien@symfony.com>
*/
class ClassExistenceResource implements SelfCheckingResourceInterface, \Serializable
{
private $resource;
private $exists;

/**
* @param string $resource The fully-qualified class name
*/
public function __construct($resource)
{
$this->resource = $resource;
$this->exists = class_exists($resource);
Copy link
Member

Choose a reason for hiding this comment

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

we should use class_exists() || interface_exists() || trait_exists() to make it usable for interfaces and traits too

}

/**
* {@inheritdoc}
*/
public function __toString()
{
return $this->resource;
}

/**
* @return string The file path to the resource
*/
public function getResource()
{
return $this->resource;
}

/**
* {@inheritdoc}
*/
public function isFresh($timestamp)
{
return class_exists($this->resource) === $this->exists;
}

/**
* {@inheritdoc}
*/
public function serialize()
{
return serialize(array($this->resource, $this->exists));
}

/**
* {@inheritdoc}
*/
public function unserialize($serialized)
{
list($this->resource, $this->exists) = unserialize($serialized);
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,54 @@
<?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\Tests\Resource;

use Symfony\Component\Config\Resource\ClassExistenceResource;

class ClassExistenceResourceTest extends \PHPUnit_Framework_TestCase
{
public function testToString()
{
$res = new ClassExistenceResource('BarClass');
$this->assertSame('BarClass', (string) $res);
}

public function testGetResource()
{
$res = new ClassExistenceResource('BarClass');
$this->assertSame('BarClass', $res->getResource());
}

public function testIsFreshWhenClassDoesNotExist()
{
$res = new ClassExistenceResource('Symfony\Component\Config\Tests\Fixtures\BarClass');

$this->assertTrue($res->isFresh(time()));

eval(<<<EOF
namespace Symfony\Component\Config\Tests\Fixtures;

class BarClass
{
}
EOF
);

$this->assertFalse($res->isFresh(time()));
Copy link
Member

Choose a reason for hiding this comment

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

Afaik, this should be assertTrue. In table form (horizontal: current state, vertical: stored state)

Class exists Class does not exists
Class exists Not fresh Fresh
Class does not exists Fresh Not fresh

Copy link
Member Author

Choose a reason for hiding this comment

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

The freshness is all about the state change. The resource is fresh if there is no change in class existence, so no need to refresh the cache. Not fresh means that the cache must be recalculated, so when the state changes. So, the table should be:

Class exists Class does not exists
Class exists Fresh Not fresh
Class does not exists Not fresh fresh

Copy link
Member

Choose a reason for hiding this comment

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

Oh, sorry. Completely messed up the definition of "fresh"...

}

public function testIsFreshWhenClassExists()
{
$res = new ClassExistenceResource('Symfony\Component\Config\Tests\Resource\ClassExistenceResourceTest');

$this->assertTrue($res->isFresh(time()));
}
}
0