8000 [Finder] Make Comparator immutable by derrabus · Pull Request #42137 · symfony/symfony · GitHub
[go: up one dir, main page]
Skip to content

[Finder] Make Comparator immutable #42137

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

Merged
merged 1 commit into from
Jul 18, 2021
Merged

[Finder] Make Comparator immutable #42137

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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-5.4.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,12 @@ Cache

* Deprecate `DoctrineProvider` because this class has been added to the `doctrine/cache` package`

Finder
------

* Deprecate `Comparator::setTarget()` and `Comparator::setOperator()`
* Add a constructor to `Comparator` that allows setting target and operator

FrameworkBundle
---------------

Expand Down
6 changes: 6 additions & 0 deletions UPGRADE-6.0.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -60,6 +60,12 @@ EventDispatcher

* Removed `LegacyEventDispatcherProxy`. Use the event dispatcher without the proxy.

Finder
------

* Remove `Comparator::setTarget()` and `Comparator::setOperator()`
* The `$target` parameter of `Comparator::__construct()` is now mandatory

Form
----

Expand Down
6 changes: 6 additions & 0 deletions src/Symfony/Component/Finder/CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,12 @@
CHANGELOG
=========

5.4.0
-----

* Deprecate `Comparator::setTarget()` and `Comparator::setOperator()`
* Add a constructor to `Comparator` that allows setting target and operator

5.0.0
-----

Expand Down
46 changes: 36 additions & 10 deletions src/Symfony/Component/Finder/Comparator/Comparator.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,27 +12,44 @@
namespace Symfony\Component\Finder\Comparator;

/**
* Comparator.
*
* @author Fabien Potencier <fabien@symfony.com>
*/
class Comparator
{
private $target;
private $operator = '==';

public function __construct(string $target = null, string $operator = '==')
{
if (null === $target) {
trigger_deprecation('symfony/finder', '5.4', 'Constructing a "%s" without setting "$target" is deprecated.', __CLASS__);
}

$this->target = $target;
$this->doSetOperator($operator);
}

/**
* Gets the target value.
*
* @return string The target value
*/
public function getTarget()
{
if (null === $this->target) {
trigger_deprecation('symfony/finder', '5.4', 'Calling "%s" without initializing the target is deprecated.', __METHOD__);
}

return $this->target;
}

/**
* @deprecated set the target via the constructor instead
*/
public function setTarget(string $target)
{
trigger_deprecation('symfony/finder', '5.4', '"%s" is deprecated. Set the target via the constructor instead.', __METHOD__);

$this->target = $target;
}

Expand All @@ -50,18 +67,14 @@ public function getOperator()
* Sets the comparison operator.
*
* @throws \InvalidArgumentException
*
* @deprecated set the operator via the constructor instead
*/
public function setOperator(string $operator)
{
if ('' === $operator) {
$operator = '==';
}
trigger_deprecation('symfony/finder', '5.4', '"%s" is deprecated. Set the operator via the constructor instead.', __METHOD__);

if (!\in_array($operator, ['>', '<', '>=', '<=', '==', '!='])) {
throw new \InvalidArgumentException(sprintf('Invalid operator "%s".', $operator));
}

$this->operator = $operator;
$this->doSetOperator('' === $operator ? '==' : $operator);
}

/**
Expand All @@ -73,6 +86,10 @@ public function setOperator(string $operator)
*/
public function test($test)
{
if (null === $this->target) {
trigger_deprecation('symfony/finder', '5.4', 'Calling "%s" without initializing the target is deprecated.', __METHOD__);
}

switch ($this->operator) {
case '>':
return $test > $this->target;
Expand All @@ -88,4 +105,13 @@ public function test($test)

return $test == $this->target;
}

private function doSetOperator(string $operator): void
{
if (!\in_array($operator, ['>', '<', '>=', '<=', '==', '!='])) {
throw new \InvalidArgumentException(sprintf('Invalid operator "%s".', $operator));
}

$this->operator = $operator;
}
}
3 changes: 1 addition & 2 deletions src/Symfony/Component/Finder/Comparator/DateComparator.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,6 @@ public function __construct(string $test)
$operator = '<';
}

$this->setOperator($operator);
$this->setTarget($target);
parent::__construct($target, $operator);
}
}
3 changes: 1 addition & 2 deletions src/Symfony/Component/Finder/Comparator/NumberComparator.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -73,7 +73,6 @@ public function __construct(?string $test)
}
}

$this->setTarget($target);
$this->setOperator($matches[1] ?? '==');
parent::__construct($target, $matches[1] ?: '==');
Copy link
Contributor

Choose a reason for hiding this comment

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

did you change ?? to ?: on purpose?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes. setOperator() accepted an empty string and I've made the constructor stricter in that regard.

}
}
24 changes: 20 additions & 4 deletions src/Symfony/Component/Finder/Tests/Comparator/ComparatorTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,13 +12,21 @@
namespace Symfony\Component\Finder\Tests\Comparator;

use PHPUnit\Framework\TestCase;
use Symfony\Bridge\PhpUnit\ExpectDeprecationTrait;
use Symfony\Component\Finder\Comparator\Comparator;

class ComparatorTest extends TestCase
{
use ExpectDeprecationTrait;

/**
* @group legacy
*/
public function testGetSetOperator()
{
$comparator = new Comparator();
$comparator = new Comparator('some target');
Copy link
Member

Choose a reason for hiding this comment

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

We have 2 tests in one here. Those should be split (which will also allow using expectException to test the exception) instead of this old way of writing tests that does not follow the best practices.

And the test about an invalid operator should also be added for the constructor.

Copy link
Member Author

Choose a reason for hiding this comment

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

#42164


$this->expectDeprecation('Since symfony/finder 5.4: "Symfony\Component\Finder\Comparator\Comparator::setOperator" is deprecated. Set the operator via the constructor instead.');
$comparator->setOperator('>');
$this->assertEquals('>', $comparator->getOperator(), '->getOperator() returns the current operator');
}
Expand All @@ -32,9 +40,16 @@ public function testInvalidOperator()
$comparator->setOperator('foo');
}


/**
* @group legacy
*/
public function testGetSetTarget()
{
$this->expectDeprecation('Since symfony/finder 5.4: Constructing a "Symfony\Component\Finder\Comparator\Comparator" without setting "$target" is deprecated.');
$comparator = new Comparator();

$this->expectDeprecation('Since symfony/finder 5.4: "Symfony\Component\Finder\Comparator\Comparator::setTarget" is deprecated. Set the target via the constructor instead.');
$comparator->setTarget(8);
$this->assertEquals(8, $comparator->getTarget(), '->getTarget() returns the target');
}
Expand All @@ -44,9 +59,10 @@ public function testGetSetTarget()
*/
public function testTestSucceeds(string $operator, string $target, string $testedValue)
{
$c = new Comparator();
$c->setOperator($operator);
$c->setTarget($target);
$c = new Comparator($target, $operator);

$this->assertSame($target, $c->getTarget());
$this->assertSame($operator, $c->getOperator());

$this->assertTrue($c->test($testedValue));
}
Expand Down
3 changes: 2 additions & 1 deletion src/Symfony/Component/Finder/composer.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,8 @@
}
],
"require": {
"php": ">=7.2.5"
"php": ">=7.2.5",
"symfony/deprecation-contracts": "^2.1|^3"
},
"autoload": {
"psr-4": { "Symfony\\Component\\Finder\\": "" },
Expand Down
0