ronantakizawa/codereview-bench huggingface dataset:
Repository: symfony/symfony
File: src/Symfony/Component/HttpKernel/HttpKernelBrowser.php (PHP)

Code before:
                        $value->getClientOriginalName(),
                        $value->getClientMimeType(),
                        $value->getError(),
                        true
                    );
                }
            }
        }

        return $filtered;
    }

    /**
     * @param Response $response
     */
    protected function filterResponse(object $response): DomResponse
    {
        // this is needed to support StreamedResponse
        $content = '';
        ob_start(static function (string $chunk) use (&$content): string {
            $content .= $chunk;

            return $chunk;
        });
        $response->sendContent();
        ob_end_clean();

        return new DomResponse($content, $response->getStatusCode(), $response->headers->all());
    }
}


Review comment (suggestion):
I really think `ob_end_clean();` should be in a `finally` block.

Code after:
                        $value->getClientOriginalName(),
                        $value->getClientMimeType(),
                        $value->getError(),
                        true
                    );
                }
            }
        }

        return $filtered;
    }

    /**
     * @param Response $response
     */
    protected function filterResponse(object $response): DomResponse
    {
        $content = '';
        ob_start(static function ($chunk) use (&$content) {
            $content .= $chunk;
        });

        try {
            $response->sendContent();
        } finally {
            ob_end_clean();
        }

        return new DomResponse($content, $response->getStatusCode(), $response->headers->all());
    }
}
Repository: symfony/symfony
File: src/Symfony/Component/HttpClient/HttplugClient.php (PHP)

Code before:
            } else {
                $body = static function (int $size) use ($body) {
                    if ($body->isSeekable()) {
                        try {
                            $body->seek(0);
                        } catch (\RuntimeException) {
                            // ignore
                        }
                    }

                    while (!$body->eof()) {
                        yield $body->read($size);
                    }
                };
            }

            $options = [
                'headers' => $headers,
                'body' => $body,
                'buffer' => $buffer,
            ];

            if ($this->honorRequestHttpVersion) {
                $options['http_version'] = $request->getProtocolVersion();
            } elseif ('1.0' === $request->getProtocolVersion() && null === $this->honorRequestHttpVersion) {
                $options['http_version'] = '1.0';
            }

            return $this->client->request($request->getMethod(), (string) $request->getUri(), $options);
        } catch (\InvalidArgumentException $e) {
            throw new RequestException($e->getMessage(), $request, $e);
        } catch (TransportExceptionInterface $e) {
            throw new NetworkException($e->getMessage(), $request, $e);
        }
    }
}


Review comment (suggestion):
```suggestion
            if (!$this->autoUpgradeHttpVersion || '1.0' === $request->getProtocolVersion()) {
                $options['http_version'] = $request->getProtocolVersion();
```

Code after:
                    if ($body->isSeekable()) {
                        try {
                            $body->seek(0);
                        } catch (\RuntimeException) {
                            // ignore
                        }
                    }

                    while (!$body->eof()) {
                        yield $body->read($size);
                    }
                };
            }

            $options = [
                'headers' => $headers,
                'body' => $body,
                'buffer' => $buffer,
            ];

            if (!$this->autoUpgradeHttpVersion || '1.0' === $request->getProtocolVersion()) {
                $options['http_version'] = $request->getProtocolVersion();
            }

            return $this->client->request($request->getMethod(), (string) $request->getUri(), $options);
        } catch (\InvalidArgumentException $e) {
            throw new RequestException($e->getMessage(), $request, $e);
        } catch (TransportExceptionInterface $e) {
            throw new NetworkException($e->getMessage(), $request, $e);
        }
    }
}
Repository: symfony/symfony
File: src/Symfony/Component/DependencyInjection/Loader/Config/ServicesConfig.php (PHP)

