Skip to content

[Console] Additional validation of mode in InputArgument and InputOption #52055

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

Draft
wants to merge 4 commits into
base: 7.4
Choose a base branch
from
Draft

[Console] Additional validation of mode in InputArgument and InputOption #52055

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
b69c2eb
Disallow setting both REQUIRED and OPTIONAL on InputArgument
jnoordsij Oct 13, 2023
fbd2554
Disallow setting combinations of NONE, REQUIRED and OPTIONAL on Input…
jnoordsij Oct 13, 2023
251b242
Add entries to CHANGELOG and UPGRADE-6.4 for deprecated modes on Inpu…
jnoordsij Oct 13, 2023
2262675
Improve codestyle for mode check in InputOption
jnoordsij Oct 15, 2023
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
6 changes: 6 additions & 0 deletions UPGRADE-6.4.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,12 @@ Cache

* [BC break] `EarlyExpirationHandler` no longer implements `MessageHandlerInterface`, rely on `AsMessageHandler` instead

Console
-----

* Deprecate passing both `InputArgument::REQUIRED` and `InputArgument::OPTIONAL` modes to `InputArgument` constructor
* Deprecate passing more than one out of `InputOption::VALUE_NONE`, `InputOption::VALUE_REQUIRED` and `InputOption::VALUE_OPTIONAL` modes to `InputOption` constructor

DependencyInjection
-------------------

Expand Down
2 changes: 2 additions & 0 deletions src/Symfony/Component/Console/CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,8 @@ CHANGELOG
* The application can also catch errors with `Application::setCatchErrors(true)`
* Add `RunCommandMessage` and `RunCommandMessageHandler`
* Dispatch `ConsoleTerminateEvent` after an exit on signal handling and add `ConsoleTerminateEvent::getInterruptingSignal()`
* Deprecate passing both `InputArgument::REQUIRED` and `InputArgument::OPTIONAL` modes to `InputArgument` constructor
* Deprecate passing more than one out of `InputOption::VALUE_NONE`, `InputOption::VALUE_REQUIRED` and `InputOption::VALUE_OPTIONAL` modes to `InputOption` constructor

