ronantakizawa/codereview-bench huggingface dataset:
Repository: symfony/symfony
File: src/Symfony/Component/DependencyInjection/Attribute/AsTagDecorator.php (PHP)

Code before:

/*
 * This file is part of the Symfony package.
 *
 * (c) Fabien Potencier <fabien@symfony.com>
 *
 * For the full copyright and license information, please view the LICENSE
 * file that was distributed with this source code.
 */

namespace Symfony\Component\DependencyInjection\Attribute;

use Symfony\Component\DependencyInjection\ContainerInterface;

/**
 * Declares a class as a decorator of all services with a specific tag.
 *
 * @author Mathias Arlaud <mathias.arlaud@gmail.com>
 */
#[\Attribute(\Attribute::TARGET_CLASS)]
class AsTagDecorator
{
    /**
     * @param string $tag       The tag name to decorate
     * @param int    $priority  The priority of this decoration when multiple decorators are declared for the same tag
     * @param int    $onInvalid The behavior to adopt when no services have the tag; must be one of the {@see ContainerInterface} constants
     */
    public function __construct(
        public string $tag,
        public int $priority = 0,
        public int $onInvalid = ContainerInterface::EXCEPTION_ON_INVALID_REFERENCE,
    ) {
    }
}


Review comment (suggestion):
would this work?
```suggestion
     * @param ContainerInterface::*_REFERENCE $onInvalid
```

Code after:

/*
 * This file is part of the Symfony package.
 *
 * (c) Fabien Potencier <fabien@symfony.com>
 *
 * For the full copyright and license information, please view the LICENSE
 * file that was distributed with this source code.
 */

namespace Symfony\Component\DependencyInjection\Attribute;

use Symfony\Component\DependencyInjection\ContainerInterface;

/**
 * Declares a class as a decorator of all services with a specific tag.
 *
 * @author Mathias Arlaud <mathias.arlaud@gmail.com>
 */
#[\Attribute(\Attribute::TARGET_CLASS)]
class AsTagDecorator
{
    /**
     * @param string                          $tag       The tag name to decorate
     * @param int                             $priority  The priority of this decoration when multiple decorators are declared for the same tag
     * @param ContainerInterface::*_REFERENCE $onInvalid The behavior to adopt when no services have the tag; must be one of the {@see ContainerInterface} constants
     */
    public function __construct(
        public string $tag,
        public int $priority = 0,
        public int $onInvalid = ContainerInterface::EXCEPTION_ON_INVALID_REFERENCE,
    ) {
    }
}
Repository: symfony/symfony
File: src/Symfony/Component/Yaml/Tests/YamlTestSuiteTest.php (PHP)

Code before:
    }

    public static function yamlTestSuiteProvider(): array
    {
        if (!file_exists(__DIR__.\DIRECTORY_SEPARATOR.'yaml-test-suite')) {
            self::markTestSkipped('The YAML test suite is not available.');
        }

        $dataDir = __DIR__.\DIRECTORY_SEPARATOR.'yaml-test-suite';
        $shortcodeDirs = glob($dataDir.\DIRECTORY_SEPARATOR.'*', \GLOB_ONLYDIR);

        $testDirs = [];
        foreach ($shortcodeDirs as $shortcodeDir) {
            $shortcode = basename($shortcodeDir);

            // ignore metadata directories referencing other test cases by name
            if (\in_array($shortcode, ['name', 'tags'], true)) {
                continue;
            }

            if (file_exists($shortcodeDir.\DIRECTORY_SEPARATOR.'in.yaml')) {
                $testDirs[] = self::createTestItem($shortcode, $shortcodeDir);
            } else {
                $subDirs = glob($shortcodeDir.\DIRECTORY_SEPARATOR.'*', \GLOB_ONLYDIR);
                foreach ($subDirs as $subDir) {
                    if (file_exists($subDir.\DIRECTORY_SEPARATOR.'in.yaml')) {
                        $shortcodeSub = $shortcode.'-'.basename($subDir);
                        $testDirs[] = self::createTestItem($shortcodeSub, $subDir);
                    }
                }
            }
        }

        return $testDirs;
    }

    private static function createTestItem(string $shortcode, string $dir): array
    {
        $isError = file_exists($dir.\DIRECTORY_SEPARATOR.'error');
        $json = file_exists($dir.\DIRECTORY_SEPARATOR.'in.json') ? json_decode(file_get_contents($dir.\DIRECTORY_SEPARATOR.'in.json'), true) : null;
        $testName = is_file($dir.\DIRECTORY_SEPARATOR.'===') ? trim(file_get_contents($dir.\DIRECTORY_SEPARATOR.'===')) : $shortcode;

        return [$testName, $shortcode, $dir, $isError, $json];
    }
}


