Feature/switch to php based configs #406
Conversation
WalkthroughServices previously declared in XML (config/services.xml) were migrated to a PHP service file (config/services.php); the XML file was removed and the DI loader in MeilisearchExtension was updated from XmlFileLoader to PhpFileLoader to load the new PHP config. Changes
Sequence Diagram(s)sequenceDiagram
participant Ext as MeilisearchExtension::load
participant LoaderOld as XmlFileLoader
participant LoaderNew as PhpFileLoader
participant Container as Service Container
rect rgba(200,230,255,0.4)
Ext->>LoaderOld: (before) load('config/services.xml')
LoaderOld->>Container: parse XML -> register services/aliases/tags
end
rect rgba(200,255,220,0.4)
Ext->>LoaderNew: (after) load('config/services.php')
LoaderNew->>Container: execute PHP file -> register services/aliases/tags
end
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)
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 |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #406 +/- ##
=======================================
Coverage 88.38% 88.38%
=======================================
Files 20 20
Lines 878 878
=======================================
Hits 776 776
Misses 102 102 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
.github/workflows/tests.yml(1 hunks)config/services.php(1 hunks)config/services.xml(0 hunks)src/DependencyInjection/MeilisearchExtension.php(1 hunks)
💤 Files with no reviewable changes (1)
- config/services.xml
🧰 Additional context used
🧬 Code graph analysis (1)
config/services.php (5)
src/Engine.php (1)
Engine(10-143)src/Services/MeilisearchService.php (1)
MeilisearchService(23-394)src/EventListener/DoctrineEventSubscriber.php (1)
DoctrineEventSubscriber(10-33)src/Services/SettingsUpdater.php (1)
SettingsUpdater(17-79)src/Services/UnixTimestampNormalizer.php (1)
UnixTimestampNormalizer(9-33)
🔇 Additional comments (1)
src/DependencyInjection/MeilisearchExtension.php (1)
21-22: LGTM! Clean migration from XML to PHP-based configuration.The switch from
XmlFileLoadertoPhpFileLoaderand fromservices.xmltoservices.phpis correct and aligns with the PR objective.
.github/workflows/tests.yml
Outdated
| env: | ||
| fail-fast: true | ||
| fail-fast: false |
There was a problem hiding this comment.
Critical: fail-fast is in the wrong location and will not work.
The fail-fast setting is a strategy-level configuration option, not an environment variable. Placing it under env: will have no effect on the CI behavior.
Move it under the strategy: block of the integration-tests job:
-env:
- fail-fast: false
-
jobs:
integration-tests:
# Will not run if the event is a PR to bump-meilisearch-v* (so a pre-release PR)
# Will still run for each push to bump-meilisearch-v*
if: github.event_name != 'pull_request' || !startsWith(github.base_ref, 'bump-meilisearch-v')
runs-on: ubuntu-latest
services:
meilisearch:
image: getmeili/meilisearch:latest
ports:
- '7700:7700'
env:
MEILI_MASTER_KEY: masterKey
MEILI_NO_ANALYTICS: true
strategy:
+ fail-fast: false
matrix:📝 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.
| env: | |
| fail-fast: true | |
| fail-fast: false | |
| jobs: | |
| integration-tests: | |
| # Will not run if the event is a PR to bump-meilisearch-v* (so a pre-release PR) | |
| # Will still run for each push to bump-meilisearch-v* | |
| if: github.event_name != 'pull_request' || !startsWith(github.base_ref, 'bump-meilisearch-v') | |
| runs-on: ubuntu-latest | |
| services: | |
| meilisearch: | |
| image: getmeili/meilisearch:latest | |
| ports: | |
| - '7700:7700' | |
| env: | |
| MEILI_MASTER_KEY: masterKey | |
| MEILI_NO_ANALYTICS: true | |
| strategy: | |
| fail-fast: false | |
| matrix: |
🤖 Prompt for AI Agents
.github/workflows/tests.yml around lines 14 to 15: the fail-fast setting is
incorrectly placed under env: (so it’s treated as an environment variable and
has no effect); remove fail-fast from the env section and add fail-fast: false
under the strategy: block of the integration-tests job (at the same indentation
level as matrix:), ensuring it is a strategy-level key rather than an env
variable.
…igs' into feature/switch-to-php-based-configs
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
config/services.php (1)
7-7: Consider removing unused variable.The
$parametersvariable is defined but never used in this configuration file.Apply this diff if parameters are not needed:
return static function(ContainerConfigurator $container) { $services = $container->services(); - $parameters = $container->parameters();
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
config/services.php(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
config/services.php (5)
src/Engine.php (1)
Engine(10-143)src/Services/MeilisearchService.php (1)
MeilisearchService(23-394)src/EventListener/DoctrineEventSubscriber.php (1)
DoctrineEventSubscriber(10-33)src/Services/SettingsUpdater.php (1)
SettingsUpdater(17-79)src/Services/UnixTimestampNormalizer.php (1)
UnixTimestampNormalizer(9-33)
🔇 Additional comments (10)
config/services.php (10)
9-12: LGTM!The Engine service definition correctly matches the constructor signature and the class alias provides convenient access.
14-21: LGTM!The MeilisearchService definition correctly matches the constructor signature with all required abstract arguments properly defined. The previous syntax issue on line 17 has been resolved.
23-25: LGTM!The deprecated alias provides backward compatibility with appropriate deprecation messaging.
27-29: LGTM!The DoctrineEventSubscriber service definition correctly matches the constructor signature.
34-44: LGTM!The Client service definition uses lazy loading appropriately and clearly documents abstract arguments to be defined by the extension.
46-51: LGTM!The client aliases are properly configured for backward compatibility and class-based dependency injection. The previous duplicate alias issue has been resolved.
53-53: LGTM!The class alias enables convenient dependency injection using the interface type.
55-62: LGTM!The SettingsUpdater service definition correctly matches the constructor signature, and the class alias enables type-based dependency injection.
64-97: LGTM!The console command services are properly configured with appropriate dependencies and tags. The command names include convenient aliases (e.g.,
meili:clear) and helpful descriptions.
99-100: LGTM!The UnixTimestampNormalizer is correctly registered as a serializer normalizer with appropriate tagging.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
config/services.php (1)
44-45: Add public modifier for consistency.The deprecated alias should be marked as
->public()to match the main service visibility and maintain consistency with other deprecated aliases in this file (lines 36-38, 59-61).Apply this diff:
$services->alias('search.search_indexer_subscriber', 'meilisearch.search_indexer_subscriber') + ->public() ->deprecate('meilisearch/search-bundle', '0.14', 'The "%alias_id%" service alias is deprecated. Use "meilisearch.search_indexer_subscriber" instead.');
🧹 Nitpick comments (1)
config/services.php (1)
66-66: Add public modifier for API consistency.The
SearchService::classalias should be marked as->public()to match the target service visibility and maintain consistency with the pattern established by theClient::classalias at lines 63-64, where the class alias is public when its target service is public.Apply this diff:
- $services->alias(SearchService::class, 'meilisearch.service'); + $services->alias(SearchService::class, 'meilisearch.service') + ->public();
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
config/services.php(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
config/services.php (5)
src/Engine.php (1)
Engine(10-143)src/EventListener/DoctrineEventSubscriber.php (1)
DoctrineEventSubscriber(10-33)src/Services/MeilisearchService.php (1)
MeilisearchService(23-394)src/Services/SettingsUpdater.php (1)
SettingsUpdater(17-79)src/Services/UnixTimestampNormalizer.php (1)
UnixTimestampNormalizer(9-33)
🔇 Additional comments (7)
config/services.php (7)
5-16: LGTM! All classes properly imported.All referenced classes are correctly imported at the top of the file.
22-25: LGTM! Engine service correctly configured.The service definition and arguments match the Engine constructor requirements.
27-38: LGTM! MeilisearchService correctly wired.The service arguments correctly match the constructor signature, using abstract placeholders for values to be provided by the extension. The deprecated alias properly maintains public visibility.
47-64: LGTM! Client service properly configured.The client service definition uses appropriate abstract arguments for values to be provided by the extension, and the deprecated alias plus class alias both maintain public visibility correctly.
68-75: LGTM! SettingsUpdater service correctly configured.The service arguments correctly match the constructor signature, and the visibility consistency between the service and its alias is maintained.
77-110: LGTM! Console commands properly configured.All console commands are correctly registered with appropriate service dependencies and console.command tags. The command names, descriptions, and arguments align well with their expected functionality.
112-113: LGTM! Normalizer correctly registered.The
UnixTimestampNormalizeris properly configured with theserializer.normalizertag for integration with Symfony's serializer component.
|
Can you rebase? |
Co-authored-by: Tomas Norkūnas <[email protected]>
Co-authored-by: Tomas Norkūnas <[email protected]>
|
Thank you @Chris53897 |
3 participants
#401
first try with
https://github.com/GromNaN/symfony-config-xml-to-php
After the merge of #407 CI is green now.
Summary by CodeRabbit