ronantakizawa/codereview-bench huggingface dataset:
Repository: symfony/symfony
File: src/Symfony/Component/Runtime/Internal/SymfonyErrorHandler.php (PHP)

Code before:
 * @author Nicolas Grekas <p@tchwork.com>
 *
 * @internal
 */
class SymfonyErrorHandler
{
    public static function register(bool $debug): void
    {
        if (!class_exists(ErrorHandler::class)) {
            BasicErrorHandler::register($debug);

            return;
        }

        error_reporting(-1);

        if (!\in_array(\PHP_SAPI, ['cli', 'phpdbg', 'embed'], true)) {
            ini_set('display_errors', $debug);
        } elseif (!filter_var(\ini_get('log_errors'), \FILTER_VALIDATE_BOOL) || \ini_get('error_log')) {
            // CLI - display errors only if they're not already logged to STDERR
            ini_set('display_errors', 1);
        }

        if (0 <= \ini_get('zend.assertions')) {
            ini_set('zend.assertions', 1);
            ini_set('assert.active', $debug);
            ini_set('assert.exception', 1);
        }

        if ($debug) {
            DebugClassLoader::enable();
        }

        ErrorHandler::register(new ErrorHandler(new BufferingLogger(), $debug));
    }
}


Review comment (suggestion):
inlining what `BasicErrorHandler::register()` does currently (prevents the call to `restore_error_handler();`, removed below)

Code after:
 * @author Nicolas Grekas <p@tchwork.com>
 *
 * @internal
 */
class SymfonyErrorHandler
{
    public static function register(bool $debug): void
    {
        if (!class_exists(ErrorHandler::class)) {
            BasicErrorHandler::register($debug);

            return;
        }

        error_reporting(-1);

        if (!\in_array(\PHP_SAPI, ['cli', 'phpdbg', 'embed'], true)) {
            ini_set('display_errors', $debug);
        } elseif (!filter_var(\ini_get('log_errors'), \FILTER_VALIDATE_BOOL) || \ini_get('error_log')) {
            // CLI - display errors only if they're not already logged to STDERR
            ini_set('display_errors', 1);
        }

        if (0 <= \ini_get('zend.assertions')) {
            ini_set('zend.assertions', (int) $debug);
        }
        ini_set('assert.active', 1);
        ini_set('assert.exception', 1);

        if ($debug) {
            DebugClassLoader::enable();
        }

        ErrorHandler::register(new ErrorHandler(new BufferingLogger(), $debug));
    }
}
Repository: symfony/symfony
File: src/Symfony/Component/Runtime/Internal/SymfonyErrorHandler.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\Runtime\Internal;

use Symfony\Component\ErrorHandler\BufferingLogger;
use Symfony\Component\ErrorHandler\DebugClassLoader;
use Symfony\Component\ErrorHandler\ErrorHandler;

/**
 * @author Nicolas Grekas <p@tchwork.com>
 *
 * @internal
 */
class SymfonyErrorHandler
{
    public static function register(bool $debug): void
    {
        BasicErrorHandler::register($debug);

        if (class_exists(ErrorHandler::class)) {
            DebugClassLoader::enable();
            restore_error_handler();
            ErrorHandler::register(new ErrorHandler(new BufferingLogger(), $debug));
        }
    }
}


Review comment (bug):
@nicolas-grekas why are you disabling assertions in non-debug mode ? We are relying on assertions in production  mode and this change breaks our application.

Code after:
/**
 * @author Nicolas Grekas <p@tchwork.com>
 *
 * @internal
 */
class SymfonyErrorHandler
{
    public static function register(bool $debug): void
    {
        if (!class_exists(ErrorHandler::class)) {
            BasicErrorHandler::register($debug);

            return;
        }

        error_reporting(-1);

        if (!\in_array(\PHP_SAPI, ['cli', 'phpdbg', 'embed'], true)) {
            ini_set('display_errors', $debug);
        } elseif (!filter_var(\ini_get('log_errors'), \FILTER_VALIDATE_BOOL) || \ini_get('error_log')) {
            // CLI - display errors only if they're not already logged to STDERR
            ini_set('display_errors', 1);
        }

        if (0 <= \ini_get('zend.assertions')) {
            ini_set('zend.assertions', (int) $debug);
        }
        ini_set('assert.active', 1);
        ini_set('assert.exception', 1);

        if ($debug) {
            DebugClassLoader::enable();
        }

        ErrorHandler::register(new ErrorHandler(new BufferingLogger(), $debug));
    }
}
Repository: symfony/symfony
File: src/Symfony/Component/Messenger/Worker.php (PHP)