Review comment (suggestion):
I suggest using `/` in the string instead of `\DIRECTORY_SEPARATOR` to make the code more readaible. This is accepted by the PHP filesystem APIs and we don't perform string-based comparisons of paths here.

Code after:
        '7Z25',
        '87E4',
        '8G76',
        '93JH',
        '98YD',
        '9BXH',
        '9C9N',
        '9DXL',
        '9JBA',
        '9KAX',
        '9MAG',
        '9MMA',
        '9MMW',
        '9MQT-01',
        '9WXW',
        '9YRD',
        'A2M4',
        'AVM7',
        'B63P',
        'BEC7',
        'BF9H',
        'BS4K',
        'BU8L',
        'C4HZ',
        'CC74',
        'CFD4',
        'CML9',
        'CN3R',
        'CTN5',
        'CUP7',
        'CVW2',
        'CXX2',
        'DFF7',
        'DK4H',
        'DK95-00',
        'DK95-01',
        'DK95-03',
        'DK95-04',
        'DK95-07',
        'DWX9',
        'E76Z',
        'EB22',
        'EHF6',
        'F2C7',
        'FH7J',
        'FRK4',
        'G5U8',
        'G992',
        'GH63',
        'H7TQ',
        'HMQ5',
Repository: symfony/symfony
File: src/Symfony/Component/Yaml/Tests/YamlTestSuiteTest.php (PHP)