Code before:
 *   parent?: string,
 *   shared?: bool,
 *   lazy?: bool|string,
 *   public?: bool,
 *   abstract?: bool,
 *   deprecated?: Deprecation,
 *   factory?: Callback,
 *   arguments?: Arguments,
 *   properties?: array<string, mixed>,
 *   configurator?: Callback,
 *   calls?: list<Call>,
 *   tags?: Tags,
 *   resource_tags?: Tags,
 *   autowire?: bool,
 *   autoconfigure?: bool,
 *   bind?: array<string, mixed>,
 *   constructor?: string,
 * }
 * @psalm-type Stack = array{
 *   stack: list<Definition|Alias|Prototype|array<class-string, Arguments|null>>,
 *   public?: bool,
 *   deprecated?: Deprecation,
 * }
 * @psalm-type Services = array{
 *   _defaults: Defaults,
 *   _instanceof: Instanceof,
 *   ...<string, Definition|Alias|Prototype|Stack>|array<class-string, Arguments|null>
 * }
 */
class ServicesConfig
{
    /**
     * @param Services $config
     */
    public function __construct(
        public readonly array $config,
    ) {
    }
}


Review comment (suggestion):
Optional keys:
```suggestion
 *   _defaults?: Defaults,
 *   _instanceof?: Instanceof,
```

Code after:
 *   parent?: string,
 *   shared?: bool,
 *   lazy?: bool|string,
 *   public?: bool,
 *   abstract?: bool,
 *   deprecated?: Deprecation,
 *   factory?: Callback,
 *   arguments?: Arguments,
 *   properties?: array<string, mixed>,
 *   configurator?: Callback,
 *   calls?: list<Call>,
 *   tags?: Tags,
 *   resource_tags?: Tags,
 *   autowire?: bool,
 *   autoconfigure?: bool,
 *   bind?: array<string, mixed>,
 *   constructor?: string,
 * }
 * @psalm-type Stack = array{
 *   stack: list<Definition|Alias|Prototype|array<class-string, Arguments|null>>,
 *   public?: bool,
 *   deprecated?: Deprecation,
 * }
 * @psalm-type Services = array{
 *   _defaults?: Defaults,
 *   _instanceof?: Instanceof,
 *   ...<string, Definition|Alias|Prototype|Stack|Arguments|null>
 * }
 */
class ServicesConfig
{
    /**
     * @param Services $config
     */
    public function __construct(
        public readonly array $config,
    ) {
    }
}
Repository: symfony/symfony
File: src/Symfony/Component/DependencyInjection/Loader/Config/ServicesConfig.php (PHP)

Code before:
 *   shared?: bool,
 *   lazy?: bool|string,
 *   public?: bool,
 *   abstract?: bool,
 *   deprecated?: Deprecation,
 *   factory?: Callback,
 *   arguments?: Arguments,
 *   properties?: array<string, mixed>,
 *   configurator?: Callback,
 *   calls?: list<Call>,
 *   tags?: Tags,
 *   resource_tags?: Tags,
 *   autowire?: bool,
 *   autoconfigure?: bool,
 *   bind?: array<string, mixed>,
 *   constructor?: string,
 * }
 * @psalm-type Stack = array{
 *   stack: list<Definition|Alias|Prototype|array<class-string, Arguments|null>>,
 *   public?: bool,
 *   deprecated?: Deprecation,
 * }
 * @psalm-type Services = array{
 *   _defaults: Defaults,
 *   _instanceof: Instanceof,
 *   ...<string, Definition|Alias|Prototype|Stack>|array<class-string, Arguments|null>
 * }
 */
class ServicesConfig
{
    /**
     * @param Services $config
     */
    public function __construct(
        public readonly array $config,
    ) {
    }
}


Review comment (suggestion):
Ideally that would be like this as you can mix class-string and regular service names as array key.
```
 *   ...<class-string, Arguments|null>
 *   ...<string, Definition|Alias|Prototype|Stack>
```

For now, we need to define a more relaxed type.
```suggestion
 *   ...<string, Definition|Alias|Prototype|Stack>|Arguments|null>
```

