10000 [Config] Improve the deprecation features by handling package and version by atailouloute · Pull Request #35871 · symfony/symfony · GitHub
[go: up one dir, main page]

Skip to content

[Config] Improve the deprecation features by handling package and version #35871

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
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
7 changes: 7 additions & 0 deletions UPGRADE-5.1.md
Original file line number Diff line number Diff line change
@@ -1,6 +1,13 @@
UPGRADE FROM 5.0 to 5.1
=======================

Config
------

* The signature of method `NodeDefinition::setDeprecated()` has been updated to `NodeDefinition::setDeprecation(string $package, string $version, string $message)`.
* The signature of method `BaseNode::setDeprecated()` has been updated to `BaseNode::setDeprecation(string $package, string $version, string $message)`.
* Passing a null message to `BaseNode::setDeprecated()` to un-deprecate a node is deprecated

Console
-------

Expand Down
7 changes: 7 additions & 0 deletions UPGRADE-6.0.md
Original file line number Diff line number Diff line change
@@ -1,6 +1,13 @@
UPGRADE FROM 5.x to 6.0
=======================

Config
------

* The signature of method `NodeDefinition::setDeprecated()` has been updated to `NodeDefinition::setDeprecation(string $package, string $version, string $message)`.
* The signature of method `BaseNode::setDeprecated()` has been updated to `BaseNode::setDeprecation(string $package, string $version, string $message)`.
* Passing a null message to `BaseNode::setDeprecated()` to un-deprecate a node is not supported anymore.

Console
-------

Expand Down
7 changes: 7 additions & 0 deletions src/Symfony/Component/Config/CHANGELOG.md
Original file line number Diff line number Diff line change
@@ -1,6 +1,13 @@
CHANGELOG
=========

5.1.0
-----

* updated the signature of method `NodeDefinition::setDeprecated()` to `NodeDefinition::setDeprecation(string $package, string $version, string $message)`
* updated the signature of method `BaseNode::setDeprecated()` to `BaseNode::setDeprecation(string $package, string $version, string $message)`
* deprecated passing a null message to `BaseNode::setDeprecated()` to un-deprecate a node

5.0.0
-----

Expand Down
3 changes: 2 additions & 1 deletion src/Symfony/Component/Config/Definition/ArrayNode.php
Original file line number Diff line number Diff line change
Expand Up @@ -227,7 +227,8 @@ protected function finalizeValue($value)
}

if ($child->isDeprecated()) {
trigger_deprecation('', '', $child->getDeprecationMessage($name, $this->getPath()));
$deprecation = $child->getDeprecation($name, $this->getPath());
trigger_deprecation($deprecation['package'], $deprecation['version'], $deprecation['message']);
}