Code before:
        'PRH3',
        'Q8AD',
        'QT73',
        'R4YG',
        'RTP8',
        'SBG9',
        'SM9W-01',
        'T4YY',
        'T5N4',
        'TL85',
        'UGM3',
        'UKK6-00',
        'Z67P',
        'ZH7C',
    ];

    #[DataProvider('yamlTestSuiteProvider')]
    public function testYamlTestSuite(string $testName, string $shortcode, string $file, bool $isError, mixed $expected)
    {
        if ('' === $file) {
            $this->markTestSkipped('The YAML test suite is not available: '.$testName);
        }

        if (\in_array($shortcode, self::SKIP, true)) {
            $this->markTestSkipped('Test case "'.$testName.'" ('.$shortcode.') is skipped.');
        }

        $isErrorExpected = false;
        if ($isError) {
            $isErrorExpected = true;
            $this->expectException(ParseException::class);
        }

        $data = Yaml::parseFile($file);

        if (!$isErrorExpected) {
            $this->assertEquals($expected, $data, 'Test case "'.$testName.'" ('.$shortcode.') failed.');
        }
    }

    public static function yamlTestSuiteProvider(): \Generator
    {
        $dataDir = self::getTestSuitePath();

        if (null === $dataDir) {
            yield 'yaml-test-suite-missing' => ['YAML Test Suite Missing', '', '', false, null];

            return;
        }

        $shortcodeDirs = glob($dataDir.'/*', \GLOB_ONLYDIR);

Review comment (suggestion):
We tend to prefer `sprintf()` usage instead of long string concatenation (it's easier to read and to update because we don't have to deal with quotes etc.)

Code after:
        'PRH3',
        'Q8AD',
        'QT73',
        'R4YG',
        'RTP8',
        'SBG9',
        'SM9W-01',
        'T4YY',
        'T5N4',
        'TL85',
        'UGM3',
        'UKK6-00',
        'Z67P',
        'ZH7C',
    ];

    #[DataProvider('yamlTestSuiteProvider')]
    public function testYamlTestSuite(string $testName, string $shortcode, string $file, bool $isErrorExpected, mixed $expected)
    {
        if ('' === $file) {
            $this->markTestSkipped(\sprintf('The YAML test suite is not available: %s', $testName));
        }

        if (\in_array($shortcode, self::SKIP, true)) {
            $this->markTestSkipped(\sprintf('Test case "%s" (%s) is skipped.', $testName, $shortcode));
        }

        if ($isErrorExpected) {
            $this->expectException(ParseException::class);
        }

        $data = Yaml::parseFile($file);

        if (!$isErrorExpected) {
            $this->assertEquals($expected, $data, \sprintf('Test case "%s" (%s) failed.', $testName, $shortcode));
        }
    }

    public static function yamlTestSuiteProvider(): \Generator
    {
        $dataDir = self::getTestSuitePath();

        if (null === $dataDir) {
            yield 'yaml-test-suite-missing' => ['YAML Test Suite Missing', '', '', false, null];

            return;
        }

        $shortcodeDirs = glob($dataDir.'/*', \GLOB_ONLYDIR);

        if (!$shortcodeDirs) {
Repository: symfony/symfony
File: src/Symfony/Component/Validator/Constraints/Existence.php (PHP)

Code before:
<?php

/*
 * This file is part of the Symfony package.
 *
 * (c) Fabien Potencier <fabien@symfony.com>
 *
 * For the full copyright and license information, please view the LICENSE
 * file that was distributed with this source code.
 */

namespace Symfony\Component\Validator\Constraints;

use Symfony\Component\Validator\Constraint;

/**
 * @author Bernhard Schussek <bschussek@gmail.com>
 */
abstract class Existence extends Composite
{
    public array|Constraint $constraints = [];

    public function __construct(mixed $constraints = null, ?array $groups = null, mixed $payload = null)
    {
        if (!$constraints instanceof Constraint && !\is_array($constraints) || \is_array($constraints) && !array_is_list($constraints)) {
            parent::__construct($constraints, $groups, $payload);
        } else {
            $this->constraints = $constraints;

            parent::__construct(null, $groups, $payload);
        }
    }

    /**
     * @deprecated since Symfony 7.4
     */
    public function getDefaultOption(): ?string
    {
        if (0 === \func_num_args() || func_get_arg(0)) {
            trigger_deprecation('symfony/validator', '7.4', 'The %s() method is deprecated.', __METHOD__);
        }

        return 'constraints';
    }

    protected function getCompositeOption(): string
    {
        return 'constraints';
    }

Review comment (suggestion):
we deprecated support for constraints having a constructor without the `#[HasNamedArguments]` attribute in 7.3, but we added the constructor here only in 7.4 when we deprecated passing option arrays to the base constraint

Code after:
<?php

/*
 * This file is part of the Symfony package.
 *
 * (c) Fabien Potencier <fabien@symfony.com>
 *
 * For the full copyright and license information, please view the LICENSE
 * file that was distributed with this source code.
 */

namespace Symfony\Component\Validator\Constraints;

use Symfony\Component\Validator\Attribute\HasNamedArguments;
use Symfony\Component\Validator\Constraint;

/**
 * @author Bernhard Schussek <bschussek@gmail.com>
 */
abstract class Existence extends Composite
{
    public array|Constraint $constraints = [];

    #[HasNamedArguments]
    public function __construct(mixed $constraints = null, ?array $groups = null, mixed $payload = null)
    {
        if (!$constraints instanceof Constraint && !\is_array($constraints) || \is_array($constraints) && !array_is_list($constraints)) {
            parent::__construct($constraints, $groups, $payload);
        } else {
            $this->constraints = $constraints;

            parent::__construct(null, $groups, $payload);
        }
    }

    /**
     * @deprecated since Symfony 7.4
     */
    public function getDefaultOption(): ?string
    {
        if (0 === \func_num_args() || func_get_arg(0)) {
            trigger_deprecation('symfony/validator', '7.4', 'The %s() method is deprecated.', __METHOD__);
        }

        return 'constraints';
    }

    protected function getCompositeOption(): string
    {
Repository: symfony/symfony
File: src/Symfony/Component/Form/Test/TypeTestCase.php (PHP)

Code before:
 */

namespace Symfony\Component\Form\Test;

use Symfony\Component\EventDispatcher\EventDispatcher;
use Symfony\Component\EventDispatcher\EventDispatcherInterface;
use Symfony\Component\Form\FormBuilder;
use Symfony\Component\Form\Test\Traits\ValidatorExtensionTrait;

abstract class TypeTestCase extends FormIntegrationTestCase
{
    /**
     * @var FormBuilder
     */
    protected $builder;

    /**
     * @var EventDispatcherInterface
     */
    protected $dispatcher;

    protected function setUp(): void
    {
        parent::setUp();

        $this->dispatcher = new EventDispatcher();
        $this->builder = new FormBuilder('', null, $this->dispatcher, $this->factory);
    }

    protected function getExtensions()
    {
        $extensions = [];

        if (\in_array(ValidatorExtensionTrait::class, class_uses($this))) {
            $extensions[] = $this->getValidatorExtension();
        }

        return $extensions;
    }

    public static function assertDateTimeEquals(\DateTime $expected, \DateTime $actual)
    {
        self::assertEquals($expected->format('c'), $actual->format('c'));
    }

    public static function assertDateIntervalEquals(\DateInterval $expected, \DateInterval $actual)
    {
        self::assertEquals($expected->format('%RP%yY%mM%dDT%hH%iM%sS'), $actual->format('%RP%yY%mM%dDT%hH%iM%sS'));
    }
}


Review comment (suggestion):
Same here, the dispatcher is protected so someone could have added expectation on the dispatcher in his tests

Code after:
 */

namespace Symfony\Component\Form\Test;

use Symfony\Component\EventDispatcher\EventDispatcherInterface;
use Symfony\Component\Form\FormBuilder;
use Symfony\Component\Form\Test\Traits\ValidatorExtensionTrait;

abstract class TypeTestCase extends FormIntegrationTestCase
{
    /**
     * @var FormBuilder
     */
    protected $builder;

    /**
     * @var EventDispatcherInterface
     */
    protected $dispatcher;

    protected function setUp(): void
    {
        parent::setUp();

        if (!isset($this->dispatcher)) {
            $this->dispatcher = $this->createMock(EventDispatcherInterface::class);
        }
        $this->builder = new FormBuilder('', null, $this->dispatcher, $this->factory);
    }

    protected function getExtensions()
    {
        $extensions = [];

        if (\in_array(ValidatorExtensionTrait::class, class_uses($this))) {
            $extensions[] = $this->getValidatorExtension();
        }

        return $extensions;
    }

    public static function assertDateTimeEquals(\DateTime $expected, \DateTime $actual)
    {
        self::assertEquals($expected->format('c'), $actual->format('c'));
    }

    public static function assertDateIntervalEquals(\DateInterval $expected, \DateInterval $actual)
    {
        self::assertEquals($expected->format('%RP%yY%mM%dDT%hH%iM%sS'), $actual->format('%RP%yY%mM%dDT%hH%iM%sS'));
    }
}
Repository: symfony/symfony
File: src/Symfony/Component/Cache/CacheItem.php (PHP)

Code before:
    private function unpack(): bool
    {
        $v = $this->value;
        $valueWrapper = self::VALUE_WRAPPER;

        if ($v instanceof $valueWrapper) {
            $this->value = $v->value;
            $this->metadata = $v->metadata;

            return true;
        }

        if (!\is_array($v) || 1 !== \count($v) || 10 !== \strlen($k = (string) array_key_first($v)) || "\x9D" !== $k[0] || "\0" !== $k[5] || "\x5F" !== $k[9]) {
            return false;
        }

        // BC with pools populated before v6.1
        $this->value = $v[$k];
        $this->metadata = unpack('Vexpiry/Nctime', substr($k, 1, -1));
        $this->metadata['expiry'] += self::METADATA_EXPIRY_OFFSET;

        return true;
    }
}

// @php-cs-fixer-ignore protected_to_private This class, while final, exposes protected properties when serialized.


Review comment (suggestion):
this class is not means to be serialized ;)
```suggestion
// @php-cs-fixer-ignore protected_to_private Friend-level scope access relies on protected properties
```

Code after:
    private function unpack(): bool
    {
        $v = $this->value;
        $valueWrapper = self::VALUE_WRAPPER;

        if ($v instanceof $valueWrapper) {
            $this->value = $v->value;
            $this->metadata = $v->metadata;

            return true;
        }

        if (!\is_array($v) || 1 !== \count($v) || 10 !== \strlen($k = (string) array_key_first($v)) || "\x9D" !== $k[0] || "\0" !== $k[5] || "\x5F" !== $k[9]) {
            return false;
        }

        // BC with pools populated before v6.1
        $this->value = $v[$k];
        $this->metadata = unpack('Vexpiry/Nctime', substr($k, 1, -1));
        $this->metadata['expiry'] += self::METADATA_EXPIRY_OFFSET;

        return true;
    }
}

// @php-cs-fixer-ignore protected_to_private Friend-level scope access relies on protected properties
Repository: symfony/symfony
File: src/Symfony/Component/Finder/Finder.php (PHP)

Code before:
        }

        foreach ($this->iterators as $it) {
            $iterator->append($it);
        }

        if ($this->sort || $this->reverseSorting) {
            $iterator = (new Iterator\SortableIterator($iterator, $this->sort, $this->reverseSorting))->getIterator();
        }

        return $iterator;
    }

    /**
     * Appends an existing set of files/directories to the finder.
     *
     * The set can be another Finder, an Iterator, an IteratorAggregate, or even a plain array.
     *
     * @param iterable<string, SplFileInfo|\SplFileInfo|string> $iterator
     *
     * @return $this
     */
    public function append(iterable $iterator): static
    {
        $it = new \ArrayIterator();
        foreach ($iterator as $file) {
            if ($file instanceof SplFileInfo) {
                $it[$file->getPathname()] = $file;
            } elseif ($file instanceof \SplFileInfo) {
                $it[$file->getPathname()] = new SplFileInfo(
                    $file->getPathname(),
                    $file->getPath(),
                    $file->getPathname(),
                );
            } else {
                $file = new \SplFileInfo($file);
                $it[$file->getPathname()] = new SplFileInfo(
                    $file->getPathname(),
                    $file->getPath(),
                    $file->getPathname(),
                );
            }
        }
        $this->iterators[] = $it;

        return $this;
    }

    /**
     * Check if any results were found.
     */