6.3
---
Expand Down
10 changes: 7 additions & 3 deletions src/Symfony/Component/Console/Input/InputArgument.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,12 +46,16 @@ class InputArgument
*/
public function __construct(string $name, int $mode = null, string $description = '', string|bool|int|float|array $default = null, \Closure|array $suggestedValues = [])
{
if (null === $mode) {
$mode = self::OPTIONAL;
} elseif ($mode > 7 || $mode < 1) {
// If not explicitly marked as required, we assume the value to be optional
$mode = self::REQUIRED === (self::REQUIRED & $mode) ? $mode : (self::OPTIONAL | $mode);
if ($mode > 7 || $mode < 1) {
throw new InvalidArgumentException(sprintf('Argument mode "%s" is not valid.', $mode));
}

if ((self::REQUIRED | self::OPTIONAL) === ((self::REQUIRED | self::OPTIONAL) & $mode)) {
trigger_deprecation('symfony/console', '6.4', 'InputArgument mode should specify either required or optional.');
}

$this->name = $name;
$this->mode = $mode;
$this->description = $description;
Expand Down
10 changes: 7 additions & 3 deletions src/Symfony/Component/Console/Input/InputOption.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -92,12 +92,16 @@ public function __construct(string $name, string|array $shortcut = null, int $mo
}
}

if (null === $mode) {
$mode = self::VALUE_NONE;
} elseif ($mode >= (self::VALUE_NEGATABLE << 1) || $mode < 1) {
// If not explicitly marked as required or optional, we assume the value accepts no input
$mode = self::VALUE_REQUIRED === (self::VALUE_REQUIRED & $mode) || self::VALUE_OPTIONAL === (self::VALUE_OPTIONAL & $mode) ? $mode : (self::VALUE_NONE | $mode);
if ($mode >= (self::VALUE_NEGATABLE << 1) || $mode < 1) {
throw new InvalidArgumentException(sprintf('Option mode "%s" is not valid.', $mode));
}

if (!in_array($mode & (self::VALUE_NONE | self::VALUE_REQUIRED | self::VALUE_OPTIONAL), [self::VALUE_NONE, self::VALUE_REQUIRED, self::VALUE_OPTIONAL], true)) {
trigger_deprecation('symfony/console', '6.4', 'InputOption mode should be either none, required or optional.');
Copy link
Member

Choose a reason for hiding this comment

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

We can add the option name to the deprecation message. That would help to find where the change need to be done when our tools don't provide the backtrace of the trigger.

}

$this->name = $name;
$this->shortcut = $shortcut;
$this->mode = $mode;
Expand Down
15 changes: 14 additions & 1 deletion src/Symfony/Component/Console/Tests/Input/InputArgumentTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@
namespace Symfony\Component\Console\Tests\Input;

use PHPUnit\Framework\TestCase;
use Symfony\Bridge\PhpUnit\ExpectDeprecationTrait;
use Symfony\Component\Console\Completion\CompletionInput;
use Symfony\Component\Console\Completion\CompletionSuggestions;
use Symfony\Component\Console\Completion\Suggestion;
Expand All @@ -20,6 +21,8 @@

class InputArgumentTest extends TestCase
{
use ExpectDeprecationTrait;

public function testConstructor()
{
$argument = new InputArgument('foo');
Expand All @@ -32,7 +35,7 @@ public function testModes()
$this->assertFalse($argument->isRequired(), '__construct() gives a "InputArgument::OPTIONAL" mode by default');

$argument = new InputArgument('foo', null);
$this->assertFalse($argument->isRequired(), '__construct() can take "InputArgument::OPTIONAL" as its mode');
$this->assertFalse($argument->isRequired(), '__construct() gives a "InputArgument::OPTIONAL" mode by default');

$argument = new InputArgument('foo', InputArgument::OPTIONAL);
$this->assertFalse($argument->isRequired(), '__construct() can take "InputArgument::OPTIONAL" as its mode');
Expand All @@ -49,6 +52,16 @@ public function testInvalidModes()
new InputArgument('foo', '-1');
}

/**
* @group legacy
*/
public function testAmbigiousRequirementSpecifierMode()
{
$this->expectDeprecation('Since symfony/console 6.4: InputArgument mode should specify either required or optional.');

new InputArgument('foo', InputArgument::OPTIONAL | InputArgument::REQUIRED);
}

public function testIsArray()
{
$argument = new InputArgument('foo', InputArgument::IS_ARRAY);
Expand Down
49 changes: 46 additions & 3 deletions src/Symfony/Component/Console/Tests/Input/InputOptionTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@
namespace Symfony\Component\Console\Tests\Input;

use PHPUnit\Framework\TestCase;
use Symfony\Bridge\PhpUnit\ExpectDeprecationTrait;
use Symfony\Component\Console\Completion\CompletionInput;
use Symfony\Component\Console\Completion\CompletionSuggestions;
use Symfony\Component\Console\Completion\Suggestion;
Expand All @@ -20,6 +21,8 @@

class InputOptionTest extends TestCase
{
use ExpectDeprecationTrait;

public function testConstructor()
{
$option = new InputOption('foo');
Expand Down Expand Up @@ -69,9 +72,9 @@ public function testModes()
$this->assertFalse($option->isValueOptional(), '__construct() gives a "InputOption::VALUE_NONE" mode by default');

$option = new InputOption('foo', 'f', null);
$this->assertFalse($option->acceptValue(), '__construct() can take "InputOption::VALUE_NONE" as its mode');
$this->assertFalse($option->isValueRequired(), '__construct() can take "InputOption::VALUE_NONE" as its mode');
$this->assertFalse($option->isValueOptional(), '__construct() can take "InputOption::VALUE_NONE" as its mode');
$this->assertFalse($option->acceptValue(), '__construct() gives a "InputOption::VALUE_NONE" mode by default');
$this->assertFalse($option->isValueRequired(), '__construct() gives a "InputOption::VALUE_NONE" mode by default');
$this->assertFalse($option->isValueOptional(), '__construct() gives a "InputOption::VALUE_NONE" mode by default');

$option = new InputOption('foo', 'f', InputOption::VALUE_NONE);
$this->assertFalse($option->acceptValue(), '__construct() can take "InputOption::VALUE_NONE" as its mode');
Expand All @@ -97,6 +100,46 @@ public function testInvalidModes()
new InputOption('foo', 'f', '-1');
}

/**
* @group legacy
*/
public function testCombiningNoneAndRequiredModeIsInvalid()
{
$this->expectDeprecation('Since symfony/console 6.4: InputOption mode should be either none, required or optional.');

new InputOption('foo', 'f', InputOption::VALUE_NONE | InputOption::VALUE_REQUIRED);
Copy link
Member

Choose a reason for hiding this comment

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

Have you considered using a data provider to reduce repetition?

}

/**
* @group legacy
*/
public function testCombiningNoneAndOptionalModeIsInvalid()
{
$this->expectDeprecation('Since symfony/console 6.4: InputOption mode should be either none, required or optional.');

new InputOption('foo', 'f', InputOption::VALUE_NONE | InputOption::VALUE_OPTIONAL);
}

/**
* @group legacy
*/
public function testCombiningRequiredAndOptionalModeIsInvalid()
{
$this->expectDeprecation('Since symfony/console 6.4: InputOption mode should be either none, required or optional.');

new InputOption('foo', 'f', InputOption::VALUE_REQUIRED | InputOption::VALUE_OPTIONAL);
}

/**
* @group legacy
*/
public function testCombiningNoneRequiredAndOptionalModeIsInvalid()
{
$this->expectDeprecation('Since symfony/console 6.4: InputOption mode should be either none, required or optional.');

new InputOption('foo', 'f', InputOption::VALUE_NONE | InputOption::VALUE_REQUIRED | InputOption::VALUE_OPTIONAL);
}

public function testEmptyNameIsInvalid()
{
$this->expectException(\InvalidArgumentException::class);
Expand Down