Made CriteriaConverter capable of using lazy constructors #643
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR refactors the CriteriaConverter constructor to use Symfony's tagged iterator functionality instead of compiler passes for dependency injection. This change addresses circular dependency issues and enables lazy loading of criterion handlers.
Key changes:
- Replaced compiler pass-based handler registration with tagged iterator injection
- Modified
CriteriaConverterclasses to accept iterable handlers instead of arrays - Deprecated
addHandler()methods in favor of service definition tags
Reviewed Changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
src/lib/Search/Legacy/Content/Common/Gateway/CriteriaConverter.php |
Updated constructor to accept iterable handlers and deprecated addHandler() method |
src/lib/Persistence/Legacy/URL/Query/CriteriaConverter.php |
Similar updates to constructor and addHandler() method deprecation |
src/lib/Resources/settings/search_engines/legacy/criterion_handlers_content.yml |
Added tagged iterator argument for content criterion handlers |
src/lib/Resources/settings/search_engines/legacy/criterion_handlers_location.yml |
Added tagged iterator argument for location criterion handlers |
src/lib/Resources/settings/storage_engines/legacy/trash.yml |
Added tagged iterator argument for trash criterion handlers |
src/lib/Resources/settings/storage_engines/legacy/url.yml |
Added tagged iterator argument for URL criterion handlers |
src/lib/Base/Container/Compiler/Search/Legacy/CriteriaConverterPass.php |
Removed entire compiler pass class as it's no longer needed |
tests/lib/Base/Container/Compiler/Search/Legacy/CriteriaConverterPassTest.php |
Removed test file for deleted compiler pass |
src/bundle/LegacySearchEngine/IbexaLegacySearchEngineBundle.php |
Removed registration of deprecated compiler pass |
tests/integration/Core/LegacyTestContainerBuilder.php |
Removed compiler pass registration from test container |
tests/lib/Persistence/Legacy/HandlerTest.php |
Refactored to use null coalescing assignment operator |
phpstan-baseline.neon |
Removed PHPStan baseline entry for deleted class |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| if ($this->handlers instanceof Traversable) { | ||
| $this->handlers = iterator_to_array($this->handlers); | ||
| } |
There was a problem hiding this comment.
Converting a Traversable to array using iterator_to_array() may cause performance issues if the iterator is large or expensive to iterate. Consider checking if this conversion is necessary or if the code can work with the iterator directly.
Copilot uses AI. Check for mistakes.
| if ($this->handlers instanceof Traversable) { | ||
| $this->handlers = iterator_to_array($this->handlers); | ||
| } |
There was a problem hiding this comment.
Converting a Traversable to array using iterator_to_array() may cause performance issues if the iterator is large or expensive to iterate. Consider checking if this conversion is necessary or if the code can work with the iterator directly.
Copilot uses AI. Check for mistakes.
Steveb-p
force-pushed
the
rfc/lazy-criteria-converter-constructor
branch
from
89b5d72 to
a5fb038
Compare
|
Description:
This PR changes
CriteriaConverterconstructor to facilitate usage of tagged iterator lists, which can be lazy in Symfony.Note
This allows all converters to no longer need to be
lazy: true.When working on addressing the early initialization issue, it turned out that one of the handlers -
Ibexa\Core\Search\Legacy\Content\Common\Gateway\CriterionHandler\FieldEmpty- contains a circular reference through multiple services.Since it depends on
FieldTypeService, it indirectly depends onCriteriaConverter, and then on itself.Container was calling the following chaing of services, resulting in infinite recursion under certain conditions:
Ibexa\Core\Repository\FieldTypeServiceIbexa\Core\Repository\RepositoryIbexa\Core\Search\Legacy\Content\Handleribexa.search.legacy.gateway.criteria_converter.contentIbexa\Core\Search\Legacy\Content\Common\Gateway\CriterionHandler\FieldEmptyibexa.api.service.field_typeIbexa\Core\Repository\FieldTypeServiceetc.
Because
!tagged_iteratorresults inTaggedIteratorArgumentdefinition, which ends up asRewindableGeneratoras service argument, this allowsCriteriaConverterto only initialize it's dependencies once they are actually being used.For QA:
Documentation: