Switch from yaml to php format services definition #373
Conversation
WalkthroughMigrates Symfony DI configs from YAML to PHP (adds Changes
Sequence Diagram(s)Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (11)
💤 Files with no reviewable changes (4)
🚧 Files skipped from review as they are similar to previous changes (6)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/DependencyInjection/BazingaGeocoderExtension.php (1)
34-58: Critical: Symfony 5.4 incompatibility with ContainerConfigurator config pattern.The
config/services.phpusesContainerConfiguratorcallback style (return static function (ContainerConfigurator $container) { ... }), which is a Symfony 6.1+ feature. However,composer.jsonrequiressymfony/framework-bundle": "^5.4 || ^6.4 || ^7.0", meaning Symfony 5.4 is supported.PhpFileLoaderin Symfony 5.4 cannot properly process this configuration pattern, breaking the bundle for all 5.4 users.Either bump the minimum Symfony version to
^6.4(removing 5.4 support) or provide a YAML-based config file for 5.4 compatibility.
🧹 Nitpick comments (2)
config/services.php (1)
21-31: Avoid double-tagging dumpers (and confirm whether “public dumpers” is still required).
Right now dumpers get tagged viainstanceof(Dumper::class)here and viaregisterForAutoconfiguration(Dumper::class)insrc/DependencyInjection/BazingaGeocoderExtension.php(Line 75-76 in provided snippet). Consider keeping only one mechanism to prevent accidental duplicate tags and to keep “public service” surface minimal.return static function (ContainerConfigurator $container) { $services = $container->services(); - $services->instanceof(Dumper::class) - ->tag('bazinga_geocoder.dumper') - ->public(); + // Dumpers are tagged via BazingaGeocoderExtension::registerForAutoconfiguration(Dumper::class). + // If dumpers must be public for BC, keep `->public()` but avoid duplicating the tag. + $services->instanceof(Dumper::class) + ->public();.editorconfig (1)
11-16: Reconsidertrim_trailing_whitespace = falsefor all YAML files. The global rule applies to 40+ YAML configuration files across the test suite with no apparent need for trailing whitespace preservation. This can preserve accidental trailing spaces and create noisy diffs. If the intent is to preserve whitespace only for specific GitHub workflow patterns, scope it to.github/**/*.{yml,yaml}instead, or remove it entirely if there's no documented use case.-[*.{yml,yaml}] -trim_trailing_whitespace = false +[.github/**/*.{yml,yaml}] +trim_trailing_whitespace = false
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
.editorconfig(1 hunks)config/profiling.php(1 hunks)config/profiling.yml(0 hunks)config/services.php(1 hunks)config/services.yml(0 hunks)src/DependencyInjection/BazingaGeocoderExtension.php(2 hunks)tests/Functional/BundleInitializationTest.php(0 hunks)tests/Functional/config/service_plugin.xml(0 hunks)
💤 Files with no reviewable changes (4)
- tests/Functional/BundleInitializationTest.php
- config/profiling.yml
- tests/Functional/config/service_plugin.xml
- config/services.yml
🧰 Additional context used
🧬 Code graph analysis (2)
config/profiling.php (1)
src/DataCollector/GeocoderDataCollector.php (1)
GeocoderDataCollector(24-116)
config/services.php (3)
src/Plugin/FakeIpPlugin.php (1)
FakeIpPlugin(27-71)src/Validator/Constraint/AddressValidator.php (1)
AddressValidator(26-81)src/DependencyInjection/BazingaGeocoderExtension.php (1)
load(47-77)
🔇 Additional comments (1)
config/profiling.php (1)
12-14: VerifyGeocoderDataCollectorvisibility requirements for your Symfony baseline.
Some Symfony versions/flows are fine with private data collectors; others (or custom tooling) may expect it to be public. If anything fetches it directly (vs via tag processing), add->public().$services->set(GeocoderDataCollector::class) + // Add if required by your Symfony version / tooling: + // ->public() ->tag('data_collector', ['template' => '@BazingaGeocoder/Collector/geocoder.html.twig', 'id' => 'geocoder']);
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
config/services.php (2)
39-41: PreventFakeIpPluginfrom ever receiving a null replacement when enabled.This matches the earlier review note: with current defaults (
->args([null, null, false])) + config defaults, it’s possible to end up callingwithText(null)at runtime iffake_ip.enabled=true,fake_ip.use_faker=false, andfake_ip.ipis unset. Add config validation insrc/DependencyInjection/BazingaGeocoderExtension.phpto reject that state.
46-47: Fix command description grammar (“an IP address”).- ->tag('console.command', ['command' => 'geocoder:geocode', 'description' => 'Geocode an address or an ip address']) + ->tag('console.command', ['command' => 'geocoder:geocode', 'description' => 'Geocode an address or an IP address'])
🧹 Nitpick comments (1)
config/services.php (1)
32-36:->private()may be unnecessary / version-sensitive in PHP configurator.Services are private by default in modern Symfony; also
->private()has been a moving target across Symfony versions. Consider removing it (or switching to the supported equivalent like->public(false)if needed), and confirm against your supported Symfony matrix.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
.editorconfig(1 hunks)composer.json(1 hunks)config/profiling.php(1 hunks)config/profiling.yml(0 hunks)config/services.php(1 hunks)config/services.yml(0 hunks)src/Command/GeocodeCommand.php(1 hunks)src/DependencyInjection/BazingaGeocoderExtension.php(2 hunks)tests/Functional/BundleInitializationTest.php(0 hunks)tests/Functional/config/service_plugin.xml(0 hunks)tests/baseline-ignore(1 hunks)
💤 Files with no reviewable changes (4)
- config/profiling.yml
- tests/Functional/config/service_plugin.xml
- tests/Functional/BundleInitializationTest.php
- config/services.yml
✅ Files skipped from review due to trivial changes (2)
- src/Command/GeocodeCommand.php
- tests/baseline-ignore
🚧 Files skipped from review as they are similar to previous changes (1)
- .editorconfig
🧰 Additional context used
🧬 Code graph analysis (3)
src/DependencyInjection/BazingaGeocoderExtension.php (2)
src/BazingaGeocoderBundle.php (1)
BazingaGeocoderBundle(24-42)src/DependencyInjection/Compiler/ProfilerPass.php (1)
ProfilerPass(25-42)
config/profiling.php (2)
src/DataCollector/GeocoderDataCollector.php (1)
GeocoderDataCollector(24-116)src/DependencyInjection/Compiler/ProfilerPass.php (2)
ProfilerPass(25-42)process(30-41)
config/services.php (3)
src/Plugin/FakeIpPlugin.php (1)
FakeIpPlugin(27-71)src/Validator/Constraint/AddressValidator.php (1)
AddressValidator(26-81)src/DependencyInjection/BazingaGeocoderExtension.php (1)
load(47-77)
🔇 Additional comments (3)
composer.json (1)
88-88: The baseline file exists and is properly formatted with 91 deprecation patterns. The phpunit configuration correctly defines a "main" testsuite pointing to the tests directory. The test script configuration is valid.src/DependencyInjection/BazingaGeocoderExtension.php (1)
34-58: Confirm Symfony version compatibility forPhpFileLoader+ PHP service config files.
PhpFileLoader+services.php/profiling.phpis correct for Symfony’s PHP-based service configuration, but it’s version-sensitive—please confirm this works across the bundle’s supported Symfony versions (especially if older Symfony is still supported). Also consider adding a return type for clarity/contract:- public function load(array $configs, ContainerBuilder $container) + public function load(array $configs, ContainerBuilder $container): voidconfig/profiling.php (1)
5-14: Verify the data collector is reachable and the Twig template path is correct.Given
src/DependencyInjection/Compiler/ProfilerPass.phpwiresGeocoderDataCollectorand Symfony’s profiler later renders the collector, please confirm the collector service visibility is OK for your Symfony matrix and that@BazingaGeocoder/Collector/geocoder.html.twigresolves. If you hit “service not found/removed” optimizations in any supported Symfony version, consider making it explicit:$services->set(GeocoderDataCollector::class) + ->public() ->tag('data_collector', ['template' => '@BazingaGeocoder/Collector/geocoder.html.twig', 'id' => 'geocoder']);
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
config/services.php (1)
38-40:FakeIpPluginnull-arg safety still needs config validation (runtime type-error risk).
This wiring defaults to[null, null, false], and whenfake_ip.enabled=true+fake_ip.use_faker=false+fake_ip.ipunset,FakeIpPlugincan end up callingwithText(null). Please add validation insrc/DependencyInjection/BazingaGeocoderExtension.phpduringload()(as previously suggested).
🧹 Nitpick comments (2)
src/Command/GeocodeCommand.php (1)
53-58: Add an option description for--provider(better UX /--helpoutput).
Right now--providershows up without any description.- ->addOption('provider', null, InputOption::VALUE_OPTIONAL); + ->addOption('provider', null, InputOption::VALUE_OPTIONAL, 'Force a specific provider (by name)');config/services.php (1)
21-23: Reconsider making allDumperservices public.
If this is only to preserve historical service-fetching behavior, consider narrowing the scope (only specific dumpers) or adding explicit public aliases instead of blanketinstanceof(...)->public().
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
.editorconfig(1 hunks)composer.json(1 hunks)config/profiling.php(1 hunks)config/profiling.yml(0 hunks)config/services.php(1 hunks)config/services.yml(0 hunks)src/Command/GeocodeCommand.php(2 hunks)src/DependencyInjection/BazingaGeocoderExtension.php(2 hunks)tests/Functional/BundleInitializationTest.php(0 hunks)tests/Functional/config/service_plugin.xml(0 hunks)tests/baseline-ignore(1 hunks)
💤 Files with no reviewable changes (4)
- config/profiling.yml
- tests/Functional/config/service_plugin.xml
- config/services.yml
- tests/Functional/BundleInitializationTest.php
🚧 Files skipped from review as they are similar to previous changes (3)
- src/DependencyInjection/BazingaGeocoderExtension.php
- config/profiling.php
- .editorconfig
🧰 Additional context used
🧬 Code graph analysis (1)
config/services.php (3)
src/Plugin/FakeIpPlugin.php (1)
FakeIpPlugin(27-71)src/Validator/Constraint/AddressValidator.php (1)
AddressValidator(26-81)src/DependencyInjection/BazingaGeocoderExtension.php (1)
load(47-77)
🪛 GitHub Actions: CI
src/Command/GeocodeCommand.php
[error] 27-27: Unknown named parameter $help. This suggests an incompatibility with the Symfony Console version (command configuration using a 'help' option/parameter no longer recognized). Review GeocodeCommand for Symfony Console compatibility and update accordingly.
composer.json
[warning] 1-1: Symfony recipes are disabled: 'symfony/flex' not found in root composer.json.
| "test": "SYMFONY_DEPRECATIONS_HELPER=ignoreFile='./tests/baseline-ignore' vendor/bin/simple-phpunit --testsuite main" | ||
| }, |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
symfony/phpunit-bridge SYMFONY_DEPRECATIONS_HELPER ignoreFile cross-platform configuration
💡 Result:
Short answer: use the SYMFONY_DEPRECATIONS_HELPER env var with the ignoreFile key (URL-encoded form), point it to a plain-text regex list, and prefer an absolute path or set it inside phpunit.xml to avoid shell/OS quoting issues. Examples below.
Why: the helper accepts a URL-encoded query string (keys like ignoreFile, baselineFile, max[...] etc.). The ignoreFile value is a filesystem path to a text file (one regexp per line). See Symfony docs. [1][2]
Examples
-
Unix / macOS (shell):
- export and run:
SYMFONY_DEPRECATIONS_HELPER='ignoreFile=./tests/deprecations-ignore' ./vendor/bin/simple-phpunit - absolute path (safer):
SYMFONY_DEPRECATIONS_HELPER='ignoreFile=/full/path/to/tests/deprecations-ignore' ./vendor/bin/simple-phpunit
Reference: docs show ignoreFile usage. [1]
- export and run:
-
Windows (CMD):
- set and run:
set SYMFONY_DEPRECATIONS_HELPER=ignoreFile=C:\path\to\tests\deprecations-ignore
vendor\bin\simple-phpunit.bat - PowerShell:
$env:SYMFONY_DEPRECATIONS_HELPER = 'ignoreFile=C:\path\to\tests\deprecations-ignore'; vendor\bin\simple-phpunit
Note: avoid single-quote URL-encoding in CMD; PowerShell accepts the quoted string. Use absolute Windows path to avoid cwd differences. [1][2]
- set and run:
-
When you must combine multiple options (or include & characters) URL-encode or quote properly:
- Unix:
SYMFONY_DEPRECATIONS_HELPER='ignoreFile=./tests/deprecations-ignore&verbose=0' ./vendor/bin/simple-phpunit - If values contain ampersands or special chars, percent-encode them (e.g. & → %26) or wrap whole value in single quotes in shells that support it. [2]
- Unix:
-
Safer: put the env var in phpunit.xml to avoid shell quoting differences:
This is cross-platform because PHPUnit reads phpunit.xml on all OSes. (Docs show using for SYMFONY_DEPRECATIONS_HELPER.) [2]
Tips
- Use absolute paths to avoid cwd differences.
- The ignoreFile format: plain text, one regexp per line; lines starting with # are comments. [1]
- If you need a generated JSON baseline use baselineFile/generateBaseline options (also URL-encoded). [2]
References
[1] Symfony PHPUnit Bridge — Ignoring deprecations / ignoreFile usage. https://symfony.com/doc/current/components/phpunit_bridge.html
[2] Symfony docs (PHPUnit Bridge configuration; environment variable examples and baseline/ignoreFile). https://symfony.com/doc/6.1/components/phpunit_bridge.html
🌐 Web query:
composer.json scripts environment variables Windows POSIX compatibility
💡 Result:
Short answer — use a cross‑platform helper (recommended) or call platform‑specific commands.
Options
- Use cross-env (simple, widely used)
- Install: npm install --save-dev cross-env.
- composer.json example (runs an npm script but same idea for any command):
"scripts": { "build-assets": "cross-env APP_ENV=production npm run build" } - cross-env lets you write POSIX style VAR=val and it works on Windows and POSIX. [1][2]
- Use an env file reader (env-cmd / cross-env-file) if you prefer files
- Put vars in a file and use env-cmd or cross-env-file to load them before the command. [2][8]
- If you cannot add Node tools, call platform-specific forms in composer scripts (less desirable)
- POSIX: "mycmd": "APP_ENV=production php bin/console …"
- Windows cmd.exe: "mycmd:windows": "set APP_ENV=production&& php bin/console …" (note spacing/&&)
- Or PowerShell: $env:APP_ENV='production'; composer …
Composer can run different script names and you can detect OS in CI to pick the right script, but this is error‑prone. [5]
Notes
- Composer supports environment substitution in some fields (e.g. scripts/extra) via ${VAR}, but setting inline env reliably across Windows and POSIX is best done with cross-env or an env-loader. [5][1]
Sources
[1] cross-env (package readme / docs).
[2] cross-env / cross-env-shell usage and alternatives (npm/Yarn docs).
[5] Composer: using environment variables in scripts and fields.
Move SYMFONY_DEPRECATIONS_HELPER to phpunit.xml for cross-platform compatibility.
The inline VAR=... cmd syntax is POSIX-shell-specific and will fail on Windows (CMD/PowerShell). Per Symfony's PHPUnit Bridge docs, set this as an environment variable in phpunit.xml.dist instead:
<php>
<env name="SYMFONY_DEPRECATIONS_HELPER" value="ignoreFile=./tests/baseline-ignore"/>
</php>This approach works across Windows, macOS, and Linux without shell-specific syntax.
🤖 Prompt for AI Agents
composer.json lines 88-89: the "test" script currently sets
SYMFONY_DEPRECATIONS_HELPER inline using POSIX shell syntax which breaks on
Windows; remove the environment variable from the npm script so the command is
just the PHPUnit invocation (vendor/bin/simple-phpunit --testsuite main) and
instead add the SYMFONY_DEPRECATIONS_HELPER env var to phpunit.xml.dist (or
phpunit.xml) by adding a <php><env name="SYMFONY_DEPRECATIONS_HELPER"
value="ignoreFile=./tests/baseline-ignore"/></php> entry so the variable is set
cross-platform.
There was a problem hiding this comment.
well then running vendor/bin/simple-phpunit will always use it and there will be no way to opt-out to check what deprecations are really triggered now and what to cleanup
There was a problem hiding this comment.
@norkunas That's a fair point! Having the flexibility to opt-out is valuable for deprecation cleanup work.
You could preserve both cross-platform compatibility and the opt-out ability by providing two scripts:
"scripts": {
"test": "SYMFONY_DEPRECATIONS_HELPER=ignoreFile='./tests/baseline-ignore' vendor/bin/simple-phpunit --testsuite main",
"test-all-deprecations": "vendor/bin/simple-phpunit --testsuite main"
}This way:
composer test→ ignores baseline deprecationscomposer test-all-deprecations→ shows all deprecations (or directvendor/bin/simple-phpunitcall)- Windows users could use the second script or override the env var
That said, if most of your development/CI happens on Unix-like systems and the current approach works for your workflow, the inline env var is perfectly reasonable. The Windows limitation may not be significant for your use case.
Summary by CodeRabbit
Chores
Tests
✏️ Tip: You can customize this high-level summary in your review settings.