8000 [RFC][Ast][Routing] Use Ast to generate the UrlGenerator by GuilhemN · Pull Request #19555 · symfony/symfony · GitHub
[go: up one dir, main page]

Skip to content

[RFC][Ast][Routing] Use Ast to generate the UrlGenerator #19555

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

Closed
wants to merge 5 commits into from
Closed
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension 10000


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Next Next commit
[AstGenerator] Init the Component
  • Loading branch information
joelwurtz authored and GuilhemN committed Aug 6, 2016
commit c2d733af643079625d0a96cfdb89a9919c117139
3 changes: 2 additions & 1 deletion composer.json
Original file line number Diff line number Diff line change
Expand Up @@ -88,7 +88,8 @@
"egulias/email-validator": "~1.2,>=1.2.8|~2.0",
"symfony/polyfill-apcu": "~1.1",
"symfony/security-acl": "~2.8|~3.0",
"phpdocumentor/reflection-docblock": "^3.0"
"phpdocumentor/reflection-docblock": "^3.0",
"nikic/PHP-Parser": "~2.0"
},
"conflict": {
"phpdocumentor/reflection-docblock": "<3.0",
Expand Down
3 changes: 3 additions & 0 deletions src/Symfony/Component/AstGenerator/.gitignore
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
vendor/
composer.lock
phpunit.xml
66 changes: 66 additions & 0 deletions src/Symfony/Component/AstGenerator/AstGeneratorChain.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,66 @@
<?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\AstGenerator;

/**
* Generator delegating the generation to a chain of generators.
*
* @author Joel Wurtz <joel.wurtz@gmail.com>
*/
class AstGeneratorChain implements AstGeneratorInterface
Copy link
Contributor
@unkind unkind Aug 6, 2016

Choose a reason for hiding this comment

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

Why this class is open for inheritance (not marked as final)?
Same question about UrlGeneratorGenerator, PhpMatcherGenerator, UniqueVariableScope.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For this class and UniqueVariableScope i agree it would make sense even if there aren't that much final classes in symfony.
But for the routing generators i would like to allow the user to easily hook into them and it could pass by inheritance.

Copy link
Contributor
@unkind unkind Aug 6, 2016

Choose a reason for hiding this comment

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

But for the routing generators i would like to allow the user to easily hook into them and it could pass by inheritance.

How? Those classes have only private members and 2 public methods. And those 2 public methods can be easily changed with composition if developer really needs it.
This is weird Symfony internal rule: to keep members private by default, but in the same time do not require final: there is no point to open them for inheritance then.

If you find valid reason to extend the classes, you can provide sane extension point(s) instead of inheritance: strategies, events, hooks, etc.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For now they are private but it may change. But you're right the class should be final as long as there are no extensions possible, i'll change ASAP.

Copy link
Contributor

Choose a reason for hiding this comment

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

For now they are private but it may change.

Sure, but instead of changing private properties to protected, you probably can introduce better extension point.

Copy link
Contributor
@ro0NL ro0NL Aug 6, 2016

Choose a reason for hiding this comment

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

Thx. I wasnt really opinionated on it, yet. https://ocramius.github.io/blog/when-to-declare-classes-final/ is also a nice read. Anyway, it looks good practice to me.. not sure if it's common enough (yet) for symfony though.

Copy link
Contributor

Choose a reason for hiding this comment

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

@Ener-Getick then maybe it would be better to lock them (final and @internal) first no?

Copy link
Contributor

Choose a reason for hiding this comment

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

Once you fix it, our comments will disappear :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@theofidry i'm using my phone tonight :P

Copy link
Contributor

Choose a reason for hiding this comment

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

Good thing, you shouldn't work a saturday evening anyway ;)

{
/** @var AstGeneratorInterface[] A list of generators */
protected $generators;

/** @var bool Whether the generation must return as soon as possible or use all generators, default to false */
protected $returnOnFirst;

public function __construct(array $generators = array(), $returnOnFirst = false)
{
$this->generators = $generators;
$this->returnOnFirst = $returnOnFirst;
}

/**
* {@inheritdoc}
*/
public function generate($object, array $context = array())
{
$nodes = array();

foreach ($this->generators as $generator) {
if ($generator instanceof AstGeneratorInterface && $generator->supportsGeneration($object)) {
$nodes = array_merge($nodes, $generator->generate($object, $context));

if ($this->returnOnFirst) {
return $nodes;
}
}
}

return $nodes;
}

/**
* {@inheritdoc}
*/
public function supportsGeneration($object)
{
foreach ($this->generators as $generator) {
if ($generator instanceof AstGeneratorInterface && $generator->supportsGeneration($object)) {
return true;
}
}

return false;
}
}
39 changes: 39 additions & 0 deletions src/Symfony/Component/AstGenerator/AstGeneratorInterface.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,39 @@
<?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\AstGenerator;

/**
* An AstGeneratorInterface is a contract to transform an object into an AST.
*
* @author Joel Wurtz <joel.wurtz@gmail.com>
*/
interface AstGeneratorInterface
{
/**
* Generate an object into an AST given a specific context.
*
* @param mixed $object Object to generate AST from
* @param array $context Context for the generator
*
* @return \PhpParser\Node[] An array of statements (AST Node)
*/
public function generate($object, array $context = array());

/**
* Check whether the given object is supported for generation by this generator.
*
* @param mixed $object Object to generate AST from
*
* @return bool
*/
public function supportsGeneration($object);
Copy link
Contributor
@unkind unkind Aug 6, 2016

Choose a reason for hiding this comment

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

This method makes me wonder: how it is possible that we don't exactly know if our generator is allowed to work with the given object? Does it even make sense to "try" generate AST tree of "random" object? Does it make sense to replace UrlGeneratorGenerator with PhpMatcherGenerator somewhere? I don't think so, they have very different semantic sense and they probably should have different (specific) interfaces: interface UrlAstGenerator, interface PhpMatcherAstGenerator if you really want to have multiple implementations.

Once interface becomes vague, it turns into wrong abstraction, because

The purpose of abstraction is not to be vague, but to create a new semantic level in which one can be absolutely precise

Also, it feels like "Tell, don't ask" violation.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This method is especially useful for chaining (@joelwurtz used it to chain generators transforming data depending on their type, really useful to allow the user to hook into the generation).
And replacing a top level routing generator by the other would not be useful indeed but you could chain them to put them in the same file for example.

Some generators could also be used in several components even if the routing component is not the best example for such use.

Copy link
Contributor

Choose a reason for hiding this comment

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

This method is especially useful for chaining

Can't the chain be done with decoration only by throwing a UnsupportedObjectException? I'm not sure if having a chain mechanism based on exceptions would be slower or not, but it would avoid to have two public methods and use generate() without trying supportsGeneration().

Copy link
Contributor Author
@GuilhemN GuilhemN Aug 6, 2016

Choose a reason for hiding this comment

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

Would be good for me as well and as the generation is almost never optional it would even be logical, i will change that !

Copy link
Contributor
@unkind unkind Aug 6, 2016

Choose a reason for hiding this comment

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

I don't really understand use-case of the chain Generator, to be honest. Probably, another abstraction is missing like interface AstWalker::walk(array $nodes): array or something?

For me, AstGeneratorInterface looks like the following generic interface:

/**
 * Maps something on something another. Very useful abstraction.
 */
interface Mapper
{
    /**
     * @return mixed something
     */
    public function map($something);
}

It generates some AST node of some object. The purpose of this interface is very unclean. But some kind of AstWalker probably makes sense if you have examples of generic AST walkers (coding style fixers, dead code removers, etc.).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is useful for normalizer generation you can take a look at the small app i wrote to illustrate how #17516 could be used.

Copy link
Contributor
@unkind unkind Aug 6, 2016

Choose a reason for hiding this comment

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

And replacing a top level routing generator by the other would not be useful indeed but you could chain them to put them in the same file for example.

Still, I don't see why UrlGeneratorGenerator and PhpMatcherGenerator should implement the generic generate($object, array $context):

final class UrlGeneratorAstGenerator 
{
    public function generate(
        RouteCollection $routeCollection,
        // By the way, these parameters can be part of constructor
        string $urlGeneratorClass,
        string $baseUrlGeneratorClass
    ): NodeList 
    {
        // ...
    }
}

final class PhpMatcherAstGenerator
{
    public function generate(RouteCollection $collection): NodeList
    {
        // ...
    }
}

$nodeList = 
    $this->urlAstGenerator->generate(...)
    ->append(
        $this->phpMatcherAstGenerator->generate(...)
    );

Generic interface adds false flexibility. Clarity of intent matters. Chain nodes, not generators.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This example looks much better indeed 👍
About NodeList, I do like the principle but should this be part of symfony ? I will give it a try :)

}
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
<?php

