Skip to content

Navigation Menu

Sign in
Appearance settings

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

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly
Appearance settings

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

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.

Sign up for GitHub

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 7 commits into from
Closed

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

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
0d343c9
[Config] Handle nullable node name
ro0NL Feb 27, 2018
986ae4c
fix inheritdocs
ro0NL Feb 27, 2018
bc1c945
fix BaseNode inheritdoc
ro0NL Feb 27, 2018
6f1db4c
use cast
ro0NL Feb 27, 2018
03ea776
fix all the inheritdocs
ro0NL Feb 27, 2018
f1eb6ec
fix @throws
ro0NL Mar 1, 2018
be2a65d
Update NodeInterface.php
ro0NL Mar 4, 2018
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
42 changes: 9 additions & 33 deletions 42 src/Symfony/Component/Config/Definition/BaseNode.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,7 @@ abstract class BaseNode implements NodeInterface
*/
public function __construct($name, NodeInterface $parent = null)
{
if (false !== strpos($name, '.')) {
if (null !== $name && false !== strpos($name, '.')) {
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes, better. Now updated.

Copy link
Contributor Author

@ro0NL ro0NL Feb 27, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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 👍

Copy link
Member

Choose a reason for hiding this comment

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

the constructor sig kinda implies a nullable value

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

touche. It does in 4.0 though. Now updated.

throw new \InvalidArgumentException('The name must not contain ".".');
}

Expand Down Expand Up @@ -170,33 +170,27 @@ public function setFinalValidationClosures(array $closures)
}

/**
* Checks if this node is required.
*
* @return bool
* {@inheritdoc}
*/
public function isRequired()
{
return $this->required;
}

/**
* Returns the name of this node.
*
* @return string The Node's name
* {@inheritdoc}
*/
public function getName()
{
return $this->name;
return null === $this->name ? '' : $this->name;
}

/**
* Retrieves the path of this node.
*
* @return string The Node's path
* {@inheritdoc}
*/
public function getPath()
{
$path = $this->name;
$path = null === $this->name ? '' : $this->name;

if (null !== $this->parent) {
$path = $this->parent->getPath().'.'.$path;
Expand All @@ -206,14 +200,7 @@ public function getPath()
}

/**
* Merges two values together.
*
* @param mixed $leftSide
* @param mixed $rightSide
*
* @return mixed The merged value
*
* @throws ForbiddenOverwriteException
* {@inheritdoc}
*/
final public function merge($leftSide, $rightSide)
{
Expand All @@ -233,11 +220,7 @@ final public function merge($leftSide, $rightSide)
}

/**
* Normalizes a value, applying all normalization closures.
*
* @param mixed $value Value to normalize
*
* @return mixed The normalized value
* {@inheritdoc}
*/
final public function normalize($value)
{
Expand Down Expand Up @@ -285,14 +268,7 @@ public function getParent()
}

/**
* Finalizes a value, applying all finalization closures.
*
* @param mixed $value The value to finalize
*
* @return mixed The finalized value
*
* @throws Exception
* @throws InvalidConfigurationException
* {@inheritdoc}
*/
final public function finalize($value)
{
Expand Down
18 changes: 3 additions & 15 deletions 18 src/Symfony/Component/Config/Definition/Builder/ArrayNodeDefinition.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,17 +47,15 @@ public function __construct($name, NodeParentInterface $parent = null)
}

/**
* Sets a custom children builder.
* {@inheritdoc}
*/
public function setBuilder(NodeBuilder $builder)
{
$this->nodeBuilder = $builder;
}

/**
* Returns a builder to add children nodes.
*
* @return NodeBuilder
* {@inheritdoc}
*/
public function children()
{
Expand Down Expand Up @@ -306,17 +304,7 @@ public function normalizeKeys($bool)
}

/**
* Appends a node definition.
*
* $node = new ArrayNodeDefinition()
* ->children()
* ->scalarNode('foo')->end()
* ->scalarNode('baz')->end()
* ->end()
* ->append($this->getBarNodeDefinition())
* ;
*
* @return $this
* {@inheritdoc}
*/
public function append(NodeDefinition $node)
{
Expand Down
23 changes: 23 additions & 0 deletions 23 src/Symfony/Component/Config/Definition/Builder/ParentNodeDefinitionInterface.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,9 +18,32 @@
*/
interface ParentNodeDefinitionInterface
{
/**
* Returns a builder to add children nodes.
*
* @return NodeBuilder
*/
public function children();

/**
* Appends a node definition.
*
* Usage:
*
* $node = $parentNode
* ->children()
* ->scalarNode('foo')->end()
* ->scalarNode('baz')->end()
* ->append($this->getBarNodeDefinition())
* ->end()
* ;
*
* @return $this
*/
public function append(NodeDefinition $node);

/**
* Sets a custom children builder.
*/
public function setBuilder(NodeBuilder $builder);
}
15 changes: 12 additions & 3 deletions 15 src/Symfony/Component/Config/Definition/NodeInterface.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,10 @@

namespace Symfony\Component\Config\Definition;

use Symfony\Component\Config\Definition\Exception\Exception;
use Symfony\Component\Config\Definition\Exception\ForbiddenOverwriteException;
use Symfony\Component\Config\Definition\Exception\InvalidConfigurationException;

/**
* Common Interface among all nodes.
*
Expand Down Expand Up @@ -59,9 +63,9 @@ public function hasDefaultValue();
public function getDefaultValue();

/**
* Normalizes the supplied value.
* Normalizes a value.
*
* @param mixed $value The value to normalize
* @param mixed $value Value to normalize
Copy link
Contributor

Choose a reason for hiding this comment

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

maybe A value to normalize ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

*
* @return mixed The normalized value
*/
Expand All @@ -73,7 +77,9 @@ public function normalize($value);
* @param mixed $leftSide
* @param mixed $rightSide
*
* @return mixed The merged values
* @return mixed The merged value
*
* @throws ForbiddenOverwriteException
*/
public function merge($leftSide, $rightSide);

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

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Now updated.

* @throws InvalidConfigurationException
*/
public function finalize($value);
}
3 changes: 0 additions & 3 deletions 3 src/Symfony/Component/Config/Definition/VariableNode.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,9 +27,6 @@ class VariableNode extends BaseNode implements PrototypeNodeInterface
protected $defaultValue;
protected $allowEmptyValue = true;

/**
* {@inheritdoc}
*/
public function setDefaultValue($value)
{
$this->defaultValueSet = true;
Expand Down
Morty Proxy This is a proxified and sanitized view of the page, visit original site.