Conversation
📝 WalkthroughWalkthroughReplaces XML-based Symfony DI in UiBundle with PHP configurators: adds PHP service files, removes corresponding XML files, updates the bundle extension to use PhpFileLoader, and updates the composer-require-checker whitelist to include Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
❌ Preview Environment deleted from BunnyshellAvailable commands:
|
73b89b5 to
51e3d4d
Compare
|
|
||
| return static function (ContainerConfigurator $container) { | ||
| $services = $container->services(); | ||
| $parameters = $container->parameters(); |
There was a problem hiding this comment.
| $parameters = $container->parameters(); |
|
|
||
| return static function (ContainerConfigurator $container) { | ||
| $services = $container->services(); | ||
| $parameters = $container->parameters(); |
There was a problem hiding this comment.
| $parameters = $container->parameters(); |
| $parameters = $container->parameters(); | ||
|
|
||
| $services->defaults() | ||
| ->public(); |
There was a problem hiding this comment.
| $parameters = $container->parameters(); | |
| $services->defaults() | |
| ->public(); |
| service('twig'), | ||
| service('security.authorization_checker'), | ||
| service('router'), | ||
| ]); |
There was a problem hiding this comment.
| ]); | |
| ]) | |
| ->public(); |
|
|
||
| return static function (ContainerConfigurator $container) { | ||
| $services = $container->services(); | ||
| $parameters = $container->parameters(); |
There was a problem hiding this comment.
| $parameters = $container->parameters(); |
| $services->set('sylius.form.type.security_login', SecurityLoginType::class) | ||
| ->tag('form.type'); |
There was a problem hiding this comment.
| $services->set('sylius.form.type.security_login', SecurityLoginType::class) | |
| ->tag('form.type'); | |
| $services->set('sylius.form.type.security_login', SecurityLoginType::class)->tag('form.type'); |
|
|
||
| return static function (ContainerConfigurator $container) { | ||
| $services = $container->services(); | ||
| $parameters = $container->parameters(); |
There was a problem hiding this comment.
| $parameters = $container->parameters(); |
| $services = $container->services(); | ||
| $parameters = $container->parameters(); | ||
| $container->import('services/controller.php'); | ||
| $container->import('services/form.php'); | ||
| $container->import('services/twig.php'); | ||
| $container->import('services/twig/**/*.php'); |
There was a problem hiding this comment.
| $services = $container->services(); | |
| $parameters = $container->parameters(); | |
| $container->import('services/controller.php'); | |
| $container->import('services/form.php'); | |
| $container->import('services/twig.php'); | |
| $container->import('services/twig/**/*.php'); | |
| $container->import('services/controller.php'); | |
| $container->import('services/form.php'); | |
| $container->import('services/twig.php'); | |
| $container->import('services/twig/**/*.php'); |
96b9e46 to
ce2d703
Compare
ce2d703 to
9aeee9e
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@src/Sylius/Bundle/UiBundle/Resources/config/services.php`:
- Around line 16-21: The configurator closure currently lacks an explicit return
type; update the anonymous function declaration that takes ContainerConfigurator
$container (the closure in services.php) to declare a void return type by adding
": void" to the function signature so the closure becomes "function
(ContainerConfigurator $container): void { ... }", keeping all existing imports
(services/controller.php, services/form.php, services/twig.php,
services/twig/**/*.php) unchanged.
In
`@src/Sylius/Bundle/UiBundle/Resources/config/services/twig/error_renderer.php`:
- Around line 18-28: The closure passed to the service configuration currently
lacks a return type; update the anonymous function signature that takes
ContainerConfigurator $container (the configurator closure creating $services
and setting 'sylius.twig.error_renderer' with TwigErrorRenderer::class and
decorating 'twig.error_renderer.html') to declare a void return type by adding
": void" to the function signature so it complies with the coding guidelines for
Symfony service configurators.
| return static function (ContainerConfigurator $container) { | ||
| $container->import('services/controller.php'); | ||
| $container->import('services/form.php'); | ||
| $container->import('services/twig.php'); | ||
| $container->import('services/twig/**/*.php'); | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
🧩 Analysis chain
🏁 Script executed:
# Check if file exists and examine the relevant lines
if [ -f "src/Sylius/Bundle/UiBundle/Resources/config/services.php" ]; then
echo "=== File exists ==="
cat -n "src/Sylius/Bundle/UiBundle/Resources/config/services.php" | head -30
else
echo "File does not exist"
fiRepository: Sylius/Sylius
Length of output: 790
🌐 Web query:
Symfony ContainerConfigurator closure return type void PHP 8.2
💡 Result:
In Symfony’s PHP service config files (e.g. config/services.php, config/packages/*.php), the file should return a closure that Symfony will execute to configure the container. That closure doesn’t need to return anything, so on PHP 8.2 you can (and in newer docs, often do) declare it as : void. [1]
<?php
// config/services.php (or config/packages/something.php)
namespace Symfony\Component\DependencyInjection\Loader\Configurator;
return static function (ContainerConfigurator $container): void {
$services = $container->services();
// configure services...
};If you want the current environment injected, add string $env (and still : void): [1]
return static function (ContainerConfigurator $container, string $env): void {
// ...
};Key point: the file returns the closure; the closure itself should return nothing (void), i.e., don’t return $container from inside it. [1]
Sources: [1] Symfony Docs – Service Container (PHP closure config + : void example).
Add an explicit void return type to the configurator closure.
The closure must declare its return type per coding guidelines. Symfony DI configuration closures return nothing and should explicitly declare : void.
♻️ Proposed change
-return static function (ContainerConfigurator $container) {
+return static function (ContainerConfigurator $container): void {🤖 Prompt for AI Agents
In `@src/Sylius/Bundle/UiBundle/Resources/config/services.php` around lines 16 -
21, The configurator closure currently lacks an explicit return type; update the
anonymous function declaration that takes ContainerConfigurator $container (the
closure in services.php) to declare a void return type by adding ": void" to the
function signature so the closure becomes "function (ContainerConfigurator
$container): void { ... }", keeping all existing imports
(services/controller.php, services/form.php, services/twig.php,
services/twig/**/*.php) unchanged.
| return static function (ContainerConfigurator $container) { | ||
| $services = $container->services(); | ||
|
|
||
| $services->set('sylius.twig.error_renderer', TwigErrorRenderer::class) | ||
| ->decorate('twig.error_renderer.html', null, 64) | ||
| ->args([ | ||
| service('.inner'), | ||
| service('twig'), | ||
| tagged_iterator('sylius.twig.error_template_finder'), | ||
| '%kernel.debug%', | ||
| ]); |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Symfony ContainerConfigurator closure return type void PHP 8.2
💡 Result:
In Symfony’s PHP service config files (e.g. config/services.php), the file returns a closure, but that closure itself should return void (it configures the container; it doesn’t return config data). Symfony’s docs show the closure with an explicit : void return type, which is fully compatible with PHP 8.2. [1]
<?php
namespace Symfony\Component\DependencyInjection\Loader\Configurator;
use Symfony\Component\DependencyInjection\Loader\Configurator\ContainerConfigurator;
return static function (ContainerConfigurator $containerConfigurator): void {
// configure services/parameters here
};If you also want the current environment injected, add the $env argument (still : void): [1]
return static function (ContainerConfigurator $containerConfigurator, string $env): void {
// ...
};Sources: [1] Symfony Docs – Service Container (PHP configurator examples showing : void).
🏁 Script executed:
fd 'error_renderer.php' src/Sylius/Bundle/UiBundle/Resources/config/services/twig/ --type fRepository: Sylius/Sylius
Length of output: 133
🏁 Script executed:
cat -n src/Sylius/Bundle/UiBundle/Resources/config/services/twig/error_renderer.phpRepository: Sylius/Sylius
Length of output: 1059
Add an explicit void return type to the configurator closure.
The closure on line 18 is missing the return type declaration required by the coding guidelines. Symfony's service configurator closures should declare : void since they configure the container without returning data.
♻️ Proposed change
-return static function (ContainerConfigurator $container) {
+return static function (ContainerConfigurator $container): void {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| return static function (ContainerConfigurator $container) { | |
| $services = $container->services(); | |
| $services->set('sylius.twig.error_renderer', TwigErrorRenderer::class) | |
| ->decorate('twig.error_renderer.html', null, 64) | |
| ->args([ | |
| service('.inner'), | |
| service('twig'), | |
| tagged_iterator('sylius.twig.error_template_finder'), | |
| '%kernel.debug%', | |
| ]); | |
| return static function (ContainerConfigurator $container): void { | |
| $services = $container->services(); | |
| $services->set('sylius.twig.error_renderer', TwigErrorRenderer::class) | |
| ->decorate('twig.error_renderer.html', null, 64) | |
| ->args([ | |
| service('.inner'), | |
| service('twig'), | |
| tagged_iterator('sylius.twig.error_template_finder'), | |
| '%kernel.debug%', | |
| ]); |
🤖 Prompt for AI Agents
In `@src/Sylius/Bundle/UiBundle/Resources/config/services/twig/error_renderer.php`
around lines 18 - 28, The closure passed to the service configuration currently
lacks a return type; update the anonymous function signature that takes
ContainerConfigurator $container (the configurator closure creating $services
and setting 'sylius.twig.error_renderer' with TwigErrorRenderer::class and
decorating 'twig.error_renderer.html') to declare a void return type by adding
": void" to the function signature so it complies with the coding guidelines for
Symfony service configurators.
Summary by CodeRabbit