Code after:
 *   shared?: bool,
 *   lazy?: bool|string,
 *   public?: bool,
 *   abstract?: bool,
 *   deprecated?: Deprecation,
 *   factory?: Callback,
 *   arguments?: Arguments,
 *   properties?: array<string, mixed>,
 *   configurator?: Callback,
 *   calls?: list<Call>,
 *   tags?: Tags,
 *   resource_tags?: Tags,
 *   autowire?: bool,
 *   autoconfigure?: bool,
 *   bind?: array<string, mixed>,
 *   constructor?: string,
 * }
 * @psalm-type Stack = array{
 *   stack: list<Definition|Alias|Prototype|array<class-string, Arguments|null>>,
 *   public?: bool,
 *   deprecated?: Deprecation,
 * }
 * @psalm-type Services = array{
 *   _defaults?: Defaults,
 *   _instanceof?: Instanceof,
 *   ...<string, Definition|Alias|Prototype|Stack|Arguments|null>
 * }
 */
class ServicesConfig
{
    /**
     * @param Services $config
     */
    public function __construct(
        public readonly array $config,
    ) {
    }
}
Repository: symfony/symfony
File: src/Symfony/Bundle/FrameworkBundle/Kernel/MicroKernelTrait.php (PHP)

Code before:
     * Gets the path to the bundles configuration file.
     */
    private function getBundlesPath(): string
    {
        return $this->getConfigDir().'/bundles.php';
    }

    public function getCacheDir(): string
    {
        if (isset($_SERVER['APP_CACHE_DIR'])) {
            return $_SERVER['APP_CACHE_DIR'].'/'.$this->environment;
        }

        return parent::getCacheDir();
    }

    public function getBuildDir(): string
    {
        if (isset($_SERVER['APP_BUILD_DIR'])) {
            return $_SERVER['APP_BUILD_DIR'].'/'.$this->environment;
        }

        return parent::getBuildDir();
    }

    public function getLogDir(): string
    {
        return $_SERVER['APP_LOG_DIR'] ?? parent::getLogDir();
    }

    public function registerBundles(): iterable
    {
        if (!is_file($bundlesPath = $this->getBundlesPath())) {
            yield new FrameworkBundle();

            return;
        }

        $contents = require $bundlesPath;
        foreach ($contents as $class => $envs) {
            if ($envs[$this->environment] ?? $envs['all'] ?? false) {
                yield new $class();
            }
        }
    }

    public function registerContainerConfiguration(LoaderInterface $loader): void
    {
        $loader->load(function (ContainerBuilder $container) use ($loader) {
            $container->loadFromExtension('framework', [
                'router' => [

Review comment (bug):
This should be nullable. Not all applications can provide such shared dir.
Setting a value that isn't a shared directory would be incorrect.

Code after:
     * Gets the path to the bundles configuration file.
     */
    private function getBundlesPath(): string
    {
        return $this->getConfigDir().'/bundles.php';
    }

    public function getCacheDir(): string
    {
        if (isset($_SERVER['APP_CACHE_DIR'])) {
            return $_SERVER['APP_CACHE_DIR'].'/'.$this->environment;
        }

        return parent::getCacheDir();
    }

    public function getBuildDir(): string
    {
        if (isset($_SERVER['APP_BUILD_DIR'])) {
            return $_SERVER['APP_BUILD_DIR'].'/'.$this->environment;
        }

        return parent::getBuildDir();
    }

    public function getShareDir(): string
    {
        if (isset($_SERVER['APP_SHARE_DIR'])) {
            return $_SERVER['APP_SHARE_DIR'].'/'.$this->environment;
        }

        return parent::getShareDir();
    }

    public function getLogDir(): string
    {
        return $_SERVER['APP_LOG_DIR'] ?? parent::getLogDir();
    }

    public function registerBundles(): iterable
    {
        if (!is_file($bundlesPath = $this->getBundlesPath())) {
            yield new FrameworkBundle();

            return;
        }

        $contents = require $bundlesPath;
        foreach ($contents as $class => $envs) {
            if ($envs[$this->environment] ?? $envs['all'] ?? false) {
                yield new $class();
Repository: symfony/symfony
File: src/Symfony/Component/HttpFoundation/AcceptHeader.php (PHP)

Code before:
    /**
     * Returns header value's string representation.
     */
    public function __toString(): string
    {
        return implode(',', $this->items);
    }

    /**
     * Tests if header has given value.
     */
    public function has(string $value): bool
    {
        return isset($this->items[$value]) || null !== $this->get($value);
    }

    /**
     * Returns given value's item, if exists.
     */
    public function get(string $value): ?AcceptHeaderItem
    {
        $queryItem = AcceptHeaderItem::fromString($value.';q=1');
        $baseValue = $queryItem->getValue();
        $queryAttributes = $queryItem->getAttributes();
        $parametersString = $this->buildCanonicalParams($queryAttributes);
        $canonicalKey = $baseValue.($parametersString ? ';'.$parametersString : '');

        if (isset($this->items[$canonicalKey])) {
            return $this->items[$canonicalKey];
        }

        // Find all matching candidates
        $candidates = [];
        foreach ($this->items as $item) {
            if ($this->matches($item, $queryItem)) {
                $candidates[] = $item;
            }
        }

        if (!$candidates) {
            return null;
        }

        $this->sortCandidates($candidates, $queryItem);

        return $candidates[0];
    }

    /**
     * Adds an item.
     *

Review comment (suggestion):
```suggestion
        $parametersString = $this->buildCanonicalParams($queryItem->getAttributes());
        $canonicalKey = $queryItem->getValue().($parametersString ? ';'.$parametersString : '');
```

Code after:
    /**
     * Returns header value's string representation.
     */
    public function __toString(): string
    {
        return implode(',', $this->items);
    }

    /**
     * Tests if header has given value.
     */
    public function has(string $value): bool
    {
        $canonicalKey = $this->getCanonicalKey(AcceptHeaderItem::fromString($value));

        return isset($this->items[$canonicalKey]);
    }

    /**
     * Returns given value's item, if exists.
     */
    public function get(string $value): ?AcceptHeaderItem
    {
        $queryItem = AcceptHeaderItem::fromString($value.';q=1');
        $canonicalKey = $this->getCanonicalKey($queryItem);

        if (isset($this->items[$canonicalKey])) {
            return $this->items[$canonicalKey];
        }

        // Collect and filter matching candidates
        if (!$candidates = array_filter($this->items, fn (AcceptHeaderItem $item) => $this->matches($item, $queryItem))) {
            return null;
        }

        usort($candidates, fn ($a, $b) =>
            $this->getSpecificity($b, $queryItem) <=> $this->getSpecificity($a, $queryItem) // Descending specificity
            ?: $b->getQuality() <=> $a->getQuality() // Descending quality
            ?: $a->getIndex() <=> $b->getIndex() // Ascending index (stability)
        );

        return reset($candidates);
    }

    /**
     * Adds an item.
     *
     * @return $this
     */
    public function add(AcceptHeaderItem $item): static
    {
Repository: symfony/symfony
File: src/Symfony/Component/HttpFoundation/AcceptHeader.php (PHP)

Code before:
                $candidates[] = $item;
            }
        }

        if (!$candidates) {
            return null;
        }

        $this->sortCandidates($candidates, $queryItem);

        return $candidates[0];
    }

    /**
     * Adds an item.
     *
     * @return $this
     */
    public function add(AcceptHeaderItem $item): static
    {
        $baseValue = $item->getValue();
        $itemAttributes = $item->getAttributes();

        // Canonicalize parameters: sort by name, format as 'name=value' or 'name'
        $parametersString = $this->buildCanonicalParams($itemAttributes);
        $canonicalKey = $baseValue.($parametersString ? ';'.$parametersString : '');

        // Add or overwrite (though duplicates unlikely after canonicalization)
        $this->items[$canonicalKey] = $item;

        $this->sorted = false;

        return $this;
    }

    /**
     * Returns all items.
     *
     * @return AcceptHeaderItem[]
     */
    public function all(): array
    {
        $this->sort();

        return $this->items;
    }

    /**
     * Filters items on their value using given regex.
     */
    public function filter(string $pattern): self

Review comment (suggestion):
```suggestion
        // Canonicalize parameters: sort by name, format as 'name=value' or 'name'
        $parametersString = $this->buildCanonicalParams($item->getAttributes());
        $canonicalKey = $item->getValue().($parametersString ? ';'.$parametersString : '');
```

Code after:
        usort($candidates, fn ($a, $b) =>
            $this->getSpecificity($b, $queryItem) <=> $this->getSpecificity($a, $queryItem) // Descending specificity
            ?: $b->getQuality() <=> $a->getQuality() // Descending quality
            ?: $a->getIndex() <=> $b->getIndex() // Ascending index (stability)
        );

        return reset($candidates);
    }

    /**
     * Adds an item.
     *
     * @return $this
     */
    public function add(AcceptHeaderItem $item): static
    {
        $this->items[$this->getCanonicalKey($item)] = $item;
        $this->sorted = false;

        return $this;
    }

    /**
     * Returns all items.
     *
     * @return AcceptHeaderItem[]
     */
    public function all(): array
    {
        $this->sort();

        return $this->items;
    }

    /**
     * Filters items on their value using given regex.
     */
    public function filter(string $pattern): self
    {
        return new self(array_filter($this->items, static fn ($item) => preg_match($pattern, $item->getValue())));
    }

    /**
     * Returns first item.
     */
    public function first(): ?AcceptHeaderItem
    {
        $this->sort();

        return $this->items ? reset($this->items) : null;
    }
Repository: symfony/symfony
File: src/Symfony/Bundle/FrameworkBundle/Kernel/MicroKernelTrait.php (PHP)

Code before:
        }
    }

    /**
     * Adds or imports routes into your application.
     *
     *     $routes->import($this->getConfigDir().'/*.{yaml,php}');
     *     $routes
     *         ->add('admin_dashboard', '/admin')
     *         ->controller('App\Controller\AdminController::dashboard')
     *     ;
     */
    private function configureRoutes(RoutingConfigurator $routes): void
    {
        $configDir = preg_replace('{/config$}', '/{config}', $this->getConfigDir());

        $routes->import($configDir.'/{routes}/'.$this->environment.'/*.{php,yaml}');
        $routes->import($configDir.'/{routes}/*.{php,yaml}');

        if (is_file($this->getConfigDir().'/routes.yaml')) {
            $routes->import($configDir.'/routes.yaml');
        } else {
            $routes->import($configDir.'/{routes}.php');
        }

        $routes->import('routing.controllers');
    }

    /**
     * Gets the path to the configuration directory.
     */
    private function getConfigDir(): string
    {
        return $this->getProjectDir().'/config';
    }

    /**
     * Gets the path to the bundles configuration file.
     */
    private function getBundlesPath(): string
    {
        return $this->getConfigDir().'/bundles.php';
    }

    public function getCacheDir(): string
    {
        if (isset($_SERVER['APP_CACHE_DIR'])) {
            return $_SERVER['APP_CACHE_DIR'].'/'.$this->environment;
        }

        return parent::getCacheDir();

Review comment (suggestion):
the kernel being autoconfigured, it'll get the `routing.controller` tag once it uses a `#[Route]` attribute

Code after:
        }
    }

    /**
     * Adds or imports routes into your application.
     *
     *     $routes->import($this->getConfigDir().'/*.{yaml,php}');
     *     $routes
     *         ->add('admin_dashboard', '/admin')
     *         ->controller('App\Controller\AdminController::dashboard')
     *     ;
     */
    private function configureRoutes(RoutingConfigurator $routes): void
    {
        $configDir = preg_replace('{/config$}', '/{config}', $this->getConfigDir());

        $routes->import($configDir.'/{routes}/'.$this->environment.'/*.{php,yaml}');
        $routes->import($configDir.'/{routes}/*.{php,yaml}');

        $routes->import('routing.controllers');

        if (is_file($this->getConfigDir().'/routes.yaml')) {
            $routes->import($configDir.'/routes.yaml');
        } else {
            $routes->import($configDir.'/{routes}.php');
        }
    }

    /**
     * Gets the path to the configuration directory.
     */
    private function getConfigDir(): string
    {
        return $this->getProjectDir().'/config';
    }

    /**
     * Gets the path to the bundles configuration file.
     */
    private function getBundlesPath(): string
    {
        return $this->getConfigDir().'/bundles.php';
    }

    public function getCacheDir(): string
    {
        if (isset($_SERVER['APP_CACHE_DIR'])) {
            return $_SERVER['APP_CACHE_DIR'].'/'.$this->environment;
        }

        return parent::getCacheDir();
Repository: symfony/symfony
File: src/Symfony/Component/HttpKernel/DataCollector/RequestDataCollector.php (PHP)

Code before:
                $command[] = '--head';
                break;
            case Request::METHOD_GET:
                break;
            default:
                $command[] = \sprintf('--request %s', $method);
        }

        $command[] = \sprintf('--url %s', escapeshellarg($url));

        // Add headers
        foreach ($request->headers->all() as $name => $value) {
            if (\is_array($value)) {
                $value = implode(', ', $value);
            }

            // Skip certain headers
            if (\in_array(strtolower($name), ['host', 'cookie'])) {
                continue;
            }

            $command[] = '--header '.escapeshellarg(ucwords($name, '-').': '.$value);
        }

        // Add cookies
        if (!empty($request->cookies->all())) {
            $cookies = [];
            foreach ($request->cookies->all() as $name => $value) {
                $cookies[] = $name.'='.$value;
            }
            $command[] = '--header '.escapeshellarg('Cookie: '.implode('; ', $cookies));
        }

        // Add body data
        $content = $this->data['content'];
        if (!empty($content) && \in_array($method, [Request::METHOD_POST, Request::METHOD_PUT, Request::METHOD_PATCH, Request::METHOD_DELETE])) {
            $command[] = '--data-raw '.$this->escapePayload($content);
        }

        return implode(" \\\n  ", $command);
    }

    public function getCurlCommand(): string
    {
        return $this->data['curlCommand'] ?? '';
    }

    private function escapePayload(string $payload): string
    {
        static $useProcess;


Review comment (suggestion):
```suggestion
        if ($request->cookies->all()) {
```

Code after:
            }

            $command[] = '--header '.escapeshellarg(ucwords($name, '-').': '.implode(', ', $values));
        }

        if ($request->cookies->all()) {
            $cookies = [];
            foreach ($request->cookies->all() as $name => $value) {
                $cookies[] = urlencode($name).'='.urlencode($value);
            }
            $command[] = '--cookie '.escapeshellarg(implode('; ', $cookies));
        }

        if ($content && \in_array($method, [Request::METHOD_POST, Request::METHOD_PUT, Request::METHOD_PATCH, Request::METHOD_DELETE], true)) {
            $command[] = '--data-raw '.$this->escapePayload($content);
        }

        return implode(" \\\n  ", $command);
    }

    public function getCurlCommand(): string
    {
        return $this->data['curlCommand'] ?? '';
    }

    private function escapePayload(string $payload): string
    {
        static $useProcess;

        if ($useProcess ??= \function_exists('proc_open') && class_exists(Process::class)) {
            return substr((new Process(['', $payload]))->getCommandLine(), 3);
        }

        if ('\\' === \DIRECTORY_SEPARATOR) {
            return '"'.str_replace('"', '""', $payload).'"';
        }

        return "'".str_replace("'", "'\\''", $payload)."'";
    }
}