try {
Expand Down
56 changes: 51 additions & 5 deletions src/Symfony/Component/Config/Definition/BaseNode.php
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,7 @@ abstract class BaseNode implements NodeInterface
protected $finalValidationClosures = [];
protected $allowOverwrite = true;
protected $required = false;
protected $deprecationMessage = null;
protected $deprecation = [];
protected $equivalentValues = [];
protected $attributes = [];
protected $pathSeparator;
Expand Down Expand Up @@ -198,12 +198,41 @@ public function setRequired(bool $boolean)
/**
* Sets this node as deprecated.
*
* @param string $package The name of the composer package that is triggering the deprecation
* @param string $version The version of the package that introduced the deprecation
* @param string $message The deprecation message to use
*
* You can use %node% and %path% placeholders in your message to display,
* respectively, the node name and its complete path.
*/
public function setDeprecated(?string $message)
public function setDeprecated(?string $package/*, string $version, string $message = 'The child node "%node%" at path "%path%" is deprecated.' */)
Copy link
Contributor

Choose a reason for hiding this comment

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

Adding the parameters at the front makes it impossible to both support 5.1+ and <5.1 as a bundle.

If you use the intended 5.1+ way:
->setDeprecated("my/bundle", "1.0", "this node is deprecated, do sth else")

  • in 5.1+ we will show the message "this node is deprecated, do sth else"
  • in <5.1 we will show the message "my/bundle"

Is there a (non-hacky) way to have the real message in both versions?

@nicolas-grekas @atailouloute

Copy link
Contributor

Choose a reason for hiding this comment

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

I just created #37284 to track this

{
$this->deprecationMessage = $message;
$args = \func_get_args();

if (\func_num_args() < 2) {
trigger_deprecation('symfony/config', '5.1', 'The signature of method "%s()" requires 3 arguments: "string $package, string $version, string $message", not defining them is deprecated.', __METHOD__);

if (!isset($args[0])) {
trigger_deprecation('symfony/config', '5.1', 'Passing a null message to un-deprecate a node is deprecated.');

$this->deprecation = [];

return;
}

$message = (string) $args[0];
$package = $version = '';
} else {
$package = (string) $args[0];
$version = (string) $args[1];
$message = (string) ($args[2] ?? 'The child node "%node%" at path "%path%" is deprecated.');
}

$this->deprecation = [
'package' => $package,
'version' => $version,
'message' => $message,
];
}

/**
Expand Down Expand Up @@ -249,7 +278,7 @@ public function isRequired()
*/
public function isDeprecated()
{
return null !== $this->deprecationMessage;
return (bool) $this->deprecation;
}

/**
Expand All @@ -259,10 +288,27 @@ public function isDeprecated()
* @param string $path the path of the node
*
* @return string
*
* @deprecated since Symfony 5.1, use "getDeprecation()" instead.
*/
public function getDeprecationMessage(string $node, string $path)
{
return strtr($this->deprecationMessage, ['%node%' => $node, '%path%' => $path]);
trigger_deprecation('symfony/config', '5.1', 'The "%s()" method is deprecated, use "getDeprecation()" instead.', __METHOD__);

return $this->getDeprecation($node, $path)['message'];
}

/**
* @param string $node The configuration node name
* @param string $path The path of the node
*/
public function getDeprecation(string $node, string $path): array
{
return [
'package' => $this->deprecation['package'] ?? '',
'version' => $this->deprecation['version'] ?? '',
'message' => strtr($this->deprecation['message'] ?? '', ['%node%' => $node, '%path%' => $path]),
];
}

/**
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -435,10 +435,13 @@ protected function createNode()
$node->addEquivalentValue(false, $this->falseEquivalent);
$node->setPerformDeepMerging($this->performDeepMerging);
$node->setRequired($this->required);
$node->setDeprecated($this->deprecationMessage);
$node->setIgnoreExtraKeys($this->ignoreExtraKeys, $this->removeExtraKeys);
$node->setNormalizeKeys($this->normalizeKeys);

if ($this->deprecation) {
$node->setDeprecated($this->deprecation['package'], $this->deprecation['version'], $this->deprecation['message']);
}

if (null !== $this->normalization) {
$node->setNormalizationClosures($this->normalization->before);
$node->setXmlRemappings($this->normalization->remappings);
Expand Down
27 changes: 24 additions & 3 deletions src/Symfony/Component/Config/Definition/Builder/NodeDefinition.php
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@ abstract class NodeDefinition implements NodeParentInterface
protected $defaultValue;
protected $default = false;
protected $required = false;
protected $deprecationMessage = null;
protected $deprecation = [];
protected $merge;
protected $allowEmptyValue = true;
protected $nullEquivalent;
Expand Down Expand Up @@ -159,14 +159,35 @@ public function isRequired()
/**
* Sets the node as deprecated.
*
* @param string $package The name of the composer package that is triggering the deprecation
* @param string $version The version of the package that introduced the deprecation
* @param string $message The deprecation message to use
*
* You can use %node% and %path% placeholders in your message to display,
* respectively, the node name and its complete path.
*
* @return $this
*/
public function setDeprecated(string $message = 'The child node "%node%" at path "%path%" is deprecated.')
public function setDeprecated(/* string $package, string $version, string $message = 'The child node "%node%" at path "%path%" is deprecated.' */)
{
$this->deprecationMessage = $message;
$args = \func_get_args();

if (\func_num_args() < 2) {
trigger_deprecation('symfony/config', '5.1', 'The signature of method "%s()" requires 3 arguments: "string $package, string $version, string $message", not defining them is deprecated.', __METHOD__);

$message = $args[0] ?? 'The child node "%node%" at path "%path%" is deprecated.';
$package = $version = '';
} else {
$package = (string) $args[0];
$version = (string) $args[1];
$message = (string) ($args[2] ?? 'The child node "%node%" at path "%path%" is deprecated.');
}

$this->deprecation = [
'package' => $package,
'version' => $version,
'message' => $message,
];

return $this;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -54,7 +54,10 @@ protected function createNode()
$node->addEquivalentValue(true, $this->trueEquivalent);
$node->addEquivalentValue(false, $this->falseEquivalent);
$node->setRequired($this->required);
$node->setDeprecated($this->deprecationMessage);

if ($this->deprecation) {
$node->setDeprecated($this->deprecation['package'], $this->deprecation['version'], $this->deprecation['message']);
}

if (null !== $this->validation) {
$node->setFinalValidationClosures($this->validation->rules);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -148,7 +148,8 @@ private function writeNode(NodeInterface $node, int $depth = 0, bool $root = fal
}

if ($child->isDeprecated()) {
$comments[] = sprintf('Deprecated (%s)', $child->getDeprecationMessage($child->getName(), $node->getPath()));
$deprecation = $child->getDeprecation($child->getName(), $node->getPath());
$comments[] = sprintf('Deprecated (%s)', ($deprecation['package'] || $deprecation['version'] ? "Since {$deprecation['package']} {$deprecation['version']}: " : '').$deprecation['message']);
}

if ($child instanceof EnumNode) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -120,7 +120,8 @@ private function writeNode(NodeInterface $node, NodeInterface $parentNode = null

// deprecated?
if ($node->isDeprecated()) {
$comments[] = sprintf('Deprecated (%s)', $node->getDeprecationMessage($node->getName(), $parentNode ? $parentNode->getPath() : $node->getPath()));
$deprecation = $node->getDeprecation($node->getName(), $parentNode ? $parentNode->getPath() : $node->getPath());
$comments[] = sprintf('Deprecated (%s)', ($deprecation['package'] || $deprecation['version'] ? "Since {$deprecation['package']} {$deprecation['version']}: " : '').$deprecation['message']);
}

// example
Expand Down
41 changes: 39 additions & 2 deletions src/Symfony/Component/Config/Tests/Definition/ArrayNodeTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -12,12 +12,15 @@
namespace Symfony\Component\Config\Tests\Definition;

use PHPUnit\Framework\TestCase;
use Symfony\Bridge\PhpUnit\ExpectDeprecationTrait;
use Symfony\Component\Config\Definition\ArrayNode;
use Symfony\Component\Config\Definition\Exception\InvalidConfigurationException;
use Symfony\Component\Config\Definition\ScalarNode;

class ArrayNodeTest extends TestCase
{
use ExpectDeprecationTrait;

public function testNormalizeThrowsExceptionWhenFalseIsNotAllowed()
{
$this->expectException('Symfony\Component\Config\Definition\Exception\InvalidTypeException');
Expand Down Expand Up @@ -227,10 +230,13 @@ public function testGetDefaultValueWithoutDefaultValue()
public function testSetDeprecated()
{
$childNode = new ArrayNode('foo');
$childNode->setDeprecated('"%node%" is deprecated');
$childNode->setDeprecated('vendor/package', '1.1', '"%node%" is deprecated');

$this->assertTrue($childNode->isDeprecated());
$this->assertSame('"foo" is deprecated', $childNode->getDeprecationMessage($childNode->getName(), $childNode->getPath()));
$deprecation = $childNode->getDeprecation($childNode->getName(), $childNode->getPath());
$this->assertSame('"foo" is deprecated', $deprecation['message']);
$this->assertSame('vendor/package', $deprecation['package']);
$this->assertSame('1.1', $deprecation['version']);

$node = new ArrayNode('root');
$node->addChild($childNode);
Expand All @@ -256,6 +262,37 @@ public function testSetDeprecated()
$this->assertTrue($deprecationTriggered, '->finalize() should trigger if the deprecated node is set');
}

/**
* @group legacy
*/
public function testUnDeprecateANode()
{
$this->expectDeprecation('Since symfony/config 5.1: The signature of method "Symfony\Component\Config\Definition\BaseNode::setDeprecated()" requires 3 arguments: "string $package, string $version, string $message", not defining them is deprecated.');
$this->expectDeprecation('Since symfony/config 5.1: Passing a null message to un-deprecate a node is deprecated.');

$node = new ArrayNode('foo');
$node->setDeprecated('"%node%" is deprecated');
$node->setDeprecated(null);

$this->assertFalse($node->isDeprecated());
}

/**
* @group legacy
*/
public function testSetDeprecatedWithoutPackageAndVersion()
{
$this->expectDeprecation('Since symfony/config 5.1: The signature of method "Symfony\Component\Config\Definition\BaseNode::setDeprecated()" requires 3 arguments: "string $package, string $version, string $message", not defining them is deprecated.');

$node = new ArrayNode('foo');
$node->setDeprecated('"%node%" is deprecated');

$deprecation = $node->getDeprecation($node->getName(), $node->getPath());
$this->assertSame('"foo" is deprecated', $deprecation['message']);
$this->assertSame('', $deprecation['package']);
$this->assertSame('', $deprecation['version']);
}

/**
* @dataProvider getDataWithIncludedExtraKeys
*/
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@
namespace Symfony\Component\Config\Tests\Definition\Builder;

use PHPUnit\Framework\TestCase;
use Symfony\Bridge\PhpUnit\ExpectDeprecationTrait;
use Symfony\Component\Config\Definition\Builder\ArrayNodeDefinition;
use Symfony\Component\Config\Definition\Builder\BooleanNodeDefinition;
use Symfony\Component\Config\Definition\Builder\NodeDefinition;
Expand All @@ -21,6 +22,8 @@

class ArrayNodeDefinitionTest extends TestCase
{
use ExpectDeprecationTrait;

public function testAppendingSomeNode()
{
$parent = new ArrayNodeDefinition('root');
Expand Down Expand Up @@ -332,13 +335,37 @@ public function testSetDeprecated()
$node = new ArrayNodeDefinition('root');
$node
->children()
->arrayNode('foo')->setDeprecated('The "%path%" node is deprecated.')->end()
->arrayNode('foo')->setDeprecated('vendor/package', '1.1', 'The "%path%" node is deprecated.')->end()
->end()
;
$deprecatedNode = $node->getNode()->getChildren()['foo'];

$this->assertTrue($deprecatedNode->isDeprecated());
$deprecation = $deprecatedNode->getDeprecation($deprecatedNode->getName(), $deprecatedNode->getPath());
$this->assertSame('The "root.foo" node is deprecated.', $deprecation['message']);
$this->assertSame('vendor/package', $deprecation['package']);
$this->assertSame('1.1', $deprecation['version']);
}

/**
* @group legacy
*/
public function testSetDeprecatedWithoutPackageAndVersion()
{
$this->expectDeprecation('Since symfony/config 5.1: The signature of method "Symfony\Component\Config\Definition\Builder\NodeDefinition::setDeprecated()" requires 3 arguments: "string $package, string $version, string $message", not defining them is deprecated.');
$node = new ArrayNodeDefinition('root');
$node
->children()
->arrayNode('foo')->setDeprecated('The "%path%" node is deprecated.')->end()
->end()
;
$deprecatedNode = $node->getNode()->getChildren()['foo'];

$this->assertTrue($deprecatedNode->isDeprecated());
$this->assertSame('The "root.foo" node is deprecated.', $deprecatedNode->getDeprecationMessage($deprecatedNode->getName(), $deprecatedNode->getPath()));
$deprecation = $deprecatedNode->getDeprecation($deprecatedNode->getName(), $deprecatedNode->getPath());
$this->assertSame('The "root.foo" node is deprecated.', $deprecation['message']);
$this->assertSame('', $deprecation['package']);
$this->assertSame('', $deprecation['version']);
}

public function testCannotBeEmptyOnConcreteNode()
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -27,11 +27,14 @@ public function testCannotBeEmptyThrowsAnException()
public function testSetDeprecated()
{
$def = new BooleanNodeDefinition('foo');
$def->setDeprecated('The "%path%" node is deprecated.');
$def->setDeprecated('vendor/package', '1.1', 'The "%path%" node is deprecated.');

$node = $def->getNode();

$this->assertTrue($node->isDeprecated());
$this->assertSame('The "foo" node is deprecated.', $node->getDeprecationMessage($node->getName(), $node->getPath()));
$deprecation = $node->getDeprecation($node->getName(), $node->getPath());
$this->assertSame('The "foo" node is deprecated.', $deprecation['message']);
$this->assertSame('vendor/package', $deprecation['package']);
$this->assertSame('1.1', $deprecation['version']);
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -63,11 +63,14 @@ public function testSetDeprecated()
{
$def = new EnumNodeDefinition('foo');
$def->values(['foo', 'bar']);
$def->setDeprecated('The "%path%" node is deprecated.');
$def->setDeprecated('vendor/package', '1.1', 'The "%path%" node is deprecated.');

$node = $def->getNode();

$this->assertTrue($node->isDeprecated());
$this->assertSame('The "foo" node is deprecated.', $def->getNode()->getDeprecationMessage($node->getName(), $node->getPath()));
$deprecation = $def->getNode()->getDeprecation($node->getName(), $node->getPath());
$this->assertSame('The "foo" node is deprecated.', $deprecation['message']);
$this->assertSame('vendor/package', $deprecation['package']);
$this->assertSame('1.1', $deprecation['version']);
}
}
Loading
0