[Config] Backport string|null api for node names #26297
Conversation
ro0NL
commented
Feb 24, 2018
•
| Q | A |
|---|---|
| Branch? | 2.7 |
| Bug fix? | yes |
| New feature? | no |
| BC breaks? | no |
| Deprecations? | no |
| Tests pass? | yes |
| Fixed tickets | #... |
| License | MIT |
| Doc PR | symfony/symfony-docs#... |
|
For 2.7 |
|
Fixed |
| @@ -143,8 +143,8 @@ public function end() | |||
| /** | |||
| * Creates a child node. | |||
| * | |||
| * @param string $name The name of the node | |||
| * @param string $type The type of the node | |||
| * @param string|null $name The name of the node | |||
There was a problem hiding this comment.
It does not make sense to change this when you don't change all the other places as well like https://github.com/ro0NL/symfony/blob/2d72b61613131160d2f4716f71bae7a55a9f4153/src/Symfony/Component/Config/Definition/Builder/NodeDefinition.php#L39 and https://github.com/ro0NL/symfony/blob/2d72b61613131160d2f4716f71bae7a55a9f4153/src/Symfony/Component/Config/Definition/Builder/NodeBuilder.php#L52
There was a problem hiding this comment.
Also what does a node without a name do?
There was a problem hiding this comment.
Ok my bad.. it's clarified only on 4.0: https://github.com/symfony/symfony/blob/4.0/src/Symfony/Component/Config/Definition/BaseNode.php#L39
Should be backported to 2.7 as string|null AFAIK, this being the first instance :)
In general it's used to create a prototyped node: https://github.com/symfony/symfony/blob/4.0/src/Symfony/Component/Config/Definition/Builder/ArrayNodeDefinition.php#L77
I think it makes sense to keep string API when using a named factory: scalarNode('string') vs node(null, 'scalar'). Which seems to be the case already; https://github.com/symfony/symfony/blob/4.0/src/Symfony/Component/Config/Definition/Builder/NodeBuilder.php#L52
|
@Tobion im leaning to revert the last Still bugged on master: symfony/src/Symfony/Component/Config/Definition/BaseNode.php Lines 42 to 45 in cd5f410 ^ strpos on nullable ^ not sure.. maybe dragons, maybe not. (my guess is it only runs for named children). Put different im not sure about fixing those on 2.7... but backporting |
|
Status: needs review to go along with #26335, applies until 3.4 |
This PR was squashed before being merged into the 4.1-dev branch (closes #26308). Discussion ---------- [Config] Introduce BuilderAwareInterface | Q | A | ------------- | --- | Branch? | master | Bug fix? | no | New feature? | yes | BC breaks? | no <!-- see https://symfony.com/bc --> | Deprecations? | no | Tests pass? | yes <!-- please add some, will be required by reviewers --> | Fixed tickets | #... <!-- #-prefixed issue number(s), if any --> | License | MIT | Doc PR | symfony/symfony-docs#... <!-- required for new features --> Split `ParentNodeDefinitionInterface` into `BuilderAwareInterface`. Use case is custom node definition (extended from VariableNodeDef) with a corresponding prototyped array node. To set the actual prototype i need the builder at definition level, provided by `ParentNodeDefinitionInterface`. However i don't implement `children()` + `append()`, i solely need the builder scope. To go after #26297 Commits ------- 1353694 [Config] Introduce BuilderAwareInterface
|
@Tobion OK for you? |
|
Thank you @ro0NL. |
This PR was merged into the 2.7 branch. Discussion ---------- [Config] Backport string|null api for node names | Q | A | ------------- | --- | Branch? | 2.7 | Bug fix? | yes | New feature? | no | BC breaks? | no <!-- see https://symfony.com/bc --> | Deprecations? | no | Tests pass? | yes <!-- please add some, will be required by reviewers --> | Fixed tickets | #... <!-- #-prefixed issue number(s), if any --> | License | MIT | Doc PR | symfony/symfony-docs#... <!-- required for new features --> Commits ------- fe586ac [Config] Backport string|null api for node names
…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 <!-- please add some, will be required by reviewers --> | Fixed tickets | #... <!-- #-prefixed issue number(s), if any --> | License | MIT | Doc PR | symfony/symfony-docs#... <!-- required for new features --> 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