8000 [Config] deprecate tree builders without root nodes by xabbuh · Pull Request #27476 · symfony/symfony · GitHub
[go: up one dir, main page]

Skip to content

[Config] deprecate tree builders without root nodes #27476

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 1 commit into from
Jun 25, 2018
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
5 changes: 5 additions & 0 deletions UPGRADE-4.2.md
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,11 @@ Cache

* Deprecated `CacheItem::getPreviousTags()`, use `CacheItem::getMetadata()` instead.

Config
------

* Deprecated constructing a `TreeBuilder` without passing root node information.

Security
--------

Expand Down
1 change: 1 addition & 0 deletions UPGRADE-5.0.md
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ Cache
Config
------

* Dropped support for constructing a `TreeBuilder` without passing root node information.
* Added the `getChildNodeDefinitions()` method to `ParentNodeDefinitionInterface`.
* The `Processor` class has been made final

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -26,10 +26,9 @@ class Configuration implements ConfigurationInterface
*/
public function getConfigTreeBuilder()
{
$treeBuilder = new TreeBuilder();
$rootNode = $treeBuilder->root('debug');
$treeBuilder = new TreeBuilder('debug');

$rootNode
$treeBuilder->getRootNode()
->children()
->integerNode('max_items')
->info('Max number of displayed items past the first level, -1 means no limit')
Expand Down
3 changes: 2 additions & 1 deletion src/Symfony/Bundle/DebugBundle/composer.json
Original file line number Diff line number Diff line change
Expand Up @@ -23,11 +23,12 @@
"symfony/var-dumper": "~4.1"
},
"require-dev": {
"symfony/config": "~3.4|~4.0",
"symfony/config": "~4.2",
"symfony/dependency-injection": "~3.4|~4.0",
"symfony/web-profiler-bundle": "~3.4|~4.0"
},
"conflict": {
"symfony/config": "<4.2",
"symfony/dependency-injection": "<3.4"
},
"suggest": {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -54,8 +54,8 @@ public function __construct(bool $debug)
*/
public function getConfigTreeBuilder()
{
$treeBuilder = new TreeBuilder();
$rootNode = $treeBuilder->root('framework');
$treeBuilder = new TreeBuilder('framework');
$rootNode = $treeBuilder->getRootNode();

$rootNode
->beforeNormalization()
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -25,11 +25,10 @@ public function __construct($customConfig = null)

public function getConfigTreeBuilder()
{
$treeBuilder = new TreeBuilder();
$rootNode = $treeBuilder->root('test');
$treeBuilder = new TreeBuilder('test');

if ($this->customConfig) {
$this->customConfig->addConfiguration($rootNode);
$this->customConfig->addConfiguration($treeBuilder->getRootNode());
}

return $treeBuilder;
Expand Down
2 changes: 1 addition & 1 deletion src/Symfony/Bundle/FrameworkBundle/composer.json
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@
"ext-xml": "*",
"symfony/cache": "~3.4|~4.0",
"symfony/dependency-injection": "^4.1.1",
"symfony/config": "~3.4|~4.0",
"symfony/config": "~4.2",
"symfony/event-dispatcher": "^4.1",
"symfony/http-foundation": "^4.1",
"symfony/http-kernel": "^4.1",
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -41,8 +41,8 @@ public function __construct(array $factories, array $userProviderFactories)
*/
public function getConfigTreeBuilder()
{
$tb = new TreeBuilder();
$rootNode = $tb->root('security');
$tb = new TreeBuilder('security');
$rootNode = $tb->getRootNode();

$rootNode
->beforeNormalization()
Expand Down
1 change: 1 addition & 0 deletions src/Symfony/Bundle/SecurityBundle/composer.json
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@
"require": {
"php": "^7.1.3",
"ext-xml": "*",
"symfony/config": "^4.2",
"symfony/security": "~4.2",
"symfony/dependency-injection": "^3.4.3|^4.0.3",
"symfony/http-kernel": "^4.1"
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -29,8 +29,8 @@ class Configuration implements ConfigurationInterface
*/
public function getConfigTreeBuilder()
{
$treeBuilder = new TreeBuilder();
$rootNode = $treeBuilder->root('twig');
$treeBuilder = new TreeBuilder('twig');
$rootNode = $treeBuilder->getRootNode();

$rootNode
->children()
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 @@ -17,7 +17,7 @@
],
"require": {
"php": "^7.1.3",
"symfony/config": "~3.4|~4.0",
"symfony/config": "~4.2",
"symfony/twig-bridge": "^3.4.3|^4.0.3",
"symfony/http-foundation": "~3.4|~4.0",
"symfony/http-kernel": "~3.4|~4.0",
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -31,10 +31,9 @@ class Configuration implements ConfigurationInterface
*/
public function getConfigTreeBuilder()
{
$treeBuilder = new TreeBuilder();
$rootNode = $treeBuilder->root('web_profiler');
$treeBuilder = new TreeBuilder('web_profiler');

$rootNode
$treeBuilder->getRootNode()
->children()
->booleanNode('toolbar')->defaultFalse()->end()
->booleanNode('intercept_redirects')->defaultFalse()->end()
Expand Down
3 changes: 1 addition & 2 deletions src/Symfony/Bundle/WebProfilerBundle/composer.json
Original file line number Diff line number Diff line change
Expand Up @@ -17,20 +17,19 @@
],
"require": {
"php": "^7.1.3",
"symfony/config": "^4.2",
"symfony/http-kernel": "~4.1",
"symfony/routing": "~3.4|~4.0",
"symfony/twig-bundle": "^3.4.3|^4.0.3",
"symfony/var-dumper": "~3.4|~4.0",
"twig/twig": "~1.34|~2.4"
},
"require-dev": {
"symfony/config": "~3.4|~4.0",
"symfony/console": "~3.4|~4.0",
"symfony/dependency-injection": "~3.4|~4.0",
"symfony/stopwatch": "~3.4|~4.0"
},
"conflict": {
"symfony/config": "<3.4",
"symfony/dependency-injection": "<3.4",
"symfony/event-dispatcher": "<3.4",
"symfony/var-dumper": "<3.4"
Expand Down
5 changes: 5 additions & 0 deletions src/Symfony/Component/Config/CHANGELOG.md
Original file line number Diff line number Diff line change
@@ -1,6 +1,11 @@
CHANGELOG
=========

4.2.0
-----

* deprecated constructing a `TreeBuilder` without passing root node information

4.1.0
-----

Expand Down
18 changes: 18 additions & 0 deletions src/Symfony/Component/Config/Definition/Builder/TreeBuilder.php
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,15 @@ class TreeBuilder implements NodeParentInterface
protected $tree;
protected $root;

public function __construct(string $name = null, string $type = 'array', NodeBuilder $builder = null)
Copy link
Contributor

Choo 179B se a reason for hiding this comment

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

how about ?NodeBuilder instead ? Use a nullable typehint instead of relying on 7.0's hackish way.

Copy link
Member

Choose a reason for hiding this comment

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

There is nothing hackish here, optional arguments are just fit here IMHO.

Copy link
Contributor

Choose a reason for hiding this comment

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

It's not about the fact that this is optionnal or not. It's because NodeBuilder $builder = null, instead of ?Nodebuilder $builder = null. Yes it's one more char, but it's more pertinent. It was a hack in 7.0 to allow a null value.

Copy link
Member

Choose a reason for hiding this comment

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

@Taluu it's not "more pertinent", is exactly the same, meaning wise. It's not a hack at all but how the language is defined.

Copy link
Member

Choose a reason for hiding this comment

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

<?php

interface A {
    public function dummy(int $arg = null);
}

class B implements A {
    public function dummy(int $arg) { }
}

Fatal error: Declaration of B::dummy(int $arg) must be compatible with A::dummy(?int $arg = NULL) in nullable.php on line 7

Note the ? in the stack trace while it is not in the signature, it's really the same for the engine

{
if (null === $name) {
@trigger_error('A tree builder without a root node is deprecated since Symfony 4.2 and will not be supported anymore in 5.0.', E_USER_DEPRECATED);
} else {
$this->root($name, $type, $builder);
}
}

/**
* Creates the root node.
*
Expand All @@ -42,6 +51,15 @@ public function root($name, $type = 'array', NodeBuilder $builder = null)
return $this->root = $builder->node($name, $type)->setParent($this);
}

public function getRootNode(): NodeDefinition
Copy link
Member

Choose a reason for hiding this comment

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

this return type is broken when using the deprecated API and not calling ->root(). I suggest throwing an exception with a meaningful message instead of letting PHP throw a TypeError (which will get reported as a bug in Symfony):

throw new \RuntimeException(sprintf('Calling %s before creating the root node is not supported. Migrate to the new constructor signature instead', __METHOD__));

{
if (null === $this->root) {
throw new \RuntimeException(sprintf('Calling %s() before creating the root node is not supported, migrate to the new constructor signature instead.', __METHOD__));
}

return $this->root;
}

/**
* Builds the tree.
*
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -212,10 +212,10 @@ public function testEndThenPartNotSpecified()
*/
protected function getTestBuilder()
{
$builder = new TreeBuilder();
$builder = new TreeBuilder('test');

return $builder
->root('test')
->getRootNode()
->children()
->variableNode('key')
->validate()
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -19,10 +19,9 @@ class TreeBuilderTest extends TestCase
{
public function testUsingACustomNodeBuilder()
{
$builder = new TreeBuilder();
$root = $builder->root('custom', 'array', new CustomNodeBuilder());
$builder = new TreeBuilder('custom', 'array', new CustomNodeBuilder());

$nodeBuilder = $root->children();
$nodeBuilder = $builder->getRootNode()->children();

$this->assertInstanceOf('Symfony\Component\Config\Tests\Fixtures\Builder\NodeBuilder', $nodeBuilder);

Expand All @@ -33,49 +32,46 @@ public function testUsingACustomNodeBuilder()

public function testOverrideABuiltInNodeType()
{
$builder = new TreeBuilder();
$root = $builder->root('override', 'array', new CustomNodeBuilder());
$builder = new TreeBuilder('override', 'array', new CustomNodeBuilder());

$definition = $root->children()->variableNode('variable');
$definition = $builder->getRootNode()->children()->variableNode('variable');

$this->assertInstanceOf('Symfony\Component\Config\Tests\Fixtures\Builder\VariableNodeDefinition', $definition);
}

public function testAddANodeType()
{
$builder = new TreeBuilder();
$root = $builder->root('override', 'array', new CustomNodeBuilder());
$builder = new TreeBuilder('override', 'array', new CustomNodeBuilder());

$definition = $root->children()->barNode('variable');
$definition = $builder->getRootNode()->children()->barNode('variable');

$this->assertInstanceOf('Symfony\Component\Config\Tests\Fixtures\Builder\BarNodeDefinition', $definition);
}

public function testCreateABuiltInNodeTypeWithACustomNodeBuilder()
{
$builder = new TreeBuilder();
$root = $builder->root('builtin', 'array', new CustomNodeBuilder());
$builder = new TreeBuilder('builtin', 'array', new CustomNodeBuilder());

$definition = $root->children()->booleanNode('boolean');
$definition = $builder->getRootNode()->children()->booleanNode('boolean');

$this->assertInstanceOf('Symfony\Component\Config\Definition\Builder\BooleanNodeDefinition', $definition);
}

public function testPrototypedArrayNodeUseTheCustomNodeBuilder()
{
$builder = new TreeBuilder();
$root = $builder->root('override', 'array', new CustomNodeBuilder());
$builder = new TreeBuilder('override', 'array', new CustomNodeBuilder());

$root = $builder->getRootNode();
$root->prototype('bar')->end();

$this->assertInstanceOf('Symfony\Component\Config\Tests\Fixtures\BarNode', $root->getNode(true)->getPrototype());
}

public function testAnExtendedNodeBuilderGetsPropagatedToTheChildren()
{
$builder = new TreeBuilder();
$builder = new TreeBuilder('propagation');

$builder->root('propagation')
$builder->getRootNode()
->children()
->setNodeClass('extended', 'Symfony\Component\Config\Definition\Builder\BooleanNodeDefinition')
->node('foo', 'extended')->end()
Expand All @@ -99,9 +95,9 @@ public function testAnExtendedNodeBuilderGetsPropagatedToTheChildren()

public function testDefinitionInfoGetsTransferredToNode()
{
$builder = new TreeBuilder();
$builder = new TreeBuilder('test');

$builder->root('test')->info('root info')
$builder->getRootNode()->info('root info')
->children()
->node('child', 'variable')->info('child info')->defaultValue('default')
->end()
Expand All @@ -116,9 +112,9 @@ public function testDefinitionInfoGetsTransferredToNode()

public function testDefinitionExampleGetsTransferredToNode()
{
$builder = new TreeBuilder();
$builder = new TreeBuilder('test');

$builder->root('test')
$builder->getRootNode()
->example(array('key' => 'value'))
->children()
->node('child', 'variable')->info('child info')->defaultValue('default')->example('example')
Expand All @@ -134,9 +130,9 @@ public function testDefinitionExampleGetsTransferredToNode()

public function testDefaultPathSeparatorIsDot()
{
$builder = new TreeBuilder();
$builder = new TreeBuilder('propagation');

$builder->root('propagation')
$builder->getRootNode()
->children()
->node('foo', 'variable')->end()
->arrayNode('child')
Expand Down Expand Up @@ -164,9 +160,9 @@ public function testDefaultPathSeparatorIsDot()

public function testPathSeparatorIsPropagatedToChildren()
{
$builder = new TreeBuilder();
$builder = new TreeBuilder('propagation');

$builder->root('propagation')
$builder->getRootNode()
->children()
->node('foo', 'variable')->end()
->arrayNode('child')
Expand All @@ -192,4 +188,13 @@ public function testPathSeparatorIsPropagatedToChildren()
$this->assertInstanceOf('Symfony\Component\Config\Definition\BaseNode', $childChildren['foo']);
$this->assertSame('propagation/child/foo', $childChildren['foo']->getPath());
}

/**
* @group legacy
* @expectedDeprecation A tree builder without a root node is deprecated since Symfony 4.2 and will not be supported anymore in 5.0.
*/
public function testInitializingTreeBuildersWithoutRootNode()
{
new TreeBuilder();
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -20,9 +20,9 @@ class FinalizationTest extends TestCase
{
public function testUnsetKeyWithDeepHierarchy()
{
$tb = new TreeBuilder();
$tb = new TreeBuilder('config', 'array');
$tree = $tb
->root('config', 'array')
->getRootNode()
->children()
->node('level1', 'array')
->canBeUnset()
Expand Down
Loading
0