[Config] deprecate tree builders without root nodes #27476

xabbuh · 2018-06-02T18:29:10Z

Q	A
Branch?	master
Bug fix?	no
New feature?	yes
BC breaks?	no
Deprecations?	yes
Tests pass?	yes
Fixed tickets
License	MIT
Doc PR

While reviewing #27472 I wondered if we really need support config trees without a root node. If we did not support it, users wouldn't create pseudo configuration classes when they were actually not needed.

nicolas-grekas · 2018-06-04T21:11:59Z

rebase needed

stof · 2018-06-05T08:38:52Z

src/Symfony/Bundle/SecurityBundle/composer.json

@@ -25,6 +25,7 @@
    "require-dev": {
        "symfony/asset": "~3.4|~4.0",
        "symfony/browser-kit": "~3.4|~4.0",
+        "symfony/config": "~4.2",


I think this can be a normal requirement instead. I don't think it is currently possible to avoid it anyway.

stof · 2018-06-05T08:40:15Z

src/Symfony/Bundle/WebProfilerBundle/composer.json

@@ -24,13 +24,13 @@
        "twig/twig": "~1.34|~2.4"
    },
    "require-dev": {
-        "symfony/config": "~3.4|~4.0",
+        "symfony/config": "~4.2",


As TwigBundle depends on it, and WebProfilerBundle depends on TwigBundle, this gets installed all the time already. I suggest making it a normal requirement instead of a conflict rule (this might even perform better in Composer, as a <4.2 conflict rule matches lots of packages)

stof · 2018-06-05T08:41:07Z

src/Symfony/Component/Config/Definition/Builder/TreeBuilder.php

+    public function __construct(string $name = null, string $type = 'array', NodeBuilder $builder = null)
+    {
+        if (null === $name) {
+            @trigger_error('A tree builder without a root node is deprecated since Symfony 4.2 and will not be supported anymore with 5.0.', E_USER_DEPRECATED);


shoudln't it be in 5.0 ?

stof · 2018-06-05T08:42:10Z

src/Symfony/Component/Config/Definition/Builder/TreeBuilder.php

@@ -41,6 +50,11 @@ public function root($name, $type = 'array', NodeBuilder $builder = null)
        return $this->root = $builder->node($name, $type)->setParent($this);
    }

+    public function getRootNode(): NodeDefinition


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__));

ro0NL · 2018-06-05T11:41:50Z

src/Symfony/Component/Config/Definition/Builder/TreeBuilder.php

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


should we deprecate public calls to root() as well? or is it still intended as a feature?

both the methods (root + getRootNode) can be seen confusing.

I do not really care to be honest. Since this is a builder there may be use cases to still call root() later. But I also see that we could probably discuss this as part of a follow up PR if you would like to open one. :)

Taluu · 2018-06-05T23:02:13Z

src/Symfony/Component/Config/Definition/Builder/TreeBuilder.php

@@ -23,6 +23,15 @@ class TreeBuilder implements NodeParentInterface
    protected $tree;
    protected $root;

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


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

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

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.

@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.

<?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

fabpot

with. a minor comment

fabpot · 2018-06-10T10:15:56Z

src/Symfony/Component/Config/Definition/Builder/TreeBuilder.php

+    public function getRootNode(): NodeDefinition
+    {
+        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__));


missing dot at the end of the exception message.

upported, migrate

nicolas-grekas · 2018-06-19T11:56:28Z

(rebase needed + comment to address)

fabpot · 2018-06-25T17:07:02Z

Thank you @xabbuh.

…abbuh) This PR was merged into the 4.2-dev branch. Discussion ---------- [Config] deprecate tree builders without root nodes | Q | A | ------------- | --- | Branch? | master | Bug fix? | no | New feature? | yes | BC breaks? | no | Deprecations? | yes | Tests pass? | yes | Fixed tickets | | License | MIT | Doc PR | While reviewing #27472 I wondered if we really need support config trees without a root node. If we did not support it, users wouldn't create pseudo configuration classes when they were actually not needed. Commits ------- c2ce153 deprecate tree builders without root nodes

apfelbox · 2019-01-27T11:44:46Z

How can we now write code that works deprecation-free in both Symfony 4.1 and 4.2?

The only thing I can come up with is:

$treeBuilder = new TreeBuilder("my_node");
$rootNode = method_exists($treeBuilder, "getRootNode")
    ? $treeBuilder->getRootNode()
    : $treeBuilder->root("my_node");

which is super hacky in two ways:

needing to duplicate the key name
code that needs to rely on method_exists() always looks like a hack to me.

@xabbuh do you have a better way?

xabbuh · 2019-01-27T11:48:19Z

That's indeed the way to go as long as you do not want to drop support for Symfony 4.1.

apfelbox · 2019-01-27T11:49:02Z

All right. Thanks for the super fast response! 🙌

…ee builder (Koc) This PR was merged into the 4.2 branch. Discussion ---------- [Config] Improve PHPdoc / IDE autocomplete for config tree builder | Q | A | ------------- | --- | Branch? | 4.2 | Bug fix? | no | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | - | License | MIT | Doc PR | - follow up of #27476 and #21047 Commits ------- 21f7977 Improve PHPdoc / IDE autocomplete for config tree builder

