-
Notifications
You must be signed in to change notification settings - Fork 34
Fix collecting custom data providers #435
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
📝 WalkthroughWalkthroughCollects custom data-provider declarations in the Meilisearch extension into a container parameter, then a compiler pass reads that parameter, validates service definitions (throws if missing), and attaches Changes
Sequence Diagram(s)sequenceDiagram
participant Ext as MeilisearchExtension
participant Container as ContainerParameters
participant Pass as DataProviderPass
participant Defs as ServiceDefinitions
Ext->>Container: setParameter('.meilisearch.custom_data_providers', customProviders)
Note over Container: Parameter stores mapping\n{ serviceId: [{index, class}, ...] }
Container->>Pass: compiler pass executes
Pass->>Container: readParameter('.meilisearch.custom_data_providers')
Pass->>Defs: for each serviceId -> locate definition
alt definition missing
Defs-->>Pass: not found
Pass-->>Ext: throw InvalidArgumentException (includes indices)
else definition present
Defs-->>Pass: definition found
Pass->>Defs: add `meilisearch.data_provider` tags with attributes
end
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes 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)
No actionable comments were generated in the recent review. 🎉 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❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #435 +/- ##
==========================================
+ Coverage 69.34% 69.80% +0.45%
==========================================
Files 30 30
Lines 1491 1500 +9
==========================================
+ Hits 1034 1047 +13
+ Misses 457 453 -4 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@src/DependencyInjection/Compiler/DataProviderPass.php`:
- Around line 23-35: Update the throw in the foreach in DataProviderPass to a
single-line throw and use the global function call: replace the multi-line throw
block with a single statement like throw new
\InvalidArgumentException(\sprintf('The service "%s" configured as a
"data_provider" for index "%s" was not found.', $serviceId,
$attributes['index'])); leaving the surrounding loop and the subsequent
->findDefinition(...)->addTag(...) call unchanged.
In `@src/DependencyInjection/MeilisearchExtension.php`:
- Around line 108-112: The current implementation builds $customProviders keyed
by the data_provider id so multiple indices referencing the same provider
overwrite each other; change MeilisearchExtension to append entries to
$customProviders (use [] to push an entry object/array containing 'service' =>
$indice['data_provider'], 'index' => $indexName, 'class' => $class) instead of
assigning by key, and update DataProviderPass (the compiler pass that reads this
parameter, e.g., DataProviderPass::process) to iterate the indexed list and
register/addTag or otherwise configure the provider for each entry so a single
provider service can be associated with multiple indices. Ensure the
DataProviderPass uses the 'service' field to group or tag the same service
multiple times rather than expecting unique keys.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/DependencyInjection/Compiler/DataProviderPass.php`:
- Around line 24-30: Replace the check using ContainerBuilder::hasDefinition
with a check that also respects aliases: in DataProviderPass where you currently
call $container->hasDefinition($serviceId) before calling
$container->findDefinition($serviceId), change the guard to use
$container->has($serviceId) so aliased service IDs are recognized; keep the
subsequent $container->findDefinition($serviceId) and the same exception message
using $serviceId and $tagsConfigs (or $indices) unchanged.
🧹 Nitpick comments (1)
src/DependencyInjection/Compiler/DataProviderPass.php (1)
20-36: Consider removing the temporary parameter after consumption.Symfony convention for dot-prefixed (
.meilisearch.custom_data_providers) internal parameters is to clean them up once consumed in the compiler pass so they don't leak into the compiled container.Proposed fix
if ($container->hasParameter('.meilisearch.custom_data_providers')) { $customProviders = $container->getParameter('.meilisearch.custom_data_providers'); + $container->getParameterBag()->remove('.meilisearch.custom_data_providers'); foreach ($customProviders as $serviceId => $tagsConfigs) {
Pull Request
Related issue
Fixes #<issue_number>
What does this PR do?
PR checklist
Please check if your PR fulfills the following requirements:
gemini 2.5 flash wrote the code
Summary by CodeRabbit