Code before:

        $this->eventDispatcher?->dispatch(new WorkerRateLimitedEvent($rateLimiter, $transportName));
        $rateLimiter->reserve()->wait();
    }

    private function flush(bool $force): bool
    {
        $unacks = $this->unacks;

        if (!$unacks->count()) {
            return false;
        }

        $this->unacks = new \SplObjectStorage();

        while ($unacks->valid()) {
            $batchHandler = $unacks->current();
            [$envelope, $transportName] = $unacks[$batchHandler];
            try {
                $this->bus->dispatch($envelope->with(new FlushBatchHandlersStamp($force)));
                $envelope = $envelope->withoutAll(NoAutoAckStamp::class);
            } catch (\Throwable $e) {
                $this->acks[] = [$transportName, $envelope, $e];
            }
            $unacks->next();
            $unacks->detach($batchHandler);
        }

        return $this->ack();
    }

    public function stop(): void
    {
        $this->logger?->info('Stopping worker.', ['transport_names' => $this->metadata->getTransportNames()]);

        $this->shouldStop = true;
    }

    public function getMetadata(): WorkerMetadata
    {
        return $this->metadata;
    }
}


Review comment (performance):
This means that we now don't call the destructor of the $batchHandler (if any) inside the try/catch.
Is that the fix or a regression?

Code after:
        $rateLimiter->reserve()->wait();
        $rateLimiter->consume();
    }

    private function flush(bool $force): bool
    {
        $unacks = $this->unacks;

        if (!$unacks->count()) {
            return false;
        }

        $this->unacks = new \SplObjectStorage();

        foreach ($unacks as $batchHandler) {
            [$envelope, $transportName, $acked] = $unacks[$batchHandler];
            try {
                $e = null;
                $this->bus->dispatch($envelope->with(new FlushBatchHandlersStamp($force)));
            } catch (\Throwable $e) {
                $envelope = $envelope->withoutAll(NoAutoAckStamp::class);
                $this->acks[] = [$transportName, $envelope, $e];
                continue;
            }

            $noAutoAckStamp = $envelope->last(NoAutoAckStamp::class);

            if (!$acked && !$noAutoAckStamp) {
                $this->acks[] = [$transportName, $envelope, $e];
            } elseif ($noAutoAckStamp) {
                $this->unacks[$noAutoAckStamp->getHandlerDescriptor()->getBatchHandler()] = [$envelope->withoutAll(AckStamp::class), $transportName, &$acked];
            }
        }

        return $this->ack();
    }

    public function stop(): void
    {
        $this->logger?->info('Stopping worker.', ['transport_names' => $this->metadata->getTransportNames()]);

        $this->shouldStop = true;
    }

    public function getMetadata(): WorkerMetadata
    {
        return $this->metadata;
    }
}
Repository: symfony/symfony
File: src/Symfony/Component/HttpFoundation/ServerEvent.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\HttpFoundation;

/**
 * A server event to send as part of the SSE streaming technique.
 *
 * @author Yonel Ceruto <open@yceruto.dev>
 */
class ServerEvent
{
    /**
     * @param string|iterable<string> $data    The event data field for the message
     * @param string|null             $type    The event type
     * @param int                     $retry   The event reconnection time in milliseconds
     * @param string|null             $id      The event ID to set the EventSource object's last event ID value
     * @param string|null             $comment The event comment
     */
    public function __construct(
        private string|iterable $data,
        private ?string $type = null,
        private int $retry = 0,
        private ?string $id = null,
        private ?string $comment = null,
    ) {
    }

    public function getData(): iterable|string
    {
        return $this->data;
    }

