[Config] Handle nullable node name + fix inheritdocs #26335

ro0NL · Feb 27, 2018

Q	A
Branch?	2.7
Bug fix?	yes
New feature?	no
BC breaks?	no-ish
Deprecations?	no
Tests pass?	yes
Fixed tickets	#...
License	MIT
Doc PR	symfony/symfony-docs#...

Small split from #26297 that can be merged until master/4.1. Whereas the doc fixes only apply until 3.4, hence the split.

Small change regarding getName/Path() for not returning a null value anymore which violates NodeInterface::getName/Path()

Remainng issue left at

symfony/src/Symfony/Component/Config/Definition/Builder/ArrayNodeDefinition.php

Lines 381 to 383 in cd5f410

    
           public function append(NodeDefinition $node) 
        
           { 
        
               $this->children[$node->name] = $node->setParent($this);

which i tend to leave untouched across all branches for now.

ro0NL · Feb 27, 2018

Added all fixes that can be merged until master. Ill change #26297 to backport string|null fixes soley (which are type declarations as of 4.0>

nicolas-grekas · Feb 27, 2018

src/Symfony/Component/Config/Definition/BaseNode.php

@@ -40,7 +40,7 @@
     */
    public function __construct($name, NodeInterface $parent = null)
    {
-        if (false !== strpos($name, '.')) {
+        if (null !== $name && false !== strpos($name, '.')) {


why not cast to string instead?
if (false !== strpos($name = (string) $name, '.')) {

yes, better. Now updated.

Didnt reassign $name btw... we need to preserve string|null on the property side. Or put different: im not sure about changing that.

we need to preserve string|null on the property side

why? would need a side effect, which one is it? casting all the time is hurting perf, that's not the side effect I'd consider good ;)

which one is it?

BC... (protected $name;). Also the constructor sig kinda implies a nullable value.

Then again.. if i got this right, it shouldnt hurt indeed. AFAIK prototyped nodes will have a string name (setName()) once being dealt with. And the calling side needs to expect string anyway due getName().

Given $arr[null] = .. equals $arr[''] = ... (see remaining issue) i think it's safe to make the change 👍

the constructor sig kinda implies a nullable value

not the docblock, isn't it? I'd support doing the change :)

touche. It does in 4.0 though. Now updated.

ro0NL · Feb 27, 2018

found a few more, derived from NodeInterface/PrototypeNodeInterface 😅

fabpot · Feb 28, 2018

src/Symfony/Component/Config/Definition/NodeInterface.php

@@ -83,6 +89,9 @@ public function merge($leftSide, $rightSide);
     * @param mixed $value The value to finalize
     *
     * @return mixed The finalized value
+     *
+     * @throws Exception


All @throws should have a description explaining when the exception can be triggered... or be removed :)

Now updated.

Simperfit · Mar 4, 2018

src/Symfony/Component/Config/Definition/NodeInterface.php

     *
-     * @param mixed $value The value to normalize
+     * @param mixed $value Value to normalize


maybe A value to normalize ?

in fact.. i think i should revert this one as the interface was already using different syntax:

https://github.com/symfony/symfony/pull/26335/files#diff-41913ce7a681852774440044e2c091bdR92

fabpot · Mar 5, 2018

src/Symfony/Component/Config/Definition/BaseNode.php

@@ -40,7 +40,7 @@
     */
    public function __construct($name, NodeInterface $parent = null)
    {
-        if (false !== strpos($name, '.')) {
+        if (false !== strpos($name = (string) $name, '.')) {


$name must be a string, so casting here should be removed. Adding this is not a bug fix.

in 4.0 it's string|null already

symfony/src/Symfony/Component/Config/Definition/BaseNode.php

Line 42 in d65c43b

public function __construct(?string $name, NodeInterface $parent = null, string $pathSeparator = self::DEFAULT_PATH_SEPARATOR)

hence #26297

practically using null is just an alias for empty string, but we use null by design (as of 2.7!):

symfony/src/Symfony/Component/Config/Definition/Builder/ArrayNodeDefinition.php

Lines 75 to 78 in d65c43b

public function prototype($type)

{

return $this->prototype = $this->getNodeBuilder()->node(null, $type)->setParent($this);

}

if we preserve null on the property side (thus no cast) we keep violating getName:

symfony/src/Symfony/Component/Config/Definition/BaseNode.php

Lines 223 to 225 in d65c43b

* @return string The Node's name

*/

public function getName()

hence the cast

nicolas-grekas · Mar 19, 2018

Thank you @ro0NL.

…0NL) This PR was squashed before being merged into the 2.7 branch (closes #26335). Discussion ---------- [Config] Handle nullable node name + fix inheritdocs | Q | A | ------------- | --- | Branch? | 2.7 | Bug fix? | yes | New feature? | no | BC breaks? | no-ish | Deprecations? | no | Tests pass? | yes  | Fixed tickets | #...  | License | MIT | Doc PR | symfony/symfony-docs#...  Small split from #26297 that can be merged until master/4.1. Whereas the doc fixes only apply until 3.4, hence the split. Small change regarding `getName/Path()` for not returning a `null` value anymore which violates `NodeInterface::getName/Path()` Remainng issue left at https://github.com/symfony/symfony/blob/cd5f4105a43208632c8b62fd75b4dad53dcd8a41/src/Symfony/Component/Config/Definition/Builder/ArrayNodeDefinition.php#L381-L383 which i tend to leave untouched across all branches for now. Commits ------- 5c3e6a9 [Config] Handle nullable node name + fix inheritdocs

[Config] Handle nullable node name

0d343c9

carsonbot added Status: Needs Review Config Bug labels Feb 27, 2018

fix inheritdocs

986ae4c

ro0NL changed the title ~~[Config] Handle nullable node name~~ [Config] Handle nullable node name + fix inheritdocs Feb 27, 2018

ro0NL mentioned this pull request Feb 27, 2018

[Config] Backport string|null api for node names #26297

Merged

nicolas-grekas added this to the 2.7 milestone Feb 27, 2018

fix BaseNode inheritdoc

bc1c945

nicolas-grekas reviewed Feb 27, 2018

View reviewed changes

use cast

6f1db4c

nicolas-grekas approved these changes Feb 27, 2018

View reviewed changes

carsonbot added Status: Reviewed and removed Status: Needs Review labels Feb 27, 2018

fix all the inheritdocs

03ea776

chalasr approved these changes Feb 28, 2018

View reviewed changes

fabpot requested changes Feb 28, 2018

View reviewed changes

carsonbot added Status: Needs Work and removed Status: Reviewed labels Feb 28, 2018

fix @throws

f1eb6ec

Simperfit reviewed Mar 4, 2018

View reviewed changes

Simperfit approved these changes Mar 4, 2018

View reviewed changes

carsonbot added Status: Reviewed and removed Status: Needs Work labels Mar 4, 2018

Update NodeInterface.php

be2a65d

fabpot requested changes Mar 5, 2018

View reviewed changes

carsonbot added Status: Needs Work and removed Status: Reviewed labels Mar 5, 2018

carsonbot removed the Status: Needs Work label Mar 14, 2018

nicolas-grekas approved these changes Mar 14, 2018

View reviewed changes

carsonbot added the Status: Reviewed label Mar 14, 2018

nicolas-grekas closed this Mar 19, 2018

	public function append(NodeDefinition $node)
	{
	$this->children[$node->name] = $node->setParent($this);

	public function prototype($type)
	{
	return $this->prototype = $this->getNodeBuilder()->node(null, $type)->setParent($this);
	}

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

Uh oh!

[Config] Handle nullable node name + fix inheritdocs #26335

[Config] Handle nullable node name + fix inheritdocs #26335

Uh oh!

Conversation

ro0NL commented Feb 27, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ro0NL commented Feb 27, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ro0NL Feb 27, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ro0NL commented Feb 27, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ro0NL Mar 5, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nicolas-grekas commented Mar 19, 2018

Uh oh!

Uh oh!

ro0NL commented Feb 27, 2018 •

edited

Loading

ro0NL Feb 27, 2018 •

edited

Loading

ro0NL Mar 5, 2018 •

edited

Loading