namespace Symfony\Component\AstGenerator\Exception;

class MissingContextException extends \RuntimeException
{
}
19 changes: 19 additions & 0 deletions src/Symfony/Component/AstGenerator/LICENSE
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
Copyright (c) 2016 Fabien Potencier

Permission is hereby granted, free of charge, to any person obtaining a copy
of this software and associated documentation files (the "Software"), to deal
in the Software without restriction, including without limitation the rights
to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
copies of the Software, and to permit persons to whom the Software is furnished
to do so, subject to the following conditions:

The above copyright notice and this permission notice shall be included in all
copies or substantial portions of the Software.

THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE
AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
THE SOFTWARE.
8 changes: 8 additions & 0 deletions src/Symfony/Component/AstGenerator/README.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
AstGenerator Component
======================

AstGenerator allows to generate PHP AST for several Component:

* Transform class, properties and types extracted from the PropertyInfo Component into POPO objects ans Normalizers
compatible with Serializer Component

88 changes: 88 additions & 0 deletions src/Symfony/Component/AstGenerator/Tests/AstGeneratorChainTest.php
B41A
Original file line number Diff line number Diff line change
@@ -0,0 +1,88 @@
<?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\AstGenerator\Tests;

use Symfony\Component\AstGenerator\AstGeneratorChain;
use Symfony\Component\AstGenerator\AstGeneratorInterface;