    /**
     * @return $this
     */
    public function setData(iterable|string $data): static

Review comment (suggestion):
```suggestion
class ServerSideEvent
```

ServerSideEvent makes the intent clear that it represents an SSE payload.

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\HttpFoundation;

/**
 * An event generated on the server intended for streaming to the client
 * as part of the SSE streaming technique.
 *
 * @implements \IteratorAggregate<string>
 *
 * @author Yonel Ceruto <open@yceruto.dev>
 */
class ServerEvent implements \IteratorAggregate
{
    /**
     * @param string|iterable<string> $data    The event data field for the message
     * @param string|null             $type    The event type
     * @param int|null                $retry   The number of milliseconds the client should wait
     *                                         before reconnecting in case of network failure
     * @param string|null             $id      The event ID to set the EventSource object's last event ID value
     * @param string|null             $comment The event comment
     */
    public function __construct(
        private string|iterable $data,
        private ?string $type = null,
        private ?int $retry = null,
        private ?string $id = null,
        private ?string $comment = null,
    ) {
    }

    public function getData(): iterable|string
    {
        return $this->data;
    }
Repository: symfony/symfony
File: src/Symfony/Component/HttpFoundation/EventStreamResponse.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\HttpFoundation;

/**
 * Represents a streaming HTTP response for sending server events
 * as part of the Server-Sent Events (SSE) streaming technique.
 *
 * @see ServerEvent
 *
 * @author Yonel Ceruto <open@yceruto.dev>
 *
 * Example usage:
 *
 *     return new EventStreamResponse(function () {
 *         while (true) {
 *             yield new ServerEvent(time(), type: 'ping');
 *
 *             if (connection_aborted()) {
 *                 break;
 *             }
 *
 *             sleep(1);
 *         }
 *     });
 */
class EventStreamResponse extends StreamedResponse
{
    /**
     * @param int $retry The event reconnection time in milliseconds
     */
    public function __construct(?callable $callback = null, int $status = 200, array $headers = [], private int $retry = 0)

Review comment (suggestion):
```suggestion
 * as part of the Server-Sent Events (SSE) streaming technique.
 *
 * To broadcast events to multiple users at once, for long-running connections and for high-traffic websites, prefer using the Mercure Symfony Component, which relies on software designed for these use cases: https://symfony.com/doc/current/mercure.html
```


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\HttpFoundation;

/**
 * Represents a streaming HTTP response for sending server events
 * as part of the Server-Sent Events (SSE) streaming technique.
 *
 * To broadcast events to multiple users at once, for long-running
 * connections and for high-traffic websites, prefer using the Mercure
 * Symfony Component, which relies on Software designed for these use
 * cases: https://symfony.com/doc/current/mercure.html
 *
 * @see ServerEvent
 *
 * @author Yonel Ceruto <open@yceruto.dev>
 *
 * Example usage:
 *
 *     return new EventStreamResponse(function () {
 *         yield new ServerEvent(time());
 *
 *         sleep(1);
 *
 *         yield new ServerEvent(time());
 *     });
 */
class EventStreamResponse extends StreamedResponse
{
    /**
     * @param int|null $retry The number of milliseconds the client should wait
     *                        before reconnecting in case of network failure
Repository: symfony/symfony
File: src/Symfony/Component/HttpFoundation/ServerEvent.php (PHP)

Code before:

    /**
     * @return $this
     */
    public function setId(string $id): static
    {
        $this->id = $id;

        return $this;
    }

    public function getComment(): ?string
    {
        return $this->comment;
    }

    public function setComment(string $comment): static
    {
        $this->comment = $comment;

        return $this;
    }

    public function __toString(): string
    {
        $event = [];

        if ($this->comment) {
            $event[] = \sprintf(': %s', $this->comment);
        }
        if ($this->id) {
            $event[] = \sprintf('id: %s', $this->id);
        }
        if ($this->retry > 0) {
            $event[] = \sprintf('retry: %s', $this->retry);
        }
        if ($this->type) {
            $event[] = \sprintf('event: %s', $this->type);
        }
        if ($this->data) {
            if (is_iterable($this->data)) {
                foreach ($this->data as $data) {
                    $event[] = \sprintf('data: %s', $data);
                }
            } else {
                $event[] = \sprintf('data: %s', $this->data);
            }
        }

        return implode("\n", $event)."\n\n";
    }

Review comment (suggestion):
On my opinion, we should provide a `stream()` method that will stream the event in the HTTP response directly instead of storing the whole content of the text to be sent in memory.

This will be especially useful for large payloads.

Code after:
    public function getId(): ?string
    {
        return $this->id;
    }