As of Symfony 4.2, creating a config tree without a root node, is deprecated. See symfony/symfony#27476.

xabbuh added this to the next milestone Jun 2, 2018

carsonbot added Status: Needs Review Config Feature Deprecation labels Jun 2, 2018

xabbuh force-pushed the deprecate-tree-builder-with-root-node branch from 4e83499 to f981459 Compare June 2, 2018 19:09

stof reviewed Jun 5, 2018

View reviewed changes

ro0NL reviewed Jun 5, 2018

View reviewed changes

Taluu reviewed Jun 5, 2018

View reviewed changes

xabbuh force-pushed the deprecate-tree-builder-with-root-node branch 3 times, most recently from a588420 to 9bd6ade Compare June 7, 2018 06:56

fabpot approved these changes Jun 10, 2018

View reviewed changes

carsonbot added Status: Reviewed and removed Status: Needs Review labels Jun 10, 2018

xabbuh force-pushed the deprecate-tree-builder-with-root-node branch from 9bd6ade to b8e8890 Compare June 10, 2018 10:19

xabbuh force-pushed the deprecate-tree-builder-with-root-node branch from b8e8890 to 056768d Compare June 19, 2018 12:33

deprecate tree builders without root nodes

c2ce153

xabbuh force-pushed the deprecate-tree-builder-with-root-node branch from 056768d to c2ce153 Compare June 19, 2018 12:34

nicolas-grekas approved these changes Jun 19, 2018

View reviewed changes

fabpot merged commit c2ce153 into symfony:master Jun 25, 2018

xabbuh deleted the deprecate-tree-builder-with-root-node branch June 25, 2018 17:40

javiereguiluz mentioned this pull request Sep 13, 2018

[Config] deprecate tree builders without root nodes symfony/symfony-docs#10339

Closed

chalasr mentioned this pull request Oct 27, 2018

Fix deprecation on Symfony 4.2 getsentry/sentry-symfony#161

Merged

nicolas-grekas modified the milestones: next, 4.2 Nov 1, 2018

This was referenced Nov 3, 2018

Release v4.2.0-BETA1 #29072

Closed

Release v4.2.0-BETA1 #29073

Merged

hacfi mentioned this pull request Dec 18, 2018

Don't use the deprecated interface with Symfony 4.2 lunetics-org/LocaleBundle#208

Merged

This was referenced Feb 14, 2019

Fix deprecation notice with Symfony 4.2 markitosgv/JWTRefreshTokenBundle#137

Merged

Fix deprecation notice with Symfony 4.2 prestaconcept/PrestaSitemapBundle#176

Merged

Koc mentioned this pull request Apr 1, 2019

[Config] Improve PHPdoc / IDE autocomplete for config tree builder #30817

Merged

sstok mentioned this pull request Apr 14, 2019

[Config] Deprecate TreeBuilder::root #31027

Merged

richgerdes pushed a commit to richgerdes/jade that referenced this pull request Aug 6, 2019

Fix depricated method of creating a root node.

da4b46b

As of Symfony 4.2, creating a config tree without a root node, is deprecated. See symfony/symfony#27476.

richgerdes mentioned this pull request Aug 6, 2019

Fix depricated method of creating a root node. trivago/jade#21

Merged

richgerdes pushed a commit to richgerdes/jade that referenced this pull request Aug 6, 2019

Fix depricated method of creating a root node.

ec027de

As of Symfony 4.2, creating a config tree without a root node, is deprecated. See symfony/symfony#27476.

lebarjack mentioned this pull request Dec 12, 2019

A tree builder without a root node is deprecated since Symfony 4.2 and will not be supported anymore in 5.0. OpenLdapObject/LdapObjectBundle#2

Open

thelem mentioned this pull request May 9, 2020

Support Symfony 5.x comur/ComurImageBundle#123

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Config] deprecate tree builders without root nodes #27476

[Config] deprecate tree builders without root nodes #27476

xabbuh commented Jun 2, 2018

nicolas-grekas commented Jun 4, 2018

stof Jun 5, 2018

stof Jun 5, 2018

stof Jun 5, 2018

stof Jun 5, 2018

ro0NL Jun 5, 2018 •

edited

Loading

xabbuh Jun 19, 2018

Taluu Jun 5, 2018

nicolas-grekas Jun 6, 2018

Taluu Jun 10, 2018

nicolas-grekas Jun 19, 2018

chalasr Jun 25, 2018

fabpot left a comment

fabpot Jun 10, 2018

fabpot Jun 10, 2018

nicolas-grekas commented Jun 19, 2018

fabpot commented Jun 25, 2018

apfelbox commented Jan 27, 2019

xabbuh commented Jan 27, 2019

apfelbox commented Jan 27, 2019

[Config] deprecate tree builders without root nodes #27476

[Config] deprecate tree builders without root nodes #27476

Conversation

xabbuh commented Jun 2, 2018

nicolas-grekas commented Jun 4, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ro0NL Jun 5, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fabpot left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nicolas-grekas commented Jun 19, 2018

fabpot commented Jun 25, 2018

apfelbox commented Jan 27, 2019

xabbuh commented Jan 27, 2019

apfelbox commented Jan 27, 2019

ro0NL Jun 5, 2018 •

edited

Loading