class AstGeneratorChainTest extends \PHPUnit_Framework_TestCase
{
public function testEmpty()
{
$generator = new AstGeneratorChain();

$this->assertFalse($generator->supportsGeneration('dummy'));
$this->assertEmpty($generator->generate('dummy'));
}

public function testSupports()
{
$generatorSub = $this->getGeneratorMock(true, array('ast'));

$generator = new AstGeneratorChain(array($generatorSub));
$this->assertTrue($generator->supportsGeneration('dummy'));
$this->assertEquals(array('ast'), $generator->generate('dummy'));
}

public function testMultiSupports()
{
$generatorSub1 = $this->getGeneratorMock(true, array('ast1'));
$generatorSub2 = $this->getGeneratorMock(true, array('ast2'));

$generator = new AstGeneratorChain(array($generatorSub1, $generatorSub2));
$this->assertTrue($generator->supportsGeneration('dummy'));
$this->assertEquals(array('ast1', 'ast2'), $generator->generate('dummy'));
}

public function testPartialSupports()
{
$generatorSub1 = $this->getGeneratorMock(true, array('ast1'));
$generatorSub2 = $this->getGeneratorMock(false);

$generator = new AstGeneratorChain(array($generatorSub1, $generatorSub2));
$this->assertTrue($generator->supportsGeneration('dummy'));
$this->assertEquals(array('ast1'), $generator->generate('dummy'));
}

public function testMultiSupportsWithFirstReturn()
{
$generatorSub1 = $this->getGeneratorMock(true, array('ast1'));
$generatorSub2 = $this->getGeneratorMock(true, array('ast2'));

$generator = new AstGeneratorChain(array($generatorSub1, $generatorSub2), true);
$this->assertTrue($generator->supportsGeneration('dummy'));
$this->assertEquals(array('ast1'), $generator->generate('dummy'));
}

private function getGeneratorMock($support, $return = null)
{
$generatorSub = $this->getMockBuilder(AstGeneratorInterface::class)->getMock();
$generatorSub
->expects($this->any())
->method('supportsGeneration')
->with('dummy')
->willReturn($support);
if (null === $return) {
$generatorSub
->expects($this->never())
->method('generate');
} else {
$generatorSub
->expects($this->any())
->method('generate')
->with('dummy', array())
->willReturn($return);
}

return $generatorSub;
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,31 @@
<?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\AstGenerator\Tests;

use Symfony\Component\AstGenerator\UniqueVariableScope;

class UniqueVariableScopeTest extends \PHPUnit_Framework_TestCase
{
public function testUniqueVariable()
{
$uniqueVariableScope = new UniqueVariableScope();

$name = $uniqueVariableScope->getUniqueName('name');
$this->assertEquals('name', $name);

$name = $uniqueVariableScope->getUniqueName('name');
$this->assertEquals('name_1', $name);

$name = $uniqueVariableScope->getUniqueName('name');
$this->assertEquals('name_2', $name);
}
}
40 changes: 40 additions & 0 deletions src/Symfony/Component/AstGenerator/UniqueVariableScope.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,40 @@
<?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\AstGenerator;

/**
* Allow to get a unique variable name for a scope (like a method).
*/
class UniqueVariableScope
{
private $registry = array();

/**
* Return an unique name for a variable.
*
* @param string $name Name of the variable
*
* @return string if not found return the $name given, if not return the name suffixed with a number
*/
public function getUniqueName($name)
{
if (!isset($this->registry[$name])) {
$this->registry[$name] = 0;

return $name;
}

++$this->registry[$name];

return sprintf('%s_%s', $name, $this->registry[$name]);
}
}
38 changes: 38 additions & 0 deletions src/Symfony/Component/AstGenerator/composer.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,38 @@
{
"name": "symfony/ast-generator",
"type": "library",
"description": "Symfony component to generate AST from and to various others components",
"keywords": ["symfony", "ast", "generator"],
"homepage": "https://symfony.com",
"license": "MIT",
"authors": [
{
"name": "Joel Wurtz",
"email": "joel.wurtz@gmail.com"
},
{
"name": "Symfony Community",
"homepage": "https://symfony.com/contributors"
}
],
"require": {
"php": ">=5.5.9",
"nikic/php-parser": "~2.0",
"symfony/property-info": "~2.8|~3.0"
},
"require-dev": {
"symfony/serializer": "~2.8|~3.0"
},
"autoload": {
"psr-4": { "Symfony\\Component\\AstGenerator\\": "" },
"exclude-from-classmap": [
"/Tests/"
]
},
"minimum-stability": "dev",
"extra": {
"branch-alias": {
"dev-master": "3.1-dev"
}
}
}
28 changes: 28 additions & 0 deletions src/Symfony/Component/AstGenerator/phpunit.xml.dist
Original file line number Diff line number Diff line change
@@ -0,0 +1,28 @@
<?xml version="1.0" encoding="UTF-8"?>

<phpunit xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
xsi:noNamespaceSchemaLocation="http://schema.phpunit.de/4.1/phpunit.xsd"
backupGlobals="false"
colors="true"
bootstrap="vendor/autoload.php"
>
<php>
<ini name="error_reporting" value="-1" />
</php>

<testsuites>
<testsuite name="Symfony AstGenerator Component Test Suite">
<directory>./Tests/</directory>
</testsuite>
</testsuites>

<filter>
<whitelist>
<directory>./</directory>
<exclude>
<directory>./Tests</directory>
<directory>./vendor</directory>
</exclude>
</whitelist>
</filter>
</phpunit>
0