    /**
     * @return $this
     */
    public function setId(string $id): static
    {
        $this->id = $id;

        return $this;
    }

    public function getComment(): ?string
    {
        return $this->comment;
    }

    public function setComment(string $comment): static
    {
        $this->comment = $comment;

        return $this;
    }

    /**
     * @return \Traversable<string>
     */
    public function getIterator(): \Traversable
    {
        static $lastRetry = null;

        $head = '';
        if ($this->comment) {
            $head .= \sprintf(': %s', $this->comment)."\n";
        }
        if ($this->id) {
            $head .= \sprintf('id: %s', $this->id)."\n";
        }
        if ($this->retry > 0 && $this->retry !== $lastRetry) {
            $head .= \sprintf('retry: %s', $lastRetry = $this->retry)."\n";
        }
        if ($this->type) {
            $head .= \sprintf('event: %s', $this->type)."\n";
        }
        yield $head;

        if ($this->data) {
            if (is_iterable($this->data)) {
Repository: symfony/symfony
File: src/Symfony/Component/Form/Extension/Core/Type/BirthdayType.php (PHP)

Code before:
    {
        $resolver->setDefaults([
            'years' => range((int) date('Y') - 120, date('Y')),
            'invalid_message' => 'Please enter a valid birthdate.',
        ]);

        $resolver->setAllowedTypes('years', 'array');
    }

    public function getParent(): ?string
    {
        return DateType::class;
    }

    public function getBlockPrefix(): string
    {
        return 'birthday';
    }

    public function buildView(FormView $view, FormInterface $form, array $options): void
    {
        parent::buildView($view, $form, $options);
        $currentMonth = date('m');
        $currentDay = date('d');
        if (!isset($view->vars['attr']['min'])) {
            $view->vars['attr']['min'] = sprintf('%d-%s-%s', reset($options['years']), $currentMonth, $currentDay);
        }
        if (!isset($view->vars['attr']['max'])) {
            $view->vars['attr']['max'] = sprintf('%d-%s-%s', end($options['years']), $currentMonth, $currentDay);
        }
    }
}


Review comment (suggestion):
I suggest using `??=` instead of this wrapping `isset` check

Code after:
    {
        $resolver->setDefaults([
            'years' => range(date('Y') - 120, date('Y')),
            'invalid_message' => 'Please enter a valid birthdate.',
        ]);

        $resolver->setAllowedTypes('years', 'array');
    }

    public function getParent(): ?string
    {
        return DateType::class;
    }

    public function getBlockPrefix(): string
    {
        return 'birthday';
    }

    public function buildView(FormView $view, FormInterface $form, array $options): void
    {
        if ('single_text' === $options['widget']) {
            $view->vars['attr']['min'] ??= \sprintf('%d-01-01', min($options['years']));
            $view->vars['attr']['max'] ??= \sprintf('%d-12-31', max($options['years']));
        }
    }
}
Repository: symfony/symfony
File: src/Symfony/Component/Semaphore/Store/LockStore.php (PHP)

Code before:
use Symfony\Component\Semaphore\Exception\SemaphoreAcquiringException;
use Symfony\Component\Semaphore\Exception\SemaphoreExpiredException;
use Symfony\Component\Semaphore\Key;
use Symfony\Component\Semaphore\PersistingStoreInterface;

/**
 * @author Alexander Schranz <alexander@sulu.io>
 */
final class LockStore implements PersistingStoreInterface
{
    public function __construct(
        private readonly LockFactory $lockFactory,
    ) {
    }

    public function save(Key $key, float $ttlInSecond): void
    {
        $locks = $this->getExistingLocks($key);

        if ([] !== $locks) {
            return;
        }

        $locks = $this->createLocks($key, $ttlInSecond);

        $key->setState(__CLASS__, $locks);
    }

    public function delete(Key $key): void
    {
        $this->releaseLocks($this->getExistingLocks($key), $key);
    }

    public function exists(Key $key): bool
    {
        $locks = $this->getExistingLocks($key);

        if (\count($locks) === $key->getWeight()) {
            return true;
        }

        $this->releaseLocks($locks, $key);

        return false;
    }

    public function putOffExpiration(Key $key, float $ttlInSecond): void
    {
        $locks = $this->getExistingLocks($key);
        foreach ($locks as $lock) {
            if ($lock->isExpired()) {

Review comment (suggestion):
we might need to implement the `markUnserializable` (like we do for LOck) to prevent people from serializing such key.

Code after:
use Symfony\Component\Lock\LockInterface;
use Symfony\Component\Semaphore\Exception\SemaphoreAcquiringException;
use Symfony\Component\Semaphore\Exception\SemaphoreExpiredException;
use Symfony\Component\Semaphore\Exception\SemaphoreReleasingException;
use Symfony\Component\Semaphore\Key;
use Symfony\Component\Semaphore\PersistingStoreInterface;

/**
 * @author Alexander Schranz <alexander@sulu.io>
 */
final class LockStore implements PersistingStoreInterface
{
    public function __construct(
        private readonly LockFactory $lockFactory,
    ) {
    }

    public function save(Key $key, float $ttlInSecond): void
    {
        if ($this->getExistingLocks($key)) {
            return;
        }

        $locks = $this->createLocks($key, $ttlInSecond);

        $key->setState(__CLASS__, $locks);
        $key->markUnserializable();
    }

    public function delete(Key $key): void
    {
        $this->releaseLocks($this->getExistingLocks($key), $key);
    }

    public function exists(Key $key): bool
    {
        return \count($this->getExistingLocks($key)) >= $key->getWeight();
    }

    public function putOffExpiration(Key $key, float $ttlInSecond): void
    {
        $locks = $this->getExistingLocks($key);

        if (\count($locks) !== $key->getWeight()) {
            $this->releaseLocks($locks, $key);

            throw new SemaphoreExpiredException($key, 'One or multiple locks were not even acquired.');
        }

        foreach ($locks as $lock) {
            $lock->refresh($ttlInSecond);
Repository: symfony/symfony
File: src/Symfony/Component/Yaml/Tests/ParserTest.php (PHP)

Code before:
        ];

        $this->assertSame($expected, $this->parser->parse($yaml));
    }

    public function testBackslashInQuotedMultiLineString()
    {
        $yaml = <<<YAML
foobar: "foo
    bar\\\\"
YAML;
        $expected = [
            'foobar' => 'foo bar\\',
        ];

        $this->assertSame($expected, $this->parser->parse($yaml));
    }

    /**
     * @dataProvider WrappedUnquotedStrings
     */
    public function testWrappedUnquotedStringWithMultipleSpacesInValue(string $yaml, array $expected)
    {
        $this->assertSame($expected, $this->parser->parse($yaml));
    }
    public static function WrappedUnquotedStrings() {
        return [
            'CurlyBraces' => [
                '{ foo: bar  bar, fiz: cat      cat }',
                [
                    'foo' => 'bar  bar',
                    'fiz' => 'cat      cat',
                ]
            ],
            'SquareBraces' => [
                '[ bar  bar, cat      cat ]',
                [
                    'bar  bar',
                    'cat      cat',
                ]
            ],
        ];
    }

    public function testParseMultiLineUnquotedString()
    {
        $yaml = <<<EOT
foo: bar
  baz
   foobar
  foo

Review comment (suggestion):
```suggestion
    public static function wrappedUnquotedStringsProvider()
    {
```

Code after:
        ];

        $this->assertSame($expected, $this->parser->parse($yaml));
    }

    public function testBackslashInQuotedMultiLineString()
    {
        $yaml = <<<YAML
foobar: "foo
    bar\\\\"
YAML;
        $expected = [
            'foobar' => 'foo bar\\',
        ];

        $this->assertSame($expected, $this->parser->parse($yaml));
    }

    /**
     * @dataProvider wrappedUnquotedStringsProvider
     */
    public function testWrappedUnquotedStringWithMultipleSpacesInValue(string $yaml, array $expected)
    {
        $this->assertSame($expected, $this->parser->parse($yaml));
    }

    public static function wrappedUnquotedStringsProvider() {
        return [
            'mapping' => [
                '{ foo: bar  bar, fiz: cat      cat }',
                [
                    'foo' => 'bar  bar',
                    'fiz' => 'cat      cat',
                ]
            ],
            'sequence' => [
                '[ bar  bar, cat      cat ]',
                [
                    'bar  bar',
                    'cat      cat',
                ]
            ],
        ];
    }

    public function testParseMultiLineUnquotedString()
    {
        $yaml = <<<EOT
foo: bar
  baz
   foobar