Review comment (performance):
should we optimize the case where the argument being appended is a Finder, to avoid iterating over it eagerly ?

Code after:
                            $file = new \SplFileInfo($file);
                        }
                        $key = $file->getPathname();
                        if (!$file instanceof SplFileInfo) {
                            $file = new SplFileInfo($key, $file->getPath(), $key);
                        }

                        yield $key => $file;
                    }
                })());
            }
        }

        if ($this->sort || $this->reverseSorting) {
            $iterator = (new SortableIterator($iterator, $this->sort, $this->reverseSorting))->getIterator();
        }

        return $iterator;
    }

    /**
     * Appends an existing set of files/directories to the finder.
     *
     * The set can be another Finder, an Iterator, an IteratorAggregate, or even a plain array.
     *
     * @param iterable<SplFileInfo|\SplFileInfo|string> $iterator
     *
     * @return $this
     */
    public function append(iterable $iterator): static
    {
        $this->iterators[] = $iterator;

        return $this;
    }

    /**
     * Check if any results were found.
     */
    public function hasResults(): bool
    {
        foreach ($this->getIterator() as $_) {
            return true;
        }

        return false;
    }

    /**
     * Counts all the results collected by the iterators.
     */
Repository: symfony/symfony
File: src/Symfony/Component/HttpFoundation/ParameterBag.php (PHP)

