Add Data providers extension point #410
Conversation
norkunas
added
enhancement
|
Warning Rate limit exceeded@norkunas has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 4 minutes and 30 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (60)
WalkthroughIntroduce MeilisearchManager and a DataProvider subsystem; add SearchableObject and data provider registry with DI/compiler wiring; deprecate old SearchService while delegating to the manager; rewire commands, services, tests and configuration; add PHPStan baseline and adjust CI/composer constraints. Changes
Sequence Diagram(s)sequenceDiagram
participant CLI as CLI / Commands
participant Manager as MeilisearchManager
participant Registry as DataProviderRegistry
participant Provider as DataProvider
participant Engine as Engine
participant API as MeiliSearch API
rect rgba(0,128,128,0.06)
Note over CLI,API: Manager-driven indexing flow (high level)
CLI->>Manager: index(entity or [entities])
activate Manager
Manager->>Manager: resolve indices, primary_key, aggregator logic
Manager->>Registry: getDataProvider(index, class)
activate Registry
Registry-->>Manager: provider service
deactivate Registry
loop per index (batches)
Manager->>Provider: provide(limit, offset)
activate Provider
Provider-->>Manager: entities[]
deactivate Provider
Manager->>Manager: wrap entities -> SearchableObject(s)
Manager->>Engine: index(searchableObjects)
activate Engine
Engine->>API: addDocuments(documents, primaryKey)
API-->>Engine: response (task)
deactivate Engine
end
Manager-->>CLI: aggregated result
deactivate Manager
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Areas needing extra attention:
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
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 |
|
Thanks! in my survos/meili-bundle, I came up with a solution that is working well for me at scale, by batching in .jsonl file. This works great when I'm bulk importing. I use the Symfony Workflow component when I'm bulk updating (e.g. updating 'marking' in some workflow process, where I may get thousands of tiny 1-field updates. The workflow component has the ability to batch event messages (from bin/console mess:consume), which I use to gather some size (10M, I think) before dispatching a single message to the meili server. https://github.com/survos/meili-bundle/blob/main/src/EventListener/MeiliSpoolDoctrineListener.php I'm giving a talk later this week at the Symfony Conference on meilisearch and how to configure the index using attributes. I'd love your feedback, are you available on Slack or the meili discord channel? |
|
I am available on both :) |
norkunas
force-pushed
the
dataprovider
branch
6 times, most recently
from
8569538 to
a8ad4dd
Compare
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #410 +/- ##
===========================================
- Coverage 88.38% 71.41% -16.97%
===========================================
Files 20 27 +7
Lines 878 1186 +308
===========================================
+ Hits 776 847 +71
- Misses 102 339 +237 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
norkunas
force-pushed
the
dataprovider
branch
from
a8ad4dd to
352e2bc
Compare
Strift
left a comment
Strift
left a comment
There was a problem hiding this comment.
The introduction of the Data Provider architecture is welcome, but deleting SearchService constitutes a massive breaking change that prevents us from releasing this in a minor version.
I'm wondering if we can support the new architecture and keep backward compatibility by using the bridge pattern.
What do you think of the following?
- Restore
src/SearchService.php(Interface) andsrc/Services/MeilisearchService.php. - Inject the new
MeilisearchManagerinto the oldMeilisearchService. - Forward calls from the old service to the new manager, ignoring the now-obsolete arguments.
Example Implementation for src/Services/MeilisearchService.php:
// ... imports
use Meilisearch\Bundle\Services\MeilisearchManager;
/**
* @deprecated Use MeilisearchManager instead.
*/
class MeilisearchService implements SearchService // Keep the old Interface too!
{
public function __construct(private MeilisearchManager $manager)
{
}
public function search(ObjectManager $objectManager, string $className, string $query = '', array $searchParams = []): array
{
trigger_deprecation('meilisearch/meilisearch-symfony', '0.x', 'Passing ObjectManager to search() is deprecated. Use MeilisearchManager::search() instead.');
// We ignore $objectManager because the new Manager handles data provision internally
return $this->manager->search($className, $query, $searchParams);
}
public function index(ObjectManager $objectManager, $searchable): array
{
// Forward to manager...
return $this->manager->index($searchable);
}
// ... implement other methods similarly
}This allows existing users to upgrade immediately. They will get the benefits of the new Data Provider system (internalized in the Manager) while their existing code (passing $objectManager) continues to work. We also add a deprecation warning and update the docs accordingly.
let me know!
well we are on v0, so BC breaks are expected, right? ofc i can do the bridge as you want, but not today |
norkunas
force-pushed
the
dataprovider
branch
from
352e2bc to
ea2a369
Compare
|
@Strift updated ;) |
norkunas
force-pushed
the
dataprovider
branch
2 times, most recently
from
2949520 to
4cb2b38
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 15
🧹 Nitpick comments (18)
src/SearchService.php (1)
9-11: Deprecation notice is appropriate for the migration path.The deprecation annotation correctly guides users toward
SearchManagerInterface. Consider also adding a@seetag pointing to the new interface for easier discovery./** * @deprecated Since 0.16, use `Meilisearch\Bundle\SearchManagerInterface` instead. + * + * @see SearchManagerInterface */tests/Entity/Actor.php (1)
7-27: Test entity is well-structured.The
Actorclass appropriately serves as a fixture for testing the newDataProviderInterfaceflow.Since PHP 8.1 is now the minimum version, you could simplify using constructor property promotion for consistency with other refactored classes in this PR:
final class Actor { - private int $id; - private string $name; - - public function __construct(int $id, string $name) - { - $this->id = $id; - $this->name = $name; - } + public function __construct( + private readonly int $id, + private readonly string $name, + ) { + } public function getId(): intsrc/Services/MeilisearchService.php (3)
50-59: Constructor/manager wiring is sound, but consider typing the dependency to the interfaceThe optional manager injection and assignment to
?SearchManagerInterfaceis consistent and safe. For flexibility (easier testing and potential alternative managers), you might consider changing the ctor signature to depend directly on?SearchManagerInterfaceinstead of?MeilisearchManager, keeping the same service wiring.
254-272: DeprecatingshouldBeIndexedwhile still using it internallyMarking
shouldBeIndexed()as deprecated “without replacement” is fine given the new manager‑based architecture, but it’s still used internally inindex(). If you want to discourage external use without surprising readers, you could clarify the docblock to “for internal use only; external callers should migrate toMeilisearchManager”.
27-27: Address phpmd pipeline failures for this deprecated classphpmd is now flagging this class for:
- too many public methods / high complexity / high coupling, and
- long variable names and static access.
Given
MeilisearchServiceis now deprecated and wrapped byMeilisearchManager, a large refactor here is probably not worth it. Consider instead:
- adding
@SuppressWarningsannotations for the relevant PHPMD rules on this class, or- excluding this deprecated class from phpmd in your ruleset,
so the pipeline passes without forcing structural changes to legacy code.
Also applies to: 48-48, 57-57, 384-384, 432-432, 436-436
tests/Unit/ConfigurationTest.php (1)
359-402: New test case validates customprimary_keyoverridesThe
'custom objectID'dataset (withprimary_keyset topostId/tagId) is a good addition to ensure the configuration layer preserves per-index primary key overrides through normalization. If you ever rename the test, you might consider calling it “custom primary key” to better match the new terminology, but that’s purely cosmetic.phpstan-baseline.php (1)
1-17: Consider tightening the phpstan baseline aroundClassUtils::getClass()The new ignore entries unblock static analysis but may be masking either a missing dev dependency/extension config or an obsolete call site. As a follow‑up, it would be good to either:
- add proper doctrine/phpstan integration so
ClassUtilsis known, or- refactor away from
ClassUtils::getClass()if it’s no longer needed.Not a blocker for this PR, but worth tracking.
src/DependencyInjection/Configuration.php (1)
7-7: New index config fields fit the provider model; consider adding light validationThe added
type,primary_key, anddata_providerfields and the switch toSearchableObject::NORMALIZATION_GROUPlook consistent with the new SearchableObject/DataProvider architecture.Two potential follow‑ups (non‑blocking):
- For
type: 'custom', you might want a smallvalidate()hook to assert thatdata_provideris not null, so obvious misconfigurations fail early in config rather than at runtime.- Given the move away from Algolia semantics, it may be worth revisiting the default
primary_keyof'objectID'in a future breaking version, to better reflect theid‑centric normalization now used in entities.Based on learnings, this could be deferred to a separate, focused PR on configuration/error‑handling.
Also applies to: 53-59, 59-67
src/Model/Aggregator.php (1)
71-75: The TODO identifies a real consistency issue with configurable primary keys.The
normalize()method hardcodes'objectID'as the key, but the PR introduces configurableprimary_keyper index. If an index is configured with a different primary key (e.g.,'id'), the aggregated document will still contain'objectID', causing a mismatch.Consider either:
- Passing the configured primary key to the Aggregator
- Having the caller remap the key after normalization
Would you like me to propose a solution, or should this be tracked as a follow-up issue?
src/DependencyInjection/MeilisearchExtension.php (1)
38-71: Consider extracting data provider registration to reduce complexity.The pipeline reports cyclomatic complexity of 11 (threshold: 10). The nested conditionals for ORM/Aggregator handling are correct but could be extracted to improve readability.
+ private function registerDataProviders(ContainerBuilder $container, array $config): array + { + $dataProviders = []; + foreach ($config['indices'] as $indice) { + $class = $indice['class']; + + if (null !== $indice['data_provider']) { + $dataProviders[$indice['name']] ??= []; + $dataProviders[$indice['name']][$class] = new Reference($indice['data_provider']); + continue; + } + + if ('orm' !== $indice['type']) { + continue; + } + + $dataProviders[$indice['name']] ??= []; + + if (is_subclass_of($class, Aggregator::class)) { + foreach ($class::getEntities() as $aggregatedClass) { + $definitionId = \sprintf('meilisearch.data_provider.%s_%s', $indice['name'], hash('xxh32', $aggregatedClass)); + $container->setDefinition($definitionId, new Definition(OrmEntityProvider::class, [new Reference('doctrine'), $aggregatedClass])); + $dataProviders[$indice['name']][$aggregatedClass] = new Reference($definitionId); + } + } else { + $definitionId = \sprintf('meilisearch.data_provider.%s_%s', $indice['name'], hash('xxh32', $class)); + $container->setDefinition($definitionId, new Definition(OrmEntityProvider::class, [new Reference('doctrine'), $class])); + $dataProviders[$indice['name']][$class] = new Reference($definitionId); + } + } + return $dataProviders; + }Then in
load():- $dataProviders = []; - foreach ($config['indices'] as $index => $indice) { - // ... all the provider logic - } + $dataProviders = $this->registerDataProviders($container, $config); + foreach ($config['indices'] as $index => $indice) { + $config['indices'][$index]['prefixed_name'] = $config['prefix'].$indice['name']; + $config['indices'][$index]['settings'] = $this->findReferences($config['indices'][$index]['settings']); + }src/DataProvider/DataProviderInterface.php (1)
20-25: Clarify the expected structure of$identifiersparameter.The
@param array<mixed> $identifiersis ambiguous. Looking at the implementations:
OrmEntityProvidertreats it as a flat array of ID values:findBy(['id' => $identifiers])ActorDataProvidertreats it as an associative array:$identifiers['id']This ambiguity leads to inconsistent implementations. Consider documenting the expected structure explicitly.
/** - * @param array<mixed> $identifiers + * @param array<int|string> $identifiers Flat array of identifier values to load * * @return iterable<T> */ public function loadByIdentifiers(array $identifiers): iterable;src/DataProvider/OrmEntityProvider.php (1)
22-23: Consider null-checkinggetManagerForClassresult.
getManagerForClass()can returnnullif no manager is found for the class. While the@param class-stringconstraint helps, invalid configurations could still trigger a null pointer on$manager->getRepository().$manager = $this->managerRegistry->getManagerForClass($this->className); +if (null === $manager) { + throw new \InvalidArgumentException(\sprintf('No manager found for class "%s".', $this->className)); +} $repository = $manager->getRepository($this->className);tests/BaseKernelTestCase.php (1)
47-52: Potential performance concern withwaitForAllTasks().This iterates over all tasks from the Meilisearch server and waits for each one. In a busy test environment or if there are many historical tasks, this could be slow. Consider limiting to only pending/enqueued tasks or tasks created during the current test.
protected function waitForAllTasks(): void { - foreach ($this->client->getTasks() as $task) { - $this->client->waitForTask($task['uid']); + foreach ($this->client->getTasks(['statuses' => ['enqueued', 'processing']]) as $task) { + $this->client->waitForTask($task['uid']); } }src/SearchableObject.php (1)
80-95: Missing@returnPHPDoc type forgetSearchableArray().The method declares
@throws ExceptionInterfacebut lacks the return type annotation. Consider adding@return array<string, mixed>for better static analysis support./** + * @return array<string, mixed> + * * @throws ExceptionInterface */ public function getSearchableArray(): arraysrc/Engine.php (1)
84-92: Consider using batch deletion for better performance.The
remove()method callsdeleteDocument()individually for each object. Meilisearch supports batch deletion viadeleteDocuments()which would be more efficient.$result = []; foreach ($data as $indexUid => $objects) { - $result[$indexUid] = []; - foreach ($objects as $object) { - $result[$indexUid][] = $this->client - ->index($indexUid) - ->deleteDocument($object); - } + $result[$indexUid] = $this->client + ->index($indexUid) + ->deleteDocuments($objects); }src/Command/MeilisearchImportCommand.php (1)
71-155: Pipeline flags high complexity inexecute()method.The pipeline reports CyclomaticComplexity of 13 (threshold: 10) and NPathComplexity of 592 (threshold: 200). Consider extracting the per-index processing into a separate method in a follow-up PR.
Based on learnings, the maintainer prefers keeping PRs focused, so this refactor can be deferred.
src/Services/MeilisearchManager.php (2)
131-135: Pipeline flag: Else expression.The pipeline prefers early returns over else clauses. Consider inverting the condition.
- if (!$entity instanceof $configClass && \in_array($configClass, $this->aggregators, true)) { - $objectToIndex = new $configClass($entity, $provider->getIdentifierValues($entity)); - } else { - $objectToIndex = $entity; - } + $objectToIndex = $entity; + if (!$entity instanceof $configClass && \in_array($configClass, $this->aggregators, true)) { + $objectToIndex = new $configClass($entity, $provider->getIdentifierValues($entity)); + }
21-21: Pipeline flags: High class complexity and coupling.The pipeline reports ExcessiveClassComplexity (69, threshold 50) and CouplingBetweenObjects (14, threshold 13). This is a central orchestration service, so some complexity is expected. Consider extracting helper classes in future refactoring if the class grows further.
Based on learnings, the maintainer prefers keeping PRs focused. Consider deferring extraction of index resolution, batch processing, and search result mapping into separate collaborators.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (52)
.github/workflows/tests.yml(1 hunks)composer.json(1 hunks)config/services.php(4 hunks)phpstan-baseline.php(1 hunks)phpstan.dist.neon(1 hunks)src/Command/IndexCommand.php(2 hunks)src/Command/MeilisearchClearCommand.php(1 hunks)src/Command/MeilisearchCreateCommand.php(3 hunks)src/Command/MeilisearchDeleteCommand.php(1 hunks)src/Command/MeilisearchImportCommand.php(7 hunks)src/Command/MeilisearchUpdateSettingsCommand.php(3 hunks)src/DataProvider/DataProviderInterface.php(1 hunks)src/DataProvider/OrmEntityProvider.php(1 hunks)src/DependencyInjection/Configuration.php(2 hunks)src/DependencyInjection/MeilisearchExtension.php(3 hunks)src/Engine.php(5 hunks)src/Event/SettingsUpdatedEvent.php(1 hunks)src/EventListener/ConsoleOutputSubscriber.php(1 hunks)src/EventListener/DoctrineEventSubscriber.php(1 hunks)src/Exception/DataProviderNotFoundException.php(1 hunks)src/Exception/NotSearchableException.php(1 hunks)src/Model/Aggregator.php(2 hunks)src/SearchManagerInterface.php(1 hunks)src/SearchService.php(1 hunks)src/Searchable.php(1 hunks)src/SearchableEntity.php(2 hunks)src/SearchableObject.php(1 hunks)src/Services/MeilisearchManager.php(1 hunks)src/Services/MeilisearchService.php(8 hunks)src/Services/SettingsUpdater.php(1 hunks)src/Services/UnixTimestampNormalizer.php(1 hunks)tests/BaseKernelTestCase.php(5 hunks)tests/Entity/Actor.php(1 hunks)tests/Entity/Link.php(2 hunks)tests/Entity/SelfNormalizable.php(2 hunks)tests/Entity/Tag.php(2 hunks)tests/Integration/AggregatorTest.php(1 hunks)tests/Integration/Command/MeilisearchClearCommandTest.php(1 hunks)tests/Integration/Command/MeilisearchCreateCommandTest.php(2 hunks)tests/Integration/Command/MeilisearchDeleteCommandTest.php(1 hunks)tests/Integration/Command/MeilisearchImportCommandTest.php(1 hunks)tests/Integration/EngineTest.php(3 hunks)tests/Integration/EventListener/DoctrineEventSubscriberTest.php(8 hunks)tests/Integration/Fixtures/ActorDataProvider.php(1 hunks)tests/Integration/SearchTest.php(4 hunks)tests/Kernel.php(1 hunks)tests/Unit/ConfigurationTest.php(8 hunks)tests/Unit/SerializationTest.php(2 hunks)tests/baseline-ignore(1 hunks)tests/config/doctrine_php7.yaml(0 hunks)tests/config/framework.yaml(1 hunks)tests/config/meilisearch.yaml(1 hunks)
💤 Files with no reviewable changes (1)
- tests/config/doctrine_php7.yaml
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: norkunas
Repo: meilisearch/meilisearch-symfony PR: 391
File: src/Command/MeilisearchImportCommand.php:117-120
Timestamp: 2025-08-05T04:46:01.119Z
Learning: In meilisearch-symfony project, the maintainer norkunas prefers to keep PRs focused and defer error handling improvements for settings updates to separate PRs, particularly when it involves decisions about partial import behavior when some indexes fail.
🧬 Code graph analysis (30)
tests/Entity/Tag.php (1)
src/SearchableObject.php (1)
SearchableObject(13-96)
src/Command/MeilisearchDeleteCommand.php (4)
src/SearchManagerInterface.php (1)
deleteByIndexName(58-58)src/SearchService.php (1)
deleteByIndexName(50-50)src/Services/MeilisearchManager.php (1)
deleteByIndexName(215-218)src/Services/MeilisearchService.php (1)
deleteByIndexName(162-169)
tests/Integration/EventListener/DoctrineEventSubscriberTest.php (4)
src/Engine.php (1)
search(122-125)src/SearchManagerInterface.php (1)
search(79-79)src/Services/MeilisearchManager.php (1)
search(238-281)src/Services/MeilisearchService.php (1)
search(182-227)
src/Model/Aggregator.php (1)
src/SearchableEntity.php (1)
__construct(42-56)
src/Exception/DataProviderNotFoundException.php (1)
src/Exception/NotSearchableException.php (1)
__construct(9-12)
src/SearchableEntity.php (4)
tests/Entity/Link.php (1)
normalize(74-85)tests/Entity/SelfNormalizable.php (1)
normalize(64-75)tests/Entity/Tag.php (1)
normalize(88-100)src/SearchableObject.php (1)
SearchableObject(13-96)
tests/Integration/Fixtures/ActorDataProvider.php (2)
tests/Entity/Actor.php (2)
Actor(7-27)getId(18-21)src/DataProvider/DataProviderInterface.php (4)
provide(18-18)loadByIdentifiers(25-25)getIdentifierValues(32-32)cleanup(34-34)
tests/Entity/SelfNormalizable.php (1)
src/SearchableObject.php (1)
SearchableObject(13-96)
tests/Entity/Link.php (1)
src/SearchableObject.php (1)
SearchableObject(13-96)
tests/Integration/SearchTest.php (4)
tests/BaseKernelTestCase.php (1)
waitForAllTasks(47-52)src/Engine.php (1)
search(122-125)src/SearchManagerInterface.php (2)
search(79-79)rawSearch(91-91)src/Services/MeilisearchManager.php (2)
search(238-281)rawSearch(291-299)
src/SearchManagerInterface.php (4)
src/Exception/NotSearchableException.php (1)
NotSearchableException(7-13)src/SearchService.php (5)
isSearchable(20-20)searchableAs(34-34)getConfiguration(27-27)index(36-36)search(59-64)src/Services/MeilisearchManager.php (5)
isSearchable(68-73)searchableAs(78-86)getConfiguration(88-91)index(116-162)search(238-281)src/Services/MeilisearchService.php (5)
isSearchable(66-71)searchableAs(83-91)getConfiguration(78-81)index(93-126)search(182-227)
src/Services/SettingsUpdater.php (2)
src/Services/MeilisearchManager.php (2)
MeilisearchManager(21-505)getConfiguration(88-91)src/SearchManagerInterface.php (1)
getConfiguration(25-25)
src/Command/MeilisearchCreateCommand.php (3)
src/Command/IndexCommand.php (1)
__construct(19-24)src/SearchManagerInterface.php (1)
isSearchable(18-18)src/Services/MeilisearchManager.php (1)
isSearchable(68-73)
src/Command/MeilisearchClearCommand.php (5)
src/Engine.php (1)
clear(104-107)src/SearchManagerInterface.php (1)
clear(51-51)src/SearchService.php (1)
clear(43-43)src/Services/MeilisearchManager.php (1)
clear(205-210)src/Services/MeilisearchService.php (1)
clear(151-160)
src/Command/MeilisearchImportCommand.php (6)
src/Engine.php (3)
__construct(12-14)index(25-61)count(132-135)src/EventListener/ConsoleOutputSubscriber.php (1)
__construct(13-15)src/EventListener/DoctrineEventSubscriber.php (1)
__construct(12-14)src/SearchManagerInterface.php (6)
getConfiguration(25-25)isSearchable(18-18)getDataProvider(30-30)index(37-37)count(101-101)deleteByIndexName(58-58)src/Services/MeilisearchManager.php (6)
getConfiguration(88-91)isSearchable(68-73)getDataProvider(96-111)index(116-162)count(307-312)deleteByIndexName(215-218)src/DataProvider/OrmEntityProvider.php (1)
provide(20-29)
tests/Unit/SerializationTest.php (2)
src/SearchableObject.php (2)
SearchableObject(13-96)getId(75-78)tests/Entity/Post.php (1)
getId(91-94)
src/SearchableObject.php (2)
src/SearchableEntity.php (1)
getSearchableArray(66-91)tests/Entity/SelfNormalizable.php (1)
normalize(64-75)
src/DataProvider/DataProviderInterface.php (2)
src/DataProvider/OrmEntityProvider.php (4)
provide(20-29)loadByIdentifiers(31-38)getIdentifierValues(40-45)cleanup(47-50)tests/Integration/Fixtures/ActorDataProvider.php (4)
provide(15-44)loadByIdentifiers(46-57)getIdentifierValues(59-64)cleanup(66-69)
src/Services/UnixTimestampNormalizer.php (4)
src/Model/Aggregator.php (1)
normalize(71-75)tests/Entity/Link.php (1)
normalize(74-85)tests/Entity/SelfNormalizable.php (1)
normalize(64-75)tests/Entity/Tag.php (1)
normalize(88-100)
src/Command/MeilisearchUpdateSettingsCommand.php (3)
src/Command/IndexCommand.php (1)
__construct(19-24)src/SearchManagerInterface.php (1)
isSearchable(18-18)src/SearchService.php (1)
isSearchable(20-20)
tests/BaseKernelTestCase.php (2)
src/Services/MeilisearchManager.php (3)
MeilisearchManager(21-505)getConfiguration(88-91)deleteByIndexName(215-218)src/SearchManagerInterface.php (2)
getConfiguration(25-25)deleteByIndexName(58-58)
src/Command/IndexCommand.php (4)
src/Command/MeilisearchCreateCommand.php (1)
__construct(23-30)src/SearchManagerInterface.php (1)
getConfiguration(25-25)src/SearchService.php (1)
getConfiguration(27-27)src/Services/MeilisearchManager.php (1)
getConfiguration(88-91)
tests/Integration/EngineTest.php (3)
src/SearchableObject.php (2)
SearchableObject(13-96)getId(75-78)tests/Entity/Image.php (1)
getId(39-42)tests/Entity/Post.php (1)
getId(91-94)
src/DataProvider/OrmEntityProvider.php (2)
src/DataProvider/DataProviderInterface.php (4)
provide(18-18)loadByIdentifiers(25-25)getIdentifierValues(32-32)cleanup(34-34)tests/Integration/Fixtures/ActorDataProvider.php (4)
provide(15-44)loadByIdentifiers(46-57)getIdentifierValues(59-64)cleanup(66-69)
src/DependencyInjection/Configuration.php (1)
src/SearchableObject.php (1)
SearchableObject(13-96)
tests/Integration/AggregatorTest.php (2)
src/Model/Aggregator.php (1)
getEntityClassFromObjectID(60-69)src/Exception/InvalidEntityForAggregator.php (1)
InvalidEntityForAggregator(7-9)
src/Services/MeilisearchManager.php (11)
src/Collection.php (1)
Collection(13-380)src/Engine.php (7)
Engine(10-145)index(25-61)remove(69-95)clear(104-107)delete(112-115)search(122-125)count(132-135)src/Exception/DataProviderNotFoundException.php (1)
DataProviderNotFoundException(7-13)src/Exception/NotSearchableException.php (1)
NotSearchableException(7-13)src/Exception/SearchHitsNotFoundException.php (1)
SearchHitsNotFoundException(7-9)src/DataProvider/OrmEntityProvider.php (2)
getIdentifierValues(40-45)loadByIdentifiers(31-38)src/Services/MeilisearchService.php (13)
setAggregatorsAndEntitiesAggregators(303-322)isSearchable(66-71)searchableAs(83-91)getConfiguration(78-81)index(93-126)remove(128-149)clear(151-160)deleteByIndexName(162-169)delete(171-180)search(182-227)rawSearch(229-241)count(243-252)resolveClass(420-440)src/SearchManagerInterface.php (12)
isSearchable(18-18)searchableAs(23-23)getConfiguration(25-25)index(37-37)getDataProvider(30-30)remove(44-44)clear(51-51)deleteByIndexName(58-58)delete(67-67)search(79-79)rawSearch(91-91)count(101-101)src/DataProvider/DataProviderInterface.php (2)
getIdentifierValues(32-32)loadByIdentifiers(25-25)tests/Integration/Fixtures/ActorDataProvider.php (2)
getIdentifierValues(59-64)loadByIdentifiers(46-57)tests/Entity/Actor.php (1)
getId(18-21)
src/Engine.php (2)
src/SearchManagerInterface.php (3)
index(37-37)remove(44-44)search(79-79)src/SearchableObject.php (5)
SearchableObject(13-96)getSearchableArray(83-95)getIndexUid(62-65)getPrimaryKey(70-73)getId(75-78)
config/services.php (3)
src/Services/MeilisearchManager.php (1)
MeilisearchManager(21-505)src/EventListener/DoctrineEventSubscriber.php (1)
DoctrineEventSubscriber(10-30)src/Services/SettingsUpdater.php (1)
SettingsUpdater(16-77)
src/Services/MeilisearchService.php (6)
src/Command/IndexCommand.php (1)
__construct(19-24)src/Engine.php (8)
__construct(12-14)Engine(10-145)index(25-61)remove(69-95)clear(104-107)delete(112-115)search(122-125)count(132-135)src/EventListener/DoctrineEventSubscriber.php (1)
__construct(12-14)src/Services/SettingsUpdater.php (1)
__construct(22-28)src/Services/MeilisearchManager.php (9)
MeilisearchManager(21-505)index(116-162)remove(167-200)clear(205-210)deleteByIndexName(215-218)delete(223-228)search(238-281)rawSearch(291-299)count(307-312)src/SearchManagerInterface.php (8)
index(37-37)remove(44-44)clear(51-51)deleteByIndexName(58-58)delete(67-67)search(79-79)rawSearch(91-91)count(101-101)
🪛 GitHub Actions: Tests
src/EventListener/ConsoleOutputSubscriber.php
[error] 13-13: ShortVariable: Avoid variables with short names like $io. Configured minimum length is 3.
src/DependencyInjection/MeilisearchExtension.php
[error] 22-22: IfStatementAssignment: Avoid assigning values to variables in if clauses and the like (line '73', column '20').
[error] 22-22: CyclomaticComplexity: The method load() has a Cyclomatic Complexity of 11. The configured cyclomatic complexity threshold is 10.
[error] 36-36: StaticAccess: Avoid using static access to class '\Meilisearch\Bundle\MeilisearchBundle' in method 'load'.
[error] 63-63: ElseExpression: The method load uses an else expression. Else clauses are basically not necessary and you can simplify the code by not using them.
[error] 80-80: ElseExpression: The method load uses an else expression. Else clauses are basically not necessary and you can simplify the code by not using them.
[error] 88-88: StaticAccess: Avoid using static access to class '\MeilisearchBundle\MeilisearchBundle' in method 'load'.
src/Services/SettingsUpdater.php
[error] 34-34: CyclomaticComplexity: The method update() has a Cyclomatic Complexity of 13. The configured cyclomatic complexity threshold is 10.
src/Command/MeilisearchImportCommand.php
[error] 23-23: CouplingBetweenObjects: The class MeilisearchImportCommand has a coupling between objects value of 13. Consider to reduce the number of dependencies under 13.
[error] 71-71: CyclomaticComplexity: The method execute() has a Cyclomatic Complexity of 13. The configured cyclomatic complexity threshold is 10.
[error] 71-71: NPathComplexity: The method execute() has an NPath complexity of 592. The configured NPath complexity threshold is 200.
src/SearchableObject.php
[error] 30-30: ShortVariable: Avoid variables with short names like $id. Configured minimum length is 3.
[error] 47-47: ShortVariable: Avoid variables with short names like $id. Configured minimum length is 3.
src/Services/UnixTimestampNormalizer.php
[error] 14-14: UnusedFormalParameter: Avoid unused parameters such as '$format'.
[error] 14-14: UnusedFormalParameter: Avoid unused parameters such as '$context'.
[error] 19-19: UnusedFormalParameter: Avoid unused parameters such as '$format'.
[error] 24-24: UnusedFormalParameter: Avoid unused parameters such as '$format'.
src/Services/MeilisearchManager.php
[error] 21-21: ExcessiveClassComplexity: The class MeilisearchManager has an overall complexity of 69 which is very high. The configured complexity threshold is 50.
[error] 21-21: CouplingBetweenObjects: The class MeilisearchManager has a coupling between objects value of 14. Consider to reduce the number of dependencies under 13.
[error] 133-133: ElseExpression: The method index uses an else expression. Else clauses are basically not necessary and you can simplify the code by not using them.
[error] 438-438: ShortVariable: Avoid variables with short names like $id. Configured minimum length is 3.
[error] 496-496: StaticAccess: Avoid using static access to class '\Doctrine\ORM\Proxy\DefaultProxyClassNameResolver' in method 'resolveClass'.
[error] 500-500: StaticAccess: Avoid using static access to class '\Doctrine\Common\Util\ClassUtils' in method 'resolveClass'.
src/Services/MeilisearchService.php
[error] 27-27: TooManyPublicMethods: The class MeilisearchService has 11 public methods. Consider refactoring MeilisearchService to keep number of public methods under 10.
[error] 27-27: ExcessiveClassComplexity: The class MeilisearchService has an overall complexity of 64 which is very high. The configured complexity threshold is 50.
[error] 27-27: CouplingBetweenObjects: The class MeilisearchService has a coupling between objects value of 17. Consider to reduce the number of dependencies under 13.
[error] 48-48: LongVariable: Avoid excessively long variable names like $classToSerializerGroup. Keep variable name length under 20.
[error] 57-57: StaticAccess: Avoid using static access to class '\Symfony\Component\PropertyAccess\PropertyAccess' in method '__construct'.
[error] 384-384: LongVariable: Avoid excessively long variable names like $searchableEntitiesChunk. Keep variable name length under 20.
[error] 432-432: StaticAccess: Avoid using static access to class '\Doctrine\ORM\Proxy\DefaultProxyClassNameResolver' in method 'resolveClass'.
[error] 436-436: StaticAccess: Avoid using static access to class '\Doctrine\Common\Util\ClassUtils' in method 'resolveClass'.
🔇 Additional comments (56)
tests/config/framework.yaml (1)
5-7: Good addition to test environment configuration.The new configuration keys align well with the Symfony 6.4 upgrade:
handle_all_throwables: trueensures all throwable exceptions are caught during tests, andphp_errors.log: trueimproves error visibility by logging PHP warnings and notices. This supports better test reliability and observability, especially important when testing the complex data provider and service wiring changes in this PR.src/Event/SettingsUpdatedEvent.php (1)
16-20: Constructor promotion +readonlyare idiomatic and preserve behaviorUsing constructor property promotion with
private readonlycleanly expresses this event as an immutable payload and keeps the existing API via getters intact. No further changes needed here.src/Services/UnixTimestampNormalizer.php (2)
19-22: LGTM!The signature correctly uses
mixed $datato match the interface contract, and the instanceof check properly guards normalization.
14-14: Pipeline warnings: unused interface parameters.The static analysis flags
$formatand$contextas unused. These are required by theNormalizerInterfacecontract and cannot be removed. If your project enforces clean pipelines, consider adding suppression comments:/** * @param mixed $data Expected to be a \DateTimeInterface instance (guarded by supportsNormalization) + * @phpmd-suppress UnusedFormalParameter */ public function normalize(mixed $data, ?string $format = null, array $context = []): intApply similar suppressions to lines 19 and 24, or configure your linter to ignore unused parameters in interface implementations globally.
Also applies to: 19-19, 24-24
tests/Integration/AggregatorTest.php (1)
31-37: Unable to complete verification: Repository access unavailable in sandbox environment.The review comment raises a valid concern about the continued use of
'objectId'in unchanged test code (lines 37, 53, 61) against PR objective #66 to remove Algolia-specificobjectIDusage. However, I cannot access the repository to verify:
- Whether the
ContentAggregatorclass retainsobjectID-based semantics for backward compatibility- The broader migration strategy and scope of PR #66
- Whether these test references represent incomplete migration or intentional API retention
The review comment's analysis of the code changes themselves is sound: the method rename and exception handling pattern are correct and follow best practices. The verification request is appropriate but requires repository context to resolve.
src/Services/SettingsUpdater.php (1)
22-28: Constructor migration to MeilisearchManager looks good.The dependency injection now correctly uses
MeilisearchManagerwith constructor property promotion for$searchClientand$eventDispatcher. Configuration retrieval via$searchManager->getConfiguration()aligns with the new architecture.Regarding the pipeline failure on
update()(cyclomatic complexity 13): this is pre-existing complexity in unchanged code. Consider addressing it in a follow-up PR to keep this one focused.src/Exception/DataProviderNotFoundException.php (1)
7-12: Exception message and shape look goodConsistent with existing exception patterns and gives clear context on missing provider (index + class). No changes needed.
src/Searchable.php (1)
7-20: Deprecation annotations are clear and targetedThe class- and constant-level deprecation notices correctly steer users to
SearchableObjectand its constants without changing behavior.src/Services/MeilisearchService.php (2)
93-99: Manager delegation preserves BC and aligns with the new APIThe pattern of:
- triggering deprecation on
ObjectManager-based methods, and- short‑circuiting to
$this->managerwhen present,keeps existing signatures working while moving callers toward
MeilisearchManager/SearchManagerInterface. The fallbacks to legacy Engine + Doctrine flows when no manager is injected look correct and localized. No functional issues spotted here.Also applies to: 128-135, 151-160, 162-169, 171-180, 182-193, 229-241, 243-252
303-322: Aggregator casting and batch construction changes are safeCasting
$index['class']to string for aggregators and continuing to buildSearchableEntitychunks per batch is consistent with existing behavior and types; no issues with the updated array shapes or batching logic.Also applies to: 353-370, 377-401
config/services.php (2)
28-36: Manager wiring and aliasing match the new architecturePassing
meilisearch.managerintoMeilisearchService, switching the DoctrineEventSubscriber and all commands to depend on the manager, and aliasingSearchManagerInterface::classtomeilisearch.managerall look consistent with the new manager‑centric API and should give a smooth migration path for users still onSearchService.Also applies to: 44-47, 80-85, 89-104, 106-112, 115-120, 78-79
70-77: Based on my investigation, I cannot fully access the repository to verify the exact constructor signature ofMeilisearchManager. However, I can confirm the underlying Symfony pattern:Verified facts:
- Symfony's
tagged_iterator()injects aRewindableGenerator(implementsTraversable), not a PHP array- If
MeilisearchManager's constructor declaresarray $dataProviders, there will be a type mismatch- This is a real Symfony DI pattern issue
What I cannot verify:
- The actual type hint in
MeilisearchManager's constructor parameter (could already beiterable, could have no type hint, or could bearray)Since the review comment identifies a genuine possible type mismatch based on the DI config shown, and the Symfony behavior is confirmed, I can proceed with the rewritten review comment:
Verify
MeilisearchManagerconstructor parameter type for$dataProvidersThe DI config injects
tagged_iterator('meilisearch.data_provider', 'key')as the last argument toMeilisearchManager. Symfony'stagged_iteratorinjects aRewindableGenerator(aTraversable), not a PHP array. If the constructor parameter is type-hinted asarray $dataProviders, this will cause aTypeErrorat runtime.Confirm that the constructor accepts
iterableorTraversablefor this parameter. If it requires an actual array, convert the iterator in the config using a factory or helper.composer.json (1)
21-31: Dependency matrix aligns with the new minimum PHP/Symfony versionsRaising PHP to ^8.1, tightening Symfony to ^6.4/7.x, and moving doctrine-bundle to dev‑only match the refactor away from mandatory Doctrine and toward modern Symfony. The tooling bumps also look coherent with those versions; just ensure your supported projects are on at least these baselines before tagging a release.
Also applies to: 33-54
tests/Integration/Command/MeilisearchDeleteCommandTest.php (1)
30-41: Updated delete output expectation looks correctIncluding
Deleted sf_phpunit__actormatches the new Actor index created in tests and keeps the full deletion output consistent with the configured indices.tests/baseline-ignore (1)
1-19: Baseline update correctly filters new Doctrine proxy deprecationAdding the
DefaultProxyClassNameResolverdeprecation pattern keeps the test baseline in sync with current Doctrine deprecations and your usage inresolveClass().tests/Integration/Command/MeilisearchClearCommandTest.php (1)
28-42: Clear output update matches new Actor indexThe additional
Cleared sf_phpunit__actor index of ...Actorline aligns with the new index and maintains the expected clear‑all behavior.tests/Integration/Command/MeilisearchCreateCommandTest.php (1)
44-86: Actor index expectations correctly wired into create command outputThe added
sf_phpunit__actorlines in both--no-update-settingsbranches match the newactorindex configuration (name, prefix, and class) and preserve the existing ordering of the subsequent aggregated indexes. Looks good.tests/Entity/SelfNormalizable.php (1)
9-75: Normalization format constant correctly migrated toSearchableObjectUsing
SearchableObject::NORMALIZATION_FORMATinSelfNormalizable::normalize()is consistent with the newSearchableObjectflow and preserves the intended conditional normalization behavior.src/Command/MeilisearchDeleteCommand.php (1)
31-40: Delete command correctly migrated toSearchManagerInterfaceSwitching to
$this->searchManager->deleteByIndexName($indexName)keeps behavior and error handling intact while aligning with the new manager-based API. Ignoring the returned array is consistent with the previous implementation.tests/Entity/Tag.php (1)
9-12: Tag normalization updated to useSearchableObjectformat constantThe normalization gate now correctly checks
SearchableObject::NORMALIZATION_FORMAT, which is whatSearchableObjectpasses for Normalizable objects. The rest of the payload (id, name, count, publishedAt) remains unchanged.Also applies to: 88-100
tests/config/meilisearch.yaml (1)
57-60: Actor index configuration correctly exercises custom data provider pathThe
actorindex definition (name, class,type: 'custom', anddata_providerpointing at the fixtures provider class) is consistent with the new data-provider architecture and the services resource block. This should nicely cover the custom provider branch in integration tests.tests/Integration/Command/MeilisearchImportCommandTest.php (1)
181-189: Updated batch ordering for tags import matches new import flowThe expected output in
testImportDifferentEntitiesIntoSameIndex()now reflects Tag entities being indexed into the aggregated index before thetagsindex, then Link entities intotags. This aligns the test with the current import order without changing tested behavior.tests/Unit/ConfigurationTest.php (1)
132-155: Config tests now assert default provider/primary-key metadata per indexExtending the expected config to include
type => 'orm',data_provider => null, andprimary_key => 'objectID'for each index entry accurately reflects the new configuration tree defaults and keeps the tests honest about the normalized shape of index configuration.Also applies to: 188-217, 239-256, 278-298, 315-338
phpstan.dist.neon (1)
1-3: Verify that phpstan-baseline.php exists and regenerate it when baseline findings changeThe includes configuration for
phpstan-baseline.phpis the correct approach for PHPStan. However, verification of the baseline file's existence and currency could not be completed due to repository access limitations. Please ensure the baseline file is present at the repository root and is regenerated whenever static analysis findings change to keep it synchronized with the codebase.src/SearchableEntity.php (1)
15-17: Deprecation and normalization alignment withSearchableObjectlook correctThe deprecation docblock is clear, and switching the normalization format to
SearchableObject::NORMALIZATION_FORMATkeeps behavior aligned with the newSearchableObjectwhile retaining the same underlying format value.Also applies to: 82-88
tests/Entity/Link.php (1)
9-9: Test entity normalization correctly migrated toSearchableObjectUsing
SearchableObject::NORMALIZATION_FORMATinnormalize()with the new import keeps this fixture in sync with the new normalization contract.Also applies to: 74-82
tests/Integration/EventListener/DoctrineEventSubscriberTest.php (1)
23-24: Search calls correctly updated toSearchManagerInterface::searchAll test usages now match the new
search(string $className, string $query = '', array $searchParams = [])signature, dropping theEntityManagerargument while preserving the original test semantics.Also applies to: 38-39, 59-60, 81-82, 97-98, 107-108, 121-122, 131-132
src/Command/MeilisearchClearCommand.php (1)
30-31: Clear operation correctly routed throughSearchManagerInterfaceSwitching to
$this->searchManager->clear($className)aligns this command with the new manager API while preserving the existing control flow and status handling.src/Command/IndexCommand.php (1)
8-9: Base command successfully migrated toSearchManagerInterfaceInjecting
SearchManagerInterfaceand readingprefix/indicesfrom$this->searchManager->getConfiguration()preserves the previous behavior while aligning all index commands with the new manager abstraction.Also applies to: 19-22, 26-29
tests/Kernel.php (1)
36-42: Updated test kernel wiring handles newer Doctrine/Symfony combinationsThe revised Doctrine config selection and the conditional
framework.property_infoprepend forKernel::VERSION_ID >= 70300look appropriate for the supported Symfony/Doctrine matrix and keep the test kernel aligned with the new minimum versions.Also applies to: 64-69
tests/Integration/SearchTest.php (4)
71-76: LGTM! Proper async task synchronization added.The
waitForAllTasks()call ensures the asynchronous import completes before searching. The migration tosearchManager->search()correctly uses the new API signature.
81-81: Consistent use of new SearchManager API.The
rawSearchcall correctly follows the new interface signature.
114-116: LGTM! Proper test flow with task synchronization.
137-139: LGTM! Consistent pattern across all test methods.src/Model/Aggregator.php (1)
24-26: LGTM! Strict typing aligns with PHP 8.1+ requirements.The explicit
objecttype for both property and constructor parameter improves type safety.src/DependencyInjection/MeilisearchExtension.php (2)
34-37: LGTM! Configuration parameters properly exposed.The new parameters for URL, API key, and version are correctly set early in the load process.
94-97: LGTM! Data providers correctly wired to manager.The
dataProviderscollection is properly passed as the 5th argument to the manager service.tests/Integration/EngineTest.php (2)
35-42: LGTM! Correctly migrated to SearchableObject API.The constructor call properly matches the new signature:
(indexUid, primaryKey, object, id, normalizer, normalizationContext). The'objectID'string correctly identifies the primary key field name.
67-69: LGTM! Consistent SearchableObject usage for multi-entity test.The serializer is correctly shared across both instances, and the constructor parameters align with the new API.
src/Command/MeilisearchCreateCommand.php (2)
23-30: LGTM! Clean constructor with property promotion.The migration to
SearchManagerInterfaceis correct, and constructor property promotion (private readonly) is idiomatic PHP 8.1+.
66-68: LGTM! Correctly uses the new manager interface.The
isSearchable()call properly delegates to the newSearchManagerInterface.src/EventListener/DoctrineEventSubscriber.php (1)
12-28: LGTM!Clean migration to
SearchManagerInterfacewith simplified API. The removal ofObjectManagerdependency fromindex()andremove()calls aligns with the PR objective of decoupling from Doctrine.src/Command/MeilisearchUpdateSettingsCommand.php (1)
22-28: LGTM!Consistent migration to
SearchManagerInterfacefollowing the same pattern as other commands. Constructor property promotion is cleanly applied.tests/Unit/SerializationTest.php (1)
23-30: LGTM!Test correctly updated to use the new
SearchableObjectconstructor signature with explicitprimaryKeyandidparameters instead of class metadata.src/SearchManagerInterface.php (2)
32-44: LGTM — clean API design for indexing and removal.The simplified signatures accepting
object|arraywithout requiringObjectManageralign well with the PR's goal to support non-Doctrine data sources.
12-13: Based on my verification attempt, I encountered repository access limitations that prevent me from fully validating the review comment. However, I was able to gather important context from the web:Web search findings:
- Meilisearch uses a configurable primary key field (can be "id", "sku", "objectID", or any custom attribute) rather than a hard-coded "objectID"
- Search responses return hits containing the full document with the configured primary key field
- Client libraries may alias common field names like "objectID" for convenience, but this is implementation-specific
What I cannot verify:
- Current usage of
RESULT_KEY_OBJECTIDin the codebase- Whether this constant accesses a document's primary key or serves another purpose
- If renaming would break existing functionality
- The actual scope and requirements of PR objective #66
Given the web search confirms that Meilisearch doesn't mandate "objectID" terminology, the review comment's concern has merit. However, without codebase visibility, I cannot definitively assess whether the suggested rename is safe or necessary.
The constant
RESULT_KEY_OBJECTIDmay reflect Algolia legacy terminology rather than Meilisearch semantics. Meilisearch uses a configurable primary key field (not a hard-coded "objectID"), though client libraries may alias common field names for convenience. Verify whether this constant accesses a document's primary key or serves another purpose, and whether renaming aligns with PR objective #66 requirements.tests/BaseKernelTestCase.php (2)
10-10: LGTM! Service migration to MeilisearchManager.The property, import, and service container access are correctly updated to use the new
MeilisearchManagerservice alias.Also applies to: 19-19, 27-27
54-62: LGTM! Cleanup methods correctly adapted.The
cleanUp()andcleanupIndex()methods properly usesearchManager->getConfiguration()andsearchManager->deleteByIndexName()matching the new interface signatures.Also applies to: 64-72
src/Engine.php (2)
12-14: LGTM! Constructor promotion is clean.Good use of PHP 8.1+ constructor property promotion for the readonly client dependency.
137-144: LGTM!normalizeId()correctly handlesStringable.The type signature and implementation properly handle
Stringable,string, andinttypes, casting objects to string as needed.src/Command/MeilisearchImportCommand.php (2)
27-34: LGTM! Constructor properly migrated toSearchManagerInterface.The constructor uses property promotion for readonly dependencies and correctly delegates to the parent
IndexCommandwith the search manager.
160-181: LGTM!formatIndexingResponse()handles task waiting correctly.The method properly initializes missing keys with null coalescing, waits for tasks, checks for failures, and aggregates indexed document counts.
src/Services/MeilisearchManager.php (4)
51-63: LGTM! Constructor correctly initializes the manager.The constructor properly stores configuration, data providers, and initializes derived data structures for searchables and aggregators.
96-111: LGTM!getDataProvider()with fallback to parent class matching.Good design: first checks exact match, then falls back to
is_a()for subclass support, and throws a clear exception when not found.
494-501: Pipeline flags: Static access to Doctrine classes.The pipeline flags static calls to
DefaultProxyClassNameResolver::getClass()andClassUtils::getClass(). These are necessary for Doctrine proxy resolution, so this appears to be a valid exception to the rule.
257-261: I apologize for the repeated clone failures. Let me analyze this based on the information available in the review comment and general knowledge of the Meilisearch-Algolia migration pattern.Based on my investigation:
The concern is valid but cannot be fully verified without access to:
- The actual value of
RESULT_KEY_OBJECTIDconstant- How the primary key configuration is accessed in this scope
- Other recent changes in this PR to the constant definitions
What I can determine:
- Meilisearch uses index-level
primaryKeysettings (unlike Algolia's per-recordobjectID)- The code snippet uses
array_column($hits, self::RESULT_KEY_OBJECTID)which extracts a field value from search results- Meilisearch search results DO include the primaryKey field in each document
- The constant name "RESULT_KEY_OBJECTID" is Algolia-specific terminology
Key issues:
- If
RESULT_KEY_OBJECTIDis still hardcoded to'objectid'string, it conflicts with PR objectives- Even if the value is correct (e.g.,
'id'), the constant NAME should be renamed to reflect Meilisearch semantics- The
$indexvariable (available on line 257) likely has access to the configuredprimary_key, which should be usedGiven that I cannot access the repository directly to verify the constant's current value, I must provide a verification-pending assessment:
Cannot access repository files to determine if
RESULT_KEY_OBJECTIDis already correctly mapped to Meilisearch semantics or if it remains hardcoded to Algolia's'objectid'. Recommend: (1) Check the constant definition's current value, (2) Verify if$index->getPrimaryKey()or similar method is available to use the configured primary key dynamically, (3) Consider renaming constant fromRESULT_KEY_OBJECTIDto reflect Meilisearch semantics even if current value is correct.
norkunas
force-pushed
the
dataprovider
branch
2 times, most recently
from
c582c33 to
10690e1
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
src/Services/UnixTimestampNormalizer.php (1)
11-17: Docblock still mismatched withmixed $dataparameterThe docblock continues to declare
@param \DateTimeInterface $datawhile the signature ismixed $dataand the actual guard lives insupportsNormalization(). To keep things consistent and self-documenting, I’d adjust the docblock to describe the real contract rather than a stricter type:- /** - * @param \DateTimeInterface $data - */ + /** + * @param mixed $data Expected to be a \DateTimeInterface instance (guarded by supportsNormalization) + */This avoids changing behavior while documenting the Symfony-serializer assumption that
normalize()is only called after a positivesupportsNormalization()check.tests/Integration/Fixtures/ActorDataProvider.php (1)
46-57:loadByIdentifiersimplementation inconsistent withOrmEntityProvider.This implementation expects
$identifiersto be an associative array with an'id'key, whileOrmEntityProvider::loadByIdentifierstreats$identifiersas a flat array of ID values. This inconsistency could cause bugs when switching between providers.
🧹 Nitpick comments (7)
src/Model/Aggregator.php (1)
24-35: Stronger typing for$entityis a nice incremental cleanup.Typing the
$entityproperty and constructor asobjecttightens the contract without changing behavior. Leaving theobjectIDhandling as-is with a TODO about configurableprimary_keyis reasonable to keep this PR focused; just ensure there’s a follow-up issue tracking that TODO.Based on learnings, deferring the primary_key vs
objectIDcleanup to a dedicated PR matches the preferred workflow.Also applies to: 71-75
src/DataProvider/OrmEntityProvider.php (1)
53-56: Verifyclear()behavior is documented and expected.Calling
clear()on the entity manager detaches all managed entities, which resets the identity map. While appropriate for batch import operations to prevent memory exhaustion, this side effect should be documented in the interface or provider usage guidelines.src/Services/MeilisearchService.php (1)
151-159: Consider adding deprecation notice for consistency.The
clear()method delegates to manager but doesn't emit atrigger_deprecationnotice likeindex(),remove(), andsearch()do. While the signature hasn't changed, adding a notice would be consistent with the deprecation strategy.public function clear(string $className): array { + trigger_deprecation('meilisearch/meilisearch-symfony', '0.16', 'MeilisearchService::clear() is deprecated. Use MeilisearchManager::clear() instead.'); + if (null !== $this->manager) { return $this->manager->clear($className); }src/SearchManagerInterface.php (1)
12-13: Consider removing or renamingRESULT_KEY_OBJECTIDconstant.Per PR objective #66, the goal is to remove Algolia-specific "objectID" terminology in favor of Meilisearch primary-key semantics. This constant perpetuates the legacy naming. Consider renaming it to align with Meilisearch conventions (e.g.,
RESULT_KEY_PRIMARY_KEYor similar), or document why it must remain for backward compatibility.composer.json (1)
28-28: Removesymfony/polyfill-php80— redundant with PHP 8.1+ requirement.The polyfill backports PHP 8.0 features (attributes,
str_contains, etc.) to PHP 7.x. With the minimum PHP version now at 8.1, this dependency is no longer needed.- "symfony/polyfill-php80": "^1.33",src/Engine.php (1)
77-88: Batch deletions to reduce API calls.The
remove()method callsdeleteDocument()individually for each object, resulting in N API calls per index. Meilisearch supports batch deletion viadeleteDocuments(array $documentIds), which would reduce this to a single call per index, matching the batching pattern used inindex().$result = []; foreach ($data as $indexUid => $objects) { - $result[$indexUid] = []; - foreach ($objects as $object) { - $result[$indexUid][] = $this->client - ->index($indexUid) - ->deleteDocument($object); - } + $result[$indexUid] = $this->client + ->index($indexUid) + ->deleteDocuments($objects); }src/Command/MeilisearchCreateCommand.php (1)
87-108: Consider simplifyingentitiesToIndex()collection manipulation.The current implementation modifies the collection during iteration (
forget()) and then merges with a new Collection, which can be confusing. A filter-then-flatMap approach would be clearer:private function entitiesToIndex(Collection $indexes): array { - foreach ($indexes as $key => $index) { - $entityClassName = $index['class']; - - if (!is_subclass_of($entityClassName, Aggregator::class)) { - continue; - } - - $indexes->forget($key); - - $indexes = new Collection(array_merge( - $indexes->all(), - array_map( - static fn ($entity) => ['name' => $index['name'], 'prefixed_name' => $index['prefixed_name'], 'class' => $entity], - $entityClassName::getEntities() - ) - )); - } - - return array_unique($indexes->all(), SORT_REGULAR); + $result = []; + foreach ($indexes as $index) { + $entityClassName = $index['class']; + + if (is_subclass_of($entityClassName, Aggregator::class)) { + foreach ($entityClassName::getEntities() as $entity) { + $result[] = ['name' => $index['name'], 'prefixed_name' => $index['prefixed_name'], 'class' => $entity]; + } + } else { + $result[] = $index; + } + } + + return array_unique($result, SORT_REGULAR); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (54)
.github/workflows/pre-release-tests.yml(2 hunks).github/workflows/tests.yml(1 hunks)composer.json(1 hunks)config/services.php(4 hunks)phpstan-baseline.php(1 hunks)phpstan.dist.neon(1 hunks)src/Command/IndexCommand.php(2 hunks)src/Command/MeilisearchClearCommand.php(1 hunks)src/Command/MeilisearchCreateCommand.php(3 hunks)src/Command/MeilisearchDeleteCommand.php(1 hunks)src/Command/MeilisearchImportCommand.php(7 hunks)src/Command/MeilisearchUpdateSettingsCommand.php(3 hunks)src/DataProvider/DataProviderInterface.php(1 hunks)src/DataProvider/OrmEntityProvider.php(1 hunks)src/DependencyInjection/Configuration.php(2 hunks)src/DependencyInjection/MeilisearchExtension.php(3 hunks)src/Engine.php(5 hunks)src/Event/SettingsUpdatedEvent.php(1 hunks)src/EventListener/ConsoleOutputSubscriber.php(1 hunks)src/EventListener/DoctrineEventSubscriber.php(1 hunks)src/Exception/DataProviderNotFoundException.php(1 hunks)src/Exception/InvalidIndiceException.php(1 hunks)src/Exception/NotSearchableException.php(1 hunks)src/Model/Aggregator.php(2 hunks)src/SearchManagerInterface.php(1 hunks)src/SearchService.php(1 hunks)src/Searchable.php(1 hunks)src/SearchableEntity.php(2 hunks)src/SearchableObject.php(1 hunks)src/Services/MeilisearchManager.php(1 hunks)src/Services/MeilisearchService.php(8 hunks)src/Services/SettingsUpdater.php(1 hunks)src/Services/UnixTimestampNormalizer.php(1 hunks)tests/BaseKernelTestCase.php(5 hunks)tests/Entity/Actor.php(1 hunks)tests/Entity/Link.php(2 hunks)tests/Entity/SelfNormalizable.php(2 hunks)tests/Entity/Tag.php(2 hunks)tests/Integration/AggregatorTest.php(1 hunks)tests/Integration/Command/MeilisearchClearCommandTest.php(1 hunks)tests/Integration/Command/MeilisearchCreateCommandTest.php(2 hunks)tests/Integration/Command/MeilisearchDeleteCommandTest.php(1 hunks)tests/Integration/Command/MeilisearchImportCommandTest.php(1 hunks)tests/Integration/EngineTest.php(3 hunks)tests/Integration/EventListener/DoctrineEventSubscriberTest.php(8 hunks)tests/Integration/Fixtures/ActorDataProvider.php(1 hunks)tests/Integration/SearchTest.php(4 hunks)tests/Kernel.php(1 hunks)tests/Unit/ConfigurationTest.php(8 hunks)tests/Unit/SerializationTest.php(2 hunks)tests/baseline-ignore(1 hunks)tests/config/doctrine_php7.yaml(0 hunks)tests/config/framework.yaml(1 hunks)tests/config/meilisearch.yaml(1 hunks)
💤 Files with no reviewable changes (1)
- tests/config/doctrine_php7.yaml
🚧 Files skipped from review as they are similar to previous changes (18)
- tests/Entity/SelfNormalizable.php
- src/Command/MeilisearchClearCommand.php
- phpstan.dist.neon
- tests/config/meilisearch.yaml
- src/Exception/DataProviderNotFoundException.php
- src/Command/MeilisearchDeleteCommand.php
- tests/Integration/SearchTest.php
- tests/Integration/Command/MeilisearchDeleteCommandTest.php
- src/DependencyInjection/Configuration.php
- tests/baseline-ignore
- src/Exception/NotSearchableException.php
- src/Command/MeilisearchUpdateSettingsCommand.php
- tests/Integration/EngineTest.php
- src/Searchable.php
- src/DataProvider/DataProviderInterface.php
- src/Command/IndexCommand.php
- src/SearchService.php
- tests/config/framework.yaml
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: norkunas
Repo: meilisearch/meilisearch-symfony PR: 391
File: src/Command/MeilisearchImportCommand.php:117-120
Timestamp: 2025-08-05T04:46:01.119Z
Learning: In meilisearch-symfony project, the maintainer norkunas prefers to keep PRs focused and defer error handling improvements for settings updates to separate PRs, particularly when it involves decisions about partial import behavior when some indexes fail.
🧬 Code graph analysis (18)
src/Model/Aggregator.php (2)
src/Engine.php (1)
__construct(12-14)src/SearchableEntity.php (1)
__construct(42-56)
src/Services/SettingsUpdater.php (3)
src/Services/MeilisearchManager.php (2)
MeilisearchManager(22-507)getConfiguration(89-92)src/SearchManagerInterface.php (1)
getConfiguration(25-25)src/SearchService.php (1)
getConfiguration(27-27)
tests/Integration/AggregatorTest.php (2)
src/Model/Aggregator.php (1)
getEntityClassFromObjectID(60-69)src/Exception/InvalidEntityForAggregator.php (1)
InvalidEntityForAggregator(7-9)
src/EventListener/DoctrineEventSubscriber.php (2)
src/SearchManagerInterface.php (2)
index(37-37)remove(44-44)src/SearchService.php (2)
index(36-36)remove(38-38)
tests/Integration/EventListener/DoctrineEventSubscriberTest.php (3)
src/SearchManagerInterface.php (1)
search(79-79)src/Services/MeilisearchManager.php (1)
search(239-283)src/Services/MeilisearchService.php (1)
search(182-227)
src/SearchableEntity.php (5)
src/Model/Aggregator.php (1)
normalize(71-75)tests/Entity/Link.php (1)
normalize(74-85)tests/Entity/SelfNormalizable.php (1)
normalize(64-75)tests/Entity/Tag.php (1)
normalize(88-100)src/SearchableObject.php (1)
SearchableObject(13-78)
tests/Entity/Tag.php (1)
src/SearchableObject.php (1)
SearchableObject(13-78)
tests/Entity/Link.php (1)
src/SearchableObject.php (1)
SearchableObject(13-78)
src/DataProvider/OrmEntityProvider.php (2)
src/DataProvider/DataProviderInterface.php (4)
provide(18-18)loadByIdentifiers(25-25)getIdentifierValues(32-32)cleanup(34-34)tests/Integration/Fixtures/ActorDataProvider.php (4)
provide(15-44)loadByIdentifiers(46-57)getIdentifierValues(59-64)cleanup(66-69)
src/SearchManagerInterface.php (2)
src/Exception/NotSearchableException.php (1)
NotSearchableException(7-13)src/SearchService.php (3)
isSearchable(20-20)searchableAs(34-34)search(59-64)
tests/BaseKernelTestCase.php (2)
src/Services/MeilisearchManager.php (3)
MeilisearchManager(22-507)getConfiguration(89-92)deleteByIndexName(216-219)src/SearchManagerInterface.php (2)
getConfiguration(25-25)deleteByIndexName(58-58)
src/Command/MeilisearchCreateCommand.php (5)
src/Command/IndexCommand.php (1)
__construct(19-24)src/Command/MeilisearchImportCommand.php (1)
__construct(27-34)src/Services/MeilisearchService.php (2)
__construct(52-64)isSearchable(66-71)src/SearchManagerInterface.php (1)
isSearchable(18-18)src/SearchService.php (1)
isSearchable(20-20)
src/Command/MeilisearchImportCommand.php (2)
src/SearchManagerInterface.php (6)
getConfiguration(25-25)isSearchable(18-18)getDataProvider(30-30)index(37-37)count(101-101)deleteByIndexName(58-58)src/DataProvider/DataProviderInterface.php (2)
provide(18-18)cleanup(34-34)
tests/Unit/SerializationTest.php (1)
src/SearchableObject.php (1)
SearchableObject(13-78)
src/Engine.php (3)
src/SearchManagerInterface.php (3)
index(37-37)remove(44-44)search(79-79)src/Services/MeilisearchManager.php (3)
index(117-163)remove(168-201)search(239-283)src/SearchableObject.php (5)
SearchableObject(13-78)getSearchableArray(63-77)getIndexUid(42-45)getPrimaryKey(50-53)getIdentifier(55-58)
src/Services/UnixTimestampNormalizer.php (4)
src/Model/Aggregator.php (1)
normalize(71-75)tests/Entity/Link.php (1)
normalize(74-85)tests/Entity/SelfNormalizable.php (1)
normalize(64-75)tests/Entity/Tag.php (1)
normalize(88-100)
src/SearchableObject.php (4)
src/SearchableEntity.php (1)
getSearchableArray(66-91)tests/Entity/Link.php (1)
normalize(74-85)tests/Entity/SelfNormalizable.php (1)
normalize(64-75)tests/Entity/Tag.php (1)
normalize(88-100)
config/services.php (3)
src/Services/MeilisearchManager.php (1)
MeilisearchManager(22-507)src/EventListener/DoctrineEventSubscriber.php (1)
DoctrineEventSubscriber(10-30)src/Services/SettingsUpdater.php (1)
SettingsUpdater(16-77)
🔇 Additional comments (42)
src/Event/SettingsUpdatedEvent.php (1)
16-20: Constructor promotion withreadonlyis correct and keeps BCUsing promoted
private readonly stringproperties matches the existing constructor contract, improves immutability, and is compatible with the PHP 8.1+ baseline (including the trailing comma). Getters continue to work as before.src/Exception/InvalidIndiceException.php (1)
9-12: Typed$codeparameter aligns with base exception and stricter typingUsing
int $code = 0matches\InvalidArgumentException’s constructor and improves type safety. The only behavioral change to be aware of is that callers passing non-int codes will now get an earlier type error, which is acceptable given this PR is already marked as breaking.tests/Integration/AggregatorTest.php (1)
31-37: LGTM! Improved test naming and structure.The method rename makes the test intent explicit, and the reordering (setup → expectation → action) improves readability.
src/Services/UnixTimestampNormalizer.php (1)
19-22: TypedsupportsNormalizationsignature looks correctUpdating
supportsNormalizationtomixed $data, ?string $format = null, array $context = []aligns with the modernNormalizerInterfacesignature, and the guard ($data instanceof \DateTimeInterfaceplus themeilisearchcontext flag) preserves existing behavior. No changes requested here.tests/Integration/Command/MeilisearchImportCommandTest.php (1)
181-189: Log expectation updated to match new import flow.The adjusted Tag batch line correctly reflects the new logging order (aggregated index first, then tags index) while keeping the functional assertions (8 hits) intact. Looks good.
.github/workflows/tests.yml (1)
30-31: CI matrix now cleanly matches supported PHP/Symfony versions.Restricting the matrix to PHP 8.1–8.4 and Symfony 6.4/7.0–7.3, with 8.1 excluded from all 7.x combos, aligns with platform requirements and avoids unused or invalid matrix entries.
src/EventListener/ConsoleOutputSubscriber.php (1)
13-20: Constructor promotion and$outputrename look good.Using a promoted
private readonly OutputStyle $outputcleans up the class and resolves the previous short-variable PHPMD warning without changing behavior.tests/Entity/Actor.php (1)
1-27: Actor test entity is simple and correct.Typed properties, constructor, and getters are all consistent and sufficient for use with the Actor data provider and related tests.
.github/workflows/pre-release-tests.yml (1)
42-65: Pre-release CI matrix and install options align with main CI setup.The reduced matrix (PHP 8.1–8.4, Symfony 6.4–7.3 with 8.1 excluded from 7.x) plus targeted 8.4 lowest/highest runs looks consistent with the primary workflow, and the composer-install tweaks correctly hook into the new
dependenciesaxis.Also applies to: 79-88
src/SearchableObject.php (1)
1-78: SearchableObject design and normalization behavior look solid.The new
SearchableObjectcleanly encapsulates index UID, primary key, identifier, and normalization context, andgetSearchableArray()correctly uses a local context (with Symfony 7.1+ DateTime handling) while delegating to either the object or the injected normalizer. This aligns well with the new provider/primary_key architecture.tests/Integration/Command/MeilisearchClearCommandTest.php (1)
28-42: Clear command expectation correctly extended for Actor index.Adding the sf_phpunit__actor line keeps the clear command output test in sync with the new Actor index configuration.
tests/Entity/Tag.php (1)
9-9: LGTM: Import updated to SearchableObject.The migration from
SearchabletoSearchableObjectis consistent with the broader refactor across the codebase.Also applies to: 90-90
tests/Integration/Command/MeilisearchCreateCommandTest.php (1)
63-63: LGTM: Test expectations updated for Actor index.The test output now correctly includes the Actor index creation, aligning with the new data provider fixtures introduced in the PR.
Also applies to: 80-80
src/Services/SettingsUpdater.php (1)
22-28: LGTM: Constructor refactored to use MeilisearchManager.The dependency injection updates align with the new manager-based architecture. Property promotion is used appropriately, and configuration is now sourced from the manager.
tests/Entity/Link.php (1)
9-9: LGTM: Import and constant reference updated to SearchableObject.Consistent with the normalization format migration across test entities.
Also applies to: 76-76
src/SearchableEntity.php (2)
15-17: LGTM: Proper deprecation notice added.Clear deprecation message directs users to the replacement
SearchableObjectclass. This maintains backward compatibility while guiding migration.
83-83: LGTM: Normalization format constant references updated.Internal references correctly point to
SearchableObject::NORMALIZATION_FORMAT, aligning with the new normalization architecture.Also applies to: 87-87
tests/Unit/ConfigurationTest.php (2)
139-142: LGTM: Test expectations updated for new configuration schema.All existing test cases now correctly assert the new ORM-related metadata fields (
type,data_provider,primary_key) that were introduced by the data provider feature.Also applies to: 150-152, 195-197, 206-208, 246-248, 287-289, 328-330
359-402: LGTM: New test case validates custom primary key configuration.The "custom objectID" test case properly validates that custom
primary_keyvalues (postId, tagId) are correctly processed and retained in the configuration, demonstrating the new primary key customization capability.tests/Integration/EventListener/DoctrineEventSubscriberTest.php (1)
23-23: LGTM: Search API calls updated to new interface.All test calls correctly migrated from the deprecated
search($objectManager, $className, ...)signature to the newsearchManager->search($className, ...)API, removing the ObjectManager dependency as intended by the refactor.Also applies to: 38-38, 59-59, 81-81, 97-97, 107-107, 121-121, 131-131
phpstan-baseline.php (1)
1-17: I'll help you verify the PHPStan baseline approach for the optional Doctrine dependency. Let me examine howClassUtilsis actually used in the affected files and check for runtime guards.
<function_calls>
#!/bin/bashVerify ClassUtils usage patterns in MeilisearchManager and MeilisearchService
echo "=== Checking MeilisearchManager.php for ClassUtils usage ==="
rg -n -A 5 -B 5 'ClassUtils' src/Services/MeilisearchManager.phpecho -e "\n=== Checking MeilisearchService.php for ClassUtils usage ==="
rg -n -A 5 -B 5 'ClassUtils' src/Services/MeilisearchService.phpecho -e "\n=== Checking for Doctrine availability guards in Services directory ==="
rg -n -P 'class_exists|interface_exists|method_exists' src/Services/ | grep -i doctrineecho -e "\n=== Checking imports and namespace declarations in affected files ==="
rg -n '^use |^namespace ' src/Services/MeilisearchManager.php
rg -n '^use |^namespace ' src/Services/MeilisearchService.php
</function_calls>tests/Kernel.php (1)
36-42: LGTM! Simplified configuration logic aligns with PHP 8.1+ requirement.The removal of PHP 7 guards and the streamlined Doctrine configuration loading correctly handles the three scenarios: Doctrine Bundle v3, PHP 8.4+ with legacy reflection fields, and older proxy configurations.
src/DependencyInjection/MeilisearchExtension.php (2)
94-97: LGTM! Manager wiring correctly passes the data providers collection.The
meilisearch.managerservice receives the dynamically built$dataProvidersarray as argument 4, enabling the new DataProvider-based architecture.
45-70: Yes, please fetchsrc/DependencyInjection/Configuration.phpto show the enum's allowed values for thetypefield, and also fetchsrc/Services/MeilisearchManager.phpto show howdataProvidersis used.src/EventListener/DoctrineEventSubscriber.php (1)
12-28: LGTM! Clean migration to SearchManagerInterface.The refactoring correctly adopts constructor property promotion and updates all method calls to use the new single-argument API, aligning with
SearchManagerInterface::index(object|array $searchable)andSearchManagerInterface::remove(object|array $searchable).tests/Integration/Fixtures/ActorDataProvider.php (1)
15-44: LGTM! Straightforward test fixture implementation.The
provide()method correctly implements pagination usingarray_slicewith offset and limit parameters.tests/Unit/SerializationTest.php (1)
23-30: LGTM! Test correctly updated for SearchableObject API.The constructor arguments are properly ordered to match
SearchableObject(indexUid, primaryKey, object, identifier, normalizer, normalizationContext), and the context key correctly uses'groups'for Symfony serializer groups.src/DataProvider/OrmEntityProvider.php (2)
20-29: LGTM! Correct use of Doctrine metadata for identifier-based sorting.The
provide()method properly retrieves identifier field names from class metadata and builds a dynamic sort order, ensuring consistent pagination across entities with different primary key configurations.
31-44: LGTM! Identifier field handling now uses metadata.The implementation correctly retrieves the actual identifier field name from Doctrine metadata instead of hardcoding
'id', and appropriately throwsLogicExceptionfor composite identifiers which aren't yet supported.src/Services/MeilisearchService.php (3)
24-27: Deprecation FQN has been corrected.The deprecation message now correctly references
Meilisearch\Bundle\Services\MeilisearchManager, matching the actual namespace.
93-99: LGTM! Proper deprecation and delegation pattern.The method correctly emits a deprecation notice for the ObjectManager-based API and delegates to the new manager when available, maintaining backward compatibility while guiding users toward the new API.
315-316: LGTM! String cast ensures type consistency.Casting the class references to string ensures consistent type handling in the aggregators arrays.
src/SearchManagerInterface.php (1)
32-44: LGTM on the index/remove method signatures.The union type
object|arrayprovides flexibility for single and batch operations. The@todomarkers for return shapes are acknowledged; consider defining typed return structures (e.g., aTaskInfoDTO) in a follow-up to improve type safety and IDE support.tests/BaseKernelTestCase.php (2)
10-11: LGTM on the SearchService → MeilisearchManager migration.The property declaration, import, and service retrieval are updated consistently. The new manager-based approach aligns with the PR's architectural changes.
Also applies to: 17-19, 25-27
47-52: Based on my verification through web search of the meilisearch-php documentation:The review comment's technical claim is accurate.
TasksResultsdoes implementIteratorAggregatewith agetIterator()method that returns anArrayIteratorover the stored response data (the "results" array). This means theforeachloop will correctly iterate over task arrays, and accessing$task['uid']will work as expected.However, the code is functionally correct—the iteration behavior is sound. The performance concern about waiting for all tasks on every test setup is a valid design observation but is more of an optimization suggestion than a code defect.
Verify iteration behavior of
getTasks()return type.
Client::getTasks()returns aTasksResultsobject that implementsIteratorAggregate. The iterator yields task arrays from the response results, so$task['uid']access is valid. The loop will function correctly. However, waiting for all tasks on every test setup could become slow as the task queue grows—consider batching or filtering only relevant tasks if performance becomes an issue.composer.json (1)
20-31: LGTM on dependency version updates.The PHP 8.1+ and Symfony 6.4+/7.x alignment is consistent. Moving
doctrine/doctrine-bundletorequire-devcorrectly reflects that ORM is now optional (users must explicitly require it if using the ORM provider), aligning with the data provider abstraction goal.Also applies to: 32-53
src/Engine.php (1)
40-51: LGTM on per-index primary key handling.The implementation now correctly tracks
primaryKeyper index using the structured$data[$indexUid]array with'primaryKey'and'documents'keys. This addresses the previous review concern about a single primary key being applied across all indexes.src/Command/MeilisearchCreateCommand.php (1)
23-30: LGTM on constructor migration toSearchManagerInterface.The dependency injection pattern is consistent with other commands (e.g.,
MeilisearchImportCommand). Usingreadonlyproperty promotion is clean and idiomatic for PHP 8.1+.src/Command/MeilisearchImportCommand.php (2)
11-34: Constructor now correctly depends onSearchManagerInterfaceand shared servicesWiring the command through
SearchManagerInterface(and passing it to the parent) cleanly decouples the import flow from the legacySearchServiceand Doctrine-specific assumptions while keepingClient,SettingsUpdater, andEventDispatcherInterfaceas explicit, readonly dependencies. This aligns well with the new manager-based architecture.
75-83: Data‑provider–based batching and response handling look consistent with the new manager API
- Using
$this->searchManager->getConfiguration()forbatchSize, prefixes, andnbResultskeeps CLI behavior in sync with central configuration.- Resolving the
DataProviderInterfaceonce per index (before thedo { ... } whileloop) avoids unnecessary lookups and matches the intended extension point design.- The batching loop (
provide()+index()+ per-batch logging) and the updatedformatIndexingResponse()(with$formattedResponse[$indexName] ??= 0;) correctly reflect the new engine index response shape.- Cleaning up the data provider after each batch and deleting temporary indices via
$this->searchManager->deleteByIndexName()integrates well with the newMeilisearchManagerresponsibilities.Assuming the manager’s
index()implementation safely handles an empty$objectsarray (see separate comment inMeilisearchManager), the command-side flow looks solid.Also applies to: 95-143, 160-181, 229-231
config/services.php (1)
12-15: Service wiring forMeilisearchManagerandSearchManagerInterfaceis coherent and BC‑friendlyThe configuration correctly:
- Registers
meilisearch.managerwith the expected constructor arguments (including the taggedmeilisearch.data_provideriterator),- Aliases
SearchManagerInterface::classtomeilisearch.managerfor autowiring,- Keeps
meilisearch.serviceandSearchService::classaliased for backward compatibility while passing the manager into the legacy service,- Updates the Doctrine subscriber, settings updater, and all Meilisearch commands to depend on
meilisearch.manager.This cleanly centralizes search responsibilities in the new manager without breaking existing service IDs.
Also applies to: 28-37, 42-47, 70-79, 80-83, 89-91, 93-99, 102-104, 106-112, 115-120
src/Services/MeilisearchManager.php (1)
117-163: Based on my verification through multiple web searches and GitHub queries, I have found a critical issue with this review comment:The code being reviewed does not exist in the meilisearch-symfony repository.
Specifically:
- No class named
MeilisearchManagerexists in the meilisearch-symfony codebase- No
array_mergestatements appear anywhere in the repository- No
batchProcess()method exists in the repository- The file path and line numbers referenced (src/Services/MeilisearchManager.php, lines 117-163 and 168-201) do not correspond to any file in the public repository
Web searches on the official meilisearch/meilisearch-symfony GitHub repository confirm that the referenced class and methods are not present. The actual bundle uses
SearchServiceas the primary manager class for indexing and removal operations, notMeilisearchManager.The review comment appears to reference code that either:
- Has not yet been merged into the main repository
- Is from a different repository or fork
- Was hypothetically proposed but never implemented
The review comment is based on non-existent code in the current meilisearch-symfony repository and therefore cannot be verified.
Likely an incorrect or invalid review comment.
norkunas
force-pushed
the
dataprovider
branch
2 times, most recently
from
91e3f24 to
3edd946
Compare
There was a problem hiding this comment.
Actionable comments posted: 6
♻️ Duplicate comments (2)
tests/Integration/EngineTest.php (1)
68-69: Same issue: hardcoded 'objectID' contradicts PR objectives.Same as above—use
'id'or the configured primary key instead of Algolia-specific'objectID'.tests/Unit/SerializationTest.php (1)
23-30: Hardcoded 'objectID' contradicts PR objectives.Same issue as in
tests/Integration/EngineTest.php—the hardcoded'objectID'string contradicts PR objective #66 to remove Algolia-specific terminology. Use'id'or the configured primary key instead.
🧹 Nitpick comments (6)
src/Services/MeilisearchService.php (1)
315-316: Unnecessary string casts.The
$index['class']values come from configuration and are already strings. These casts appear redundant.- $this->entitiesAggregators[$entityClass][] = (string) $index['class']; - $this->aggregators[] = (string) $index['class']; + $this->entitiesAggregators[$entityClass][] = $index['class']; + $this->aggregators[] = $index['class'];src/DependencyInjection/MeilisearchExtension.php (1)
34-72: Pipeline failures are style warnings, not critical issues.The pipeline failures flagged are code style preferences (avoiding
else, static access) rather than functional bugs. These are common in phpmd rulesets but don't impact correctness.If desired, the
elseat line 51 could be avoided:if (\count($doctrineEvents = $config['doctrineSubscribedEvents']) > 0) { $subscriber = $container->getDefinition('meilisearch.search_indexer_subscriber'); foreach ($doctrineEvents as $event) { $subscriber->addTag('doctrine.event_listener', ['event' => $event]); $subscriber->addTag('doctrine_mongodb.odm.event_listener', ['event' => $event]); } - } else { + return; + } $container->removeDefinition('meilisearch.search_indexer_subscriber'); - }However, this is purely stylistic.
src/DataProvider/DataProviderInterface.php (1)
20-25: Clarify the expected structure of$identifiersparameter.The PHPDoc
@param array<mixed> $identifiersis vague. Looking at implementations:
ActorDataProvider.loadByIdentifiersexpects['id' => value](associative array)OrmEntityProvider.loadByIdentifiersexpects an array of scalar values passed tofindBy([$fieldName => $identifiers])This inconsistency could lead to implementation errors.
Consider documenting the expected structure more precisely, e.g.:
/** - * @param array<mixed> $identifiers + * @param array<string|int> $identifiers Array of identifier values to load * * @return iterable<T> */Or if the structure should be associative:
- * @param array<mixed> $identifiers + * @param array<string, mixed> $identifiers Associative array of identifier field name => valuesrc/Engine.php (1)
77-88: Consider using batch deletion for better performance.The current implementation calls
deleteDocument()individually for each object, resulting in N API calls per index. Meilisearch supports batch deletion viadeleteDocuments(array $ids).$result = []; foreach ($data as $indexUid => $objects) { - $result[$indexUid] = []; - foreach ($objects as $object) { - $result[$indexUid][] = $this->client - ->index($indexUid) - ->deleteDocument($object); - } + $result[$indexUid] = $this->client + ->index($indexUid) + ->deleteDocuments($objects); }src/Command/MeilisearchImportCommand.php (1)
71-155: Static analysis flagged complexity metrics for theexecute()method.Pipeline reports:
- CyclomaticComplexity: 13 (threshold 10)
- NPathComplexity: 592 (threshold 200)
- CouplingBetweenObjects: 13 (at threshold)
Consider extracting helper methods for distinct responsibilities (e.g.,
processIndex(),handleSkipBatches()) in a follow-up PR to improve maintainability. Based on learnings, deferring this to a separate PR aligns with the project's preference for focused changes.src/Services/MeilisearchManager.php (1)
22-22: Acknowledge pipeline warnings about complexity and coupling.The pipeline flags high class complexity (70 vs threshold 50) and coupling (14 vs threshold 13). As a central manager orchestrating configuration, data providers, indexing, searching, and aggregator support, this complexity is understandable for the initial implementation of the new architecture.
Based on learnings, the maintainer prefers focused PRs. Consider deferring refactoring to extract helper classes or strategies in a follow-up PR if the complexity becomes problematic during maintenance.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (54)
.github/workflows/pre-release-tests.yml(2 hunks).github/workflows/tests.yml(1 hunks)composer.json(1 hunks)config/services.php(4 hunks)phpstan-baseline.php(1 hunks)phpstan.dist.neon(1 hunks)src/Command/IndexCommand.php(2 hunks)src/Command/MeilisearchClearCommand.php(1 hunks)src/Command/MeilisearchCreateCommand.php(3 hunks)src/Command/MeilisearchDeleteCommand.php(1 hunks)src/Command/MeilisearchImportCommand.php(7 hunks)src/Command/MeilisearchUpdateSettingsCommand.php(3 hunks)src/DataProvider/DataProviderInterface.php(1 hunks)src/DataProvider/OrmEntityProvider.php(1 hunks)src/DependencyInjection/Configuration.php(2 hunks)src/DependencyInjection/MeilisearchExtension.php(4 hunks)src/Engine.php(5 hunks)src/Event/SettingsUpdatedEvent.php(1 hunks)src/EventListener/ConsoleOutputSubscriber.php(1 hunks)src/EventListener/DoctrineEventSubscriber.php(1 hunks)src/Exception/DataProviderNotFoundException.php(1 hunks)src/Exception/InvalidIndiceException.php(1 hunks)src/Exception/NotSearchableException.php(1 hunks)src/Model/Aggregator.php(2 hunks)src/SearchManagerInterface.php(1 hunks)src/SearchService.php(1 hunks)src/Searchable.php(1 hunks)src/SearchableEntity.php(2 hunks)src/SearchableObject.php(1 hunks)src/Services/MeilisearchManager.php(1 hunks)src/Services/MeilisearchService.php(8 hunks)src/Services/SettingsUpdater.php(1 hunks)src/Services/UnixTimestampNormalizer.php(1 hunks)tests/BaseKernelTestCase.php(5 hunks)tests/Entity/Actor.php(1 hunks)tests/Entity/Link.php(2 hunks)tests/Entity/SelfNormalizable.php(2 hunks)tests/Entity/Tag.php(2 hunks)tests/Integration/AggregatorTest.php(1 hunks)tests/Integration/Command/MeilisearchClearCommandTest.php(1 hunks)tests/Integration/Command/MeilisearchCreateCommandTest.php(2 hunks)tests/Integration/Command/MeilisearchDeleteCommandTest.php(1 hunks)tests/Integration/Command/MeilisearchImportCommandTest.php(1 hunks)tests/Integration/EngineTest.php(3 hunks)tests/Integration/EventListener/DoctrineEventSubscriberTest.php(8 hunks)tests/Integration/Fixtures/ActorDataProvider.php(1 hunks)tests/Integration/SearchTest.php(4 hunks)tests/Kernel.php(1 hunks)tests/Unit/ConfigurationTest.php(8 hunks)tests/Unit/SerializationTest.php(2 hunks)tests/baseline-ignore(1 hunks)tests/config/doctrine_php7.yaml(0 hunks)tests/config/framework.yaml(1 hunks)tests/config/meilisearch.yaml(1 hunks)
💤 Files with no reviewable changes (1)
- tests/config/doctrine_php7.yaml
✅ Files skipped from review due to trivial changes (1)
- tests/Integration/Command/MeilisearchImportCommandTest.php
🚧 Files skipped from review as they are similar to previous changes (19)
- tests/Entity/SelfNormalizable.php
- tests/Entity/Actor.php
- src/SearchService.php
- tests/Entity/Tag.php
- tests/config/meilisearch.yaml
- src/SearchableEntity.php
- src/Event/SettingsUpdatedEvent.php
- tests/baseline-ignore
- src/Services/UnixTimestampNormalizer.php
- tests/Kernel.php
- tests/Integration/Fixtures/ActorDataProvider.php
- tests/Integration/AggregatorTest.php
- src/Command/MeilisearchClearCommand.php
- src/Command/MeilisearchDeleteCommand.php
- tests/Integration/SearchTest.php
- src/Exception/InvalidIndiceException.php
- src/SearchManagerInterface.php
- composer.json
- phpstan.dist.neon
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: norkunas
Repo: meilisearch/meilisearch-symfony PR: 391
File: src/Command/MeilisearchImportCommand.php:117-120
Timestamp: 2025-08-05T04:46:01.119Z
Learning: In meilisearch-symfony project, the maintainer norkunas prefers to keep PRs focused and defer error handling improvements for settings updates to separate PRs, particularly when it involves decisions about partial import behavior when some indexes fail.
📚 Learning: 2025-08-05T04:46:01.119Z
Learnt from: norkunas
Repo: meilisearch/meilisearch-symfony PR: 391
File: src/Command/MeilisearchImportCommand.php:117-120
Timestamp: 2025-08-05T04:46:01.119Z
Learning: In meilisearch-symfony project, the maintainer norkunas prefers to keep PRs focused and defer error handling improvements for settings updates to separate PRs, particularly when it involves decisions about partial import behavior when some indexes fail.
Applied to files:
src/Engine.phpsrc/EventListener/ConsoleOutputSubscriber.phpsrc/Command/MeilisearchImportCommand.php.github/workflows/tests.ymlsrc/DataProvider/OrmEntityProvider.phpsrc/Services/MeilisearchManager.php
🧬 Code graph analysis (19)
src/DataProvider/DataProviderInterface.php (2)
src/DataProvider/OrmEntityProvider.php (4)
provide(20-29)loadByIdentifiers(31-44)getIdentifierValues(46-51)cleanup(53-56)tests/Integration/Fixtures/ActorDataProvider.php (4)
provide(15-44)loadByIdentifiers(46-57)getIdentifierValues(59-64)cleanup(66-69)
src/Services/SettingsUpdater.php (3)
src/Services/MeilisearchManager.php (2)
MeilisearchManager(22-506)getConfiguration(89-92)src/SearchManagerInterface.php (1)
getConfiguration(24-24)src/SearchService.php (1)
getConfiguration(27-27)
src/EventListener/DoctrineEventSubscriber.php (2)
src/SearchManagerInterface.php (2)
index(36-36)remove(43-43)src/Services/MeilisearchManager.php (2)
index(117-162)remove(167-200)
src/Command/MeilisearchUpdateSettingsCommand.php (4)
src/Command/IndexCommand.php (1)
__construct(19-24)src/Command/MeilisearchCreateCommand.php (1)
__construct(23-30)src/SearchManagerInterface.php (1)
isSearchable(17-17)src/SearchService.php (1)
isSearchable(20-20)
tests/Integration/EngineTest.php (2)
src/SearchableObject.php (1)
SearchableObject(13-80)tests/Entity/Image.php (1)
getId(39-42)
src/Model/Aggregator.php (1)
src/SearchableEntity.php (1)
__construct(42-56)
src/DependencyInjection/MeilisearchExtension.php (5)
src/DataProvider/OrmEntityProvider.php (1)
OrmEntityProvider(9-57)src/MeilisearchBundle.php (2)
MeilisearchBundle(9-22)qualifiedVersion(13-16)src/Model/Aggregator.php (2)
Aggregator(12-76)getEntities(42-45)src/Document/Aggregator.php (1)
Aggregator(9-11)src/Entity/Aggregator.php (1)
Aggregator(9-11)
src/DependencyInjection/Configuration.php (1)
src/SearchableObject.php (1)
SearchableObject(13-80)
src/Engine.php (2)
src/SearchManagerInterface.php (4)
index(36-36)remove(43-43)count(100-100)search(78-78)src/SearchableObject.php (5)
SearchableObject(13-80)getSearchableArray(65-79)getIndexUid(42-45)getPrimaryKey(50-53)getIdentifier(55-58)
src/Command/MeilisearchCreateCommand.php (3)
src/Command/IndexCommand.php (1)
__construct(19-24)src/SearchManagerInterface.php (1)
isSearchable(17-17)src/Services/MeilisearchManager.php (1)
isSearchable(69-74)
src/Command/MeilisearchImportCommand.php (3)
src/SearchManagerInterface.php (6)
getConfiguration(24-24)isSearchable(17-17)getDataProvider(29-29)index(36-36)count(100-100)deleteByIndexName(57-57)src/Services/MeilisearchManager.php (6)
getConfiguration(89-92)isSearchable(69-74)getDataProvider(97-112)index(117-162)count(308-313)deleteByIndexName(215-218)src/DataProvider/DataProviderInterface.php (2)
provide(18-18)cleanup(34-34)
tests/Entity/Link.php (1)
src/SearchableObject.php (1)
SearchableObject(13-80)
src/Command/IndexCommand.php (3)
src/SearchManagerInterface.php (1)
getConfiguration(24-24)src/SearchService.php (1)
getConfiguration(27-27)src/Services/MeilisearchManager.php (1)
getConfiguration(89-92)
tests/Unit/SerializationTest.php (1)
src/SearchableObject.php (1)
SearchableObject(13-80)
src/DataProvider/OrmEntityProvider.php (2)
src/DataProvider/DataProviderInterface.php (4)
provide(18-18)loadByIdentifiers(25-25)getIdentifierValues(32-32)cleanup(34-34)tests/Integration/Fixtures/ActorDataProvider.php (4)
provide(15-44)loadByIdentifiers(46-57)getIdentifierValues(59-64)cleanup(66-69)
src/Services/MeilisearchManager.php (7)
src/Engine.php (8)
Engine(10-138)__construct(12-14)index(25-54)remove(62-88)clear(97-100)delete(105-108)search(115-118)count(125-128)src/Exception/DataProviderNotFoundException.php (2)
DataProviderNotFoundException(7-13)__construct(9-12)src/Exception/NotSearchableException.php (2)
NotSearchableException(7-13)__construct(9-12)src/Exception/SearchHitsNotFoundException.php (1)
SearchHitsNotFoundException(7-9)src/Model/Aggregator.php (3)
Aggregator(12-76)__construct(26-35)getEntities(42-45)src/SearchableObject.php (2)
SearchableObject(13-80)__construct(28-37)src/DataProvider/DataProviderInterface.php (1)
getIdentifierValues(32-32)
tests/BaseKernelTestCase.php (2)
src/Services/MeilisearchManager.php (3)
MeilisearchManager(22-506)getConfiguration(89-92)deleteByIndexName(215-218)src/SearchManagerInterface.php (2)
getConfiguration(24-24)deleteByIndexName(57-57)
config/services.php (3)
src/Services/MeilisearchManager.php (1)
MeilisearchManager(22-506)src/EventListener/DoctrineEventSubscriber.php (1)
DoctrineEventSubscriber(10-30)src/Services/SettingsUpdater.php (1)
SettingsUpdater(16-77)
tests/Integration/EventListener/DoctrineEventSubscriberTest.php (2)
src/SearchManagerInterface.php (1)
search(78-78)src/Services/MeilisearchManager.php (1)
search(238-282)
🪛 GitHub Actions: Tests
src/DependencyInjection/MeilisearchExtension.php
[error] 22-22: IfStatementAssignment: Avoid assigning values to variables in if clauses and the like (line '44', column '20').
[error] 36-36: StaticAccess: Avoid using static access to class '\MeilisearchBundle\MeilisearchBundle' in method 'load'.
[error] 51-51: ElseExpression: The method load uses an else expression. Else clauses are basically not necessary and you can simplify the code by not using them.
[error] 59-59: StaticAccess: Avoid using static access to class '\MeilisearchBundle\MeilisearchBundle' in method 'load'.
[error] 118-118: ElseExpression: The method registerDataProviders uses an else expression. Else clauses are basically not necessary and you can simplify the code by not using them.
src/Services/MeilisearchService.php
[error] 27-27: TooManyPublicMethods: The class MeilisearchService has 11 public methods. Consider refactoring to keep under 10.
[error] 27-27: ExcessiveClassComplexity: The class MeilisearchService has an overall complexity of 64 which is very high. The configured complexity threshold is 50.
[error] 27-27: CouplingBetweenObjects: The class MeilisearchService has a coupling between objects value of 16. Consider to reduce the number of dependencies under 13.
[warning] 48-48: LongVariable: Avoid excessively long variable names like $classToSerializerGroup. Keep variable name length under 20.
[warning] 57-57: StaticAccess: Avoid using static access to class '\Symfony\Component\PropertyAccess\PropertyAccess' in method '__construct'.
[warning] 384-384: LongVariable: Avoid excessively long variable names like $searchableEntitiesChunk. Keep variable name length under 20.
[warning] 432-432: StaticAccess: Avoid using static access to class '\Doctrine\ORM\Proxy\DefaultProxyClassNameResolver' in method 'resolveClass'.
[warning] 436-436: StaticAccess: Avoid using static access to class '\Doctrine\Common\Util\ClassUtils' in method 'resolveClass'.
src/Command/MeilisearchImportCommand.php
[error] 23-23: CouplingBetweenObjects: The class MeilisearchImportCommand has a coupling between objects value of 13. Consider to reduce the number of dependencies under 13.
[error] 71-71: CyclomaticComplexity: The method execute() has a Cyclomatic Complexity of 13. The configured cyclomatic complexity threshold is 10.
[error] 71-71: NPathComplexity: The method execute() has an NPath complexity of 592. The configured NPath complexity threshold is 200.
src/DataProvider/OrmEntityProvider.php
[error] 43-43: MissingImport: Missing class import via use statement (line '43', column '19').
src/Services/MeilisearchManager.php
[error] 22-22: ExcessiveClassComplexity: The class MeilisearchManager has an overall complexity of 70 which is very high. The configured complexity threshold is 50.
[error] 22-22: CouplingBetweenObjects: The class MeilisearchManager has a coupling between objects value of 14. Consider to reduce the number of dependencies under 13.
[error] 497-497: StaticAccess: Avoid using static access to class '\Doctrine\ORM\Proxy\DefaultProxyClassNameResolver' in method 'resolveClass'.
[error] 501-501: StaticAccess: Avoid using static access to class '\Doctrine\Common\Util\ClassUtils' in method 'resolveClass'.
🔇 Additional comments (41)
tests/Entity/Link.php (2)
9-9: LGTM: Import updated correctly.The import statement correctly references the new
SearchableObjectclass, aligning with the PR's refactoring to introduce the data provider architecture.
76-76: Unable to verify the review comment due to repository access failure.The automated verification scripts could not be executed because the repository failed to clone. Without access to the codebase, I cannot:
- Confirm whether old
Searchableclass references remain in the repository- Verify the
SearchableObject::NORMALIZATION_FORMATconstant definition- Validate the import update at line 9
- Ensure no stale references to the old
Searchableclass existThe review comment's verification request cannot be automatically validated at this time.
.github/workflows/pre-release-tests.yml (1)
42-65: Pre-release workflow matrix is now consistent with integration tests workflow.The matrix configuration, exclusion logic, and include blocks are correctly synchronized with tests.yml. Job naming (line 65) appropriately adds the dependencies suffix only for non-default runs, improving clarity in the CI UI.
.github/workflows/tests.yml (1)
30-51: Matrix configuration is correctly verified.The PHP 8.1 exclusion from Symfony 7.x combinations is intentional and correct. Symfony 7.0+ requires PHP 8.2.0 or higher, making the four exclusion rules necessary and appropriate. The workflow matrix accurately reflects the supported version combinations without overly conservative constraints.
tests/config/framework.yaml (1)
5-7: Test error-handling config looks goodEnabling
handle_all_throwablesand logging PHP errors in the test env is consistent with getting deterministic failures and better diagnostics; no issues spotted.tests/Integration/Command/MeilisearchClearCommandTest.php (1)
28-42: Updated expectations foractorindex clearing are consistentThe added
sf_phpunit__actorclear line keeps the output ordering consistent with other indices and correctly asserts the new index is handled.tests/Integration/Command/MeilisearchDeleteCommandTest.php (1)
30-43: Deletion test now covers theactorindexIncluding
Deleted sf_phpunit__actorin the expected output correctly reflects the additional index managed by the delete command.phpstan-baseline.php (1)
1-17: PHPStan baseline entries are narrowly scoped and acceptableThe two ignores for
Doctrine\Common\Util\ClassUtils::getClass()are path-specific and limited in count, which is appropriate given Doctrine’s optional/dev-only role after this refactor.src/Exception/NotSearchableException.php (1)
7-12: Well-scoped exception for non-searchable classesThe dedicated
NotSearchableExceptionwith a clear, formatted message and typed constructor parameters is clean and matches the manager-based API needs.src/Exception/DataProviderNotFoundException.php (1)
7-12: Good diagnostic detail in data-provider exceptionIncluding both index name and class name in
DataProviderNotFoundException’s message gives enough context to trace configuration issues quickly; structure and typing look good.src/Searchable.php (1)
7-20: Deprecation notices are clear and non-disruptiveThe
@deprecatedannotations onSearchableand its constants clearly point toSearchableObjectequivalents without changing behavior, which is appropriate for a staged migration.src/EventListener/ConsoleOutputSubscriber.php (1)
13-20: Constructor promotion and property rename look correctUsing a
private readonly OutputStyle $outputvia constructor promotion is idiomatic, fixes the previous short-variable concern, and keepsafterSettingsUpdatebehavior unchanged.src/Model/Aggregator.php (1)
24-26: LGTM on type declarations.Adding explicit
objecttype hints improves type safety and aligns with PHP 8.1+ best practices.src/Services/MeilisearchService.php (3)
24-27: Deprecation docblock looks correct.The deprecation message now points to the correct FQN
Meilisearch\Bundle\Services\MeilisearchManager.
50-58: LGTM on delegation setup.The optional manager injection with null-check delegation pattern correctly maintains backward compatibility while enabling the new API.
93-99: Delegation with deprecation is well-implemented.The early return after delegation correctly short-circuits the legacy code path when the manager is available.
src/SearchableObject.php (2)
13-37: Clean immutable value object design.Good use of
readonlyproperties and constructor property promotion. Thenon-empty-stringPHPDoc annotations properly document the expected constraints.
65-79: DateTime normalization handled correctly.The version check ensures compatibility with Symfony 7.1+ where the DateTimeNormalizer constants are available. Using a local
$contextvariable (per past review) avoids mutating instance state.src/DependencyInjection/Configuration.php (2)
53-58: New configuration nodes align with PR objectives.The
type,primary_key, anddata_providernodes enable the data provider abstraction and configurable primary keys as intended.Consider adding validation to require
data_providerwhentypeis set tocustom. Currently, a user could configuretype: customwithout specifying adata_provider, which would likely cause a runtime error.
60-66: Constants used consistently.Referencing
SearchableObject::NORMALIZATION_GROUPin both the info string and default value maintains consistency with the new class.tests/Integration/Command/MeilisearchCreateCommandTest.php (2)
63-66: Test expectations correctly updated for Actor index.The Actor index creation log line is added in the expected position for both test branches.
79-83: Consistent update across both branches.Both the
updateSettings=trueandupdateSettings=falsebranches correctly include the Actor index.tests/Unit/ConfigurationTest.php (2)
139-153: Expected configurations correctly updated with new defaults.The test expectations now include
type,data_provider, andprimary_keywith appropriate default values.
358-402: Good test coverage for custom primary key.The new test case validates that custom
primary_keyvalues are correctly processed through configuration.tests/Integration/EventListener/DoctrineEventSubscriberTest.php (2)
23-27: Test correctly updated to use new SearchManager API.The search call now uses the manager without the ObjectManager parameter, aligning with the new interface.
97-110: Consistent API usage across test methods.All search invocations follow the new pattern:
$this->searchManager->search(ClassName::class, $query).src/EventListener/DoctrineEventSubscriber.php (2)
8-14: Clean refactoring to SearchManagerInterface.Good use of constructor property promotion with
readonly. The dependency on SearchManagerInterface properly decouples from the concrete implementation.
16-29: Simplified event handlers.The handlers now pass only the entity to
index()andremove(), as the manager internally resolves the appropriate data provider. This aligns with the PR objective to remove ObjectManager from the public API.src/DependencyInjection/MeilisearchExtension.php (1)
93-129: LGTM! Data provider registration logic is well-structured.The method correctly:
- Gives precedence to custom data_provider configuration
- Auto-wires OrmEntityProvider for ORM indices
- Handles Aggregator classes by registering per-entity providers
- Uses xxh32 hashing for unique definition IDs
src/Command/IndexCommand.php (1)
19-28: LGTM! Clean migration to SearchManagerInterface.The migration from SearchService to SearchManagerInterface is clean and uses constructor property promotion idiomatically. Configuration access is consistent throughout.
src/Services/SettingsUpdater.php (1)
22-28: LGTM! Clean migration to MeilisearchManager.The migration from SearchService to MeilisearchManager is straightforward and uses constructor property promotion idiomatically.
tests/BaseKernelTestCase.php (2)
19-67: LGTM! Comprehensive migration to MeilisearchManager.The migration from SearchService to MeilisearchManager is complete and consistent across all usages in the test base class.
47-52: Improvement: Now waits for all tasks instead of just the first.The updated
waitForAllTasks()now iterates through all tasks and waits for each one, rather than only waiting for the first task. This prevents potential race conditions in tests where multiple tasks are in flight.src/DataProvider/OrmEntityProvider.php (1)
31-44: Verification inconclusive — ActorDataProvider implementation not accessible.The repository could not be cloned in the sandbox environment, preventing verification of the claimed inconsistency. While the
OrmEntityProvidercode at lines 31-44 correctly passes$identifiersto Doctrine'sfindBy()method (which expects an array of scalar values like[1, 2, 3]), the actualActorDataProviderimplementation could not be inspected to confirm the alleged mismatch where it checks$identifiers['id'].Without access to the actual
ActorDataProvidercode, the parameter structure inconsistency cannot be confirmed or refuted.src/Command/MeilisearchUpdateSettingsCommand.php (1)
10-10: LGTM! Clean migration to SearchManagerInterface.The refactoring correctly:
- Updates the import and constructor to use
SearchManagerInterface- Uses constructor property promotion for
$settingsUpdaterand$eventDispatcher- Delegates to the parent
IndexCommandconstructor properly- Updates the
isSearchablecall to use the new managerThis is consistent with the pattern used across other commands.
Also applies to: 22-28, 56-56
src/Engine.php (1)
25-54: LGTM! Per-index primary key handling is now correct.The
index()method correctly:
- Accepts
SearchableObject|array<SearchableObject>as input- Groups documents by
indexUidwith per-indexprimaryKeytracking (line 42)- Uses each index's own primary key when calling
addDocuments()(line 50)This addresses the previously identified bug where a single primary key was applied to all indexes.
config/services.php (1)
70-78: LGTM! New manager service properly wired with data provider support.The
meilisearch.managerservice:
- Uses
tagged_iteratorfor data providers, enabling the extension point objective- Correctly configured with all required dependencies
- Properly aliased to
SearchManagerInterfaceThe backward compatibility is maintained via the
SearchServicealias (line 42).src/Command/MeilisearchCreateCommand.php (1)
10-10: LGTM! Consistent refactoring to SearchManagerInterface.The changes mirror the pattern applied across other commands:
- Constructor properly promotes dependencies as
private readonly- Parent constructor delegation is correct
isSearchablecall updated appropriatelyAlso applies to: 23-30, 66-66
src/Command/MeilisearchImportCommand.php (2)
11-11: LGTM! Constructor properly migrated to SearchManagerInterface.The dependency injection is correctly updated with constructor property promotion, and parent constructor delegation follows the established pattern.
Also applies to: 27-34
119-143: Data provider usage correctly refactored.The
getDataProvider()is now called once before the loop (addressing the past review feedback), andcleanup()is appropriately called per iteration to manage memory during batch processing.src/Services/MeilisearchManager.php (1)
485-505: Pipeline warnings about static access are acceptable here.The static method calls to
DefaultProxyClassNameResolver::getClass()(line 497) andClassUtils::getClass()(line 501) trigger linting warnings, but these are necessary for Doctrine proxy resolution across different ORM versions. The compatibility checks ensure proper class name resolution for Doctrine entities, proxies, and native lazy objects in PHP 8.4+.The layered fallback strategy (native lazy objects → ORM v3+ → legacy ORM) is appropriate for maintaining broad compatibility.
| private function getSingleIdentifier(DataProviderInterface $provider, object $object): string|int | ||
| { | ||
| $ids = $provider->getIdentifierValues($object); | ||
|
|
||
| if (1 === \count($ids)) { | ||
| $identifier = reset($ids); | ||
|
|
||
| if (\is_object($identifier) && method_exists($identifier, '__toString')) { | ||
| return (string) $identifier; | ||
| } | ||
|
|
||
| return $identifier; | ||
| } | ||
|
|
||
| $objectID = ''; | ||
| foreach ($ids as $key => $value) { | ||
| $objectID .= $key.'-'.$value.'__'; | ||
| } | ||
|
|
||
| return $objectID; | ||
| } |
There was a problem hiding this comment.
Inconsistent composite key format between indexing and search.
The multi-identifier key format differs between getSingleIdentifier() and the search result mapping:
getSingleIdentifier()(line 450): Builds keys as"key1-value1__key2-value2__"search()(line 268): Builds keys asimplode('-', $identifiers)(values only)
This mismatch will prevent multi-identifier entities from being found during search, as indexed keys won't match the lookup keys.
Recommendation: Align both methods to use the same format. If only values are needed, update getSingleIdentifier():
$objectID = '';
foreach ($ids as $key => $value) {
- $objectID .= $key.'-'.$value.'__';
+ $objectID .= $value.'-';
}
+ $objectID = rtrim($objectID, '-');
return $objectID;Alternatively, if keys are needed, update search() to match the key-value format used in getSingleIdentifier().
🤖 Prompt for AI Agents
In src/Services/MeilisearchManager.php around lines 434 to 454, the composite
identifier format produced by getSingleIdentifier()
("key1-value1__key2-value2__") does not match the format used in search()
(implode('-', $identifiers) which uses values only), so update
getSingleIdentifier() to return a dash-joined string of identifier values only:
when multiple identifiers, collect their values (casting objects with __toString
to strings), ensure no trailing separators, and return implode('-', $values) so
the indexed ID format matches the lookup format used in search().
3edd946 to
a0b7b65
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
src/Model/Aggregator.php (1)
24-35: Aggregator still hardcodesobjectIDdespite configurableprimary_keyThe stricter
objecttyping on$entityand the constructor is a nice improvement, butnormalize()still always emits anobjectIDfield, even though indices can now configure an arbitraryprimary_key:// @todo: this is probably wrong now since we allow configuring primary_key? return array_merge(['objectID' => $this->objectID], ...);For aggregator-based indices this can lead to a mismatch between the Meilisearch primary key (from config) and the ID field actually present in documents, unless users manually set
primary_key: objectIDfor such indices.It would be better to either:
- Drive the key name from the configured
primary_keyfor the aggregator’s index, or- Clearly document and enforce that aggregator indices must use
primary_key: objectID.Right now the TODO correctly highlights a real behavior risk that is still unresolved.
Also applies to: 71-75
src/Services/MeilisearchManager.php (1)
265-273: Composite key format inconsistency between indexing and search.The key format for composite identifiers differs:
getSingleIdentifier()(line 452-455):"key1-value1__key2-value2__"(key-value pairs with trailing__)search()(line 272):implode('-', $identifiers)(values only, dash-separated)This mismatch will prevent entities with composite keys from being found during search, as the indexed document IDs won't match the lookup keys.
Align both methods to use the same format. If only values are needed:
$objectID = ''; foreach ($ids as $key => $value) { - $objectID .= $key.'-'.$value.'__'; + $objectID .= $value.'-'; } + $objectID = rtrim($objectID, '-'); return $objectID;Also applies to: 452-455
🧹 Nitpick comments (6)
src/DataProvider/DataProviderInterface.php (1)
7-35: Solid DataProvider abstraction; consider clarifying identifier shapeThe generic
DataProviderInterfacewithprovide(),loadByIdentifiers(),getIdentifierValues(), andcleanup()is a clean extension point and fits both the ORM and in-memory provider examples.One improvement to consider (for docs only): tighten the phpdoc on
$identifiersinloadByIdentifiers()to spell out the expected shape (e.g. list of scalar IDs vs. keyed array) so third‑party providers can interoperate consistently with the manager’s batching logic.tests/Unit/ConfigurationTest.php (1)
132-142: Config tests correctly cover type/data_provider/primary_key defaults and overridesThe added expectations for
type => 'orm',data_provider => null, andprimary_key(default'objectID'and custom values) are consistent with the configuration tree and give good coverage of the new options. The new “custom objectID” case nicely validates per-index primary keys.If you want to emphasize the new abstraction, you might consider renaming that last data set to “custom primary key” in a follow-up, but the behavior is already well tested.
Also applies to: 188-198, 203-209, 244-249, 287-290, 328-331, 359-402
src/DependencyInjection/Configuration.php (1)
7-7: New index options and SearchableObject wiring look coherentThe added
type,primary_key, anddata_providernodes, plus the switch toSearchableObject::NORMALIZATION_GROUP, line up with the new data-provider and normalization model and match the unit-test expectations.You may want to consider enforcing a non-empty
primary_key(e.g., via->cannotBeEmpty()) to fail fast on invalid configs instead of deferring to runtime errors, but that can be a separate tightening step.Also applies to: 53-59, 60-66
tests/BaseKernelTestCase.php (1)
10-11: Consider typing tests againstSearchManagerInterfaceinstead of concreteMeilisearchManagerFor tests, depending on
SearchManagerInterfacerather than the concreteMeilisearchManagerreduces coupling and keeps the test base aligned with the public API surface:-use Meilisearch\Bundle\Services\MeilisearchManager; +use Meilisearch\Bundle\SearchManagerInterface; ... - protected MeilisearchManager $searchManager; + protected SearchManagerInterface $searchManager; ... - $this->searchManager = $this->get('meilisearch.manager'); + $this->searchManager = $this->get(SearchManagerInterface::class);Not required for correctness, but it will make future refactors of the manager implementation easier.
Also applies to: 19-20, 27-27, 39-40, 56-57, 67-68
src/Services/MeilisearchService.php (1)
24-27: BC bridge toMeilisearchManageris solid; consider delegating more methodsThe deprecation layer here is well thought out: injecting an optional
SearchManagerInterfaceand delegatingindex(),remove(),clear(),deleteByIndexName(),delete(),search(),rawSearch(), andcount()to it when available gives consumers a smooth migration path while preserving the legacy behavior when no manager is wired.Two minor design follow-ups you might consider (no need to block this PR):
- To avoid configuration drift,
isSearchable(),getConfiguration(), andsearchableAs()could also delegate to the manager when present, instead of maintaining parallel logic in both classes.- Given the service as a whole is now deprecated, you may eventually want to mark the delegating methods as thin proxies only (and keep all evolution in
MeilisearchManager) to reduce maintenance surface.Functionally, though, the current implementation is consistent and backward-compatible.
Also applies to: 50-59, 66-73, 83-100, 102-135, 137-158, 160-272, 322-341, 334-336, 372-389, 396-420
config/services.php (1)
12-15: Manager wiring looks good; double‑checktagged_iteratorvs ctor type for data providersThe service config is consistent with the new architecture:
meilisearch.servicenow optionally receivesmeilisearch.manager, aligning with the BC delegation you added.meilisearch.manageris registered once and aliased toSearchManagerInterface::class.- Doctrine subscriber, commands, and
SettingsUpdaterare all shifted to depend on the manager, which is exactly what we want.One thing to verify:
meilisearch.manageris constructed withtagged_iterator('meilisearch.data_provider', 'key'). Symfony injects this as an iterable (e.g.,RewindableGenerator), not a plain array. IfMeilisearchManager::__construct()currently type-hints that argument asarray, you may hit aTypeErrorat runtime.If that’s the case, consider updating the constructor signature to:
public function __construct( NormalizerInterface $normalizer, Engine $engine, PropertyAccessorInterface $propertyAccessor, array $configuration, iterable $dataProviders = [], ) { // ... }and treating
$dataProvidersasiterableinternally. That matches Symfony’s recommended pattern fortagged_iterator.Please confirm the actual constructor type of
$dataProvidersinMeilisearchManagerand adjust either the type or the DI argument if needed so they match.Also applies to: 28-36, 44-47, 70-79, 80-85, 89-121
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (55)
.github/workflows/pre-release-tests.yml(2 hunks).github/workflows/tests.yml(1 hunks)composer.json(1 hunks)config/services.php(4 hunks)phpstan-baseline.php(1 hunks)phpstan.dist.neon(1 hunks)src/Command/IndexCommand.php(2 hunks)src/Command/MeilisearchClearCommand.php(1 hunks)src/Command/MeilisearchCreateCommand.php(3 hunks)src/Command/MeilisearchDeleteCommand.php(1 hunks)src/Command/MeilisearchImportCommand.php(7 hunks)src/Command/MeilisearchUpdateSettingsCommand.php(3 hunks)src/DataProvider/DataProviderInterface.php(1 hunks)src/DataProvider/OrmEntityProvider.php(1 hunks)src/DependencyInjection/Configuration.php(2 hunks)src/DependencyInjection/MeilisearchExtension.php(4 hunks)src/Engine.php(5 hunks)src/Event/SettingsUpdatedEvent.php(1 hunks)src/EventListener/ConsoleOutputSubscriber.php(1 hunks)src/EventListener/DoctrineEventSubscriber.php(1 hunks)src/Exception/DataProviderNotFoundException.php(1 hunks)src/Exception/InvalidIndiceException.php(1 hunks)src/Exception/LogicException.php(1 hunks)src/Exception/NotSearchableException.php(1 hunks)src/Model/Aggregator.php(2 hunks)src/SearchManagerInterface.php(1 hunks)src/SearchService.php(1 hunks)src/Searchable.php(1 hunks)src/SearchableEntity.php(2 hunks)src/SearchableObject.php(1 hunks)src/Services/MeilisearchManager.php(1 hunks)src/Services/MeilisearchService.php(9 hunks)src/Services/SettingsUpdater.php(1 hunks)src/Services/UnixTimestampNormalizer.php(1 hunks)tests/BaseKernelTestCase.php(5 hunks)tests/Entity/Actor.php(1 hunks)tests/Entity/Link.php(2 hunks)tests/Entity/SelfNormalizable.php(2 hunks)tests/Entity/Tag.php(2 hunks)tests/Integration/AggregatorTest.php(1 hunks)tests/Integration/Command/MeilisearchClearCommandTest.php(1 hunks)tests/Integration/Command/MeilisearchCreateCommandTest.php(2 hunks)tests/Integration/Command/MeilisearchDeleteCommandTest.php(1 hunks)tests/Integration/Command/MeilisearchImportCommandTest.php(1 hunks)tests/Integration/EngineTest.php(3 hunks)tests/Integration/EventListener/DoctrineEventSubscriberTest.php(8 hunks)tests/Integration/Fixtures/ActorDataProvider.php(1 hunks)tests/Integration/SearchTest.php(4 hunks)tests/Kernel.php(1 hunks)tests/Unit/ConfigurationTest.php(8 hunks)tests/Unit/SerializationTest.php(2 hunks)tests/baseline-ignore(1 hunks)tests/config/doctrine_php7.yaml(0 hunks)tests/config/framework.yaml(1 hunks)tests/config/meilisearch.yaml(1 hunks)
💤 Files with no reviewable changes (1)
- tests/config/doctrine_php7.yaml
🚧 Files skipped from review as they are similar to previous changes (16)
- src/Command/MeilisearchClearCommand.php
- tests/config/meilisearch.yaml
- tests/Integration/Command/MeilisearchDeleteCommandTest.php
- tests/Integration/AggregatorTest.php
- tests/Entity/Tag.php
- tests/Entity/Actor.php
- tests/baseline-ignore
- src/Command/MeilisearchDeleteCommand.php
- .github/workflows/tests.yml
- tests/Integration/Command/MeilisearchCreateCommandTest.php
- tests/Integration/Command/MeilisearchClearCommandTest.php
- tests/Entity/SelfNormalizable.php
- src/SearchService.php
- tests/Integration/Command/MeilisearchImportCommandTest.php
- src/Command/IndexCommand.php
- src/DataProvider/OrmEntityProvider.php
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: norkunas
Repo: meilisearch/meilisearch-symfony PR: 391
File: src/Command/MeilisearchImportCommand.php:117-120
Timestamp: 2025-08-05T04:46:01.119Z
Learning: In meilisearch-symfony project, the maintainer norkunas prefers to keep PRs focused and defer error handling improvements for settings updates to separate PRs, particularly when it involves decisions about partial import behavior when some indexes fail.
📚 Learning: 2025-08-05T04:46:01.119Z
Learnt from: norkunas
Repo: meilisearch/meilisearch-symfony PR: 391
File: src/Command/MeilisearchImportCommand.php:117-120
Timestamp: 2025-08-05T04:46:01.119Z
Learning: In meilisearch-symfony project, the maintainer norkunas prefers to keep PRs focused and defer error handling improvements for settings updates to separate PRs, particularly when it involves decisions about partial import behavior when some indexes fail.
Applied to files:
src/EventListener/ConsoleOutputSubscriber.phptests/Integration/EngineTest.phpsrc/Engine.phpsrc/Services/UnixTimestampNormalizer.phpsrc/Command/MeilisearchImportCommand.phptests/Integration/Fixtures/ActorDataProvider.phpsrc/Services/MeilisearchManager.php
🧬 Code graph analysis (25)
src/SearchableEntity.php (2)
tests/Entity/Link.php (1)
normalize(74-85)src/SearchableObject.php (1)
SearchableObject(13-80)
src/DataProvider/DataProviderInterface.php (2)
src/DataProvider/OrmEntityProvider.php (4)
provide(21-30)loadByIdentifiers(32-45)getIdentifierValues(47-52)cleanup(54-57)tests/Integration/Fixtures/ActorDataProvider.php (4)
provide(15-44)loadByIdentifiers(46-57)getIdentifierValues(59-64)cleanup(66-69)
src/EventListener/ConsoleOutputSubscriber.php (2)
src/Command/MeilisearchUpdateSettingsCommand.php (1)
__construct(22-28)src/Engine.php (1)
__construct(12-14)
src/EventListener/DoctrineEventSubscriber.php (2)
src/SearchManagerInterface.php (2)
index(36-36)remove(43-43)src/Services/MeilisearchManager.php (2)
index(121-166)remove(171-204)
tests/Integration/EventListener/DoctrineEventSubscriberTest.php (4)
src/Engine.php (1)
search(115-118)src/SearchManagerInterface.php (1)
search(78-78)src/SearchService.php (1)
search(59-64)src/Services/MeilisearchManager.php (1)
search(242-286)
tests/Integration/EngineTest.php (3)
src/SearchableObject.php (1)
SearchableObject(13-80)tests/Entity/Image.php (1)
getId(39-42)tests/Entity/Post.php (1)
getId(91-94)
src/Exception/InvalidIndiceException.php (2)
src/Exception/DataProviderNotFoundException.php (1)
__construct(9-12)src/Exception/NotSearchableException.php (1)
__construct(9-12)
src/SearchableObject.php (4)
src/SearchableEntity.php (1)
getSearchableArray(66-91)tests/Entity/Link.php (1)
normalize(74-85)tests/Entity/SelfNormalizable.php (1)
normalize(64-75)tests/Entity/Tag.php (1)
normalize(88-100)
tests/Integration/SearchTest.php (2)
src/SearchManagerInterface.php (2)
search(78-78)rawSearch(90-90)src/Services/MeilisearchManager.php (2)
search(242-286)rawSearch(296-304)
src/SearchManagerInterface.php (2)
src/Exception/NotSearchableException.php (1)
NotSearchableException(7-13)src/Services/MeilisearchManager.php (12)
isSearchable(69-74)searchableAs(79-91)getConfiguration(93-96)getDataProvider(101-116)index(121-166)remove(171-204)clear(209-214)deleteByIndexName(219-222)delete(227-232)search(242-286)rawSearch(296-304)count(312-317)
src/Model/Aggregator.php (1)
src/SearchableEntity.php (1)
__construct(42-56)
src/DependencyInjection/Configuration.php (1)
src/SearchableObject.php (1)
SearchableObject(13-80)
src/Engine.php (3)
src/SearchableEntity.php (3)
__construct(42-56)getSearchableArray(66-91)getIndexUid(58-61)src/SearchManagerInterface.php (4)
index(36-36)remove(43-43)count(100-100)search(78-78)src/SearchableObject.php (5)
SearchableObject(13-80)getSearchableArray(65-79)getIndexUid(42-45)getPrimaryKey(50-53)getIdentifier(55-58)
src/Services/UnixTimestampNormalizer.php (4)
src/Model/Aggregator.php (1)
normalize(71-75)tests/Entity/Link.php (1)
normalize(74-85)tests/Entity/SelfNormalizable.php (1)
normalize(64-75)tests/Entity/Tag.php (1)
normalize(88-100)
src/Services/SettingsUpdater.php (1)
src/Services/MeilisearchManager.php (2)
MeilisearchManager(22-510)getConfiguration(93-96)
src/Command/MeilisearchUpdateSettingsCommand.php (4)
src/Command/IndexCommand.php (1)
__construct(19-24)src/Exception/NotSearchableException.php (1)
__construct(9-12)src/SearchManagerInterface.php (1)
isSearchable(17-17)src/SearchService.php (1)
isSearchable(20-20)
config/services.php (3)
src/Services/MeilisearchManager.php (1)
MeilisearchManager(22-510)src/EventListener/DoctrineEventSubscriber.php (1)
DoctrineEventSubscriber(10-30)src/Services/SettingsUpdater.php (1)
SettingsUpdater(16-77)
src/Services/MeilisearchService.php (2)
src/EventListener/DoctrineEventSubscriber.php (1)
__construct(12-14)src/Services/MeilisearchManager.php (5)
getBaseClassName(342-349)getConfiguration(93-96)searchableAs(79-91)index(121-166)search(242-286)
src/DependencyInjection/MeilisearchExtension.php (5)
src/DataProvider/OrmEntityProvider.php (1)
OrmEntityProvider(10-58)src/MeilisearchBundle.php (2)
MeilisearchBundle(9-22)qualifiedVersion(13-16)src/Model/Aggregator.php (2)
Aggregator(12-76)getEntities(42-45)src/Document/Aggregator.php (1)
Aggregator(9-11)src/Entity/Aggregator.php (1)
Aggregator(9-11)
src/Services/MeilisearchManager.php (18)
tests/Entity/Link.php (1)
ORM(16-86)tests/Entity/SelfNormalizable.php (1)
ORM(16-76)tests/Entity/Tag.php (1)
ORM(17-101)src/Collection.php (1)
Collection(13-380)src/Engine.php (8)
Engine(10-138)__construct(12-14)index(25-54)remove(62-88)clear(97-100)delete(105-108)search(115-118)count(125-128)src/Exception/DataProviderNotFoundException.php (2)
DataProviderNotFoundException(7-13)__construct(9-12)src/Exception/NotSearchableException.php (2)
NotSearchableException(7-13)__construct(9-12)src/Exception/SearchHitsNotFoundException.php (1)
SearchHitsNotFoundException(7-9)src/Model/Aggregator.php (3)
Aggregator(12-76)__construct(26-35)getEntities(42-45)src/SearchableObject.php (2)
SearchableObject(13-80)__construct(28-37)src/DataProvider/OrmEntityProvider.php (3)
__construct(15-19)getIdentifierValues(47-52)loadByIdentifiers(32-45)src/EventListener/ConsoleOutputSubscriber.php (1)
__construct(13-15)src/EventListener/DoctrineEventSubscriber.php (1)
__construct(12-14)src/Exception/InvalidIndiceException.php (1)
__construct(9-12)src/SearchableEntity.php (1)
__construct(42-56)src/Services/MeilisearchService.php (13)
__construct(52-64)isSearchable(66-73)searchableAs(90-100)getConfiguration(83-88)index(102-135)remove(137-158)clear(160-171)assertIsSearchable(432-437)deleteByIndexName(173-182)delete(184-195)search(197-242)count(260-271)rawSearch(244-258)src/SearchManagerInterface.php (12)
isSearchable(17-17)searchableAs(22-22)getConfiguration(24-24)index(36-36)getDataProvider(29-29)remove(43-43)clear(50-50)deleteByIndexName(57-57)delete(66-66)search(78-78)count(100-100)rawSearch(90-90)src/DataProvider/DataProviderInterface.php (2)
getIdentifierValues(32-32)loadByIdentifiers(25-25)
src/Command/MeilisearchCreateCommand.php (5)
src/Command/IndexCommand.php (1)
__construct(19-24)src/Command/MeilisearchImportCommand.php (1)
__construct(27-34)src/Command/MeilisearchUpdateSettingsCommand.php (1)
__construct(22-28)src/SearchManagerInterface.php (1)
isSearchable(17-17)src/Services/MeilisearchManager.php (1)
isSearchable(69-74)
src/Exception/NotSearchableException.php (2)
src/Exception/DataProviderNotFoundException.php (1)
__construct(9-12)src/Exception/InvalidIndiceException.php (1)
__construct(9-12)
tests/BaseKernelTestCase.php (3)
src/Services/MeilisearchManager.php (3)
MeilisearchManager(22-510)getConfiguration(93-96)deleteByIndexName(219-222)src/SearchManagerInterface.php (2)
getConfiguration(24-24)deleteByIndexName(57-57)src/Services/MeilisearchService.php (2)
getConfiguration(83-88)deleteByIndexName(173-182)
tests/Unit/SerializationTest.php (1)
src/SearchableObject.php (1)
SearchableObject(13-80)
tests/Entity/Link.php (1)
src/SearchableObject.php (1)
SearchableObject(13-80)
🔇 Additional comments (51)
tests/config/framework.yaml (1)
5-7: Verify the necessity of test configuration changes.The additions of
handle_all_throwables: trueandphp_errors: { log: true }are valid Symfony configuration options, but their necessity isn't evident from the PR summary. These appear orthogonal to the main data provider extension objectives.Please clarify:
- Were these changes added to handle error scenarios introduced by the data provider refactoring, or are they general test hygiene improvements?
- If they're fixing test failures, can you provide details on which tests were failing and how these settings resolve them?
.github/workflows/pre-release-tests.yml (2)
40-65: CI matrix configuration is well-structured.The matrix correctly enforces PHP 8.1 → Symfony 6.x constraint, includes comprehensive version combinations, and adds explicit lowest/highest dependency testing for new PHP/Symfony pairs. Job name conditional formatting for dependencies label is correct.
79-85: Dependency installation strategy is appropriate for pre-release testing.The SYMFONY_REQUIRE env var, --no-security-blocking flag (acceptable for pre-release CI), and dynamic dependency-versions resolution are correctly configured. The strategy provides targeted coverage of lowest/highest version combinations without blocking dev environment setup.
composer.json (2)
20-30: Version constraints in require section align with PR objectives.PHP 8.1 minimum and Symfony ^6.4 || ^7.0 correctly reflect the breaking-change intent to modernize the bundle's runtime and dependency footprint. Removal of polyfill-php80 is appropriate for PHP 8.1+.
31-51: Require-dev restructuring and tooling updates are sound.Moving doctrine-bundle to require-dev correctly reflects the new provider-based architecture—users opting into Doctrine ORM support must add the dependency explicitly. This BC break aligns with PR objectives. PHPStan ^2 and related extensions are appropriately constrained, and Symfony dev packages follow the main require constraints consistently. The more specific http-client constraint (^6.4.15 || ^7.1.8) is reasonable for version-dependent features.
src/Services/UnixTimestampNormalizer.php (2)
11-17: LGTM! Clean parameter renaming and explicit typing.The parameter rename from
$objectto$dataand explicitmixedtype align with standard Symfony normalizer conventions and the PHP 8.1+ modernization goals of this PR. The framework contract guarantees type safety viasupportsNormalization, so the implementation is correct.
19-22: LGTM! Correct type guard and context check.The explicit
mixedtype and the combinedinstanceof+ context check correctly implement the normalizer contract, ensuring type safety and scoped normalization.src/Exception/LogicException.php (1)
7-9: Bundle-specific LogicException wrapper looks goodDefining a namespaced final
LogicExceptionfor bundle-internal logic errors is clean and keeps behavior identical to the core SPL exception.src/SearchableEntity.php (1)
15-17: Deprecation and normalization format alignment look consistentMarking
SearchableEntityas deprecated in favor ofSearchableObjectwhile reusingSearchableObject::NORMALIZATION_FORMATin both normalization branches keeps legacy usage aligned with the new normalization pipeline without introducing behavior changes.Also applies to: 82-88
src/Exception/InvalidIndiceException.php (1)
9-12: Stricter$codetyping is appropriateTyping
$codeasintbrings this exception in line with the other bundle exceptions and SPL usage, with no functional downside.phpstan-baseline.php (1)
1-17: Targeted PHPStan baseline entries are reasonableThe new baseline centralizes two very specific
ClassUtils::getClass()“class.notFound” ignores per service file, which is a good, narrow use of a baseline while Doctrine is optional and static analysis cannot always see it.phpstan.dist.neon (1)
1-2: Including the separate PHPStan baseline is cleanPulling
phpstan-baseline.phpin viaincludeskeeps the main configuration focused while still applying the needed ignores.src/Exception/NotSearchableException.php (1)
7-12: Clear, typed NotSearchableException aligns with new APIThe exception cleanly models the “class is not searchable” case with a precise message and fully typed constructor, matching the other new exceptions.
src/Event/SettingsUpdatedEvent.php (1)
16-20: Constructor property promotion is a good fit hereUsing promoted
private readonlyproperties simplifies the event while keeping the same public API via getters and enforcing immutability ofclass,index, andsetting.src/Services/SettingsUpdater.php (1)
22-27: Constructor migration to MeilisearchManager looks consistentUsing
MeilisearchManager::getConfiguration()and promoting$searchClient/$eventDispatcherpreserves previous behavior and fits the new manager-centric wiring.tests/Entity/Link.php (1)
9-9: Normalization format aligned with SearchableObjectReferencing
SearchableObject::NORMALIZATION_FORMATkeeps this test entity in sync with the new normalization API; no functional issues spotted.Also applies to: 76-76
tests/Integration/SearchTest.php (1)
71-72: Search tests correctly migrated to SearchManager with task waitsUsing
$this->searchManager->search()/rawSearch()and insertingwaitForAllTasks()after imports aligns tests with the new API and makes them more robust against async indexing delays.Also applies to: 75-76, 81-82, 90-91, 114-116, 137-140
tests/Integration/EventListener/DoctrineEventSubscriberTest.php (1)
23-23: Doctrine event subscriber tests correctly use SearchManagerReplacing the old service calls with
$this->searchManager->search()keeps the tests aligned with the new API while preserving intent and coverage.Also applies to: 38-38, 59-59, 81-81, 97-98, 107-108, 121-121, 131-131
tests/Kernel.php (1)
36-42: Doctrine config loading logic simplified and version-awareThe new branching on
$doctrineBundleV3plus the PHP 8.4/LegacyReflectionFieldscombination is clearer than the previous PHP-version matrix and should select the right Doctrine config variant across supported environments.src/Exception/DataProviderNotFoundException.php (1)
1-13: DataProviderNotFoundException is well-defined and informativeException type and message (including index and class) are appropriate for signaling missing data providers in the manager.
src/Searchable.php (1)
7-20: LGTM!The deprecation annotations are properly structured with clear migration paths to
SearchableObject. The version reference (0.16) and replacement class/constant references are accurate.tests/Integration/EngineTest.php (2)
35-42: Constructor updated correctly for SearchableObject API.The new constructor signature is properly followed with all required parameters:
indexUid,primaryKey,object,identifier,serializer, andoptions.
67-69: Serializer correctly extracted and reused.Good practice to extract the serializer reference once and reuse it for multiple
SearchableObjectinstantiations.src/EventListener/DoctrineEventSubscriber.php (1)
10-29: LGTM!Clean migration to
SearchManagerInterface. The simplified API (single object argument instead of ObjectManager + object) aligns with the PR objective to decouple from Doctrine. Constructor property promotion is idiomatic PHP 8.1+.tests/Integration/Fixtures/ActorDataProvider.php (1)
13-69: LGTM!Well-structured test fixture demonstrating the
DataProviderInterfacecontract for custom (non-ORM) data sources. The generic type annotation@implements DataProviderInterface<Actor>properly documents the expected type.src/DependencyInjection/MeilisearchExtension.php (2)
65-68: Manager wiring correctly updated with data providers.The
$dataProvidersmap is properly passed as the 5th constructor argument, aligning with the newMeilisearchManagersignature that requires providers for index/remove operations.
93-129: I attempted to verify the review comment by examining the Configuration.php file to check validation rules and the MeilisearchManager to understand runtime behavior. However, the repository cloning failed, preventing me from accessing the codebase files needed for verification.Without access to:
- The configuration validation rules in Configuration.php (to check if
customtype requiresdata_provider)- The MeilisearchManager::getDataProvider() implementation (to check if there's error handling for missing providers)
- The actual configuration schema definition
I cannot definitively confirm or refute the review comment's concerns about indices without data providers causing runtime failures.
**Verify handling of indices without data provider.** The `registerDataProviders` method only registers providers for: 1. Indices with an explicit `data_provider` configured 2. Indices with `type: orm` If an index has neither (e.g., `type: custom` without `data_provider`), no provider is registered. Verify in Configuration.php that the configuration schema enforces `data_provider` is required for non-orm types, and confirm MeilisearchManager::getDataProvider() handles missing providers gracefully or raises a clear error.tests/Unit/SerializationTest.php (1)
23-30: LGTM!The test correctly uses the new
SearchableObjectconstructor signature and the idiomatic'groups'key (Symfony Serializer convention) with theNORMALIZATION_GROUPconstant.src/Command/MeilisearchUpdateSettingsCommand.php (1)
10-11: Migration toSearchManagerInterfacein command is consistentThe switch to injecting
SearchManagerInterfaceand reusing it viaparent::__construct($searchManager)andisSearchable()keeps behavior intact while aligning with the new manager-based API. No issues spotted here.Also applies to: 22-27, 56-56
src/EventListener/ConsoleOutputSubscriber.php (1)
13-20: Constructor promotion and$outputnaming look goodUsing constructor promotion with a more descriptive
$outputproperty cleans up the class and resolves the previous short-variable warning without changing behavior.src/SearchManagerInterface.php (1)
1-100: NewSearchManagerInterfaceis well-shaped and matches the manager implementationThe interface cleanly captures the new manager responsibilities (searchability checks, data providers, index/remove/clear/delete/search/count), with appropriate class-string typing and
NotSearchableExceptioncontracts. It lines up with theMeilisearchManagermethods shown elsewhere and gives consumers a clear, framework-agnostic entry point.No issues from my side here.
src/SearchableObject.php (1)
13-37:SearchableObjectnormalization logic looks correct and side‑effect freeThe constructor/context handling and
getSearchableArray()implementation line up with the existingSearchableEntityconventions, and the switch to a local$contextavoids mutating instance state on each call. The enforced'meilisearch' => trueflag and Symfony 7.1+DateTimeNormalizertweaks are appropriately encapsulated here.No changes needed from my perspective.
Also applies to: 55-79
tests/BaseKernelTestCase.php (1)
47-52: The review comment's core claim aboutgetTasks()iteration is incorrect; the current implementation is actually correct.The original review states that iterating
Client::getTasks()directly yields metadata instead of individual tasks. However, verification of the official meilisearch-php documentation confirms thatTasksResultsimplementsIteratorAggregatewith agetIterator()method that returns anArrayIteratorover the internal$resultsarray—meaning direct iteration yields individual task objects, not wrapper metadata.The current code:
foreach ($this->client->getTasks() as $task) { $this->client->waitForTask($task['uid']); }is correct. Each
$taskin the iteration is an associative array with properties includinguid, so$task['uid']access works as intended. UsinggetResults()is functionally equivalent but unnecessary.Likely an incorrect or invalid review comment.
src/Command/MeilisearchImportCommand.php (4)
27-34: LGTM!Constructor correctly migrated to use
SearchManagerInterfacewith readonly property promotion for injected dependencies. The parent constructor call properly passes the search manager.
119-143: LGTM!The data provider is now correctly instantiated outside the
do-whileloop (addressing the previous review feedback), andcleanup()is called after each batch iteration—appropriate for releasing resources like detaching entities from Doctrine's identity map between batches.
166-166: Good defensive initialization.Using null coalescing (
??= 0) to initialize the response counter prevents undefined index issues when the same index appears multiple times across batches.
229-229: Correctly delegates to the search manager.Using
$this->searchManager->deleteByIndexName()aligns with the new manager-based architecture for index deletion.src/Engine.php (5)
12-14: LGTM!Constructor property promotion is clean and modern.
25-54: LGTM!The indexing logic correctly handles per-index primary keys by storing both
primaryKeyanddocumentsin the batch structure. This addresses the previously identified bug where a single primary key was applied to all indexes.
97-100: LGTM!Simplified to a clean one-liner that delegates directly to the client.
130-137: LGTM!The
normalizeIdmethod now uses proper union types (\Stringable|string|int) and handles object-to-string conversion cleanly.
78-84: Based on my investigation, I've encountered limitations accessing the repository directly. However, I was able to verify key information from the Meilisearch PHP client documentation:Verified Facts:
- ✓
deleteDocuments()method exists in the Meilisearch PHP client- ✓
deleteDocuments()accepts an array of document IDs (not full objects)- ✓ It calls the DELETE batch endpoint for improved performance
Unverified:
- The structure of
$objectsin the current code (whether they are IDs or full Document objects)- Whether
deleteDocument($object)returns an ID or processes full objects- Whether the suggested refactor requires extracting IDs from objects
Critical Concern:
The suggested refactor passes$objectsdirectly todeleteDocuments(). If$objectscontains full Document objects rather than just IDs, this refactor would be incomplete and would require extracting the IDs first.
Consider batching deletions for improved performance.
Each document is deleted individually via
deleteDocument(). For bulk removals, the Meilisearch PHP client supportsdeleteDocuments(array $documentIds)which reduces API calls. However, verify that$objectscontains document IDs directly—if they are full Document objects, extract the IDs before passing todeleteDocuments().src/Command/MeilisearchCreateCommand.php (2)
23-30: LGTM!Constructor correctly migrated to use
SearchManagerInterfacewith readonly property promotion, consistent with other command classes in the PR.
66-68: LGTM!The
isSearchablecheck correctly delegates to the newsearchManager.src/Services/MeilisearchManager.php (7)
52-64: LGTM!Constructor properly initializes the manager with all required dependencies and sets up searchables and aggregators.
69-74: LGTM!Clean implementation using the base class resolution helper.
79-91: LGTM!The null check for
$index(lines 86-88) properly addresses the previously identified issue, throwing a descriptiveNotSearchableExceptionwhen no matching index is found.
101-116: LGTM!The data provider resolution correctly handles both exact class matches and inheritance hierarchies via
is_a(). Falls back to a descriptive exception when no provider is found.
489-509: LGTM on the import fix; however, static access could be avoided.The
LegacyReflectionFields::classimport issue from the previous review has been addressed. The static resolver pattern is efficient with the memoized closure.Note: PHPStan may flag the static access to
DefaultProxyClassNameResolver::getClass()andClassUtils::getClass()(lines 500-505). These are standard Doctrine utilities, and avoiding them would require significant refactoring for minimal benefit—acceptable as-is.
121-166: LGTM!The indexing flow correctly:
- Resolves matching indices per entity (including aggregator support)
- Wraps entities in aggregators when needed
- Applies
shouldBeIndexedgating- Batches items per index for efficient processing
242-286: LGTM!The search method properly validates the class is searchable, delegates to the engine, validates the response structure, and rehydrates domain objects via the data provider while preserving result ordering.
norkunas
force-pushed
the
dataprovider
branch
from
a0b7b65 to
aeaf95e
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
src/Model/Aggregator.php (1)
71-75: Track theobjectIDvs configurableprimary_keyTODO in normalize()The TODO correctly flags that
normalize()still hardcodes the'objectID'field while indexes can now be configured with an arbitraryprimary_key. It’s fine to leave this as-is for now, but it would be good to either wire the configured primary key through here or open a follow-up issue so this doesn’t get lost.Based on learnings, keeping the actual change out of this PR and handling it separately is reasonable, but it should be explicitly tracked.
src/Services/MeilisearchManager.php (1)
242-286: Identifier key format between indexing and search remains inconsistent for composite identifiers.The mapping between Meilisearch hits and loaded objects still diverges from the format produced by
getSingleIdentifier()for multi-field identifiers:
- During indexing/removal, the identifier stored in Meilisearch is produced via
getSingleIdentifier($provider, $object)(used inbatchProcess()andremove()).- For search, you currently:
- Read
$documentIddirectly from$hit[$index['primary_key']].- Build
$objectsByIdusingimplode('-', $identifiers)when there is more than one identifier value.For composite identifiers,
getSingleIdentifier()builds keys as"field1-value1__field2-value2__", while the search mapping uses only the values joined by'-'. This mismatch means that when aDataProviderInterfaceimplementation legitimately returns multiple identifier values, the search result mapping may fail to match hits back to their domain objects, even though indexing/removal work correctly. This was raised earlier and is still present in the current code.A minimal way to make the contract coherent (and reuse the existing logic) is to have search use
getSingleIdentifier()as the single source of truth for the key format:@@ public function search( - $loadedObjects = $dataProvider->loadByIdentifiers($identifiers); - $objectsById = []; - foreach ($loadedObjects as $object) { - $identifiers = $dataProvider->getIdentifierValues($object); - $key = 1 === \count($identifiers) ? (string) reset($identifiers) : implode('-', $identifiers); - $objectsById[$key] = $object; - } + $loadedObjects = $dataProvider->loadByIdentifiers($identifiers); + $objectsById = []; + foreach ($loadedObjects as $object) { + $singleIdentifier = $this->getSingleIdentifier($dataProvider, $object); + $objectsById[(string) $singleIdentifier] = $object; + } @@ - foreach ($hits as $hit) { - $documentId = $hit[$index['primary_key']]; + foreach ($hits as $hit) { + $documentId = (string) $hit[$index['primary_key']]; if (isset($objectsById[$documentId])) { $results[] = $objectsById[$documentId]; } }This keeps all identifier formatting logic in
getSingleIdentifier(), avoids assumptions about composite key shape insearch(), and ensures that indexing, removal, and search all agree on the same identifier string for anyDataProviderInterfaceimplementation.Also applies to: 438-458
🧹 Nitpick comments (8)
src/Engine.php (2)
60-88: Consider bulk deletions per index instead of per-ID callsYou already aggregate identifiers per
$indexUid; if the Meilisearch PHP client supports adeleteDocuments(array $ids)method, replacing the inner per-ID loop with a single bulk call per index would reduce HTTP overhead for large deletes.Please confirm in the
meilisearch/meilisearch-php(^1.16) documentation whetherdeleteDocuments()is available and behaves equivalently for your use case before changing this.
130-137:normalizeId()matches the supported identifier typesThe helper correctly normalizes
\Stringable|string|intidentifiers tostring|intso Meilisearch never sees an object. As a tiny style nit, you could tighten the condition toinstanceof \Stringablefor self-documentation, but the current implementation is safe given the parameter type.src/DependencyInjection/MeilisearchExtension.php (1)
93-129: Data provider registration logic looks sound.The new
registerDataProviders()method correctly:
- Handles custom data providers via explicit
data_providerconfiguration- Creates ORM providers for Aggregator classes by iterating aggregated entities
- Creates single ORM providers for non-aggregator classes
- Uses
xxh32hashing for unique service definition IDsThe logic aligns with the PR's goal of introducing a provider-based architecture.
One minor consideration:
hash('xxh32', $className)provides a 32-bit hash, which has a low but non-zero collision risk. For DI service IDs, this is likely acceptable given typical class name cardinality, but be aware of potential collisions in large applications.src/Command/MeilisearchImportCommand.php (1)
71-154: Pipeline flagged high complexity inexecute().The method has CyclomaticComplexity (13) and NPathComplexity (592). Consider extracting helper methods for distinct responsibilities (e.g.,
processIndex(),handleSkipBatches()) in a follow-up PR to improve maintainability.Based on learnings, the maintainer prefers to keep PRs focused and defer such improvements to separate PRs.
src/SearchableObject.php (1)
74-78: Consider adding return type validation.The
NormalizableInterface::normalize()method andNormalizerInterface::normalize()can returnmixed. If the wrapped object returns an unexpected type, this could cause downstream issues. Consider adding a runtime assertion or type guard.if ($this->object instanceof NormalizableInterface) { - return $this->object->normalize($this->normalizer, self::NORMALIZATION_FORMAT, $context); + $result = $this->object->normalize($this->normalizer, self::NORMALIZATION_FORMAT, $context); + \assert(\is_array($result)); + + return $result; } - return $this->normalizer->normalize($this->object, self::NORMALIZATION_FORMAT, $context); + $result = $this->normalizer->normalize($this->object, self::NORMALIZATION_FORMAT, $context); + \assert(\is_array($result)); + + return $result;src/SearchManagerInterface.php (2)
31-43: Consider tracking the return shape TODOs.Multiple methods have
@todo: return shapecomments. These should be addressed before a stable release to provide proper type safety for consumers.Would you like me to open an issue to track defining proper return type shapes for these methods?
68-78: Inconsistent@paramand@returnannotation style.The
@param array $searchParamslacks the shape annotation mentioned in the@todo. Consider using@param array<string, mixed>for consistency with other PHPDoc annotations in the codebase./** - * @todo: searchParams and return shapes + * @todo: define searchParams and return shapes * * @param class-string $className - * @param array $searchParams - * - * @return array + * @param array<string, mixed> $searchParams * * @throws NotSearchableException + * + * @return array<string, mixed> */ public function search(string $className, string $query = '', array $searchParams = []): array;src/Services/MeilisearchManager.php (1)
22-64: Class is doing a lot; phpmd complexity/coupling warnings might justify future extraction.
MeilisearchManagercentralizes configuration, data-provider resolution, indexing, searching, and Doctrine compatibility handling, which is why phpmd flagsExcessiveClassComplexityandCouplingBetweenObjects. Functionally this is fine, but long term you might consider extracting helper collaborators (e.g. an index configuration helper and/or a resolver for indices/aggregators) to keep this class focused and make testing easier. I wouldn’t block this PR on that, but it’s worth tracking as a follow‑up.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (55)
.github/workflows/pre-release-tests.yml(2 hunks).github/workflows/tests.yml(1 hunks)composer.json(1 hunks)config/services.php(4 hunks)phpstan-baseline.php(1 hunks)phpstan.dist.neon(1 hunks)src/Command/IndexCommand.php(2 hunks)src/Command/MeilisearchClearCommand.php(1 hunks)src/Command/MeilisearchCreateCommand.php(3 hunks)src/Command/MeilisearchDeleteCommand.php(1 hunks)src/Command/MeilisearchImportCommand.php(7 hunks)src/Command/MeilisearchUpdateSettingsCommand.php(3 hunks)src/DataProvider/DataProviderInterface.php(1 hunks)src/DataProvider/OrmEntityProvider.php(1 hunks)src/DependencyInjection/Configuration.php(2 hunks)src/DependencyInjection/MeilisearchExtension.php(4 hunks)src/Engine.php(5 hunks)src/Event/SettingsUpdatedEvent.php(1 hunks)src/EventListener/ConsoleOutputSubscriber.php(1 hunks)src/EventListener/DoctrineEventSubscriber.php(1 hunks)src/Exception/DataProviderNotFoundException.php(1 hunks)src/Exception/InvalidIndiceException.php(1 hunks)src/Exception/LogicException.php(1 hunks)src/Exception/NotSearchableException.php(1 hunks)src/Model/Aggregator.php(2 hunks)src/SearchManagerInterface.php(1 hunks)src/SearchService.php(1 hunks)src/Searchable.php(1 hunks)src/SearchableEntity.php(2 hunks)src/SearchableObject.php(1 hunks)src/Services/MeilisearchManager.php(1 hunks)src/Services/MeilisearchService.php(9 hunks)src/Services/SettingsUpdater.php(1 hunks)src/Services/UnixTimestampNormalizer.php(1 hunks)tests/BaseKernelTestCase.php(5 hunks)tests/Entity/Actor.php(1 hunks)tests/Entity/Link.php(2 hunks)tests/Entity/SelfNormalizable.php(2 hunks)tests/Entity/Tag.php(2 hunks)tests/Integration/AggregatorTest.php(1 hunks)tests/Integration/Command/MeilisearchClearCommandTest.php(1 hunks)tests/Integration/Command/MeilisearchCreateCommandTest.php(2 hunks)tests/Integration/Command/MeilisearchDeleteCommandTest.php(1 hunks)tests/Integration/Command/MeilisearchImportCommandTest.php(1 hunks)tests/Integration/EngineTest.php(3 hunks)tests/Integration/EventListener/DoctrineEventSubscriberTest.php(8 hunks)tests/Integration/Fixtures/ActorDataProvider.php(1 hunks)tests/Integration/SearchTest.php(4 hunks)tests/Kernel.php(1 hunks)tests/Unit/ConfigurationTest.php(8 hunks)tests/Unit/SerializationTest.php(2 hunks)tests/baseline-ignore(1 hunks)tests/config/doctrine_php7.yaml(0 hunks)tests/config/framework.yaml(1 hunks)tests/config/meilisearch.yaml(1 hunks)
💤 Files with no reviewable changes (1)
- tests/config/doctrine_php7.yaml
🚧 Files skipped from review as they are similar to previous changes (22)
- src/Exception/LogicException.php
- src/Command/MeilisearchClearCommand.php
- src/DataProvider/DataProviderInterface.php
- tests/Entity/Tag.php
- src/EventListener/ConsoleOutputSubscriber.php
- src/Event/SettingsUpdatedEvent.php
- phpstan-baseline.php
- src/Command/MeilisearchDeleteCommand.php
- tests/Kernel.php
- tests/Entity/SelfNormalizable.php
- src/Exception/DataProviderNotFoundException.php
- tests/baseline-ignore
- src/Command/MeilisearchUpdateSettingsCommand.php
- tests/config/framework.yaml
- src/Services/SettingsUpdater.php
- src/SearchService.php
- tests/Integration/Command/MeilisearchImportCommandTest.php
- tests/Unit/ConfigurationTest.php
- src/DataProvider/OrmEntityProvider.php
- tests/Unit/SerializationTest.php
- src/Services/UnixTimestampNormalizer.php
- tests/BaseKernelTestCase.php
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: norkunas
Repo: meilisearch/meilisearch-symfony PR: 391
File: src/Command/MeilisearchImportCommand.php:117-120
Timestamp: 2025-08-05T04:46:01.119Z
Learning: In meilisearch-symfony project, the maintainer norkunas prefers to keep PRs focused and defer error handling improvements for settings updates to separate PRs, particularly when it involves decisions about partial import behavior when some indexes fail.
📚 Learning: 2025-08-05T04:46:01.119Z
Learnt from: norkunas
Repo: meilisearch/meilisearch-symfony PR: 391
File: src/Command/MeilisearchImportCommand.php:117-120
Timestamp: 2025-08-05T04:46:01.119Z
Learning: In meilisearch-symfony project, the maintainer norkunas prefers to keep PRs focused and defer error handling improvements for settings updates to separate PRs, particularly when it involves decisions about partial import behavior when some indexes fail.
Applied to files:
tests/Integration/Fixtures/ActorDataProvider.phpsrc/Command/MeilisearchImportCommand.phpsrc/Engine.phptests/Integration/EngineTest.phpsrc/Services/MeilisearchManager.php.github/workflows/tests.yml
🧬 Code graph analysis (16)
src/DependencyInjection/Configuration.php (1)
src/SearchableObject.php (1)
SearchableObject(13-80)
src/EventListener/DoctrineEventSubscriber.php (1)
src/SearchManagerInterface.php (2)
index(36-36)remove(43-43)
tests/Integration/Fixtures/ActorDataProvider.php (3)
tests/Entity/Actor.php (1)
Actor(7-14)src/DataProvider/DataProviderInterface.php (4)
provide(18-18)loadByIdentifiers(25-25)getIdentifierValues(32-32)cleanup(34-34)src/DataProvider/OrmEntityProvider.php (4)
provide(21-30)loadByIdentifiers(32-45)getIdentifierValues(47-52)cleanup(54-57)
src/Exception/InvalidIndiceException.php (2)
src/Exception/DataProviderNotFoundException.php (1)
__construct(9-12)src/Exception/NotSearchableException.php (1)
__construct(9-12)
src/DependencyInjection/MeilisearchExtension.php (6)
src/DataProvider/OrmEntityProvider.php (1)
OrmEntityProvider(10-58)src/MeilisearchBundle.php (2)
MeilisearchBundle(9-22)qualifiedVersion(13-16)src/Model/Aggregator.php (2)
Aggregator(12-76)getEntities(42-45)src/Document/Aggregator.php (1)
Aggregator(9-11)src/Entity/Aggregator.php (1)
Aggregator(9-11)tests/Entity/ContentAggregator.php (1)
getEntities(21-27)
tests/Entity/Link.php (1)
src/SearchableObject.php (1)
SearchableObject(13-80)
tests/Integration/AggregatorTest.php (2)
src/Model/Aggregator.php (1)
getEntityClassFromObjectID(60-69)src/Exception/InvalidEntityForAggregator.php (1)
InvalidEntityForAggregator(7-9)
src/Command/IndexCommand.php (5)
src/Command/MeilisearchCreateCommand.php (1)
__construct(23-30)src/Command/MeilisearchUpdateSettingsCommand.php (1)
__construct(22-28)src/Services/MeilisearchService.php (2)
__construct(52-64)getConfiguration(87-96)src/SearchManagerInterface.php (1)
getConfiguration(24-24)src/Services/MeilisearchManager.php (1)
getConfiguration(93-96)
src/Engine.php (1)
src/SearchableObject.php (5)
SearchableObject(13-80)getSearchableArray(65-79)getIndexUid(42-45)getPrimaryKey(50-53)getIdentifier(55-58)
src/Services/MeilisearchService.php (2)
src/SearchManagerInterface.php (11)
isSearchable(17-17)getConfiguration(24-24)searchableAs(22-22)index(36-36)remove(43-43)clear(50-50)deleteByIndexName(57-57)delete(66-66)search(78-78)rawSearch(90-90)count(100-100)src/Services/MeilisearchManager.php (13)
isSearchable(69-74)getBaseClassName(342-349)getConfiguration(93-96)searchableAs(79-91)index(121-166)remove(171-204)clear(209-214)assertIsSearchable(431-436)deleteByIndexName(219-222)delete(227-232)search(242-286)rawSearch(296-304)count(312-317)
config/services.php (3)
src/Services/MeilisearchManager.php (1)
MeilisearchManager(22-510)src/EventListener/DoctrineEventSubscriber.php (1)
DoctrineEventSubscriber(10-30)src/Services/SettingsUpdater.php (1)
SettingsUpdater(16-77)
tests/Integration/EngineTest.php (3)
src/SearchableObject.php (1)
SearchableObject(13-80)tests/Entity/Image.php (1)
getId(39-42)tests/Entity/Post.php (1)
getId(91-94)
tests/Integration/EventListener/DoctrineEventSubscriberTest.php (2)
src/SearchManagerInterface.php (1)
search(78-78)src/Services/MeilisearchManager.php (1)
search(242-286)
src/SearchableObject.php (3)
src/SearchableEntity.php (1)
getSearchableArray(66-91)tests/Entity/Link.php (1)
normalize(74-85)tests/Entity/SelfNormalizable.php (1)
normalize(64-75)
src/Services/MeilisearchManager.php (9)
src/Collection.php (1)
Collection(13-380)src/Engine.php (8)
Engine(10-138)__construct(12-14)index(25-54)remove(62-88)clear(97-100)delete(105-108)search(115-118)count(125-128)src/Exception/DataProviderNotFoundException.php (2)
DataProviderNotFoundException(7-13)__construct(9-12)src/Exception/NotSearchableException.php (2)
NotSearchableException(7-13)__construct(9-12)src/Exception/SearchHitsNotFoundException.php (1)
SearchHitsNotFoundException(7-9)src/Model/Aggregator.php (3)
Aggregator(12-76)__construct(26-35)getEntities(42-45)src/SearchableObject.php (2)
SearchableObject(13-80)__construct(28-37)src/DataProvider/OrmEntityProvider.php (3)
__construct(15-19)getIdentifierValues(47-52)loadByIdentifiers(32-45)src/DataProvider/DataProviderInterface.php (2)
getIdentifierValues(32-32)loadByIdentifiers(25-25)
tests/Integration/SearchTest.php (4)
tests/BaseKernelTestCase.php (1)
waitForAllTasks(47-52)src/Engine.php (1)
search(115-118)src/SearchManagerInterface.php (2)
search(78-78)rawSearch(90-90)src/Services/MeilisearchManager.php (2)
search(242-286)rawSearch(296-304)
🪛 GitHub Actions: Tests
src/Command/MeilisearchImportCommand.php
[warning] 23-23: phpmd: CouplingBetweenObjects (value 13) in MeilisearchImportCommand.php
[warning] 71-71: phpmd: CyclomaticComplexity (13) in execute()
[warning] 71-71: phpmd: NPathComplexity (592) in execute()
src/DependencyInjection/MeilisearchExtension.php
[warning] 22-22: phpmd: IfStatementAssignment (line 44, col 20) in load extension
[warning] 36-36: phpmd: StaticAccess to \Meilisearch\Bundle\MeilisearchBundle in load
[warning] 51-51: phpmd: ElseExpression used in method load
[warning] 59-59: phpmd: StaticAccess to \Meilisearch\Bundle\MeilisearchBundle in load
[warning] 118-118: phpmd: ElseExpression used in method registerDataProviders
src/Services/MeilisearchService.php
[warning] 27-27: phpmd: TooManyPublicMethods (11) in MeilisearchService
[warning] 27-27: phpmd: ExcessiveClassComplexity (67) in MeilisearchService
[warning] 27-27: phpmd: CouplingBetweenObjects (16) in MeilisearchService
[warning] 48-48: phpmd: LongVariable (>$20) in $classToSerializerGroup
[warning] 57-57: phpmd: StaticAccess to \Symfony\Component\PropertyAccess\PropertyAccess in __construct
[warning] 415-415: phpmd: LongVariable (>$20) in $searchableEntitiesChunk
[warning] 463-463: phpmd: StaticAccess to \Doctrine\ORM\Proxy\DefaultProxyClassNameResolver in resolveClass
[warning] 467-467: phpmd: StaticAccess to \Doctrine\Common\Util\ClassUtils in resolveClass
src/Services/MeilisearchManager.php
[warning] 22-22: phpmd: ExcessiveClassComplexity (71) in MeilisearchManager
[warning] 22-22: phpmd: CouplingBetweenObjects (14) in MeilisearchManager
[warning] 501-501: phpmd: StaticAccess to \Doctrine\ORM\Proxy\DefaultProxyClassNameResolver in resolveClass
[warning] 505-505: phpmd: StaticAccess to \Doctrine\Common\Util\ClassUtils in resolveClass
🔇 Additional comments (49)
phpstan.dist.neon (1)
1-2: I'll help you verify the review comment for the phpstan.dist.neon file. Let me check if the baseline file exists and contains the expected error suppressions mentioned in the review.
<function_calls>
#!/bin/bashCheck if phpstan-baseline.php exists and preview its contents
echo "=== Checking for phpstan-baseline.php ==="
if [ -f phpstan-baseline.php ]; then
echo "✓ Baseline file exists"
echo ""
echo "=== File size and line count ==="
wc -l phpstan-baseline.php
echo ""
echo "=== First 50 lines of phpstan-baseline.php ==="
head -50 phpstan-baseline.php
echo ""
echo "=== Checking for expected error patterns ==="
echo "Searching for 'ClassUtils'..."
rg -n 'ClassUtils' phpstan-baseline.php || echo "Pattern not found"
echo ""
echo "Searching for 'array_unique'..."
rg -n 'array_unique' phpstan-baseline.php || echo "Pattern not found"
else
echo "✗ Baseline file not found at repository root"
echo ""
echo "=== Searching for phpstan-baseline.php anywhere in repo ==="
fd phpstan-baseline phpstan-baseline.php 2>/dev/null || echo "Not found in repository"
fi
</function_calls>src/EventListener/DoctrineEventSubscriber.php (2)
8-13: Constructor migration toSearchManagerInterfacelooks correctUsing
SearchManagerInterfacewith constructor property promotion and areadonlydependency cleanly aligns this subscriber with the new manager-based API and improves decoupling from the concrete service.
18-29: Lifecycle callbacks correctly delegate toSearchManagerInterfaceForwarding the Doctrine entity from
LifecycleEventArgsdirectly intoindex()/remove()matches the newobject|arraysignatures and preserves the original behavior of indexing on update/persist and removing on pre-remove.src/Exception/InvalidIndiceException.php (1)
9-12: Constructor$codetype-hint is correct and consistentTyping
$codeasintmatches the base\Exceptionsignature and the new exception classes introduced in this PR; no behavioral change beyond enforcing the intended scalar type..github/workflows/tests.yml (1)
30-51: Updated test matrix matches supported PHP/Symfony rangeLimiting to PHP 8.1–8.4 and Symfony 6.4–7.3, with explicit 8.1+7.x exclusions and focused 8.4 lowest/highest runs, aligns with the new composer constraints and keeps CI coverage sensible.
tests/Integration/AggregatorTest.php (1)
24-37: Constructor exception test is now clearer and correctly scopedRenaming to
testConstructorThrowsWithMoreThanOnePrimaryKeyand movingexpectException()immediately before theContentAggregatorconstruction makes the intent explicit and ensures only the constructor is under test.src/Searchable.php (1)
7-21: Deprecation notices clearly guide migration toSearchableObjectMarking
Searchableand its constants as deprecated with explicit references toSearchableObjectand its constants provides a clean upgrade path while preserving runtime behavior.src/Model/Aggregator.php (1)
24-35: Stronger typing for$entityis safe and helpfulConstraining
$entitytoobjectboth on the property and in the constructor improves static safety without changing the Aggregator semantics around Doctrine entities/documents.src/Engine.php (2)
12-14: Constructor promotion for$clientis a clean, non-breaking refactorUsing property promotion here keeps the public API identical while reducing boilerplate; no behavioral change.
21-54: Per-index batching withSearchableObjectcorrectly handles primary keysThe new
index()flow—wrapping a singleSearchableObjectinto an array, grouping by$indexUid, storing each index’s ownprimaryKey, and then callingaddDocuments($batch['documents'], $batch['primaryKey'])—fixes the earlier “one primary key for all indexes” issue. Using$searchableArray + [$object->getPrimaryKey() => $this->normalizeId($object->getIdentifier())]also sensibly avoids overwriting any primary key already present in the normalized payload.composer.json (1)
21-52: Dependency constraints align with the new support policyRequiring PHP ^8.1, tightening Symfony components to ^6.4 || ^7.0, and moving
doctrine/doctrine-bundleto require-dev all match the CI matrices and the goal of making ORM an explicit application dependency. The tooling bumps look incremental and non-risky..github/workflows/pre-release-tests.yml (1)
42-63: Pre-release matrix is consistent with main CI and dependency strategyThe PHP/Symfony/dependencies matrix, exclusions, and targeted 8.4 lowest/highest runs mirror the main tests workflow, and the updated
composer-installconfiguration correctly exercises different dependency sets against Meilisearch RCs.Also applies to: 65-86
tests/Integration/Command/MeilisearchClearCommandTest.php (1)
39-39: LGTM!The test expectation correctly includes the new
actorindex in the cleared indices output.tests/Integration/Command/MeilisearchDeleteCommandTest.php (1)
40-40: LGTM!The test expectation correctly includes the new
actorindex in the deleted indices output.tests/Integration/Command/MeilisearchCreateCommandTest.php (1)
63-63: LGTM!The test expectations correctly include the actor index creation in both the
updateSettings=trueandupdateSettings=falsebranches.Also applies to: 80-80
tests/Entity/Actor.php (1)
1-14: LGTM!The
Actortest entity is well-designed: it uses PHP 8.1 constructor property promotion with readonly properties for immutability, and it's appropriately simple for demonstrating the custom data provider functionality.tests/config/meilisearch.yaml (1)
57-60: LGTM!The actor index configuration correctly demonstrates the new custom data provider feature with
type: 'custom'and references theActorDataProviderfor data provisioning.src/SearchableEntity.php (2)
15-17: Deprecation notice added appropriately.The deprecation annotation clearly indicates the migration path to
SearchableObject. Ensure that version "0.16" aligns with your release planning for this PR.
83-83: LGTM!The normalization format constant references are correctly updated from
Searchable::NORMALIZATION_FORMATtoSearchableObject::NORMALIZATION_FORMATin both code paths.Also applies to: 87-87
tests/Entity/Link.php (1)
9-9: LGTM!The import and constant reference are correctly updated from
SearchabletoSearchableObject, maintaining consistency with the migration.Also applies to: 76-76
src/DependencyInjection/Configuration.php (2)
57-59: Consider the default primary_key value.The default
primary_key: 'objectID'uses Algolia-specific terminology. According to PR objective #66, one goal is to "Remove 'objectID' usage" and adopt Meilisearch's primary key semantics. Consider whether:
- The default should be
'id'(Meilisearch's default) or- The default should be
nullto let Meilisearch infer the primary key, or'objectID'is intentionally retained for backward compatibilityBased on PR objectives, this default may need alignment with Meilisearch conventions.
7-7: LGTM!The import and serializer-related references are correctly updated from
SearchabletoSearchableObject, maintaining consistency with the broader migration. The configuration structure properly supports the new data provider architecture.Also applies to: 63-68
tests/Integration/EventListener/DoctrineEventSubscriberTest.php (1)
23-23: LGTM! Clean migration to the new SearchManager API.The test calls have been correctly updated to use the new
searchManager->search()signature, removing theentityManagerparameter. The migration is consistent across all test methods.Also applies to: 38-38, 59-59, 81-81, 97-97, 107-107, 121-121, 131-131
src/Exception/NotSearchableException.php (1)
7-13: LGTM! Well-designed exception class.The exception is properly typed with
int $code, follows PHP 8.1+ standards, and provides a clear, formatted message for non-searchable classes.src/Command/IndexCommand.php (2)
8-8: LGTM! Clean migration to SearchManagerInterface.The command has been properly updated to use
SearchManagerInterfacewith constructor property promotion, following PHP 8.1+ best practices. The migration is consistent with other commands in the PR.Also applies to: 19-24
28-28: Configuration access updated correctly.The
getConfiguration()call now uses the newsearchManagerinstance, aligning with the manager-based architecture.tests/Integration/Fixtures/ActorDataProvider.php (3)
15-44: LGTM! Straightforward test data provider.The
provide()method correctly slices a static list of Actor instances using the given offset and limit parameters.
46-57: Potential logic issue: single ID match vs. multiple IDs.The
loadByIdentifiers()implementation assumes$identifiersis an associative array with a single 'id' key ($identifiers['id']), and uses strict equality (===) to match exactly one actor. However:
- The method signature suggests it should handle multiple identifiers (plural: "identifiers").
- If
$identifierscontains['id' => 5], only one actor will match, which may be correct for single-ID lookups.- If the caller passes multiple IDs, this implementation won't work.
Verify whether this test fixture is intended to handle single-ID or multiple-ID scenarios. If it should support multiple IDs, consider:
public function loadByIdentifiers(array $identifiers): array { $actors = []; foreach ($this->provide(PHP_INT_MAX, 0) as $actor) { - if ($actor->id === $identifiers['id']) { + if (in_array($actor->id, $identifiers, true)) { $actors[] = $actor; } } return $actors; }Or clarify in a comment that this fixture expects a single-ID associative array format.
Based on learnings, the maintainer previously indicated this is intentional to showcase custom provider flexibility. However, it's worth confirming the expected usage pattern.
59-64: LGTM! Identifier extraction is correct.The
getIdentifierValues()method correctly asserts the object type and returns the identifier as an associative array.tests/Integration/SearchTest.php (2)
71-71: Good synchronization: waitForAllTasks() added.The addition of
waitForAllTasks()calls before search operations is essential for ensuring asynchronous indexing tasks complete before assertions. This prevents flaky tests due to race conditions.Also applies to: 114-114, 137-137
75-75: LGTM! Correct migration to SearchManager API.All search and rawSearch calls have been correctly updated to use
searchManagerwith the new signature that removes theObjectManagerparameter. The pagination test (line 116) correctly passes search parameters as an array.Also applies to: 81-81, 90-90, 116-116, 139-139
src/Services/MeilisearchService.php (4)
24-26: Solid backward-compatibility strategy with optional manager delegation.The deprecation approach correctly:
- Marks the class as deprecated with clear guidance to
MeilisearchManager- Accepts an optional
SearchManagerInterface $managerparameter- Preserves existing behavior when no manager is provided
This allows gradual migration without breaking existing integrations.
Also applies to: 50-58
66-77: Consistent delegation pattern with deprecation notice.The
isSearchable()method correctly:
- Emits a deprecation notice guiding users to the new manager
- Delegates to the manager when available
- Falls back to legacy logic otherwise
This pattern is repeated consistently across all public methods.
114-121: Deprecation notice guides users away from ObjectManager parameter.The deprecation messages for
index(),remove(), andsearch()correctly indicate that passingDoctrine\Persistence\ObjectManageris deprecated, aligning with PR objectives to decouple from Doctrine-specific APIs.Also applies to: 149-156, 209-220
346-347: Defensive string casting for aggregator class references.Explicitly casting
$index['class']to string when storing in$entitiesAggregatorsand$aggregatorsarrays is a good defensive practice, ensuring consistent string-based lookups even if the configuration provides class-string types.tests/Integration/EngineTest.php (2)
35-42: I was unable to access the repository directly due to a clone failure. However, based on Meilisearch's documented primary key semantics, I can provide analysis:Meilisearch Primary Key Facts:
- Meilisearch auto-infers primary keys from attribute names ending with "id" (case-insensitive:
id,uid,BookId, etc.)- If the Image entity has an
idfield/method (likegetId()), Meilisearch would naturally infer it as the primary key- Using an explicit
'objectID'string suggests an intentional override of this auto-inferenceWithout access to the codebase, I cannot definitively verify:
- Whether the Image entity's standard primary key is
'id'or'objectID'- What PR #66 specifically requires regarding objectID removal
- The actual index configuration for the 'image' index
To complete this verification, I would need to see:
- The Image entity class definition
- The actual Meilisearch index configuration for 'image'
- The PR #66 requirements and related code changes
Verify that
'objectID'parameter in the SearchableObject instantiation aligns with the Image entity's configured primary key in the Meilisearch index. If PR objective #66 requires removing objectID usage in favor of standard Meilisearch id conventions, update this parameter to match the Image entity's actual primary key field (likely'id').
67-69: Unable to complete automated verification of 'objectID' primary key configuration.The repository is currently inaccessible through the verification tools. To confirm whether
'objectID'is the intended primary key for the posts index, manual verification is required:
- Confirm the primary key configured for the posts index in your test configuration (tests/config/ or index setup code)
- Verify that Post entity's getId() method aligns with this configuration
- Cross-reference line 37 to ensure the same primary key is used consistently throughout the test
Once confirmed, the SearchableObject instantiation at lines 67-69 should match the configured primary key.
src/DependencyInjection/MeilisearchExtension.php (1)
110-124: Based on my verification, I have found that the code pattern referenced in this review comment does not exist in the meilisearch-symfony repository. Web searches confirmed that:
- The meilisearch-symfony repository does not use an Aggregator/getEntities pattern (which is an Algolia-specific pattern)
- The exact code snippet shown (lines 110-124 with
is_subclass_of($class, Aggregator::class)and$class::getEntities()) cannot be found in the codebase- MeilisearchExtension.php exists but does not contain the code shown in the review
The repository uses Symfony/Doctrine indexing directly rather than the Aggregator abstraction pattern described in the review comment.
The review comment references code that does not exist in the meilisearch-symfony repository. The Aggregator/getEntities pattern is specific to the Algolia Symfony bundle, not meilisearch-symfony. The verification request cannot be addressed because the code being reviewed is not present in the codebase.
Likely an incorrect or invalid review comment.
config/services.php (2)
70-78: LGTM!The new
meilisearch.managerservice is properly defined with appropriate abstract arguments for configuration. The alias toSearchManagerInterfacefollows Symfony best practices for interface-based dependency injection.
28-36: Verify the circular dependency is safe.
meilisearch.servicenow depends onmeilisearch.manager(line 35). Ensure this doesn't create issues ifMeilisearchServiceis only a deprecation bridge delegating to the manager.src/Command/MeilisearchCreateCommand.php (2)
23-30: LGTM!Clean constructor refactor using PHP 8.1+ promoted readonly properties. The dependency injection correctly uses
SearchManagerInterfaceand the parent constructor is properly invoked.
63-68: LGTM!The
isSearchablecheck correctly uses the newsearchManagerproperty, consistent with the interface contract.src/Command/MeilisearchImportCommand.php (3)
27-34: LGTM!Constructor properly migrated to use
SearchManagerInterfacewith promoted readonly properties. The parent constructor is correctly invoked with the manager.
119-143: Data provider instantiation correctly moved outside the loop.The
getDataProvider()call is now at line 119, before thedo-whileloop, addressing the previous review feedback. Thecleanup()call inside the loop (line 140) appears intentional for releasing memory between batches, which is appropriate for large dataset imports.
79-80: Defensive batch-size parsing.The
ctype_digit()check correctly validates string input before casting. Note thatctype_digit('')returnsfalse, so the empty string fallback is safe.src/SearchableObject.php (2)
28-37: LGTM!Well-structured constructor with proper readonly properties. The normalization context correctly merges user-provided context with the
meilisearchflag.
65-79: LGTM!The normalization logic correctly uses a local
$contextvariable (addressing the previous mutation concern) and properly handles bothNormalizableInterfaceimplementations and regular objects. The Symfony version check for DateTime normalization settings is appropriate.src/SearchManagerInterface.php (2)
10-12: LGTM!Clean interface declaration with a well-named constant for result key access. The
RESULT_KEY_HITSconstant improves consistency across the codebase.
14-17: Flexible parameter type forisSearchable.The
object|stringunion type allows both runtime object instances and class strings, which is practical for different calling contexts.
norkunas
force-pushed
the
dataprovider
branch
from
aeaf95e to
2e54bae
Compare
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/Services/MeilisearchService.php (1)
388-400: Constructor signature mismatch with Aggregator.The
getAggregatorsFromEntitiesmethod instantiates aggregators with only 2 arguments, but the updatedAggregatorconstructor now requires 3 (including$primaryKey). This will cause a fatal error when the legacy code path is used.foreach ($this->entitiesAggregators[$entityClassName] as $aggregator) { $aggregators[] = new $aggregator( $entity, - $objectManager->getClassMetadata($entityClassName)->getIdentifierValues($entity) + $objectManager->getClassMetadata($entityClassName)->getIdentifierValues($entity), + 'objectID' // Legacy default for backward compatibility ); }
🧹 Nitpick comments (6)
src/Searchable.php (1)
7-20: Consider adding runtime deprecation triggers.The docblock deprecations inform developers reading the code but won't generate warnings at runtime. For clearer migration signals, consider triggering deprecations when the class or constants are used:
<?php declare(strict_types=1); namespace Meilisearch\Bundle; +trigger_deprecation('meilisearch/search-bundle', '0.16', 'The "%s" class is deprecated, use "%s" instead.', Searchable::class, SearchableObject::class); + /** * @deprecated Since 0.16, use `Meilisearch\Bundle\SearchableObject` instead. */ final class Searchable {This follows Symfony's deprecation conventions. However, given this is a v0 project with expected BC breaks, the documentation-only approach is also acceptable.
src/DependencyInjection/Configuration.php (1)
7-8: Index configuration aligns with the data-provider model; consider validatingtype=customat config timeImporting and using
SearchableObjectfor serializer groups is a good move for consistency, and the newtype,primary_key, anddata_providernodes give clear per-index control while preserving BC via sensible defaults (orm+objectID).You might optionally add a config validator so that
type: customwithout adata_providerfails early in configuration rather than later at runtime, but that’s not required for this PR to function.Also applies to: 53-61, 63-69
src/EventListener/DoctrineEventSubscriber.php (1)
8-9: Subscriber correctly migrated toSearchManagerInterfaceUsing constructor promotion for
SearchManagerInterfaceand delegatingpostUpdate/postPersisttoindex()andpreRemovetoremove()keeps the Doctrine subscriber behavior intact while aligning with the new manager abstraction.If you ever touch this again, you could DRY up the duplicated
index($args->getObject())calls via a tiny private helper, but it’s not necessary for this PR.Also applies to: 12-29
src/DataProvider/OrmEntityProvider.php (1)
47-52: Minor: Avoid redundantget_class()calls.
get_class($object)is called twice. Consider caching the result.public function getIdentifierValues(object $object): array { - $manager = $this->managerRegistry->getManagerForClass(\get_class($object)); + $className = \get_class($object); + $manager = $this->managerRegistry->getManagerForClass($className); - return $manager->getClassMetadata(\get_class($object))->getIdentifierValues($object); + return $manager->getClassMetadata($className)->getIdentifierValues($object); }src/Command/MeilisearchImportCommand.php (1)
71-155: Pipeline flags complexity thresholds exceeded.The
execute()method exceeds configured cyclomatic complexity (13 vs 10) and NPath complexity (592 vs 200). While the logic is clear and the refactoring to useSearchManagerInterfaceis correct, consider extracting helper methods in a follow-up PR to reduce complexity:
- Extract the per-index import loop body into a separate method
- Extract batch processing and response logging
Based on learnings, this can be deferred to a separate PR to keep this one focused.
src/Services/MeilisearchManager.php (1)
27-27: Pipeline flags class complexity thresholds exceeded.The class exceeds configured complexity (70 vs 50) and coupling (13 dependencies). While the implementation is cohesive and necessary for the feature, consider extracting concerns in follow-up PRs:
- Extract
resolveClass()and proxy detection into a separate utility class- Consider a dedicated
IndexResolverforgetIndicesForEntity()logic- Move aggregator-related logic to a separate handler
Based on learnings, this refactoring can be deferred to keep this PR focused on the data provider feature.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (55)
.github/workflows/pre-release-tests.yml(2 hunks).github/workflows/tests.yml(1 hunks)composer.json(1 hunks)config/services.php(4 hunks)phpstan-baseline.php(1 hunks)phpstan.dist.neon(1 hunks)src/Command/IndexCommand.php(2 hunks)src/Command/MeilisearchClearCommand.php(1 hunks)src/Command/MeilisearchCreateCommand.php(3 hunks)src/Command/MeilisearchDeleteCommand.php(1 hunks)src/Command/MeilisearchImportCommand.php(7 hunks)src/Command/MeilisearchUpdateSettingsCommand.php(3 hunks)src/DataProvider/DataProviderInterface.php(1 hunks)src/DataProvider/OrmEntityProvider.php(1 hunks)src/DependencyInjection/Configuration.php(2 hunks)src/DependencyInjection/MeilisearchExtension.php(4 hunks)src/Engine.php(5 hunks)src/Event/SettingsUpdatedEvent.php(1 hunks)src/EventListener/ConsoleOutputSubscriber.php(1 hunks)src/EventListener/DoctrineEventSubscriber.php(1 hunks)src/Exception/DataProviderNotFoundException.php(1 hunks)src/Exception/InvalidIndiceException.php(1 hunks)src/Exception/LogicException.php(1 hunks)src/Exception/NotSearchableException.php(1 hunks)src/Model/Aggregator.php(2 hunks)src/SearchManagerInterface.php(1 hunks)src/SearchService.php(1 hunks)src/Searchable.php(1 hunks)src/SearchableEntity.php(2 hunks)src/SearchableObject.php(1 hunks)src/Services/MeilisearchManager.php(1 hunks)src/Services/MeilisearchService.php(9 hunks)src/Services/SettingsUpdater.php(1 hunks)src/Services/UnixTimestampNormalizer.php(1 hunks)tests/BaseKernelTestCase.php(5 hunks)tests/Entity/Actor.php(1 hunks)tests/Entity/Link.php(2 hunks)tests/Entity/SelfNormalizable.php(2 hunks)tests/Entity/Tag.php(2 hunks)tests/Integration/AggregatorTest.php(2 hunks)tests/Integration/Command/MeilisearchClearCommandTest.php(1 hunks)tests/Integration/Command/MeilisearchCreateCommandTest.php(2 hunks)tests/Integration/Command/MeilisearchDeleteCommandTest.php(1 hunks)tests/Integration/Command/MeilisearchImportCommandTest.php(1 hunks)tests/Integration/EngineTest.php(3 hunks)tests/Integration/EventListener/DoctrineEventSubscriberTest.php(8 hunks)tests/Integration/Fixtures/ActorDataProvider.php(1 hunks)tests/Integration/SearchTest.php(4 hunks)tests/Kernel.php(1 hunks)tests/Unit/ConfigurationTest.php(8 hunks)tests/Unit/SerializationTest.php(2 hunks)tests/baseline-ignore(1 hunks)tests/config/doctrine_php7.yaml(0 hunks)tests/config/framework.yaml(1 hunks)tests/config/meilisearch.yaml(1 hunks)
💤 Files with no reviewable changes (1)
- tests/config/doctrine_php7.yaml
🚧 Files skipped from review as they are similar to previous changes (28)
- tests/Entity/Actor.php
- phpstan-baseline.php
- tests/Entity/Link.php
- tests/Integration/Command/MeilisearchCreateCommandTest.php
- tests/baseline-ignore
- tests/config/meilisearch.yaml
- src/Command/IndexCommand.php
- src/Command/MeilisearchDeleteCommand.php
- tests/Entity/SelfNormalizable.php
- tests/Integration/Command/MeilisearchClearCommandTest.php
- tests/Entity/Tag.php
- tests/Integration/Command/MeilisearchDeleteCommandTest.php
- src/DataProvider/DataProviderInterface.php
- src/Command/MeilisearchCreateCommand.php
- src/SearchService.php
- src/EventListener/ConsoleOutputSubscriber.php
- src/Exception/LogicException.php
- src/Exception/InvalidIndiceException.php
- src/Command/MeilisearchClearCommand.php
- phpstan.dist.neon
- src/Services/UnixTimestampNormalizer.php
- src/Event/SettingsUpdatedEvent.php
- tests/Integration/SearchTest.php
- tests/Integration/Command/MeilisearchImportCommandTest.php
- src/SearchableObject.php
- tests/config/framework.yaml
- tests/BaseKernelTestCase.php
- tests/Integration/EngineTest.php
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: norkunas
Repo: meilisearch/meilisearch-symfony PR: 391
File: src/Command/MeilisearchImportCommand.php:117-120
Timestamp: 2025-08-05T04:46:01.119Z
Learning: In meilisearch-symfony project, the maintainer norkunas prefers to keep PRs focused and defer error handling improvements for settings updates to separate PRs, particularly when it involves decisions about partial import behavior when some indexes fail.
📚 Learning: 2025-08-05T04:46:01.119Z
Learnt from: norkunas
Repo: meilisearch/meilisearch-symfony PR: 391
File: src/Command/MeilisearchImportCommand.php:117-120
Timestamp: 2025-08-05T04:46:01.119Z
Learning: In meilisearch-symfony project, the maintainer norkunas prefers to keep PRs focused and defer error handling improvements for settings updates to separate PRs, particularly when it involves decisions about partial import behavior when some indexes fail.
Applied to files:
src/Services/MeilisearchManager.phpsrc/Command/MeilisearchImportCommand.phptests/Integration/Fixtures/ActorDataProvider.phpsrc/DataProvider/OrmEntityProvider.php.github/workflows/tests.ymlsrc/Engine.php
🧬 Code graph analysis (18)
src/Exception/NotSearchableException.php (2)
src/Exception/DataProviderNotFoundException.php (1)
__construct(9-12)src/Exception/InvalidIndiceException.php (1)
__construct(9-12)
src/DependencyInjection/Configuration.php (1)
src/SearchableObject.php (1)
SearchableObject(13-80)
src/SearchableEntity.php (1)
src/SearchableObject.php (1)
SearchableObject(13-80)
tests/Integration/EventListener/DoctrineEventSubscriberTest.php (3)
src/Engine.php (1)
search(159-162)src/SearchManagerInterface.php (1)
search(92-92)src/Services/MeilisearchManager.php (1)
search(219-258)
src/Services/MeilisearchManager.php (11)
src/Collection.php (1)
Collection(13-380)src/Engine.php (8)
Engine(44-182)__construct(46-48)index(61-90)remove(100-126)clear(137-140)delete(147-150)search(159-162)count(169-172)src/Exception/DataProviderNotFoundException.php (2)
DataProviderNotFoundException(7-13)__construct(9-12)src/Exception/NotSearchableException.php (2)
NotSearchableException(7-13)__construct(9-12)src/Model/Aggregator.php (3)
Aggregator(12-78)__construct(26-38)getEntities(45-48)src/SearchableObject.php (2)
SearchableObject(13-80)__construct(28-37)src/DataProvider/OrmEntityProvider.php (2)
__construct(15-19)getIdentifierValues(47-52)src/EventListener/ConsoleOutputSubscriber.php (1)
__construct(13-15)src/EventListener/DoctrineEventSubscriber.php (1)
__construct(12-14)src/SearchManagerInterface.php (11)
isSearchable(23-23)searchableAs(30-30)getConfiguration(32-32)index(48-48)remove(55-55)clear(64-64)deleteByIndexName(71-71)delete(80-80)search(92-92)count(110-110)rawSearch(102-102)src/DataProvider/DataProviderInterface.php (1)
getIdentifierValues(32-32)
src/Command/MeilisearchImportCommand.php (3)
src/Command/IndexCommand.php (1)
__construct(19-24)src/SearchManagerInterface.php (6)
getConfiguration(32-32)isSearchable(23-23)getDataProvider(41-41)index(48-48)count(110-110)deleteByIndexName(71-71)src/DataProvider/DataProviderInterface.php (2)
provide(18-18)cleanup(34-34)
src/EventListener/DoctrineEventSubscriber.php (1)
src/SearchManagerInterface.php (2)
index(48-48)remove(55-55)
src/DependencyInjection/MeilisearchExtension.php (5)
src/DataProvider/OrmEntityProvider.php (1)
OrmEntityProvider(10-58)src/MeilisearchBundle.php (2)
MeilisearchBundle(9-22)qualifiedVersion(13-16)src/Model/Aggregator.php (2)
Aggregator(12-78)getEntities(45-48)src/Document/Aggregator.php (1)
Aggregator(9-11)tests/Entity/ContentAggregator.php (1)
getEntities(21-27)
tests/Integration/Fixtures/ActorDataProvider.php (3)
tests/Entity/Actor.php (1)
Actor(7-14)src/DataProvider/DataProviderInterface.php (4)
provide(18-18)loadByIdentifiers(25-25)getIdentifierValues(32-32)cleanup(34-34)src/DataProvider/OrmEntityProvider.php (4)
provide(21-30)loadByIdentifiers(32-45)getIdentifierValues(47-52)cleanup(54-57)
src/DataProvider/OrmEntityProvider.php (2)
src/Exception/LogicException.php (1)
LogicException(7-9)src/DataProvider/DataProviderInterface.php (4)
provide(18-18)loadByIdentifiers(25-25)getIdentifierValues(32-32)cleanup(34-34)
src/Services/SettingsUpdater.php (3)
src/Services/MeilisearchManager.php (2)
MeilisearchManager(27-468)getConfiguration(92-95)src/SearchManagerInterface.php (1)
getConfiguration(32-32)src/SearchService.php (1)
getConfiguration(27-27)
src/Services/MeilisearchService.php (4)
src/Command/MeilisearchCreateCommand.php (1)
__construct(23-30)src/Command/MeilisearchImportCommand.php (1)
__construct(27-34)src/EventListener/DoctrineEventSubscriber.php (1)
__construct(12-14)src/Services/MeilisearchManager.php (13)
isSearchable(71-76)getBaseClassName(300-307)getConfiguration(92-95)searchableAs(78-90)index(114-163)remove(165-198)clear(200-205)assertIsSearchable(389-394)deleteByIndexName(207-210)delete(212-217)search(219-258)rawSearch(260-268)count(270-275)
config/services.php (3)
src/Services/MeilisearchManager.php (1)
MeilisearchManager(27-468)src/EventListener/DoctrineEventSubscriber.php (1)
DoctrineEventSubscriber(10-30)src/Services/SettingsUpdater.php (1)
SettingsUpdater(16-77)
tests/Unit/SerializationTest.php (4)
src/SearchableObject.php (1)
SearchableObject(13-80)tests/Entity/Comment.php (1)
getId(69-72)tests/Entity/Post.php (1)
getId(91-94)tests/BaseKernelTestCase.php (1)
get(42-45)
src/Command/MeilisearchUpdateSettingsCommand.php (3)
src/Services/MeilisearchManager.php (1)
isSearchable(71-76)src/SearchManagerInterface.php (1)
isSearchable(23-23)src/SearchService.php (1)
isSearchable(20-20)
src/SearchManagerInterface.php (1)
src/Exception/NotSearchableException.php (1)
NotSearchableException(7-13)
tests/Integration/AggregatorTest.php (2)
src/Model/Aggregator.php (2)
getEntityClassFromObjectID(63-72)normalize(74-77)src/Exception/InvalidEntityForAggregator.php (1)
InvalidEntityForAggregator(7-9)
src/Engine.php (2)
src/SearchManagerInterface.php (4)
index(48-48)remove(55-55)count(110-110)search(92-92)src/SearchableObject.php (5)
SearchableObject(13-80)getSearchableArray(65-79)getIndexUid(42-45)getPrimaryKey(50-53)getIdentifier(55-58)
🪛 GitHub Actions: Tests
src/Services/MeilisearchManager.php
[error] 27-27: ExcessiveClassComplexity The class MeilisearchManager has an overall complexity of 70 which is very high. The configured complexity threshold is 50.
[error] 27-27: CouplingBetweenObjects The class MeilisearchManager has a coupling between objects value of 13. Consider to reduce the number of dependencies under 13.
src/Command/MeilisearchImportCommand.php
[error] 23-23: CouplingBetweenObjects The class MeilisearchImportCommand has a coupling between objects value of 13. Consider to reduce the number of dependencies under 13.
[error] 71-71: CyclomaticComplexity The method execute() has a Cyclomatic Complexity of 13. The configured cyclomatic complexity threshold is 10.
[error] 71-71: NPathComplexity The method execute() has an NPath complexity of 592. The configured NPath complexity threshold is 200.
src/DependencyInjection/MeilisearchExtension.php
[error] 44-44: IfStatementAssignment Avoid assigning values to variables in if clauses and the like (line '44', column '20').
[error] 36-36: StaticAccess Avoid using static access to class '\Meilisearch\Bundle\MeilisearchBundle' in method 'load'.
[error] 51-51: ElseExpression The method load uses an else expression. Else clauses are basically not necessary and you can simplify the code by not using them.
[error] 59-59: StaticAccess Avoid using static access to class '\Meilisearch\Bundle\MeilisearchBundle' in method 'load'.
src/Services/SettingsUpdater.php
[error] 34-34: CyclomaticComplexity The method update() has a Cyclomatic Complexity of 13. The configured cyclomatic complexity threshold is 10.
src/Services/MeilisearchService.php
[error] 27-27: TooManyPublicMethods The class MeilisearchService has 11 public methods. Consider refactoring MeilisearchService to keep number of public methods under 10.
[error] 27-27: ExcessiveClassComplexity The class MeilisearchService has an overall complexity of 67 which is very high. The configured complexity threshold is 50.
[error] 27-27: CouplingBetweenObjects The class MeilisearchService has a coupling between objects value of 16. Consider to reduce the number of dependencies under 13.
[error] 48-48: LongVariable Avoid excessively long variable names like $classToSerializerGroup. Keep variable name length under 20.
[error] 57-57: StaticAccess Avoid using static access to class '\Symfony\Component\PropertyAccess\PropertyAccess' in method '__construct'.
[error] 415-415: LongVariable Avoid excessively long variable names like $searchableEntitiesChunk. Keep variable name length under 20.
[error] 463-463: StaticAccess Avoid using static access to class '\Doctrine\ORM\Proxy\DefaultProxyClassNameResolver' in method 'resolveClass'.
[error] 467-467: StaticAccess Avoid using static access to class '\Doctrine\Common\Util\ClassUtils' in method 'resolveClass'.
🔇 Additional comments (47)
tests/Integration/AggregatorTest.php (2)
27-28: LGTM!Formatting improvement adds visual separation before the method call.
31-38: Since I'm unable to access the repository directly to verify the Post entity structure and ContentAggregator constructor signature, I cannot complete the verification. Let me provide a rewritten review comment that reflects this limitation:
Verify that the primary key field names 'objectId' and 'url' exist on the Post entity and match the ContentAggregator constructor expectations.
The test passes
['objectId', 'url']as an array of primary keys to verify the constructor throwsInvalidEntityForAggregatorwhen given multiple keys. Confirm these field names correspond to actual properties on the Post entity and that the constructor's third parameter ('objectId') correctly identifies the single primary key being tested.tests/Integration/EventListener/DoctrineEventSubscriberTest.php (1)
23-23: LGTM! Consistent migration to the new SearchManagerInterface API.All search calls are correctly updated to use the new
searchManager->search(className, query)signature, removing theentityManagerparameter. The test coverage for persist, update, and remove operations with both regular and ObjectId-based entities is appropriate.Also applies to: 38-38, 59-59, 81-81, 97-97, 107-107, 121-121, 131-131
tests/Unit/ConfigurationTest.php (2)
139-153: LGTM! Configuration tests properly updated for new ORM data provider fields.The expected configuration correctly includes the new
type,data_provider, andprimary_keyfields with appropriate defaults ('orm',null,'objectID').
358-402: Good addition of custom primary key test case.This test case validates the fix for issue #66, ensuring user-defined primary keys (
postId,tagId) are properly preserved in the processed configuration rather than defaulting toobjectID..github/workflows/pre-release-tests.yml (2)
42-65: LGTM! CI matrix correctly updated for new PHP/Symfony requirements.The matrix properly reflects the version bumps to PHP 8.1+ and Symfony 6.4+. The excludes for PHP 8.1 with Symfony 7.x are correct since Symfony 7 requires PHP 8.2+. The addition of
lowest/highestdependency testing provides good edge-case coverage.
84-85: Good improvement to dependency installation.Using
dependency-versions: "${{ matrix.dependencies }}"with the matrix dimension enables proper testing of both lowest and highest dependency versions, improving compatibility assurance.composer.json (3)
21-29: LGTM! Version constraints properly updated for PHP 8.1+ and Symfony 6.4+.The unified Symfony version constraints (
^6.4 || ^7.0) across all packages ensure consistent compatibility. Removal ofsymfony/polyfill-php80is correct since PHP 8.1 is now the minimum.
38-51: Dev dependency updates look appropriate.Minor version bumps for linting and static analysis tools are safe. The Symfony dev package constraints correctly mirror the production requirements.
32-32: I'll help you verify the review comment about thedoctrine/doctrine-bundlebreaking change. Let me search for upgrade documentation and verify the actual changes.
<function_calls>
#!/bin/bashCheck for upgrade documentation mentioning the doctrine-bundle change
rg -i -n "doctrine.*bundle|require.*doctrine|breaking" README.md CHANGELOG.md UPGRADE.md UPGRADING.md 2>/dev/null || echo "No matches found in common upgrade docs"
List all markdown files at root to see what documentation exists
echo "=== Documentation files at repository root ==="
fd -e md -x basename {} ; 2>/dev/null | sort
</function_calls>Now let me verify the actual change in composer.json:
<function_calls>
#!/bin/bashCheck the current state of composer.json
echo "=== Current composer.json (showing relevant sections) ==="
cat composer.json | head -50echo ""
echo "=== Searching for doctrine-bundle in composer.json ==="
rg -n "doctrine.*bundle" composer.json -A 1 -B 1
</function_calls>src/SearchableEntity.php (1)
15-17: Deprecation marker and normalization constant update are consistent with the newSearchableObjectflowThe deprecation docblock cleanly points callers to
Meilisearch\Bundle\SearchableObject, and switching toSearchableObject::NORMALIZATION_FORMATin both normalization paths keeps this legacy wrapper aligned with the new normalization semantics without changing behavior.Also applies to: 82-88
.github/workflows/tests.yml (1)
30-31: CI matrix now matches supported PHP/Symfony versionsRestricting the matrix to PHP 8.1–8.4 and Symfony 6.4–7.3, with targeted include/exclude rules, correctly reflects the new platform constraints and avoids unused PHP 7.4 entries.
src/Exception/NotSearchableException.php (1)
1-13: Exception is well-typed and consistent with existing exception patternsThe
NotSearchableExceptionconstructor signature and message format match the other bundle exceptions and PHP 8.1 typing expectations; nothing to change here.tests/Kernel.php (1)
34-42: Doctrine test-kernel config switching matches the new support windowUsing
BlacklistSchemaAssetFilterandLegacyReflectionFieldsas feature flags to selectdoctrine.yaml/doctrine_v2.yaml/doctrine_old_proxy.yamlis a reasonable simplification now that PHP 7 support is gone, and it keeps tests adaptable across Doctrine bundle/ORM versions.src/Exception/DataProviderNotFoundException.php (1)
1-13: Clear, typed exception for missing data providersThe
DataProviderNotFoundExceptioncleanly encodes both index and class in the message and uses a properly typed$codeparameter, matching the style of other exceptions in the bundle.src/Command/MeilisearchUpdateSettingsCommand.php (1)
10-11: Command is correctly wired to the newSearchManagerInterfacePassing
SearchManagerInterfaceinto the parentIndexCommandand switching the guard to$this->searchManager->isSearchable($entityClassName)aligns this command with the new manager abstraction, while leaving the existing settings-update logic intact.Also applies to: 22-28, 56-57
tests/Integration/Fixtures/ActorDataProvider.php (1)
1-70: LGTM - Clean test fixture demonstrating custom data provider implementation.The implementation correctly showcases the flexibility of the
DataProviderInterface, with in-memory actor data and appropriate handling for the test context.src/DependencyInjection/MeilisearchExtension.php (2)
34-42: LGTM - Data provider registration integrated cleanly.The early parameter initialization and
registerDataProviderscall before index iteration ensures data providers are available when configuring the manager service.
93-129: Well-structured data provider registration logic.The method correctly handles three scenarios:
- Explicit
data_providerconfiguration → uses service reference- ORM type with Aggregator → creates per-entity
OrmEntityProviderdefinitions- ORM type without Aggregator → creates single
OrmEntityProviderdefinitionThe use of
xxh32hash ensures unique service IDs without collisions.src/Model/Aggregator.php (2)
26-38: Good implementation of configurable primary key.The constructor now accepts
primaryKeyas a promoted property, and thenormalize()method correctly uses it instead of the hardcoded'objectID'key. This addresses PR objective #66 (removing objectID assumptions).
74-77: I'm experiencing a technical issue accessing the repository for verification. Let me attempt to provide guidance based on the information available:Unable to complete automated verification due to repository access issues.
The review comment requests verification that all
Aggregatorinstantiation sites have been updated to pass theprimaryKeyargument as a breaking change. To properly verify this, I would need to:
- Search for all
new Aggregator(instantiations across the codebase- Examine
MeilisearchManager::index()to confirm it passes 3 arguments- Examine
MeilisearchService::getAggregatorsFromEntities()to confirm it passes 3 arguments- Check for any remaining 2-argument instantiation patterns
Since I cannot access the repository at this time, I cannot definitively verify whether:
- All instantiation sites have been properly updated
- There are any remaining outdated 2-argument calls
- The breaking change has been fully propagated
Please retry verification once repository access is restored, or manually confirm the instantiation sites mentioned in the comment have been updated with the new 3-argument
primaryKeyparameter.tests/Unit/SerializationTest.php (1)
23-30: LGTM - Test correctly updated for newSearchableObjectAPI.The test properly uses the new constructor signature with explicit
primaryKeyparameter and the renamedgroupsoption key.src/Services/SettingsUpdater.php (1)
22-28: LGTM - Clean dependency injection update.The constructor correctly uses
MeilisearchManagerfor configuration retrieval while maintaining promoted properties for the client and event dispatcher.src/DataProvider/OrmEntityProvider.php (2)
1-58: LGTM - Solid ORM data provider implementation.The implementation correctly uses Doctrine metadata to handle identifier fields dynamically, addressing the previous concern about hardcoded
'id'fields.
54-57: Review requires manual verification of usage context.The concern raised is theoretically valid based on Doctrine/Symfony architecture.
getManagerForClass()returns the same EntityManager service instance (shared by default), and callingclear()on it detaches all managed entities from that instance. This could indeed affect concurrent operations or mid-batch processing if other code is using entities from the same manager.However, the actual risk depends on when and how
cleanup()is called in practice:
- Safe: If called only at absolute end-of-operation boundaries (after all batch items processed, all indices handled)
- Risky: If called during active processing where other code still references entities from that manager
Unable to verify the actual call sites and timing due to repository access issues. Manual code inspection is needed to confirm whether
cleanup()is consistently called at safe boundaries (as expected in batch/import context) or if there's genuine concurrent access risk.src/Services/MeilisearchService.php (2)
24-27: LGTM - Deprecation notice correctly points toMeilisearchManager.The namespace typo from the previous review has been fixed.
50-64: Clean optional manager injection for backward compatibility.The nullable
$managerparameter allows existing code to continue working while enabling delegation to the new manager when injected.config/services.php (2)
70-78: LGTM - Manager service correctly wired.The
meilisearch.managerservice definition matches theMeilisearchManagerconstructor signature with all required abstract arguments.
28-46: LGTM - Legacy service updated with manager delegation.The
meilisearch.servicenow receives the manager as its 5th argument, enabling the deprecation bridge pattern. TheDoctrineEventSubscribercorrectly uses the manager directly.src/Command/MeilisearchImportCommand.php (4)
27-34: Constructor refactored to use SearchManagerInterface.The dependency injection now correctly uses the new
SearchManagerInterfaceabstraction, aligning with the provider-based architecture. The parent constructor call properly passes the manager.
119-143: Data provider correctly obtained outside the loop.The
getDataProvider()call is now properly placed before thedo-whileloop, addressing the previous review feedback. Thecleanup()call per iteration is appropriate for releasing resources (e.g., Doctrine'sclear()) after each batch.One observation: the loop condition
count($objects) >= $batchSizemeans if the last batch returns exactly$batchSizeobjects, an extra iteration with 0 results will occur. This is typically acceptable but worth noting.
160-181: LGTM - Defensive null-coalescing added.The
$formattedResponse[$indexName] ??= 0on line 166 correctly handles the case where an index hasn't been seen yet, preventing undefined index notices.
206-232: Index swap and deletion logic updated to use manager.The
swapIndices()method correctly uses$this->searchManager->deleteByIndexName()(line 229) instead of direct client calls, maintaining consistency with the new manager-based architecture.src/SearchManagerInterface.php (3)
1-18: Well-defined interface contract with proper PHPStan types.The interface is well-structured with clear method contracts and appropriate PHPStan type imports from
Engine. TheRESULT_KEY_HITSconstant provides a consistent way to access search results.
34-41: Template annotation for type-safe data provider.Good use of generics with
@template T of objectto ensure type safety between the class parameter and the returnedDataProviderInterface<T>.
82-92: Search method returns domain objects, not raw hits.The
search()method correctly returnslist<T>(domain objects), distinguishing it fromrawSearch()which returns the rawSearchResponse. This separation of concerns is a good design choice.src/Engine.php (4)
46-48: Clean constructor with property promotion.Good use of PHP 8.1+ constructor property promotion for concise initialization.
61-90: Index method correctly handles per-index primary keys.The restructured batch format with
['primaryKey' => ..., 'documents' => []](lines 78-79) ensures each index uses its own primary key when callingaddDocuments()(line 86). This addresses the previous review feedback about primary key handling.
174-181: Simplified ID normalization with Stringable support.The
normalizeId()method now properly handles\Stringableobjects by casting to string (line 177), which is cleaner than the previousmethod_existscheck.
115-126: Consider batching document deletions for performance.The
remove()method currently callsdeleteDocument()individually for each identifier (lines 119-121). Meilisearch supports batch deletion viadeleteDocuments(array $ids)which reduces API calls and improves performance.However, this refactoring changes the return type from a list of individual document deletion tasks to a single batch task response. Before applying this optimization, verify:
- How calling code handles the return value and whether it can adapt to receiving a single task instead of a list
- Whether the current per-document approach is intentional for tracking purposes
If callers can handle the return type change, the suggested refactoring is the recommended approach per Meilisearch documentation.
src/Services/MeilisearchManager.php (7)
7-9: Doctrine reflection classes properly imported.The
LegacyReflectionFieldsandDefaultProxyClassNameResolverclasses are now properly imported, addressing the previous review feedback about unimported class constants.
71-76: isSearchable correctly resolves base class name.The method properly resolves proxy/lazy objects to their base class before checking searchability.
78-90: Null check added for searchableAs.The null guard on line 85-87 correctly throws
NotSearchableExceptionwhen the index is not found, addressing previous review feedback.
114-163: Index method properly handles aggregators and batching.The
index()method correctly:
- Resolves matching indices including aggregator support
- Wraps entities in aggregator objects when needed (lines 130-136)
- Respects
shouldBeIndexedgating (lines 138-140)- Batches items per index for efficient processing
165-198: Remove method batches deletions correctly.The
remove()method properly chunks objects bybatchSize(line 192) before calling the engine. Thearray_merge(...$responses)is safe for empty arrays since PHP 7.4+, addressing the previous review feedback.
219-258: Search method loads domain objects via data provider.The implementation correctly:
- Validates searchability
- Extracts identifiers from hits
- Bulk-loads entities via
loadByIdentifiers()(reducing DB queries per PR objective #134)- Preserves search result ordering
447-467: Robust class resolution for Doctrine proxies and PHP 8.4 lazy objects.The
resolveClass()method correctly handles:
- PHP 8.4 native lazy objects (line 453-455)
- Doctrine ORM v3+ proxies (lines 458-460)
- Legacy Doctrine ORM (line 463)
The static memoization of the resolver callable is a good optimization.
norkunas
force-pushed
the
dataprovider
branch
2 times, most recently
from
44eb1a0 to
a224b01
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
tests/Integration/AggregatorTest.php (1)
53-61: Assertion expects literal string instead of actual entity identifier.The assertion on Line 61 expects
$serializedData['objectId']to equal the string literal'objectId', but it should equal the actual Post entity's identifier (the$postIdcaptured on Line 48). The serialized data should contain the real object identifier from the proxy, not the key name itself.Apply this diff to fix the assertion:
$proxy = $this->entityManager->getReference(Post::class, $postId); $this->assertInstanceOf(Proxy::class, $proxy); $contentAggregator = new ContentAggregator($proxy, ['objectId'], 'objectId'); /** @var Serializer $serializer */ $serializer = $this->get('serializer'); $serializedData = $contentAggregator->normalize($serializer); $this->assertNotEmpty($serializedData); - $this->assertEquals('objectId', $serializedData['objectId']); + $this->assertEquals($postId, $serializedData['objectId']);tests/Integration/EngineTest.php (1)
35-41: Avoid reintroducing Algolia-specific'objectID'in SearchableObject testsBoth
SearchableObjectinstances still hardcode'objectID'as the primary key, which goes against the PR’s goal of moving to Meilisearch-styleprimary_key(typicallyid) instead of Algolia terminology.Consider switching these to the configured primary key (very likely
'id') so tests reflect the new semantics:- $searchableImage = new SearchableObject( - $this->getPrefix().'image', - 'objectID', + $searchableImage = new SearchableObject( + $this->getPrefix().'image', + 'id', $image, $image->getId(), $this->get('serializer'), ['groups' => ['force_empty']], ); @@ - $postSearchable1 = new SearchableObject($this->getPrefix().'posts', 'objectID', $post1, $post1->getId(), $serializer); - $postSearchable2 = new SearchableObject($this->getPrefix().'posts', 'objectID', $post2, $post2->getId(), $serializer); + $postSearchable1 = new SearchableObject($this->getPrefix().'posts', 'id', $post1, $post1->getId(), $serializer); + $postSearchable2 = new SearchableObject($this->getPrefix().'posts', 'id', $post2, $post2->getId(), $serializer);Also applies to: 67-69
src/Services/MeilisearchManager.php (1)
241-246: CRITICAL: Composite key format mismatch between search and indexing.The composite identifier format in
search()differs fromgetSingleIdentifier():
- Here (search, line 244):
implode('-', $identifiers)→"value1-value2"- getSingleIdentifier (lines 410-415):
"$key-$value__"→"key1-value1__key2-value2__"Entities with composite primary keys indexed via
getSingleIdentifier()will never be found during search because the lookup key format doesn't match the indexed identifier format.Apply this diff to align the formats:
$objectsById = []; foreach ($loadedObjects as $object) { $identifiers = $dataProvider->getIdentifierValues($object); - $key = 1 === \count($identifiers) ? (string) reset($identifiers) : implode('-', $identifiers); + if (1 === \count($identifiers)) { + $key = (string) reset($identifiers); + } else { + $key = ''; + foreach ($identifiers as $idKey => $idValue) { + $key .= $idKey.'-'.$idValue.'__'; + } + } $objectsById[$key] = $object; }
🧹 Nitpick comments (7)
phpstan-baseline.php (1)
16-21: Defensiveisset()check on typed array.This baseline entry suppresses the warning about
isset()on a key that's typed as always existing. While the type system guaranteeshitsexists, the defensive check is harmless. Consider removing theisset()check in the source if the type is reliable, which would eliminate this baseline entry.tests/BaseKernelTestCase.php (1)
19-19: Consider usingSearchManagerInterfacefor consistency.The property is typed with the concrete
MeilisearchManagerclass rather thanSearchManagerInterface. While acceptable for test infrastructure, using the interface would be more consistent with the production code patterns in this PR.- protected MeilisearchManager $searchManager; + protected SearchManagerInterface $searchManager;This would also require updating the import on line 10.
src/DataProvider/DataProviderInterface.php (1)
20-25: Consider documenting the expected$identifiersstructure more precisely.The
@param array<mixed> $identifiersannotation is intentionally flexible, but the two implementations in this PR interpret it differently:
OrmEntityProvider: expects a flat array of identifier values (e.g.,[1, 2, 3])ActorDataProvider: expects an associative array (e.g.,['id' => 1])While this flexibility is by design (per maintainer's clarification), adding a brief note in the PHPDoc about the expected structure could help future implementers understand the contract better.
/** - * @param array<mixed> $identifiers + * @param array<mixed> $identifiers An array of identifier values. The structure + * depends on the caller context and implementation. * * @return iterable<T> */src/Services/SettingsUpdater.php (1)
22-28: PreferSearchManagerInterfaceover concreteMeilisearchManager
SettingsUpdateronly needs access to configuration, which is exposed onSearchManagerInterface. Typing the constructor against the concreteMeilisearchManagermakes this service harder to reuse or test.You can loosen the coupling with a minimal change:
-use Meilisearch\Bundle\Services\SettingsUpdater; +use Meilisearch\Bundle\SearchManagerInterface; +use Meilisearch\Bundle\Services\SettingsUpdater; @@ - public function __construct( - MeilisearchManager $searchManager, + public function __construct( + SearchManagerInterface $searchManager, private readonly Client $searchClient, private readonly EventDispatcherInterface $eventDispatcher, ) { $this->configuration = $searchManager->getConfiguration(); }src/DataProvider/OrmEntityProvider.php (1)
54-57: Targetclear()to the indexed class incleanup()The provider looks solid overall (paging, identifier handling, composite-id guard). One small refinement:
cleanup()currently callsclear()without arguments, which wipes all managed entities for that manager. To avoid surprising other consumers sharing the same EM, you could restrict it to the configured class:- public function cleanup(): void - { - $this->managerRegistry->getManagerForClass($this->className)->clear(); - } + public function cleanup(): void + { + $this->managerRegistry->getManagerForClass($this->className)->clear($this->className); + }tests/Unit/SerializationTest.php (1)
23-30: Align SerializationTest with Meilisearchprimary_keynamingThis test now constructs
SearchableObjectwith'objectID'as the primary key, even though the assertion only inspects the normalized payload. To keep tests consistent with the newprimary_keysemantics, consider switching toid:- $searchablePost = new SearchableObject( - 'posts', - 'objectID', + $searchablePost = new SearchableObject( + 'posts', + 'id', $post, $post->getId(), self::getContainer()->get('serializer'), ['groups' => [SearchableObject::NORMALIZATION_GROUP]] );src/Services/MeilisearchManager.php (1)
329-329: Unnecessary string cast.The cast
(string) $classNameis redundant since$classNameis already aclass-stringfrom the loop iteration andis_subclass_of()check.Apply this diff:
- $this->aggregators[] = (string) $className; + $this->aggregators[] = $className;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (55)
.github/workflows/pre-release-tests.yml(2 hunks).github/workflows/tests.yml(1 hunks)composer.json(1 hunks)config/services.php(4 hunks)phpstan-baseline.php(1 hunks)phpstan.dist.neon(1 hunks)src/Command/IndexCommand.php(2 hunks)src/Command/MeilisearchClearCommand.php(1 hunks)src/Command/MeilisearchCreateCommand.php(3 hunks)src/Command/MeilisearchDeleteCommand.php(1 hunks)src/Command/MeilisearchImportCommand.php(7 hunks)src/Command/MeilisearchUpdateSettingsCommand.php(3 hunks)src/DataProvider/DataProviderInterface.php(1 hunks)src/DataProvider/OrmEntityProvider.php(1 hunks)src/DependencyInjection/Configuration.php(2 hunks)src/DependencyInjection/MeilisearchExtension.php(4 hunks)src/Engine.php(5 hunks)src/Event/SettingsUpdatedEvent.php(1 hunks)src/EventListener/ConsoleOutputSubscriber.php(1 hunks)src/EventListener/DoctrineEventSubscriber.php(1 hunks)src/Exception/DataProviderNotFoundException.php(1 hunks)src/Exception/InvalidIndiceException.php(1 hunks)src/Exception/LogicException.php(1 hunks)src/Exception/NotSearchableException.php(1 hunks)src/Model/Aggregator.php(3 hunks)src/SearchManagerInterface.php(1 hunks)src/SearchService.php(1 hunks)src/Searchable.php(1 hunks)src/SearchableEntity.php(2 hunks)src/SearchableObject.php(1 hunks)src/Services/MeilisearchManager.php(1 hunks)src/Services/MeilisearchService.php(9 hunks)src/Services/SettingsUpdater.php(1 hunks)src/Services/UnixTimestampNormalizer.php(1 hunks)tests/BaseKernelTestCase.php(5 hunks)tests/Entity/Actor.php(1 hunks)tests/Entity/Link.php(2 hunks)tests/Entity/SelfNormalizable.php(2 hunks)tests/Entity/Tag.php(2 hunks)tests/Integration/AggregatorTest.php(2 hunks)tests/Integration/Command/MeilisearchClearCommandTest.php(1 hunks)tests/Integration/Command/MeilisearchCreateCommandTest.php(2 hunks)tests/Integration/Command/MeilisearchDeleteCommandTest.php(1 hunks)tests/Integration/Command/MeilisearchImportCommandTest.php(1 hunks)tests/Integration/EngineTest.php(3 hunks)tests/Integration/EventListener/DoctrineEventSubscriberTest.php(8 hunks)tests/Integration/Fixtures/ActorDataProvider.php(1 hunks)tests/Integration/SearchTest.php(4 hunks)tests/Kernel.php(1 hunks)tests/Unit/ConfigurationTest.php(8 hunks)tests/Unit/SerializationTest.php(2 hunks)tests/baseline-ignore(1 hunks)tests/config/doctrine_php7.yaml(0 hunks)tests/config/framework.yaml(1 hunks)tests/config/meilisearch.yaml(1 hunks)
💤 Files with no reviewable changes (1)
- tests/config/doctrine_php7.yaml
🚧 Files skipped from review as they are similar to previous changes (17)
- tests/Entity/Actor.php
- tests/Integration/SearchTest.php
- src/EventListener/ConsoleOutputSubscriber.php
- src/SearchService.php
- src/Exception/InvalidIndiceException.php
- tests/Integration/Command/MeilisearchCreateCommandTest.php
- tests/Entity/SelfNormalizable.php
- tests/config/meilisearch.yaml
- src/SearchManagerInterface.php
- .github/workflows/pre-release-tests.yml
- tests/Integration/EventListener/DoctrineEventSubscriberTest.php
- tests/baseline-ignore
- phpstan.dist.neon
- src/Model/Aggregator.php
- src/Services/UnixTimestampNormalizer.php
- tests/Integration/Command/MeilisearchDeleteCommandTest.php
- .github/workflows/tests.yml
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: norkunas
Repo: meilisearch/meilisearch-symfony PR: 391
File: src/Command/MeilisearchImportCommand.php:117-120
Timestamp: 2025-08-05T04:46:01.119Z
Learning: In meilisearch-symfony project, the maintainer norkunas prefers to keep PRs focused and defer error handling improvements for settings updates to separate PRs, particularly when it involves decisions about partial import behavior when some indexes fail.
📚 Learning: 2025-08-05T04:46:01.119Z
Learnt from: norkunas
Repo: meilisearch/meilisearch-symfony PR: 391
File: src/Command/MeilisearchImportCommand.php:117-120
Timestamp: 2025-08-05T04:46:01.119Z
Learning: In meilisearch-symfony project, the maintainer norkunas prefers to keep PRs focused and defer error handling improvements for settings updates to separate PRs, particularly when it involves decisions about partial import behavior when some indexes fail.
Applied to files:
tests/Integration/Fixtures/ActorDataProvider.phpsrc/Command/MeilisearchImportCommand.phptests/Integration/EngineTest.phpsrc/DataProvider/OrmEntityProvider.phpsrc/Services/MeilisearchManager.phpsrc/Engine.php
🧬 Code graph analysis (25)
src/Command/MeilisearchUpdateSettingsCommand.php (3)
src/Command/IndexCommand.php (1)
__construct(19-24)src/Services/MeilisearchService.php (2)
__construct(52-64)isSearchable(66-77)src/SearchManagerInterface.php (1)
isSearchable(23-23)
tests/Entity/Tag.php (1)
src/SearchableObject.php (1)
SearchableObject(13-80)
src/Exception/DataProviderNotFoundException.php (2)
src/Exception/InvalidIndiceException.php (1)
__construct(9-12)src/Exception/NotSearchableException.php (1)
__construct(9-12)
src/Command/MeilisearchClearCommand.php (4)
src/Engine.php (1)
clear(137-140)src/SearchManagerInterface.php (1)
clear(64-64)src/Services/MeilisearchManager.php (1)
clear(200-205)src/Services/MeilisearchService.php (1)
clear(172-183)
src/Services/SettingsUpdater.php (1)
src/SearchManagerInterface.php (1)
getConfiguration(32-32)
src/Command/MeilisearchDeleteCommand.php (4)
src/SearchManagerInterface.php (1)
deleteByIndexName(71-71)src/SearchService.php (1)
deleteByIndexName(50-50)src/Services/MeilisearchManager.php (1)
deleteByIndexName(207-210)src/Services/MeilisearchService.php (1)
deleteByIndexName(185-194)
src/DependencyInjection/MeilisearchExtension.php (6)
src/DataProvider/OrmEntityProvider.php (1)
OrmEntityProvider(10-58)src/MeilisearchBundle.php (2)
MeilisearchBundle(9-22)qualifiedVersion(13-16)src/Model/Aggregator.php (2)
Aggregator(15-84)getEntities(51-54)src/Entity/Aggregator.php (1)
Aggregator(9-11)src/Document/Aggregator.php (1)
Aggregator(9-11)tests/Entity/ContentAggregator.php (1)
getEntities(21-27)
tests/Kernel.php (1)
src/DependencyInjection/MeilisearchExtension.php (1)
load(22-73)
tests/Entity/Link.php (1)
src/SearchableObject.php (1)
SearchableObject(13-80)
src/DependencyInjection/Configuration.php (1)
src/SearchableObject.php (1)
SearchableObject(13-80)
src/EventListener/DoctrineEventSubscriber.php (2)
src/SearchManagerInterface.php (2)
index(48-48)remove(55-55)src/Services/MeilisearchManager.php (2)
index(114-163)remove(165-198)
src/Command/MeilisearchImportCommand.php (4)
src/SearchManagerInterface.php (6)
getConfiguration(32-32)isSearchable(23-23)getDataProvider(41-41)index(48-48)count(110-110)deleteByIndexName(71-71)src/Services/MeilisearchManager.php (6)
getConfiguration(92-95)isSearchable(71-76)getDataProvider(97-112)index(114-163)count(270-275)deleteByIndexName(207-210)src/DataProvider/DataProviderInterface.php (2)
provide(18-18)cleanup(34-34)src/DataProvider/OrmEntityProvider.php (2)
provide(21-30)cleanup(54-57)
config/services.php (3)
src/Services/MeilisearchManager.php (1)
MeilisearchManager(27-468)src/EventListener/DoctrineEventSubscriber.php (1)
DoctrineEventSubscriber(10-30)src/Services/SettingsUpdater.php (1)
SettingsUpdater(16-77)
src/Exception/NotSearchableException.php (2)
src/Exception/DataProviderNotFoundException.php (1)
__construct(9-12)src/Exception/InvalidIndiceException.php (1)
__construct(9-12)
src/DataProvider/DataProviderInterface.php (2)
src/DataProvider/OrmEntityProvider.php (4)
provide(21-30)loadByIdentifiers(32-45)getIdentifierValues(47-52)cleanup(54-57)tests/Integration/Fixtures/ActorDataProvider.php (4)
provide(15-44)loadByIdentifiers(46-57)getIdentifierValues(59-64)cleanup(66-69)
src/Command/MeilisearchCreateCommand.php (4)
src/Command/IndexCommand.php (1)
__construct(19-24)src/SearchManagerInterface.php (1)
isSearchable(23-23)src/SearchService.php (1)
isSearchable(20-20)src/Services/MeilisearchManager.php (1)
isSearchable(71-76)
tests/Integration/EngineTest.php (1)
src/SearchableObject.php (1)
SearchableObject(13-80)
src/DataProvider/OrmEntityProvider.php (2)
src/Exception/LogicException.php (1)
LogicException(7-9)src/DataProvider/DataProviderInterface.php (4)
provide(18-18)loadByIdentifiers(25-25)getIdentifierValues(32-32)cleanup(34-34)
tests/Unit/SerializationTest.php (1)
src/SearchableObject.php (1)
SearchableObject(13-80)
src/SearchableObject.php (4)
src/SearchableEntity.php (3)
__construct(42-56)getIndexUid(58-61)getSearchableArray(66-91)tests/Entity/Link.php (1)
normalize(74-85)tests/Entity/SelfNormalizable.php (1)
normalize(64-75)tests/Entity/Tag.php (1)
normalize(88-100)
src/Command/IndexCommand.php (2)
src/SearchManagerInterface.php (1)
getConfiguration(32-32)src/Services/MeilisearchManager.php (1)
getConfiguration(92-95)
tests/Integration/AggregatorTest.php (2)
src/Model/Aggregator.php (2)
getEntityClassFromObjectID(69-78)normalize(80-83)src/Exception/InvalidEntityForAggregator.php (1)
InvalidEntityForAggregator(7-9)
src/Services/MeilisearchManager.php (9)
src/Engine.php (8)
Engine(44-182)__construct(46-48)index(61-90)remove(100-126)clear(137-140)delete(147-150)search(159-162)count(169-172)src/Exception/DataProviderNotFoundException.php (2)
DataProviderNotFoundException(7-13)__construct(9-12)src/Exception/NotSearchableException.php (2)
NotSearchableException(7-13)__construct(9-12)src/Model/Aggregator.php (2)
Aggregator(15-84)__construct(32-44)src/SearchableObject.php (2)
SearchableObject(13-80)__construct(28-37)src/EventListener/ConsoleOutputSubscriber.php (1)
__construct(13-15)src/EventListener/DoctrineEventSubscriber.php (1)
__construct(12-14)src/SearchManagerInterface.php (12)
isSearchable(23-23)searchableAs(30-30)getConfiguration(32-32)index(48-48)getDataProvider(41-41)remove(55-55)clear(64-64)deleteByIndexName(71-71)delete(80-80)search(92-92)count(110-110)rawSearch(102-102)src/DataProvider/DataProviderInterface.php (2)
getIdentifierValues(32-32)loadByIdentifiers(25-25)
src/Services/MeilisearchService.php (2)
src/EventListener/DoctrineEventSubscriber.php (1)
__construct(12-14)src/Services/MeilisearchManager.php (13)
isSearchable(71-76)getBaseClassName(300-307)getConfiguration(92-95)searchableAs(78-90)index(114-163)remove(165-198)clear(200-205)assertIsSearchable(389-394)deleteByIndexName(207-210)delete(212-217)search(219-258)rawSearch(260-268)count(270-275)
tests/BaseKernelTestCase.php (3)
src/Services/MeilisearchManager.php (3)
MeilisearchManager(27-468)getConfiguration(92-95)deleteByIndexName(207-210)src/Collection.php (2)
get(30-37)Collection(13-380)src/SearchManagerInterface.php (2)
getConfiguration(32-32)deleteByIndexName(71-71)
🔇 Additional comments (37)
tests/config/framework.yaml (1)
5-7: Sensible additions to test error handling configuration.The two new framework configuration keys enhance error logging and handling during test execution:
handle_all_throwables: trueensures comprehensive error handling, which is appropriate for catching issues early in testsphp_errors: { log: true }enables PHP error logging, improving observabilityThese align well with the PR's broader refactoring toward improved exception handling and new exception classes.
Please verify that these settings don't introduce unexpected logging verbosity or test failures in your CI pipeline, particularly if tests check for specific error counts or output.
src/Event/SettingsUpdatedEvent.php (1)
16-21: LGTM! Clean modernization to constructor property promotion.The refactor to use constructor property promotion with
readonlymodifiers is a good improvement that reduces boilerplate while maintaining the same public API. The immutability provided byreadonlyis appropriate for an event object.tests/Integration/Command/MeilisearchClearCommandTest.php (1)
39-39: LGTM!The test expectation correctly includes the new actor index in the cleared indices output, aligning with the new
Actorentity andActorDataProviderintroduced in this PR.src/Exception/LogicException.php (1)
1-9: LGTM!Clean exception wrapper that provides a bundle-specific
LogicExceptiontype, following the same pattern as other exceptions inMeilisearch\Bundle\Exceptionnamespace.src/SearchableEntity.php (2)
15-17: Good deprecation notice.The deprecation annotation clearly points users to the replacement class
SearchableObject.
83-83: Based on my research, I've identified an issue with the review comment. The web search consistently shows that the correct class is\Meilisearch\Bundle\Searchable, notSearchableObject. Let me provide the rewritten review comment:
Verify the class name reference.
The code at lines 83 and 87 references
SearchableObject::NORMALIZATION_FORMAT, but the documented constant in the Meilisearch Symfony bundle is\Meilisearch\Bundle\Searchable::NORMALIZATION_FORMAT(notSearchableObject). Confirm whether the code should useSearchableinstead, or ifSearchableObjectis a custom class in this codebase.src/Exception/NotSearchableException.php (1)
7-13: LGTM!Well-structured exception class following the same pattern as sibling exceptions (
InvalidIndiceException,DataProviderNotFoundException). Theinttype hint on$codeis correctly applied.phpstan-baseline.php (1)
4-15: Verify runtime guards for optional Doctrine dependency.These baseline entries suppress
ClassUtils::getClass()errors since Doctrine is now a dev dependency. Ensure that usages ofClassUtilsinMeilisearchManager.phpandMeilisearchService.phpare guarded withclass_exists()checks at runtime to prevent errors when Doctrine is not installed.tests/Integration/AggregatorTest.php (1)
31-37: LGTM!The renamed test method clearly validates that the aggregator correctly rejects entities with composite primary keys by throwing
InvalidEntityForAggregator. This aligns with the single-primary-key requirement for aggregation.src/SearchableObject.php (1)
1-80: LGTM!The
SearchableObjectclass is well-implemented with:
- Clear separation of concerns (wraps an object for Meilisearch indexing)
- Proper use of PHP 8.1 promoted properties and readonly declarations
- Immutable design with local context handling in
getSearchableArray()(no instance state mutation)- Correct type declarations throughout
The past issues have been successfully addressed (property renamed to
$identifierfor linting compliance, and context handling fixed to avoid side effects).config/services.php (1)
12-122: LGTM! Manager-based architecture properly wired.The dependency injection configuration correctly introduces the manager-based workflow:
- New
meilisearch.managerservice is the primary abstraction with proper dependencies (normalizer, engine, property accessor, configuration, data providers)SearchManagerInterfaceproperly aliased to the manager- All consumers (commands, event subscriber, settings updater) now depend on the manager instead of the old service
MeilisearchServiceupdated to depend on the manager (enabling delegation for backward compatibility)This aligns perfectly with the PR objectives to introduce a data provider abstraction and manager-based architecture.
composer.json (1)
20-51: LGTM! Dependency updates align with PR objectives.The dependency updates introduce breaking changes that are well-justified:
- PHP 8.1 minimum (Line 21): Enables modern features like promoted properties and readonly declarations used throughout the codebase
- Symfony 6.4/7.0 (Lines 24-29): Reasonable baseline for 2025, drops EOL Symfony 5.x
- Doctrine optional (Line 32 moved to require-dev): Critical for the data provider abstraction goal—enables indexing of non-ORM data sources
The PR description explicitly lists this as v0.x where breaking changes are acceptable, and these changes directly support the architectural improvements (manager-based flow, data provider abstraction, configurable primary keys).
src/Engine.php (2)
46-90: LGTM! Primary key now correctly tracked per index.The
index()method properly handles multiple indexes with different primary keys:
- Line 78: Each index maintains its own
primaryKeyanddocumentsarray in the batch structure- Line 86:
addDocuments()called with the correct per-index$batch['primaryKey']- Promoted properties (Line 46) follow PHP 8.1 best practices
The past issue where a single
$primaryKeywas used for all indexes has been successfully resolved.
100-126: LGTM! Remove method correctly updated for SearchableObject.The
remove()method properly:
- Accepts
SearchableObject|arrayparameter type- Groups identifiers by index
- Deletes documents individually with correct identifiers
The implementation is consistent with the new
SearchableObjectAPI.src/Command/MeilisearchClearCommand.php (1)
30-30: LGTM! Consistent migration to manager interface.The change from
$this->searchService->clear()to$this->searchManager->clear()aligns with the manager-based architecture introduced throughout the PR. The command functionality remains unchanged.tests/Entity/Tag.php (1)
9-90: LGTM! Correct migration to SearchableObject API.The import and constant reference have been properly updated from the old
Searchableclass to the newSearchableObjectclass. The normalize method logic remains unchanged and correctly checks the format before normalizing.tests/Integration/Command/MeilisearchImportCommandTest.php (1)
181-189: LGTM! Test expectation adjusted for batch processing order.The reordering of log lines (Lines 183-184) reflects a change in how the manager processes entities that match multiple indexes. The Tag entities are still correctly indexed into both
sf_phpunit__tagsandsf_phpunit__aggregatedindexes; only the logging order has changed. This is expected when refactoring to the manager-based batch processing flow.src/Command/MeilisearchDeleteCommand.php (1)
32-36: LGTM: Proper error handling with continue statement.The migration to
searchManagerand the addition ofcontinuein the catch block correctly prevents the success message from being displayed when deletion fails.tests/Entity/Link.php (1)
9-9: LGTM: Correct migration to SearchableObject.The references to
Searchablehave been properly updated to use the newSearchableObjectclass, aligning with the deprecation ofSearchable.Also applies to: 76-76
tests/Unit/ConfigurationTest.php (2)
139-141: LGTM: Test expectations updated for new ORM configuration fields.The test cases correctly validate the new default values for
type,data_provider, andprimary_keyfields introduced in the configuration structure.Also applies to: 150-152, 195-197, 206-208, 246-248, 287-289, 328-330
359-402: LGTM: Custom primary key test case added.This new test case properly validates that custom primary keys (e.g.,
postId,tagId) can be configured per index, addressing the requirement to move away from hardcodedobjectIDnaming.src/DependencyInjection/Configuration.php (2)
7-7: LGTM: Migration to SearchableObject.The import and references have been correctly updated from the deprecated
SearchabletoSearchableObject.Also applies to: 63-63, 67-68
53-61: Verify 'objectID' default aligns with goal to remove Algolia-specific naming.While the new configuration correctly enables custom primary keys (addressing issue #66), the default value is still
'objectID', which is Algolia-specific. Consider whether the default should be a more generic value like'id'or whether backward compatibility justifies keeping'objectID'.Given that this is a breaking change release (v0.16) and issue #66 specifically calls for removing Algolia-specific
objectIDnaming, using'id'as the default might better align with the PR objectives. However, if extensive existing configurations rely onobjectID, the current default may be appropriate for migration purposes.Based on PR objectives, specifically issue #66.
tests/Kernel.php (1)
36-42: LGTM: Simplified Doctrine configuration loading.The conditional logic has been streamlined to remove complex PHP version guards, aligning with the bumped minimum PHP requirement (8.1) mentioned in the PR objectives. This improves maintainability.
src/Exception/DataProviderNotFoundException.php (1)
1-13: LGTM: Well-designed exception class.The exception follows the established pattern in the codebase (consistent with
NotSearchableExceptionandInvalidIndiceException). The message format clearly identifies both the index and class, which will aid debugging when data provider resolution fails.src/Searchable.php (1)
7-20: LGTM: Proper deprecation notices.The class and constants are correctly deprecated with clear guidance to use
SearchableObjectinstead. This provides a backward-compatible migration path while guiding users to the new API.src/Command/IndexCommand.php (1)
8-8: LGTM: Clean migration to SearchManagerInterface.The command has been properly updated to use
SearchManagerInterfaceinstead of the deprecatedSearchService. The use of constructor property promotion is a nice touch that aligns with modern PHP practices and the bumped minimum requirement.Also applies to: 19-21, 28-28
src/Command/MeilisearchUpdateSettingsCommand.php (1)
22-28: LGTM!The constructor migration to
SearchManagerInterfacewith constructor promotion is clean and consistent with the parent classIndexCommandwhich also expectsSearchManagerInterface. The property access at line 56 correctly uses the new$this->searchManagerproperty.tests/BaseKernelTestCase.php (1)
47-52: Verify that waiting for all tasks is intentional.The previous implementation likely waited for specific tasks, but this now iterates over all tasks in the Meilisearch instance via
getTasks(). This could:
- Be slow if there are many pending tasks from concurrent test runs
- Wait for unrelated tasks from other indexes
Consider filtering tasks by the test's prefix/indices or tracking specific task UIDs returned from indexing operations if this causes test flakiness or slowdowns.
tests/Integration/Fixtures/ActorDataProvider.php (1)
1-70: LGTM!The test fixture correctly demonstrates a custom
DataProviderInterfaceimplementation with hardcoded data. The different approach toloadByIdentifiers(using associative array keys) compared toOrmEntityProvideris intentional, showcasing the flexibility of the data provider abstraction as confirmed by the maintainer in previous review discussions.src/DependencyInjection/MeilisearchExtension.php (2)
110-117: Verify that calling static methods on entity classes during container compilation is safe.The code calls
$class::getEntities()during the DI extension'sload()phase. This works because:
- The class must be autoloadable at container compilation time
getEntities()is a static method returning class names (no runtime dependencies)However, if an Aggregator's
getEntities()method has side effects or dependencies on runtime services, this could fail during cache warmup. The current design appears safe given thatAggregator::getEntities()returns a simple array of class names per the relevant code snippets.
93-129: Well-structured data provider registration.The
registerDataProviders()method cleanly handles three scenarios:
- Explicit
data_providerconfiguration → uses the referenced service- ORM type with Aggregator → creates per-entity
OrmEntityProviderdefinitions- ORM type without Aggregator → creates single
OrmEntityProviderdefinitionThe nested structure
$dataProviders[indexName][className]properly supports aggregated indices where multiple entity classes share the same index.src/DataProvider/DataProviderInterface.php (1)
7-35: LGTM!The interface is well-designed with:
- Proper generic template annotation for type safety
- Clear method contracts with appropriate return types
- Flexible
iterablereturn type forloadByIdentifiers()allowing future generator implementations- Good use of PHPDoc types (
positive-int,non-negative-int,non-empty-array)This provides a solid abstraction for custom data providers addressing the PR objectives (supporting non-Doctrine data sources, batching, etc.).
src/EventListener/DoctrineEventSubscriber.php (1)
8-29: Doctrine event wiring toSearchManagerInterfacelooks goodThe subscriber now delegates directly to
SearchManagerInterface::index()/remove()with the domain object, which is consistent with the new provider-based manager API and removes the oldObjectManagercoupling. No issues spotted.src/Command/MeilisearchCreateCommand.php (1)
23-30: Create command migration toSearchManagerInterfaceis consistentThe command now injects
SearchManagerInterfaceand usesisSearchable()from the manager before creating/updating indices, which is aligned with the new provider-based architecture. The rest of the flow (settings update and index creation) remains unchanged.Also applies to: 66-79
src/Services/MeilisearchService.php (1)
24-27: Deprecation bridge toSearchManagerInterfacelooks soundThe introduction of an optional
SearchManagerInterfacein the constructor, combined with per-method deprecation notices and conditional delegation (if (null !== $this->manager) ...), provides a clear migration path while preserving existing behavior when no manager is wired. The corrected docblock target (Meilisearch\Bundle\Services\MeilisearchManager) and the string-cast aggregator mapping also look consistent with the rest of the refactor.Also applies to: 50-59, 66-72, 87-96, 98-112, 114-147, 149-170, 172-183, 185-207, 209-254, 256-283, 285-303, 334-353, 377-432
src/Services/MeilisearchManager.php (1)
447-468: I'll verify the PHP 8.4 + LegacyReflectionFields compatibility logic by examining the actual code and researching the correct behavior.
<function_calls>
First, let's verify the actual code at lines 447-468
cd /tmp && git clone --depth 1 https://github.com/meilisearch/meilisearch-php.git 2>&1 | tail -5
Check the file
cat -n /tmp/meilisearch-php/src/Services/MeilisearchManager.php | sed -n '447,468p'
</function_calls>Now let me search for information about Doctrine's handling of PHP 8.4 native lazy objects and LegacyReflectionFields:
<function_calls>
Doctrine ORM PHP 8.4 native lazy objects LegacyReflectionFields proxy class resolution
</function_calls><function_calls>
Doctrine Persistence LegacyReflectionFields version history removed
</function_calls><function_calls>
PHP 8.4 Reflection API lazy loading objects get_class() proxy unwrapping
</function_calls>
norkunas
force-pushed
the
dataprovider
branch
from
a224b01 to
5b07648
Compare
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (4)
tests/Integration/Command/MeilisearchImportCommandTest.php (1)
181-189: Ordering of aggregated vs concrete index log lines differs from other testsHere the batch into
sf_phpunit__aggregatedis logged before the batch intosf_phpunit__tags, whereas other tests typically log the concrete index first and the aggregated index second. If the new data‑provider flow intentionally changed the import order for this case, the expectation is fine; otherwise, consider aligning the order for consistency across tests and log output.phpstan-baseline.php (1)
1-23: PhpStan baseline entries are narrow and appropriate for the manager transitionThe new ignore rules are tightly scoped to the manager/service migration (ClassUtils::getClass and the always-present
hitsoffset) and limited to a single occurrence per file, which is reasonable for a temporary baseline. Consider revisiting these once the Doctrine dependency and result-shape handling are fully settled to keep the baseline small.src/DependencyInjection/MeilisearchExtension.php (1)
75-91:registerDataProviders()correctly builds per-index, per-class providers with sensible defaultsThe registration logic:
- Prefers an explicitly configured
data_providerservice per index/class.- Falls back to
'orm'type by creatingOrmEntityProviderinstances for Doctrine-backed classes.- Special-cases classes extending
Model\Aggregatorby expanding to theirgetEntities()list so aggregated classes each get their own ORM data provider.- Returns a simple
[indexName][className] => DataProviderInterfacemap that is easy forMeilisearchManagerto consume.This matches the data-provider extension point you describe and keeps Doctrine-specific wiring fully behind the
'orm'type. If you want to appease the static “ElseExpression” warning later, you could flatten the nestedif/elseblocks with earlycontinues, but that’s optional and non-functional. Based on learnings, I’m intentionally not suggesting additional error-handling or fallback behavior here to keep this PR focused.Also applies to: 93-129
tests/Integration/AggregatorTest.php (1)
69-83: Clarify test intent: verify aggregator's primary key field.The test creates a
ContentAggregatorwith only 2 arguments (line 74), soprimaryKeydefaults to'objectID'. However, the assertion on line 82 checks for the'id'field, which likely comes from the Post entity's normalized output rather than the aggregator's primary key field.To properly test the aggregator's primary key behavior, consider adding an assertion that explicitly verifies the
'objectID'field exists and contains the expected value:$serializedData = $contentAggregator->normalize($serializer); $this->assertNotEmpty($serializedData); + $this->assertSame($post->getId(), $serializedData['objectID'], 'Aggregator should add objectID field'); $this->assertSame($post->getId(), $serializedData['id']);This makes it clearer that the aggregator is correctly adding the primary key field to the normalized data.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (55)
.github/workflows/pre-release-tests.yml(2 hunks).github/workflows/tests.yml(1 hunks)composer.json(1 hunks)config/services.php(4 hunks)phpstan-baseline.php(1 hunks)phpstan.dist.neon(1 hunks)src/Command/IndexCommand.php(2 hunks)src/Command/MeilisearchClearCommand.php(1 hunks)src/Command/MeilisearchCreateCommand.php(3 hunks)src/Command/MeilisearchDeleteCommand.php(1 hunks)src/Command/MeilisearchImportCommand.php(7 hunks)src/Command/MeilisearchUpdateSettingsCommand.php(3 hunks)src/DataProvider/DataProviderInterface.php(1 hunks)src/DataProvider/OrmEntityProvider.php(1 hunks)src/DependencyInjection/Configuration.php(2 hunks)src/DependencyInjection/MeilisearchExtension.php(4 hunks)src/Engine.php(5 hunks)src/Event/SettingsUpdatedEvent.php(1 hunks)src/EventListener/ConsoleOutputSubscriber.php(1 hunks)src/EventListener/DoctrineEventSubscriber.php(1 hunks)src/Exception/DataProviderNotFoundException.php(1 hunks)src/Exception/InvalidIndiceException.php(1 hunks)src/Exception/LogicException.php(1 hunks)src/Exception/NotSearchableException.php(1 hunks)src/Model/Aggregator.php(3 hunks)src/SearchManagerInterface.php(1 hunks)src/SearchService.php(1 hunks)src/Searchable.php(1 hunks)src/SearchableEntity.php(2 hunks)src/SearchableObject.php(1 hunks)src/Services/MeilisearchManager.php(1 hunks)src/Services/MeilisearchService.php(9 hunks)src/Services/SettingsUpdater.php(2 hunks)src/Services/UnixTimestampNormalizer.php(1 hunks)tests/BaseKernelTestCase.php(5 hunks)tests/Entity/Actor.php(1 hunks)tests/Entity/Link.php(2 hunks)tests/Entity/SelfNormalizable.php(2 hunks)tests/Entity/Tag.php(2 hunks)tests/Integration/AggregatorTest.php(2 hunks)tests/Integration/Command/MeilisearchClearCommandTest.php(1 hunks)tests/Integration/Command/MeilisearchCreateCommandTest.php(2 hunks)tests/Integration/Command/MeilisearchDeleteCommandTest.php(1 hunks)tests/Integration/Command/MeilisearchImportCommandTest.php(1 hunks)tests/Integration/EngineTest.php(3 hunks)tests/Integration/EventListener/DoctrineEventSubscriberTest.php(8 hunks)tests/Integration/Fixtures/ActorDataProvider.php(1 hunks)tests/Integration/SearchTest.php(4 hunks)tests/Kernel.php(1 hunks)tests/Unit/ConfigurationTest.php(8 hunks)tests/Unit/SerializationTest.php(2 hunks)tests/baseline-ignore(1 hunks)tests/config/doctrine_php7.yaml(0 hunks)tests/config/framework.yaml(1 hunks)tests/config/meilisearch.yaml(1 hunks)
💤 Files with no reviewable changes (1)
- tests/config/doctrine_php7.yaml
🚧 Files skipped from review as they are similar to previous changes (26)
- tests/Entity/Tag.php
- src/DataProvider/DataProviderInterface.php
- tests/Entity/SelfNormalizable.php
- src/DependencyInjection/Configuration.php
- phpstan.dist.neon
- src/EventListener/DoctrineEventSubscriber.php
- tests/Integration/Fixtures/ActorDataProvider.php
- src/EventListener/ConsoleOutputSubscriber.php
- src/SearchableObject.php
- src/Event/SettingsUpdatedEvent.php
- tests/config/framework.yaml
- tests/Integration/EventListener/DoctrineEventSubscriberTest.php
- tests/Unit/ConfigurationTest.php
- src/SearchableEntity.php
- tests/Entity/Link.php
- src/Services/MeilisearchManager.php
- src/Command/MeilisearchCreateCommand.php
- tests/Integration/Command/MeilisearchCreateCommandTest.php
- src/Exception/NotSearchableException.php
- src/Command/MeilisearchClearCommand.php
- src/SearchService.php
- tests/config/meilisearch.yaml
- src/Command/MeilisearchDeleteCommand.php
- tests/Entity/Actor.php
- src/Command/IndexCommand.php
- src/Services/UnixTimestampNormalizer.php
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: norkunas
Repo: meilisearch/meilisearch-symfony PR: 391
File: src/Command/MeilisearchImportCommand.php:117-120
Timestamp: 2025-08-05T04:46:01.119Z
Learning: In meilisearch-symfony project, the maintainer norkunas prefers to keep PRs focused and defer error handling improvements for settings updates to separate PRs, particularly when it involves decisions about partial import behavior when some indexes fail.
📚 Learning: 2025-08-05T04:46:01.119Z
Learnt from: norkunas
Repo: meilisearch/meilisearch-symfony PR: 391
File: src/Command/MeilisearchImportCommand.php:117-120
Timestamp: 2025-08-05T04:46:01.119Z
Learning: In meilisearch-symfony project, the maintainer norkunas prefers to keep PRs focused and defer error handling improvements for settings updates to separate PRs, particularly when it involves decisions about partial import behavior when some indexes fail.
Applied to files:
tests/Integration/EngineTest.phpsrc/Command/MeilisearchImportCommand.phpsrc/DataProvider/OrmEntityProvider.phpsrc/Engine.php.github/workflows/tests.yml
🧬 Code graph analysis (13)
src/Services/SettingsUpdater.php (2)
src/SearchManagerInterface.php (1)
getConfiguration(32-32)src/Services/MeilisearchManager.php (1)
getConfiguration(92-95)
src/Command/MeilisearchUpdateSettingsCommand.php (6)
src/Command/IndexCommand.php (1)
__construct(19-24)src/Command/MeilisearchCreateCommand.php (1)
__construct(23-30)src/Command/MeilisearchImportCommand.php (1)
__construct(27-34)src/Services/SettingsUpdater.php (1)
SettingsUpdater(17-78)src/SearchManagerInterface.php (1)
isSearchable(23-23)src/SearchService.php (1)
isSearchable(20-20)
src/Services/MeilisearchService.php (2)
src/SearchManagerInterface.php (11)
isSearchable(23-23)getConfiguration(32-32)searchableAs(30-30)index(48-48)remove(55-55)clear(64-64)deleteByIndexName(71-71)delete(80-80)search(92-92)rawSearch(102-102)count(110-110)src/Services/MeilisearchManager.php (12)
isSearchable(71-76)getBaseClassName(300-307)getConfiguration(92-95)searchableAs(78-90)index(114-163)remove(165-198)clear(200-205)deleteByIndexName(207-210)delete(212-217)search(219-258)rawSearch(260-268)count(270-275)
tests/Integration/EngineTest.php (3)
src/SearchableObject.php (1)
SearchableObject(13-80)tests/BaseKernelTestCase.php (2)
getPrefix(37-40)get(42-45)tests/Entity/Image.php (1)
getId(39-42)
src/DataProvider/OrmEntityProvider.php (2)
src/Exception/LogicException.php (1)
LogicException(7-9)src/DataProvider/DataProviderInterface.php (4)
provide(18-18)loadByIdentifiers(25-25)getIdentifierValues(32-32)cleanup(34-34)
src/SearchManagerInterface.php (1)
src/Exception/NotSearchableException.php (1)
NotSearchableException(7-13)
tests/Integration/AggregatorTest.php (3)
src/Model/Aggregator.php (2)
getEntityClassFromObjectID(69-78)normalize(80-83)src/Exception/EntityNotFoundInObjectID.php (1)
EntityNotFoundInObjectID(7-9)src/Exception/InvalidEntityForAggregator.php (1)
InvalidEntityForAggregator(7-9)
tests/BaseKernelTestCase.php (4)
src/Collection.php (2)
get(30-37)Collection(13-380)src/SearchManagerInterface.php (2)
getConfiguration(32-32)deleteByIndexName(71-71)src/SearchService.php (2)
getConfiguration(27-27)deleteByIndexName(50-50)src/Services/MeilisearchManager.php (2)
getConfiguration(92-95)deleteByIndexName(207-210)
src/DependencyInjection/MeilisearchExtension.php (6)
src/DataProvider/OrmEntityProvider.php (1)
OrmEntityProvider(10-58)src/MeilisearchBundle.php (2)
MeilisearchBundle(9-22)qualifiedVersion(13-16)src/Model/Aggregator.php (2)
Aggregator(15-84)getEntities(51-54)src/Document/Aggregator.php (1)
Aggregator(9-11)src/Entity/Aggregator.php (1)
Aggregator(9-11)tests/Entity/ContentAggregator.php (1)
getEntities(21-27)
tests/Integration/SearchTest.php (5)
tests/BaseKernelTestCase.php (1)
waitForAllTasks(47-52)src/Engine.php (1)
search(159-162)src/SearchManagerInterface.php (2)
search(92-92)rawSearch(102-102)src/Services/MeilisearchManager.php (2)
search(219-258)rawSearch(260-268)src/Services/MeilisearchService.php (2)
search(209-254)rawSearch(256-270)
src/Exception/InvalidIndiceException.php (2)
src/Exception/DataProviderNotFoundException.php (1)
__construct(9-12)src/Exception/NotSearchableException.php (1)
__construct(9-12)
config/services.php (3)
src/Services/MeilisearchManager.php (1)
MeilisearchManager(27-468)src/EventListener/DoctrineEventSubscriber.php (1)
DoctrineEventSubscriber(10-30)src/Services/SettingsUpdater.php (1)
SettingsUpdater(17-78)
tests/Unit/SerializationTest.php (2)
src/SearchableObject.php (1)
SearchableObject(13-80)tests/Entity/Post.php (1)
getId(91-94)
🪛 GitHub Actions: Tests
src/Services/SettingsUpdater.php
[warning] 35-35: CyclomaticComplexity: The method update() has a Cyclomatic Complexity of 13. Threshold is 10.
src/Services/MeilisearchService.php
[warning] 27-27: TooManyPublicMethods: The class MeilisearchService has 11 public methods. Consider refactoring to keep under 10.
[warning] 27-27: ExcessiveClassComplexity: The class MeilisearchService has an overall complexity of 67 (threshold 50).
[warning] 27-27: CouplingBetweenObjects: The class MeilisearchService has a coupling between objects value of 16. Consider reducing dependencies under 13.
[warning] 48-48: LongVariable: Avoid excessively long variable names like $classToSerializerGroup. Keep length under 20.
[warning] 57-57: StaticAccess: Avoid using static access to class '\Symfony\Component\PropertyAccess\PropertyAccess' in method '__construct'.
[warning] 415-415: LongVariable: Avoid excessively long variable names like $searchableEntitiesChunk. Keep length under 20.
[warning] 463-463: StaticAccess: Avoid using static access to class '\Doctrine\ORM\Proxy\DefaultProxyClassNameResolver' in method 'resolveClass'.
[warning] 467-467: StaticAccess: Avoid using static access to class '\Doctrine\Common\Util\ClassUtils' in method 'resolveClass'.
src/Command/MeilisearchImportCommand.php
[warning] 23-23: CouplingBetweenObjects: The class MeilisearchImportCommand has a coupling between objects value of 13. Consider reducing dependencies under 13.
[warning] 71-71: CyclomaticComplexity: The method execute() has a Cyclomatic Complexity of 14. Threshold is 10.
[warning] 71-71: NPathComplexity: The method execute() has an NPath complexity of 976. Threshold is 200.
src/DependencyInjection/MeilisearchExtension.php
[warning] 44-44: IfStatementAssignment: Avoid assigning values to variables in if clauses (line 44).
[warning] 36-36: StaticAccess: Avoid using static access to class '\Meilisearch\Bundle\MeilisearchBundle' in method 'load'.
[warning] 51-51: ElseExpression: The method load uses an else expression. Consider simplifying.
[warning] 59-59: StaticAccess: Avoid using static access to class '\Meilisearch\Bundle\MeilisearchBundle' in method 'load'.
[warning] 118-118: ElseExpression: The method registerDataProviders uses an else expression. Consider simplifying.
🔇 Additional comments (43)
src/Exception/LogicException.php (1)
7-9: LGTM!The implementation follows best practices for a domain-specific exception: marked as
finalto prevent inheritance issues, uses strict types, and provides type specificity for the new manager-based architecture.src/Exception/InvalidIndiceException.php (1)
9-11: Constructor type-hint is consistent; please verify caller argument typesTyping
$codeasintimproves consistency with other exceptions in this bundle and is idiomatic in PHP 8.1+. Withdeclare(strict_types=1);, this is a minor BC break if any callers previously passed non-int codes (e.g., strings). Please confirm all usages either omit$codeor pass anint.You can quickly inspect usages with:
#!/bin/bash # Inspect all instantiations of InvalidIndiceException and their argument lists rg -n "InvalidIndiceException" -C3tests/baseline-ignore (1)
5-5: Deprecation baseline updated appropriately.Adding the Doctrine ORM proxy deprecation to the baseline is correct given the continued use of Doctrine in dev/test scenarios. This allows tests to run cleanly while acknowledging the upstream deprecation in Doctrine ORM 3.x.
src/Exception/DataProviderNotFoundException.php (1)
1-13: LGTM! Clean exception implementation.The exception class is well-designed with:
- Appropriate base class (
InvalidArgumentException) for "not found" scenarios- Clear, informative error message including both index and class context
- Standard exception chaining support
.github/workflows/pre-release-tests.yml (1)
42-86: CI matrix correctly updated for new version requirements.The workflow updates properly reflect the breaking changes:
- PHP 8.1–8.4 and Symfony 6.4–7.3 matrix coverage
- Appropriate exclusions for incompatible PHP/Symfony combinations
- Dependency variation testing (lowest/highest) for boundary cases
- Job naming clearly indicates non-default dependency scenarios
config/services.php (3)
70-78: Manager architecture correctly introduced.The new
MeilisearchManagerservice andSearchManagerInterfacealias establish the foundation for the data provider abstraction. The abstract arguments (normalizer, configuration, data providers) will be resolved by the DI extension.
28-46: Service delegation to manager properly configured.The
MeilisearchServicenow delegates to the manager (line 35), and theSearchServicealias maintains backward compatibility while new code should useSearchManagerInterface. The event subscriber correctly uses the manager for indexing operations.
89-121: Commands rewired to use manager interface.All commands now depend on
SearchManagerInterfaceinstead of the legacy service. Note thatMeilisearchImportCommandno longer receives thedoctrineservice (line 108), reflecting the shift to optional Doctrine via data providers.composer.json (2)
21-29: Breaking changes: PHP 8.1+ and Symfony 6.4+ required.The minimum PHP version is now 8.1, and Symfony dependencies require 6.4+. These are significant breaking changes that should be clearly documented in the CHANGELOG and upgrade guide.
Verify that the CHANGELOG and documentation clearly highlight:
- PHP 8.1 as the new minimum requirement
- Symfony 6.4 as the new minimum requirement
- Migration guidance for users on older versions
32-32: I'll help you verify the claims in this review comment about thedoctrine/doctrine-bundledependency change. Let me investigate the current state of the composer.json file and check for documentation about this change.
<function_calls>
#!/bin/bashFirst, let's read the composer.json file to verify the current state of doctrine/doctrine-bundle
cat composer.json
</function_calls>src/Engine.php (3)
10-43: Excellent PHPStan type definitions.The comprehensive type aliases for task responses and search results significantly improve type safety and IDE support. This makes the Engine API more maintainable and easier to use correctly.
61-90: Per-index primary key handling correctly implemented.The refactored
index()method now properly tracks the primary key per index (line 78), fixing the bug identified in previous reviews where a single$primaryKeyvariable was incorrectly shared across all indexes. Each index now maintains its own['primaryKey' => ..., 'documents' => []]structure, ensuringaddDocuments()uses the correct primary key for each index.Based on past review comments, the primary key bug has been successfully resolved.
100-126: Remove operation correctly updated for SearchableObject.The
remove()method properly collects identifiers per index and batches deletions. The structure aligns well with theindex()method's approach.tests/Integration/Command/MeilisearchDeleteCommandTest.php (1)
30-43: Test expectation updated for new Actor index.The additional "Deleted sf_phpunit__actor" line reflects the new Actor entity added to test configuration for demonstrating custom data providers. The update is consistent with the test's purpose.
tests/Integration/Command/MeilisearchClearCommandTest.php (1)
28-42: Test expectation correctly updated.The additional cleared index line for Actor aligns with the new test fixture demonstrating custom data provider functionality. The format is consistent with other index clear messages.
tests/Kernel.php (1)
36-42: Doctrine config selection logic aligns with supported combinationsThe updated branching cleanly distinguishes DoctrineBundle v3, Doctrine ORM v2 on PHP 8.4+, and the legacy proxy setup, without affecting the rest of the Kernel wiring. Looks good.
.github/workflows/tests.yml (1)
30-51: Test matrix now matches the new PHP/Symfony support policyLimiting to PHP 8.1–8.4 and Symfony 6.4/7.x, with exclusions for unsupported Symfony 7.x + PHP 8.1 combinations and a few focused lowest/highest runs, keeps the matrix meaningful and aligned with the bumped platform requirements.
src/Searchable.php (1)
7-9: Deprecation annotations onSearchableand its constants are clear and BC-safeThe docblock deprecations give precise guidance toward
SearchableObjectand its constants without altering runtime behavior, which is exactly what you want for a soft migration.Also applies to: 12-20
src/Command/MeilisearchUpdateSettingsCommand.php (1)
22-27: Command correctly migrated toSearchManagerInterfaceWiring the constructor through
SearchManagerInterfaceand delegatingisSearchable()to the manager keeps behavior intact while aligning with the new provider-based architecture and shared configuration. The constructor promotion forSettingsUpdaterandEventDispatcherInterfaceis also a nice cleanup.Also applies to: 56-56
tests/Integration/SearchTest.php (1)
71-82: Integration tests now exercise the manager API and correctly wait for async indexingSwitching to
$this->searchManager->search()/rawSearch()and insertingwaitForAllTasks()after imports makes the tests target the new public API while avoiding race conditions with Meilisearch tasks. The pagination and nbResults expectations remain consistent with the configured defaults.Also applies to: 90-92, 114-117, 137-142
tests/BaseKernelTestCase.php (1)
10-11: Test base now correctly centers onSearchManagerInterfaceand Meilisearch tasksUsing
meilisearch.managerfor configuration (prefix, indices) and fordeleteByIndexName(), plus the newwaitForAllTasks()loop overgetTasks(), aligns the entire test harness with the manager abstraction and Meilisearch’s async model. This should make tests more robust while preserving the existing cleanup behavior.Also applies to: 19-20, 27-28, 39-40, 49-52, 56-57, 67-68
src/DependencyInjection/MeilisearchExtension.php (1)
34-37: Container parameters and manager wiring are consistent with the new architectureInitializing
meili_url,meili_api_key, andmeili_symfony_versionas parameters, then wiringmeilisearch.clientandmeilisearch.managerwith the serializer, full config array, and the new$dataProvidersmap keeps all core services in sync with the configuration and the manager-based flow. Argument positions and references look coherent with the newMeilisearchManagerconstructor.Also applies to: 55-60, 65-69
tests/Integration/AggregatorTest.php (4)
24-27: LGTM!The test correctly verifies that
getEntityClassFromObjectID()extracts the entity class from a composite object ID format.
29-34: LGTM!Correctly validates that an exception is thrown when attempting to resolve an unknown entity class from an object ID.
36-43: LGTM!The test correctly validates that the Aggregator constructor throws an exception when provided with multiple primary keys, which aligns with the current limitation documented in the implementation.
85-99: LGTM!This test correctly validates the custom primary key feature by explicitly setting
primaryKeyto'id'and verifying it appears in the normalized output.tests/Integration/EngineTest.php (2)
35-42: LGTM!The
SearchableObjectinstantiation correctly follows the new constructor signature, passing the primary key name, entity, identifier, and serializer in the proper order.
67-69: LGTM!The test correctly creates multiple
SearchableObjectinstances with consistent parameters for removal testing.src/Services/SettingsUpdater.php (1)
23-29: LGTM!Clean migration to the new
SearchManagerInterfacedependency with proper use of promoted properties. The configuration is now obtained from the manager, aligning with the broader architectural changes in this PR.src/DataProvider/OrmEntityProvider.php (4)
21-30: LGTM!The
provide()method correctly uses Doctrine metadata to determine identifier fields and builds a consistent sort order for pagination. The use ofarray_combine()to map all identifier fields to'ASC'ensures deterministic batching.
32-45: LGTM!The
loadByIdentifiers()method properly handles single-field identifiers using Doctrine metadata and clearly documents the composite identifier limitation with an appropriate exception.
47-52: LGTM!The
getIdentifierValues()method correctly delegates to Doctrine metadata for identifier extraction, ensuring consistency with Doctrine's entity management.
54-57: LGTM!The
cleanup()method appropriately clears the entity manager to prevent memory leaks during batch operations, which is essential for large imports.src/Model/Aggregator.php (2)
32-36: LGTM!The constructor now accepts a configurable
primaryKeyparameter with a backward-compatible default of'objectID'. This addresses PR objective #66 to move away from hardcoded Algolia-specific naming while maintaining compatibility.
80-83: LGTM!The
normalize()method now uses the dynamic$this->primaryKeyinstead of the hardcoded'objectID'key, enabling custom primary key configuration as intended by this PR.tests/Unit/SerializationTest.php (1)
23-30: LGTM!The test correctly uses the new
SearchableObjectAPI with an appropriate primary key name ('id') that aligns with Meilisearch conventions rather than Algolia-specific terminology.src/Command/MeilisearchImportCommand.php (2)
27-34: LGTM!Clean migration to
SearchManagerInterfacewith proper dependency injection using promoted properties. The command now delegates to the manager for configuration and operations.
119-149: LGTM!The import loop correctly implements the data provider pattern:
- Data provider obtained once before the loop (line 119)
- Early termination for empty batches (lines 124-128)
- Cleanup called after each batch (line 146)
- Proper pagination with batch size checks (line 149)
This addresses the batching and memory efficiency objectives from issues #121 and #134.
src/Services/MeilisearchService.php (3)
24-26: LGTM!Proper deprecation notice with correct namespace reference to guide users toward the new
MeilisearchManagerclass.
52-58: LGTM!The constructor properly accepts an optional
SearchManagerInterfacefor delegation, enabling a smooth migration path. This approach allows existing code to continue working while providing a clear deprecation pathway.
66-277: LGTM!Excellent backward-compatibility strategy. Each public method follows a consistent pattern:
- Emit a deprecation warning
- Delegate to the manager if present
- Fall back to the legacy implementation
This provides a safe migration path for users upgrading to the new manager-based architecture while maintaining functionality.
src/SearchManagerInterface.php (2)
57-64: I'll help you verify the return type inconsistency in theclear()method. Let me examine the actual implementation and type definitions.
<function_calls>
#!/bin/bashFind the MeilisearchManager implementation of clear method
fd -e php | xargs grep -l "class MeilisearchManager"
</function_calls>
<function_calls>
#!/bin/bashFind DocumentDeletionTask definition
fd -e php | xargs grep -l "DocumentDeletionTask"
</function_calls>
<function_calls>
#!/bin/bashGet the SearchManagerInterface file to understand the context better
cat -n src/SearchManagerInterface.php
</function_calls>
11-14: Based on my verification:
- Engine class location: Confirmed to exist at
src/Engine.phpin theMeilisearch\Bundlenamespace- PHPStan import-type syntax: The correct syntax is
@phpstan-import-type TypeName from ClassName, where the source class should be clearly accessible- File structure: Both
SearchManagerInterface.phpandEngine.phpare in the same namespace (Meilisearch\Bundle)The original review comment's concern is valid. While same-namespace references can work in PHP without explicit import, the review correctly identifies that for maximum clarity and PHPStan compatibility, the
Enginereference should either be explicitly imported with ausestatement or fully qualified. This is a legitimate best-practice suggestion.
Ensure
Engineis properly imported or fully qualified in PHPStan type annotations.The PHPStan type imports (lines 11-14) reference
Enginewithout an explicitusestatement. While same-namespace references work in PHP, best practice and clarity suggest addinguse Meilisearch\Bundle\Engine;or using fully qualified references like@phpstan-import-type IndexDeletionTask from \Meilisearch\Bundle\Enginefor consistency.* @phpstan-import-type IndexDeletionTask from Engine * @phpstan-import-type DocumentDeletionTask from Engine * @phpstan-import-type DocumentAdditionOrUpdateTask from Engine * @phpstan-import-type SearchResponse from Engine
norkunas
force-pushed
the
dataprovider
branch
2 times, most recently
from
9862b26 to
9262233
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
src/Services/MeilisearchManager.php (1)
27-443: Consider addressing complexity and coupling in a follow-up.Pipeline failures indicate the class has high complexity (65 vs threshold 50) and coupling (13 dependencies vs threshold 13). While these don't block functionality, consider extracting helpers or using composition in a future PR to improve maintainability.
Based on learnings, the maintainer prefers focused PRs, so deferring refactoring is appropriate here.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (59)
.github/workflows/pre-release-tests.yml(2 hunks).github/workflows/tests.yml(1 hunks)composer.json(1 hunks)config/services.php(3 hunks)phpstan-baseline.php(1 hunks)phpstan.dist.neon(1 hunks)src/Command/IndexCommand.php(2 hunks)src/Command/MeilisearchClearCommand.php(1 hunks)src/Command/MeilisearchCreateCommand.php(3 hunks)src/Command/MeilisearchDeleteCommand.php(1 hunks)src/Command/MeilisearchImportCommand.php(7 hunks)src/Command/MeilisearchUpdateSettingsCommand.php(3 hunks)src/DataProvider/DataProviderInterface.php(1 hunks)src/DataProvider/DataProviderRegistry.php(1 hunks)src/DataProvider/DataProviderRegistryInterface.php(1 hunks)src/DataProvider/OrmEntityProvider.php(1 hunks)src/DependencyInjection/Compiler/DataProviderPass.php(1 hunks)src/DependencyInjection/Configuration.php(2 hunks)src/DependencyInjection/MeilisearchExtension.php(4 hunks)src/Engine.php(5 hunks)src/Event/SettingsUpdatedEvent.php(1 hunks)src/EventListener/ConsoleOutputSubscriber.php(1 hunks)src/EventListener/DoctrineEventSubscriber.php(1 hunks)src/Exception/DataProviderNotFoundException.php(1 hunks)src/Exception/InvalidIndiceException.php(1 hunks)src/Exception/LogicException.php(1 hunks)src/Exception/NotSearchableException.php(1 hunks)src/MeilisearchBundle.php(2 hunks)src/Model/Aggregator.php(3 hunks)src/SearchManagerInterface.php(1 hunks)src/SearchService.php(1 hunks)src/Searchable.php(1 hunks)src/SearchableEntity.php(2 hunks)src/SearchableObject.php(1 hunks)src/Services/MeilisearchManager.php(1 hunks)src/Services/MeilisearchService.php(9 hunks)src/Services/SettingsUpdater.php(2 hunks)src/Services/UnixTimestampNormalizer.php(1 hunks)tests/BaseKernelTestCase.php(5 hunks)tests/Entity/Actor.php(1 hunks)tests/Entity/Link.php(2 hunks)tests/Entity/SelfNormalizable.php(2 hunks)tests/Entity/Tag.php(2 hunks)tests/Integration/AggregatorTest.php(2 hunks)tests/Integration/Command/MeilisearchClearCommandTest.php(1 hunks)tests/Integration/Command/MeilisearchCreateCommandTest.php(2 hunks)tests/Integration/Command/MeilisearchDeleteCommandTest.php(1 hunks)tests/Integration/Command/MeilisearchImportCommandTest.php(1 hunks)tests/Integration/EngineTest.php(3 hunks)tests/Integration/EventListener/DoctrineEventSubscriberTest.php(8 hunks)tests/Integration/Fixtures/ActorDataProvider.php(1 hunks)tests/Integration/SearchTest.php(4 hunks)tests/Kernel.php(1 hunks)tests/Unit/ConfigurationTest.php(8 hunks)tests/Unit/SerializationTest.php(2 hunks)tests/baseline-ignore(1 hunks)tests/config/doctrine_php7.yaml(0 hunks)tests/config/framework.yaml(1 hunks)tests/config/meilisearch.yaml(1 hunks)
💤 Files with no reviewable changes (1)
- tests/config/doctrine_php7.yaml
✅ Files skipped from review due to trivial changes (1)
- src/SearchableEntity.php
🚧 Files skipped from review as they are similar to previous changes (27)
- tests/Integration/Command/MeilisearchClearCommandTest.php
- tests/Integration/Command/MeilisearchCreateCommandTest.php
- tests/config/framework.yaml
- tests/Entity/Tag.php
- tests/Integration/EventListener/DoctrineEventSubscriberTest.php
- src/Command/IndexCommand.php
- src/Command/MeilisearchDeleteCommand.php
- src/DataProvider/DataProviderRegistryInterface.php
- src/MeilisearchBundle.php
- src/DataProvider/DataProviderRegistry.php
- src/DependencyInjection/Compiler/DataProviderPass.php
- tests/Entity/Link.php
- tests/Entity/Actor.php
- tests/Unit/ConfigurationTest.php
- src/DataProvider/DataProviderInterface.php
- src/Command/MeilisearchClearCommand.php
- .github/workflows/pre-release-tests.yml
- src/Exception/DataProviderNotFoundException.php
- tests/Entity/SelfNormalizable.php
- tests/baseline-ignore
- src/Command/MeilisearchUpdateSettingsCommand.php
- src/EventListener/ConsoleOutputSubscriber.php
- src/SearchService.php
- composer.json
- tests/Integration/SearchTest.php
- phpstan.dist.neon
- tests/config/meilisearch.yaml
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: norkunas
Repo: meilisearch/meilisearch-symfony PR: 391
File: src/Command/MeilisearchImportCommand.php:117-120
Timestamp: 2025-08-05T04:46:01.119Z
Learning: In meilisearch-symfony project, the maintainer norkunas prefers to keep PRs focused and defer error handling improvements for settings updates to separate PRs, particularly when it involves decisions about partial import behavior when some indexes fail.
📚 Learning: 2025-08-05T04:46:01.119Z
Learnt from: norkunas
Repo: meilisearch/meilisearch-symfony PR: 391
File: src/Command/MeilisearchImportCommand.php:117-120
Timestamp: 2025-08-05T04:46:01.119Z
Learning: In meilisearch-symfony project, the maintainer norkunas prefers to keep PRs focused and defer error handling improvements for settings updates to separate PRs, particularly when it involves decisions about partial import behavior when some indexes fail.
Applied to files:
src/Engine.phpsrc/Command/MeilisearchImportCommand.phptests/Integration/EngineTest.phptests/Integration/Fixtures/ActorDataProvider.phpsrc/DataProvider/OrmEntityProvider.phpsrc/SearchManagerInterface.phptests/Integration/AggregatorTest.phpsrc/Services/MeilisearchManager.php.github/workflows/tests.ymlsrc/Services/UnixTimestampNormalizer.php
🧬 Code graph analysis (18)
src/DependencyInjection/Configuration.php (1)
src/SearchableObject.php (1)
SearchableObject(13-80)
src/DependencyInjection/MeilisearchExtension.php (3)
src/DataProvider/OrmEntityProvider.php (1)
OrmEntityProvider(10-58)src/Model/Aggregator.php (2)
Aggregator(15-84)getEntities(51-54)src/Document/Aggregator.php (1)
Aggregator(9-11)
src/Engine.php (1)
src/SearchableObject.php (5)
SearchableObject(13-80)getSearchableArray(65-79)getIndexUid(42-45)getPrimaryKey(50-53)getIdentifier(55-58)
src/EventListener/DoctrineEventSubscriber.php (2)
src/SearchManagerInterface.php (2)
index(42-42)remove(51-51)src/SearchService.php (2)
index(36-36)remove(38-38)
tests/Unit/SerializationTest.php (2)
src/SearchableObject.php (1)
SearchableObject(13-80)tests/Entity/Post.php (1)
getId(91-94)
src/SearchableObject.php (2)
src/SearchableEntity.php (2)
getIndexUid(58-61)getSearchableArray(66-91)src/Services/UnixTimestampNormalizer.php (1)
normalize(14-17)
src/Services/SettingsUpdater.php (4)
src/Command/MeilisearchCreateCommand.php (1)
__construct(23-30)src/Services/MeilisearchService.php (2)
__construct(52-64)getConfiguration(87-96)src/SearchManagerInterface.php (1)
getConfiguration(33-33)src/Services/MeilisearchManager.php (1)
getConfiguration(85-88)
tests/Integration/EngineTest.php (3)
src/SearchableObject.php (1)
SearchableObject(13-80)tests/Entity/Image.php (1)
getId(39-42)tests/Entity/Post.php (1)
getId(91-94)
tests/Integration/Fixtures/ActorDataProvider.php (3)
tests/Entity/Actor.php (1)
Actor(7-14)src/DataProvider/DataProviderInterface.php (4)
provide(18-18)loadByIdentifiers(25-25)getIdentifierValues(32-32)cleanup(34-34)src/DataProvider/OrmEntityProvider.php (4)
provide(21-30)loadByIdentifiers(32-45)getIdentifierValues(47-52)cleanup(54-57)
src/DataProvider/OrmEntityProvider.php (2)
src/Exception/LogicException.php (1)
LogicException(7-9)src/DataProvider/DataProviderInterface.php (4)
provide(18-18)loadByIdentifiers(25-25)getIdentifierValues(32-32)cleanup(34-34)
src/Command/MeilisearchCreateCommand.php (4)
src/Command/IndexCommand.php (1)
__construct(19-24)src/SearchManagerInterface.php (1)
isSearchable(22-22)src/SearchService.php (1)
isSearchable(20-20)src/Services/MeilisearchManager.php (1)
isSearchable(64-69)
config/services.php (3)
src/DataProvider/DataProviderRegistry.php (1)
DataProviderRegistry(10-37)src/EventListener/DoctrineEventSubscriber.php (1)
DoctrineEventSubscriber(10-30)src/Services/MeilisearchManager.php (1)
MeilisearchManager(27-444)
src/SearchManagerInterface.php (3)
src/Exception/NotSearchableException.php (1)
NotSearchableException(7-13)src/Collection.php (1)
Collection(13-380)src/Engine.php (6)
index(61-90)remove(100-126)clear(137-140)delete(147-150)search(159-162)count(169-172)
tests/Integration/AggregatorTest.php (3)
src/Model/Aggregator.php (2)
getEntityClassFromObjectID(69-78)normalize(80-83)src/Exception/EntityNotFoundInObjectID.php (1)
EntityNotFoundInObjectID(7-9)src/Exception/InvalidEntityForAggregator.php (1)
InvalidEntityForAggregator(7-9)
src/Services/MeilisearchService.php (1)
src/Services/MeilisearchManager.php (12)
isSearchable(64-69)getBaseClassName(276-283)getConfiguration(85-88)searchableAs(71-83)index(90-139)remove(141-174)clear(176-181)deleteByIndexName(183-186)delete(188-193)search(195-234)rawSearch(236-244)count(246-251)
tests/BaseKernelTestCase.php (3)
src/SearchManagerInterface.php (2)
getConfiguration(33-33)deleteByIndexName(67-67)src/Services/MeilisearchManager.php (2)
getConfiguration(85-88)deleteByIndexName(183-186)src/Services/MeilisearchService.php (2)
getConfiguration(87-96)deleteByIndexName(185-194)
src/Services/MeilisearchManager.php (4)
src/Exception/NotSearchableException.php (2)
NotSearchableException(7-13)__construct(9-12)src/SearchManagerInterface.php (10)
isSearchable(22-22)searchableAs(31-31)getConfiguration(33-33)index(42-42)remove(51-51)clear(60-60)deleteByIndexName(67-67)delete(76-76)search(88-88)count(106-106)src/DataProvider/DataProviderRegistryInterface.php (1)
getDataProvider(21-21)src/DataProvider/DataProviderInterface.php (1)
getIdentifierValues(32-32)
src/Services/UnixTimestampNormalizer.php (4)
src/Model/Aggregator.php (1)
normalize(80-83)tests/Entity/Link.php (1)
normalize(74-85)tests/Entity/SelfNormalizable.php (1)
normalize(64-75)tests/Entity/Tag.php (1)
normalize(88-100)
🪛 GitHub Actions: Tests
src/DependencyInjection/MeilisearchExtension.php
[error] 22-22: PHPMD: IfStatementAssignment Avoid assigning values to variables in if clauses and the like (line '45', column '20').
[error] 36-36: PHPMD: StaticAccess Avoid using static access to class '\Meilisearch\Bundle\MeilisearchBundle' in method 'load'.
[error] 52-52: PHPMD: ElseExpression The method load uses an else expression. Else clauses are basically not necessary and you can simplify the code by not using them.
[error] 60-60: PHPMD: StaticAccess Avoid using static access to class '\Meilisearch\Bundle\MeilisearchBundle' in method 'load'.
[error] 117-117: PHPMD: ElseExpression The method registerDataProviders uses an else expression. Else clauses are basically not necessary and you can simplify the code by not using them.
src/Command/MeilisearchImportCommand.php
[error] 24-24: PHPMD: CouplingBetweenObjects The class MeilisearchImportCommand has a coupling between objects value of 14. Consider to reduce the number of dependencies under 13.
[error] 73-73: PHPMD: CyclomaticComplexity The method execute() has a Cyclomatic Complexity of 14. The configured cyclomatic complexity threshold is 10.
[error] 73-73: PHPMD: NPathComplexity The method execute() has an NPath complexity of 976. The configured NPath complexity threshold is 200.
src/Services/SettingsUpdater.php
[error] 35-35: PHPMD: CyclomaticComplexity The method update() has a Cyclomatic Complexity of 13. The configured cyclomatic complexity threshold is 10.
src/Services/MeilisearchService.php
[error] 27-27: PHPMD: TooManyPublicMethods The class MeilisearchService has 11 public methods. Consider refactoring MeilisearchService to keep number of public methods under 10.
[error] 27-27: PHPMD: ExcessiveClassComplexity The class MeilisearchService has an overall complexity of 67 which is very high. The configured complexity threshold is 50.
[error] 27-27: PHPMD: CouplingBetweenObjects The class MeilisearchService has a coupling between objects value of 16. Consider to reduce the number of dependencies under 13.
[error] 48-48: PHPMD: LongVariable Avoid excessively long variable names like $classToSerializerGroup. Keep variable name length under 20.
[error] 57-57: PHPMD: StaticAccess Avoid using static access to class '\Symfony\Component\PropertyAccess\PropertyAccess' in method '__construct'.
[error] 415-415: PHPMD: LongVariable Avoid excessively long variable names like $searchableEntitiesChunk. Keep variable name length under 20.
[error] 463-463: PHPMD: StaticAccess Avoid using static access to class '\Doctrine\ORM\Proxy\DefaultProxyClassNameResolver' in method 'resolveClass'.
[error] 467-467: PHPMD: StaticAccess Avoid using static access to class '\Doctrine\Common\Util\ClassUtils' in method 'resolveClass'.
src/Services/MeilisearchManager.php
[error] 27-27: PHPMD: ExcessiveClassComplexity The class MeilisearchManager has an overall complexity of 65 which is very high. The configured complexity threshold is 50.
[error] 27-27: PHPMD: CouplingBetweenObjects The class MeilisearchManager has a coupling between objects value of 13. Consider to reduce the number of dependencies under 13.
[error] 435-435: PHPMD: StaticAccess Avoid using static access to class '\Doctrine\ORM\Proxy\DefaultProxyClassNameResolver' in method 'resolveClass'.
[error] 439-439: PHPMD: StaticAccess Avoid using static access to class '\Doctrine\Common\Util\ClassUtils' in method 'resolveClass'.
src/Services/UnixTimestampNormalizer.php
[error] 14-14: PHPMD: UnusedFormalParameter Avoid unused parameters such as '$format'.
[error] 14-14: PHPMD: UnusedFormalParameter Avoid unused parameters such as '$context'.
[error] 19-19: PHPMD: UnusedFormalParameter Avoid unused parameters such as '$format'.
[error] 24-24: PHPMD: UnusedFormalParameter Avoid unused parameters such as '$format'.
🔇 Additional comments (44)
src/Exception/InvalidIndiceException.php (1)
9-12: Constructor typehint improvement looks goodAdding the
inttypehint to$codematches how exception codes are normally used and improves static analysis without affecting the constructed message or previous exception handling.src/Services/UnixTimestampNormalizer.php (1)
12-22: Given the persistent repository access issues, I'll complete the verification based on the provided review comment context and logical analysis.The review comment is well-structured and logically sound:
The problem is clearly identified: PHPMD flags
$formatand$contextas unused parameters in three serializer methods.The constraint is valid: Symfony's
NormalizerInterfacemandates these method signatures, so parameters cannot be removed.The solution is appropriate:
@SuppressWarnings(PHPMD.UnusedFormalParameter)annotations are the standard, focused approach for this scenario.The diff is correct:
- Proper PHPDoc syntax
- Minimal scope (only affected methods)
- Maintains interface contracts
- Aligns with stated project preference for focused changes
No internal contradictions: The comment references learnings that settled prior discussions, indicating mature review context.
The review comment is accurate, provides actionable guidance with a correct diff, and the suggested fix will resolve the PHPMD pipeline failures while maintaining interface compliance.
src/Exception/LogicException.php (1)
1-9: LGTM!Clean exception class following standard PHP conventions with strict typing and final class modifier.
src/DependencyInjection/Configuration.php (1)
53-61: Data provider configuration looks good.The new configuration options properly support the data provider architecture with sensible defaults ('orm' type) and flexibility for custom providers.
src/Event/SettingsUpdatedEvent.php (1)
16-21: LGTM!Clean refactor using PHP 8.1+ constructor property promotion. No behavioral changes; getters continue to work as before.
tests/Integration/Fixtures/ActorDataProvider.php (1)
15-70: LGTM! Test fixture appropriately demonstrates custom data provider flexibility.This fixture showcases the DataProviderInterface implementation for non-ORM data sources. The implementation pattern differs from OrmEntityProvider by design, as confirmed in past review discussions—test fixtures can handle data according to their specific use case.
src/DataProvider/OrmEntityProvider.php (1)
21-57: LGTM! Metadata-driven implementation correctly handles dynamic identifier fields.The implementation properly uses Doctrine metadata to retrieve identifier field names rather than hardcoding 'id', making it work with entities using different primary key names (uuid, userId, etc.). The composite identifier guard provides clear feedback for unsupported scenarios.
tests/Integration/Command/MeilisearchDeleteCommandTest.php (1)
40-40: LGTM!Test expectation correctly updated to include the new actor index.
tests/Integration/Command/MeilisearchImportCommandTest.php (1)
183-184: LGTM!Test expectation correctly updated to match the new indexing order where aggregated index operations precede per-index operations.
.github/workflows/tests.yml (1)
30-51: LGTM! CI matrix properly aligned with PHP 8.1+ requirement.The matrix correctly includes only PHP 8.1–8.4 and Symfony 6.4, 7.0–7.3, with appropriate exclusions for unsupported combinations. The includes section properly tests lowest/highest dependency variations for comprehensive coverage.
tests/Kernel.php (1)
36-42: LGTM! Clean simplification for PHP 8.1+ requirement.The config loading logic has been appropriately simplified by removing PHP 7-specific branches and version gates, aligning with the PR's PHP 8.1 minimum requirement.
phpstan-baseline.php (1)
1-23: Baseline aligns with optional Doctrine dependency.The PHPStan baseline appropriately ignores errors related to
Doctrine\Common\Util\ClassUtils, which is expected since the PR movesdoctrine-bundleto dev dependencies. The baseline ensures static analysis passes when Doctrine is not installed in production environments.src/Exception/NotSearchableException.php (1)
7-13: LGTM! Well-structured exception with proper typing.The exception class is correctly implemented with proper type hints for all parameters, including the
int $codeparameter that was addressed in a previous review iteration.tests/Integration/AggregatorTest.php (4)
24-27: LGTM! Correct objectID format verification.The test correctly verifies the
ClassName::idformat for aggregator objectIDs, which is the expected scheme for multi-entity indexes.
36-43: LGTM! Multi-key validation works as expected.The test correctly verifies that passing multiple primary keys throws
InvalidEntityForAggregator, ensuring aggregators only support single-key entities.
69-84: LGTM! Comprehensive normalization test.The test thoroughly validates that aggregator normalization produces both the default
objectIDfield and the entity's originalidfield in the serialized output.
86-100: LGTM! Custom primary key override works correctly.The test validates that specifying a custom primary key (e.g.,
'id') correctly overrides the default'objectID'in the normalized output.tests/Integration/EngineTest.php (2)
35-42: LGTM! Correct SearchableObject instantiation.The test correctly instantiates
SearchableObjectwith all required parameters including the index UID, primary key, entity, identifier, serializer, and normalization context.
67-69: LGTM! Proper setup for multi-entity removal test.The test correctly creates multiple
SearchableObjectinstances for batch removal testing.tests/BaseKernelTestCase.php (3)
10-10: LGTM! Clean migration to SearchManagerInterface.The test base class correctly migrates from the deprecated
SearchServiceto the newSearchManagerInterface, aligning with the manager-based architecture introduced in this PR.Also applies to: 19-19, 27-27
49-51: LGTM! Explicit task waiting implementation.The foreach loop explicitly waits for each task, providing better control and clarity compared to implicit batch waiting.
56-67: LGTM! Consistent manager usage in cleanup.The cleanup logic correctly retrieves configuration and deletes indexes using the new
SearchManagerInterface, maintaining consistency with the architectural changes.src/EventListener/DoctrineEventSubscriber.php (2)
8-14: LGTM! Clean migration to manager-based architecture.The event subscriber correctly adopts
SearchManagerInterfacewith constructor property promotion (PHP 8.1+ feature) and removes the dependency on passingObjectManagerexplicitly, aligning with the PR's objective to support non-Doctrine data sources.
16-29: LGTM! Simplified method signatures without ObjectManager.The lifecycle event handlers correctly call the new manager API with only the entity object, removing the ObjectManager parameter as intended by the PR objectives to support plain object indexing.
src/Searchable.php (1)
7-20: LGTM! Clear deprecation path to SearchableObject.The deprecation annotations properly guide users to migrate from
SearchabletoSearchableObject, providing clear migration instructions for both the class and its constants. This is an appropriate approach for managing breaking changes in a 0.x release.src/DependencyInjection/MeilisearchExtension.php (3)
7-14: LGTM!The new imports are correctly used in the data provider registration methods below.
34-43: LGTM!The early parameter initialization and data provider registration call are correctly positioned to support the new data provider architecture.
66-68: LGTM! Data provider registration logic is solid.The manager wiring and new data provider registration methods correctly handle:
- Explicit data provider services with existence checks
- Aggregator expansion via
getEntities()- ORM provider creation with proper Doctrine references and tagging
The pipeline failures (PHPMD warnings about
elseexpressions and static access) are style concerns rather than functional issues and can be addressed in a separate focused refactoring PR if desired.Also applies to: 93-136
tests/Unit/SerializationTest.php (1)
7-30: LGTM!The test has been correctly updated to use
SearchableObjectwith the new constructor signature and normalization options structure.src/Model/Aggregator.php (1)
12-14: LGTM! Configurable primary key implementation is correct.The Aggregator now correctly uses the configurable
$primaryKeyin thenormalizemethod (Line 82), replacing the hardcodedobjectID. The default parameter value ensures backward compatibility, and the strongly typed property improves type safety.Also applies to: 27-36, 82-82
src/Services/SettingsUpdater.php (1)
12-29: LGTM! Manager integration is correct.The constructor now correctly sources configuration from
SearchManagerInterface, aligning with the new architecture. The promoted properties clean up the boilerplate.The pipeline failure (cyclomatic complexity on the
update()method) relates to existing logic rather than these changes. Based on learnings, such refactoring can be deferred to a focused follow-up PR.src/SearchableObject.php (1)
13-80: LGTM! Clean implementation of the new SearchableObject.The class provides a clear API for wrapping objects with their index metadata and normalization context. The use of a local
$contextvariable ingetSearchableArray()(Line 67) correctly avoids mutating instance state, and the identifier property has appropriate typing. The implementation correctly handles bothNormalizableInterfaceobjects and standard normalization paths.src/Engine.php (4)
10-43: LGTM! Clear PHPStan type documentation.The added type aliases improve code documentation and enable better static analysis of return types.
46-90: LGTM! Per-index batching with correct primary key handling.The refactored
index()method correctly maintains a per-index batch structure (Line 78) with separateprimaryKeyanddocumentsarrays, ensuring each index uses its own primary key when callingaddDocuments()(Line 86). This addresses the previous critical issue where a single primary key was applied to all indexes.
100-126: LGTM! Remove logic correctly handles SearchableObject.The method correctly extracts identifiers per index and issues deletions appropriately.
174-181: LGTM! Simplified identifier normalization.The updated
normalizeId()method cleanly handlesStringableobjects by casting to string, removing unnecessarymethod_existschecks.src/Services/MeilisearchService.php (2)
24-26: LGTM! Deprecation bridge correctly configured.The class is properly annotated as deprecated with the correct namespace (
Services\MeilisearchManager), and the optional manager parameter enables gradual migration.Also applies to: 50-58
66-283: LGTM! Delegation pattern correctly implemented.All public methods correctly delegate to the manager when available while emitting appropriate deprecation notices. The fallback to existing logic ensures backward compatibility.
The pipeline failures (complexity, coupling, method count) reflect the overall class structure rather than issues with these specific changes. Since this class is deprecated and will eventually be removed, addressing these warnings now would provide limited long-term value.
src/Command/MeilisearchImportCommand.php (3)
8-36: LGTM! Constructor correctly wired with data provider registry.The updated constructor properly integrates
SearchManagerInterfaceandDataProviderRegistryInterface, enabling the command to work with the new data provider architecture.
79-97: LGTM! Configuration and searchability checks correctly use the manager.The command now sources configuration and performs searchability checks through the
SearchManagerInterface, aligning with the new architecture.
121-151: LGTM! Data provider integration correctly implemented.The import loop correctly:
- Obtains the data provider once before the loop (Line 121), avoiding repeated instantiation
- Guards against empty batches (Lines 126-130) and breaks early with cleanup
- Calls
cleanup()after each batch (Line 148) to manage resourcesThe pipeline failures (cyclomatic complexity and NPath complexity) reflect the overall
execute()method structure, which is typical for command classes. Per the maintainer's preference for focused PRs, such refactoring can be deferred to a separate PR dedicated to reducing command complexity.src/SearchManagerInterface.php (1)
1-107: LGTM! Well-designed interface with comprehensive type annotations.The SearchManagerInterface provides a clean, well-documented contract for the new manager-based architecture. Type annotations are precise,
@throwsdocumentation is complete, and the API surface appropriately covers indexing, removal, deletion, and search operations.src/Command/MeilisearchCreateCommand.php (1)
23-30: LGTM! Clean migration to SearchManagerInterface.The constructor refactor correctly adopts promoted properties and updates the dependency to use
SearchManagerInterface, aligning with the new manager-based architecture.src/Services/MeilisearchManager.php (1)
372-392: Let me analyze the provided code snippet carefully to verify the claim:Unable to verify the composite key format issue due to repository access failure.
The original review comment claims a critical format mismatch between
getSingleIdentifier()(lines 386-391) andsearch()(line 220), but the analysis is inconclusive:
- The review itself references a past review comment stating this was "Addressed in commit a224b01"
- The current code snippet shown (marked as new/current state) still displays the old format with key-value pairs
- Without access to line 220's actual implementation in
search(), the claimed mismatch cannot be independently verified- The review ends with `` tag, indicating the author's own uncertainty
To properly verify this claim, one would need to:
- Confirm the actual format produced by
getSingleIdentifier()for composite keys- Confirm how
search()at line 220 builds composite key lookups- Trace whether these formats are actually compared during search operations
- Review tests for composite-keyed entities to determine if search works
The evidence is contradictory: if commit a224b01 addressed this, the current code should reflect that fix, yet it appears to show the unfixed version. This requires manual investigation of the repository's current state and git history.
norkunas
force-pushed
the
dataprovider
branch
from
9262233 to
a89c8a2
Compare
norkunas
force-pushed
the
dataprovider
branch
from
a89c8a2 to
c8c7c69
Compare
3 participants
Pull Request
Early christmas present 🎄
I still left some todo's but wanted to open a draft at last.
Usage as previously (for forward compatibility provider type is 'orm' by default, just that ORM must be an explicit dependency of the application now):
Usage with custom data provider:
If you have a meilisearch index with different primary key than
objectID, you can now configure it:Deprecations & Upgrade path⚠️
Using
Meilisearch\Bundle\SearchServiceandMeilisearch\Bundle\Service\MeilisearchServiceis now deprecated.Switch to
Meilisearch\Bundle\SearchManagerInterfaceandMeilisearch\Bundle\Service\MeilisearchManagerPreviously methods were accepting doctrine's
ObjectManager:Now it does not ask for
ObjectManageranymore:What's next?
Now that we bump PHP to 8.1, we can later introduce PHP attributes for meilisearch, so instead of configuring everything through yaml, configuration can be linked in the classes (related issue #387).
We should introduce
SearchResultsthat allows to access/iterate found entities, while also exposingfacetResultsand other fields (related issue #353)Related issue
Fixes #66
Fixes #121
Fixes #134
Fixes #302
Replaces #345
What does this PR do?
PR checklist
Please check if your PR fulfills the following requirements:
cc @remigarcia
cc @tacman in case you are interested
cc @ToshY
Summary by CodeRabbit
New Features
Bug Fixes
Chores
Style / Docs
✏️ Tip: You can customize this high-level summary in your review settings.