Code before:
use Symfony\Component\HttpFoundation\Exception\BadRequestException;
use Symfony\Component\HttpFoundation\Exception\UnexpectedValueException;

/**
 * ParameterBag is a container for key/value pairs.
 *
 * @author Fabien Potencier <fabien@symfony.com>
 *
 * @implements \IteratorAggregate<string, mixed>
 */
class ParameterBag implements \IteratorAggregate, \Countable
{
    /**
     * @param array<string, mixed> $parameters
     */
    public function __construct(
        protected array $parameters = [],
    ) {
    }

    /**
     * Returns the parameters.
     *
     * @param string|null $key The name of the parameter to return or null to get them all
     *
     * @return array<string, mixed>
     *
     * @throws BadRequestException if the value is not an array
     */
    public function all(?string $key = null): array
    {
        if (null === $key) {
            return $this->parameters;
        }

        if (!\is_array($value = $this->parameters[$key] ?? [])) {
            throw new BadRequestException(\sprintf('Unexpected value for parameter "%s": expecting "array", got "%s".', $key, get_debug_type($value)));
        }

        return $value;
    }

    /**
     * Returns the parameter keys.
     *
     * @return array<int, string>
     */
    public function keys(): array
    {
        return array_keys($this->parameters);
    }

Review comment (suggestion):
this is not true. When passing a key, the returned array might not be indexed by strings. This would require a dynamic return type to distinguish the 2 cases.

Code after:
use Symfony\Component\HttpFoundation\Exception\BadRequestException;
use Symfony\Component\HttpFoundation\Exception\UnexpectedValueException;

/**
 * ParameterBag is a container for key/value pairs.
 *
 * @author Fabien Potencier <fabien@symfony.com>
 *
 * @implements \IteratorAggregate<string, mixed>
 */
class ParameterBag implements \IteratorAggregate, \Countable
{
    /**
     * @param array<string, mixed> $parameters
     */
    public function __construct(
        protected array $parameters = [],
    ) {
    }

    /**
     * Returns the parameters.
     *
     * @template TKey of string|null
     *
     * @param TKey $key The name of the parameter to return or null to get them all
     *
     * @return (TKey is null ? array<string, mixed> : array<mixed>)
     *
     * @throws BadRequestException if the value is not an array
     */
    public function all(?string $key = null): array
    {
        if (null === $key) {
            return $this->parameters;
        }

        if (!\is_array($value = $this->parameters[$key] ?? [])) {
            throw new BadRequestException(\sprintf('Unexpected value for parameter "%s": expecting "array", got "%s".', $key, get_debug_type($value)));
        }

        return $value;
    }

    /**
     * Returns the parameter keys.
     *
     * @return list<string>
     */
    public function keys(): array
    {