[Maintenance] Fix ORM deprecations#17657
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughThis pull request refactors method signatures and dependency injections across many bundles. It replaces concrete repository and expression types with their more generic interface or specific types (e.g., OrderItemRepositoryInterface, RepositoryInterface) and updates Doctrine DQL parsing (switching from Lexer to TokenType). Several tests and query builder expressions now include explicit parameter settings and return types. Additionally, event listener imports have been shifted from the ORM to the Persistence namespace, and minor annotations and constructor properties have been adjusted for clarity and type safety. Changes
Sequence Diagram(s)Suggested reviewers
Poem
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
❌ Preview Environment deleted from BunnyshellAvailable commands:
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (5)
src/Sylius/Bundle/CoreBundle/spec/Doctrine/ORM/Handler/ResourceDeleteHandlerSpec.php (1)
19-19: Remove unused import.The
QueryExceptionis imported but never used in this file.-use Doctrine\ORM\Query\QueryException;src/Sylius/Bundle/ProductBundle/Doctrine/ORM/Query/AST/Function/AbstractJsonFunctionNode.php (1)
21-21: Consider removing unused Lexer import.The Lexer class is still imported but only used as a type hint in the
matchStringLiteralmethod parameter. Consider updating the parameter type to use TokenIterator instead.-use Doctrine\ORM\Query\Lexer; +use Doctrine\ORM\Query\TokenIterator; -private function matchStringLiteral(Parser $parser, Lexer $lexer): Literal +private function matchStringLiteral(Parser $parser, TokenIterator $lexer): LiteralAlso applies to: 147-147
src/Sylius/Bundle/ReviewBundle/EventListener/ReviewChangeListener.php (1)
21-21: Inconsistent readonly usage.The class is marked as
readonlybut the constructor parameter isn't. For consistency, the parameter should also bereadonlysince it's only set during construction and never modified.- public function __construct(private ReviewableRatingUpdaterInterface $averageRatingUpdater) + public function __construct(private readonly ReviewableRatingUpdaterInterface $averageRatingUpdater)Also applies to: 23-23
src/Sylius/Bundle/ProductBundle/spec/EventListener/SelectProductAttributeChoiceRemoveListenerSpec.php (1)
56-57: Consider using a mock repository instead of direct instantiation.Direct instantiation of ProductAttributeValueRepository makes the test more brittle and couples it to the concrete implementation. Consider using a mock repository instead:
-$productAttributeValueRepository = new ProductAttributeValueRepository($entityManager->getWrappedObject(), new ClassMetadata('Sylius\Component\Product\Model\ProductAttributeValue')); +$productAttributeValueRepository = $this->prophesize(ProductAttributeValueRepositoryInterface::class); +$entityManager->getRepository('Sylius\Component\Product\Model\ProductAttributeValue')->willReturn($productAttributeValueRepository->reveal());src/Sylius/Bundle/OrderBundle/spec/NumberGenerator/SequentialOrderNumberGeneratorSpec.php (1)
20-20: LGTM! Good move towards interface-based programming.Replacing concrete
EntityRepositorywithRepositoryInterfaceimproves code flexibility and follows dependency inversion principle.This change makes the code more maintainable by:
- Reducing coupling to concrete implementations
- Making it easier to swap repository implementations
- Improving testability through better abstraction
Also applies to: 25-25, 36-36, 49-49
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (39)
src/Sylius/Bundle/ApiBundle/spec/CommandHandler/Cart/RemoveItemFromCartHandlerSpec.php(4 hunks)src/Sylius/Bundle/ApiBundle/spec/Doctrine/ORM/QueryExtension/Common/NonArchivedExtensionSpec.php(1 hunks)src/Sylius/Bundle/ApiBundle/spec/Doctrine/ORM/QueryExtension/Shop/Address/ShopUserBasedExtensionSpec.php(2 hunks)src/Sylius/Bundle/ApiBundle/spec/Doctrine/ORM/QueryExtension/Shop/Currency/ChannelBasedExtensionSpec.php(4 hunks)src/Sylius/Bundle/ApiBundle/spec/Doctrine/ORM/QueryExtension/Shop/Customer/ShopUserBasedExtensionSpec.php(4 hunks)src/Sylius/Bundle/ApiBundle/spec/Doctrine/ORM/QueryExtension/Shop/ExchangeRate/ChannelBasedExtensionSpec.php(2 hunks)src/Sylius/Bundle/ApiBundle/spec/Doctrine/ORM/QueryExtension/Shop/Order/ShopUserBasedExtensionSpec.php(3 hunks)src/Sylius/Bundle/ApiBundle/spec/Doctrine/ORM/QueryExtension/Shop/Order/StateBasedExtensionSpec.php(2 hunks)src/Sylius/Bundle/ApiBundle/spec/Doctrine/ORM/QueryExtension/Shop/Order/VisitorBasedExtensionSpec.php(2 hunks)src/Sylius/Bundle/ApiBundle/spec/Doctrine/ORM/QueryExtension/Shop/OrderItem/ShopUserBasedExtensionSpec.php(4 hunks)src/Sylius/Bundle/ApiBundle/spec/Doctrine/ORM/QueryExtension/Shop/OrderItem/VisitorBasedExtensionSpec.php(4 hunks)src/Sylius/Bundle/ApiBundle/spec/Doctrine/ORM/QueryExtension/Shop/Product/TaxonBasedExtensionSpec.php(2 hunks)src/Sylius/Bundle/ApiBundle/spec/Doctrine/ORM/QueryExtension/Shop/ProductReview/AcceptedExtensionSpec.php(1 hunks)src/Sylius/Bundle/ApiBundle/spec/Doctrine/ORM/QueryExtension/Shop/Taxon/ChannelBasedExtensionSpec.php(1 hunks)src/Sylius/Bundle/ApiBundle/spec/Doctrine/ORM/QueryExtension/Shop/Taxon/EnabledChildrenExtensionSpec.php(1 hunks)src/Sylius/Bundle/ApiBundle/spec/Resolver/UriTemplateParentResourceResolverSpec.php(4 hunks)src/Sylius/Bundle/ApiBundle/spec/StateProvider/Shop/Product/ProductAttributeValue/CollectionProviderSpec.php(2 hunks)src/Sylius/Bundle/AttributeBundle/Doctrine/ORM/Subscriber/LoadMetadataSubscriber.php(3 hunks)src/Sylius/Bundle/AttributeBundle/spec/Doctrine/ORM/Subscriber/LoadMetadataSubscriberSpec.php(5 hunks)src/Sylius/Bundle/CoreBundle/Doctrine/DQL/DateFormat.php(1 hunks)src/Sylius/Bundle/CoreBundle/Doctrine/DQL/Day.php(2 hunks)src/Sylius/Bundle/CoreBundle/Doctrine/DQL/Hour.php(2 hunks)src/Sylius/Bundle/CoreBundle/Doctrine/DQL/Month.php(2 hunks)src/Sylius/Bundle/CoreBundle/Doctrine/DQL/Week.php(2 hunks)src/Sylius/Bundle/CoreBundle/Doctrine/DQL/Year.php(2 hunks)src/Sylius/Bundle/CoreBundle/Doctrine/ORM/Handler/ResourceDeleteHandler.php(1 hunks)src/Sylius/Bundle/CoreBundle/Doctrine/ORM/SqlWalker/OrderByIdentifierSqlWalker.php(1 hunks)src/Sylius/Bundle/CoreBundle/Remover/ReviewerReviewsRemover.php(2 hunks)src/Sylius/Bundle/CoreBundle/spec/Doctrine/ORM/Handler/ResourceDeleteHandlerSpec.php(2 hunks)src/Sylius/Bundle/CoreBundle/spec/EventListener/CanonicalizerListenerSpec.php(1 hunks)src/Sylius/Bundle/CoreBundle/spec/Order/NumberGenerator/SequentialOrderNumberGeneratorSpec.php(3 hunks)src/Sylius/Bundle/CoreBundle/spec/Remover/ReviewerReviewsRemoverSpec.php(1 hunks)src/Sylius/Bundle/LocaleBundle/spec/Checker/LocaleUsageCheckerSpec.php(3 hunks)src/Sylius/Bundle/OrderBundle/spec/NumberGenerator/SequentialOrderNumberGeneratorSpec.php(3 hunks)src/Sylius/Bundle/ProductBundle/Doctrine/ORM/Query/AST/Function/AbstractJsonFunctionNode.php(6 hunks)src/Sylius/Bundle/ProductBundle/spec/EventListener/SelectProductAttributeChoiceRemoveListenerSpec.php(3 hunks)src/Sylius/Bundle/ReviewBundle/EventListener/ReviewChangeListener.php(1 hunks)src/Sylius/Bundle/ReviewBundle/spec/EventListener/ReviewChangeListenerSpec.php(1 hunks)src/Sylius/Bundle/UserBundle/spec/EventListener/PasswordUpdaterListenerSpec.php(1 hunks)
✅ Files skipped from review due to trivial changes (2)
- src/Sylius/Bundle/CoreBundle/Doctrine/ORM/Handler/ResourceDeleteHandler.php
- src/Sylius/Bundle/CoreBundle/spec/EventListener/CanonicalizerListenerSpec.php
🔇 Additional comments (55)
src/Sylius/Bundle/ApiBundle/spec/Doctrine/ORM/QueryExtension/Shop/Order/StateBasedExtensionSpec.php (2)
69-69: LGTM! More precise type hint for neq comparison.The change from
Expr\FunctoExpr\Comparisonbetter reflects the actual return type of Doctrine'sneq()method.
127-127: LGTM! More precise type hint for eq comparison.The change from
Expr\FunctoExpr\Comparisonbetter reflects the actual return type of Doctrine'seq()method.src/Sylius/Bundle/CoreBundle/spec/Doctrine/ORM/Handler/ResourceDeleteHandlerSpec.php (1)
71-74: LGTM! Good improvement to exception testing.The changes improve the test by using a concrete exception instance instead of a class reference, which better reflects real-world scenarios and follows PHPSpec best practices.
src/Sylius/Bundle/LocaleBundle/spec/Checker/LocaleUsageCheckerSpec.php (3)
17-19: LGTM: New imports align with ORM refactoring.The added Doctrine ORM imports support the transition to using EntityPersister, which is part of the broader ORM deprecation fixes.
55-63: LGTM: Improved test setup using EntityPersister.The refactoring properly transitions from direct repository mocking to using EntityPersister, which better reflects actual Doctrine ORM behavior. The setup is well-structured and maintains test integrity while addressing ORM deprecations.
90-98: LGTM: Consistent test implementation for negative case.The negative test case maintains consistency with the positive case's approach, properly using EntityPersister for the ORM interaction.
src/Sylius/Bundle/ProductBundle/Doctrine/ORM/Query/AST/Function/AbstractJsonFunctionNode.php (7)
25-25: LGTM: TokenType import added.The addition of the TokenType import aligns with the migration from Lexer token types to TokenType constants.
59-60: LGTM: Updated token type constants in parse method.The token type constants have been correctly updated from Lexer to TokenType for identifier and parenthesis matching.
Also applies to: 68-68
76-76: LGTM: Updated token type constants in parseOptionalArguments method.Token type checks have been properly updated to use TokenType constants for close parenthesis and comma tokens.
Also applies to: 79-79
92-92: LGTM: Updated token type constant in parseArguments method.The comma token type has been correctly updated to use TokenType constant.
117-117: LGTM: Updated token type constant in parseStringLiteral method.The string token type check has been properly updated to use TokenType constant.
133-139: LGTM: Updated token type constants in parseAlphaNumericLiteral method.Token type constants have been correctly updated for string, integer, and float tokens in both the switch case and match statement.
149-149: LGTM: Updated token type constant in matchStringLiteral method.The string token type has been properly updated to use TokenType constant.
src/Sylius/Bundle/ReviewBundle/spec/EventListener/ReviewChangeListenerSpec.php (1)
18-18: LGTM! Good migration to Persistence events.The change to use
Doctrine\Persistence\Event\LifecycleEventArgsaligns with best practices for broader compatibility while correctly maintaining ORM-specific events where needed.src/Sylius/Bundle/UserBundle/spec/EventListener/PasswordUpdaterListenerSpec.php (1)
16-16: LGTM! Appropriate use of Persistence events.The change to use
Doctrine\Persistence\Event\LifecycleEventArgsis correct and maintains compatibility with the test scenarios.src/Sylius/Bundle/AttributeBundle/Doctrine/ORM/Subscriber/LoadMetadataSubscriber.php (3)
18-18: LGTM: Import updated to use ClassMetadata.The import statement has been correctly updated to use
ClassMetadatainstead ofClassMetadataInfoas part of the ORM deprecation fixes.
50-51: LGTM: Method signature and PHPDoc updated to use ClassMetadata.The method signature and PHPDoc annotation have been correctly updated to use
ClassMetadatainstead ofClassMetadataInfo.Also applies to: 57-57
76-77: LGTM: Method signature and PHPDoc updated to use ClassMetadata.The method signature and PHPDoc annotation have been correctly updated to use
ClassMetadatainstead ofClassMetadataInfo.Also applies to: 83-83
src/Sylius/Bundle/AttributeBundle/spec/Doctrine/ORM/Subscriber/LoadMetadataSubscriberSpec.php (3)
19-19: LGTM: Import updated to use ClassMetadata.The import statement has been correctly updated to use
ClassMetadatainstead ofClassMetadataInfoas part of the ORM deprecation fixes.
63-67: LGTM: Test method signature and field mappings updated to use ClassMetadata.The test method signature and field mappings have been correctly updated to use
ClassMetadatainstead ofClassMetadataInfo. The field mappings structure remains consistent with the expected format.Also applies to: 71-77
113-113: LGTM: Test method signatures updated to use ClassMetadata.The remaining test method signatures have been correctly updated to use
ClassMetadatainstead ofClassMetadataInfo.Also applies to: 132-132, 149-149
src/Sylius/Bundle/CoreBundle/spec/Remover/ReviewerReviewsRemoverSpec.php (2)
20-20: LGTM! Import statement added for the new interface type.The import statement for
RepositoryInterfacefrom the Resource component is correctly added.
27-33: LGTM! Repository type updated to use interface.The repository type has been correctly updated from
EntityRepositorytoRepositoryInterface, aligning with the PR's objective of fixing ORM deprecations. The test behavior remains unchanged.src/Sylius/Bundle/CoreBundle/Remover/ReviewerReviewsRemover.php (3)
21-21: LGTM! Import statement updated to use new namespace.The import statement for
RepositoryInterfacehas been correctly updated to use the new namespace from the Resource component.
25-30: LGTM! Repository type updated with proper generic type hint.The changes improve type safety by:
- Adding PHPDoc to specify that the repository handles
ReviewInterfaceentities- Updating the repository type to use
RepositoryInterface
39-40: LGTM! Type hint added for review variable.The type hint
@var ReviewInterface $reviewhas been correctly added to improve type safety in the foreach loop.src/Sylius/Bundle/ProductBundle/spec/EventListener/SelectProductAttributeChoiceRemoveListenerSpec.php (3)
16-29: LGTM! Import changes align with ORM deprecation fixes.The switch from ORM to Persistence namespace for LifecycleEventArgs and addition of necessary Doctrine DBAL/ORM imports are appropriate for addressing ORM deprecations.
42-50: LGTM! QueryBuilder approach improves type safety.The updated method signature with QueryBuilder, Connection, and Query parameters aligns with modern Doctrine practices and provides better type safety.
80-88: LGTM! QueryBuilder setup follows best practices.The QueryBuilder setup with proper parameter binding and mocking of all necessary methods follows Doctrine best practices and ensures secure query execution.
src/Sylius/Bundle/CoreBundle/Doctrine/DQL/DateFormat.php (1)
22-22: LGTM! Well-structured ORM deprecation fixes.The changes correctly:
- Replace deprecated Lexer with TokenType for token matching
- Add explicit return types for better type safety
- Maintain consistent token matching behavior
Also applies to: 30-38, 40-40
src/Sylius/Bundle/CoreBundle/Doctrine/DQL/Day.php (1)
23-23: LGTM! Consistent ORM deprecation fixes.The changes correctly replace deprecated Lexer constants with TokenType while preserving the platform-specific SQL generation logic.
Also applies to: 32-38
src/Sylius/Bundle/CoreBundle/Doctrine/DQL/Year.php (1)
23-23: LGTM! Consistent with other DQL function updates.The changes follow the same pattern of replacing deprecated Lexer constants with TokenType, maintaining consistency across the DQL function implementations.
Also applies to: 32-37
src/Sylius/Bundle/CoreBundle/Doctrine/DQL/Week.php (1)
23-23: LGTM! Completes the consistent DQL function updates.The changes maintain the systematic pattern of replacing deprecated Lexer constants with TokenType across all DQL functions.
Also applies to: 32-37
src/Sylius/Bundle/CoreBundle/Doctrine/DQL/Month.php (1)
23-23: LGTM! Consistent with Hour.php changes.The TokenType migration follows the same pattern as Hour.php, maintaining consistency across DQL functions.
Also applies to: 32-33, 37-37
src/Sylius/Bundle/ApiBundle/spec/Doctrine/ORM/QueryExtension/Common/NonArchivedExtensionSpec.php (1)
66-66: LGTM! Enhanced test specification.The change properly specifies that
andWhere()returns the QueryBuilder instance, better reflecting the actual behavior and enabling method chaining in the implementation.src/Sylius/Bundle/CoreBundle/spec/Order/NumberGenerator/SequentialOrderNumberGeneratorSpec.php (1)
22-22: LGTM! Good use of interface instead of concrete class.The change from
EntityRepositorytoRepositoryInterfaceacross all method signatures is a good practice that reduces coupling and increases flexibility.Also applies to: 28-28, 41-41, 58-58
src/Sylius/Bundle/ApiBundle/spec/Doctrine/ORM/QueryExtension/Shop/Taxon/EnabledChildrenExtensionSpec.php (1)
67-69: LGTM! Proper method chaining in query builder specs.The addition of
willReturn($queryBuilder)to the method chain correctly reflects Doctrine's fluent interface pattern and ensures proper testing of query builder behavior.src/Sylius/Bundle/ApiBundle/spec/Doctrine/ORM/QueryExtension/Shop/ProductReview/AcceptedExtensionSpec.php (1)
73-73: LGTM! Consistent query builder method chaining.The addition of
willReturn($queryBuilder)tosetParameter()maintains consistency with Doctrine's fluent interface pattern.src/Sylius/Bundle/CoreBundle/Doctrine/ORM/SqlWalker/OrderByIdentifierSqlWalker.php (1)
26-26: LGTM! Good addition of return type declaration.Adding the
: stringreturn type towalkSelectStatementimproves type safety and aligns with modern PHP coding practices.src/Sylius/Bundle/ApiBundle/spec/CommandHandler/Cart/RemoveItemFromCartHandlerSpec.php (1)
22-22: LGTM! Good use of interface instead of concrete implementation.Replacing
OrderItemRepositorywithOrderItemRepositoryInterfaceimproves code flexibility and follows the dependency inversion principle.Also applies to: 30-30, 42-42, 62-62, 82-82
src/Sylius/Bundle/ApiBundle/spec/Doctrine/ORM/QueryExtension/Shop/Order/VisitorBasedExtensionSpec.php (1)
85-87: LGTM! Improved expression type specificity.The changes enhance type safety and clarity by:
- Using specific expression types (Comparison, Andx, Orx) instead of generic Expr
- Using explicit string SQL conditions for better readability
Also applies to: 100-106
src/Sylius/Bundle/ApiBundle/spec/Doctrine/ORM/QueryExtension/Shop/Product/TaxonBasedExtensionSpec.php (1)
85-85: LGTM! Updated expression types for query builder.The changes correctly update the expression types to match Doctrine ORM's type system:
- Using Expr\Func for 'in' expressions
- Updating andX method parameter types
Also applies to: 102-103
src/Sylius/Bundle/ApiBundle/spec/Doctrine/ORM/QueryExtension/Shop/Taxon/ChannelBasedExtensionSpec.php (1)
95-95: LGTM! Corrected getRootAliases return type.Updated getRootAliases() to return an array ['o'] instead of a string 'o', which aligns with Doctrine ORM's expected return type.
src/Sylius/Bundle/ApiBundle/spec/Resolver/UriTemplateParentResourceResolverSpec.php (3)
19-21: LGTM: ORM imports added for explicit entity management.The addition of ORM-specific imports enhances type safety and clarity in the tests.
125-126: Enhance test coverage for entity loading.The switch from
findOneBytoloadwith EntityPersister provides more granular control but should be verified for edge cases.Consider adding test cases for:
- Invalid entity persister configurations
- Edge cases in load parameters
100-106: Verify the repository initialization approach.The test now uses a more explicit approach with EntityRepository, UnitOfWork, and EntityPersister. While this provides better control over entity management, ensure this matches the actual implementation's behavior.
✅ Verification successful
🏁 Script executed:
#!/bin/bash # Check if the implementation matches the test setup ast-grep --pattern 'class UriTemplateParentResourceResolver { $$$ public function __construct( $$$ ) { $$$ } $$$ }'Length of output: 239
I'm going to run a more targeted search on the implementation to verify that it matches the repository initialization from the test. Let's check for the explicit instantiation with EntityRepository in the UriTemplateParentResourceResolver class.
🏁 Script executed:
#!/bin/bash # Locate the implementation file (if any) for UriTemplateParentResourceResolver. fd "UriTemplateParentResourceResolver.php" # Search for repository initialization in the implementation file. rg "new\s+EntityRepository\s*\(" src/Sylius/Bundle/ApiBundle/Resolver/UriTemplateParentResourceResolver.phpLength of output: 227
Repository initialization approach appears valid.
The implementation for UriTemplateParentResourceResolver does not instantiate an EntityRepository directly. Instead, it relies on the entity manager’s getRepository method to resolve repositories—which is the expected Doctrine behavior. The test’s explicit instantiation of a repository with new EntityRepository (with an appropriate ClassMetadata) adequately mirrors this behavior for isolated testing.src/Sylius/Bundle/ApiBundle/spec/Doctrine/ORM/QueryExtension/Shop/Customer/ShopUserBasedExtensionSpec.php (1)
71-71: LGTM: Improved type safety in query expressions.The change from
Expr\FunctoExpr\Comparisonprovides better type safety and clarity in the query building process.Also applies to: 81-82
src/Sylius/Bundle/ApiBundle/spec/StateProvider/Shop/Product/ProductAttributeValue/CollectionProviderSpec.php (1)
21-22: LGTM: Enhanced query type specificity.Replacing AbstractQuery with concrete Query class improves type safety without affecting functionality.
src/Sylius/Bundle/ApiBundle/spec/Doctrine/ORM/QueryExtension/Shop/Address/ShopUserBasedExtensionSpec.php (1)
150-151: LGTM: Enhanced parameter handling in query builder.The addition of explicit parameter setting improves the clarity and reliability of the query building process.
Also applies to: 179-180
src/Sylius/Bundle/ApiBundle/spec/Doctrine/ORM/QueryExtension/Shop/ExchangeRate/ChannelBasedExtensionSpec.php (1)
143-143: LGTM! Proper parameter binding added.The addition of
setParametercalls for currency values improves security by preventing SQL injection and follows Doctrine's best practices.Also applies to: 180-180
src/Sylius/Bundle/ApiBundle/spec/Doctrine/ORM/QueryExtension/Shop/Currency/ChannelBasedExtensionSpec.php (1)
130-131: LGTM! Correct expression type for IN clause.The change to
Expr\Funcis appropriate for the IN clause, and the parameter binding for the currencies array is properly implemented.Also applies to: 142-145, 169-170, 181-184
src/Sylius/Bundle/ApiBundle/spec/Doctrine/ORM/QueryExtension/Shop/Order/ShopUserBasedExtensionSpec.php (1)
103-104: LGTM! Correct expression type for equality checks.The change to
Expr\Comparisonis appropriate for equality checks usingeq(), aligning with Doctrine's expression system.Also applies to: 113-115, 183-184, 193-195
src/Sylius/Bundle/ApiBundle/spec/Doctrine/ORM/QueryExtension/Shop/OrderItem/ShopUserBasedExtensionSpec.php (1)
104-105: LGTM! Consistent expression type usage.The changes maintain consistency with other extensions by using
Expr\Comparisonfor equality checks, following the same pattern as in Order/ShopUserBasedExtensionSpec.php.Also applies to: 116-118, 187-188, 199-201
src/Sylius/Bundle/ApiBundle/spec/Doctrine/ORM/QueryExtension/Shop/OrderItem/VisitorBasedExtensionSpec.php (2)
86-88: LGTM! Good improvement in type safety.The change from generic
Expr\Functo specific expression types (Comparison,Andx,Orx) makes the code more explicit about the types of expressions being used. This aligns well with the PR's objective of fixing ORM deprecations.Also applies to: 164-166
103-110: LGTM! Improved expression handling.The changes improve readability and maintainability by:
- Using direct string conditions for IS NULL/IS NOT NULL checks
- Explicitly constructing AND/OR conditions with specific expression types
- Maintaining the same logical conditions while using updated Doctrine ORM APIs
Also applies to: 181-188
| use Doctrine\ORM\Query\Lexer; | ||
| use Doctrine\ORM\Query\Parser; | ||
| use Doctrine\ORM\Query\SqlWalker; | ||
| use Doctrine\ORM\Query\TokenType; |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
LGTM! Proper migration from Lexer to TokenType.
The changes correctly replace deprecated Lexer constants with TokenType constants for token parsing, maintaining the same functionality while using the new recommended approach.
Let's verify if there are any remaining usages of the deprecated Lexer class in DQL functions:
Also applies to: 32-33, 37-37
🏁 Script executed:
#!/bin/bash
# Search for any remaining Lexer usages in DQL functions
rg -l "use.*Lexer" "src/Sylius/Bundle/CoreBundle/Doctrine/DQL/"Length of output: 120
Action Required: Update DateFormat.php to Use TokenType
While the changes in Hour.php correctly migrate to TokenType, our search revealed that "src/Sylius/Bundle/CoreBundle/Doctrine/DQL/DateFormat.php" still imports or uses the deprecated Lexer. Please update DateFormat.php accordingly to ensure all DQL functions align with the new token parsing approach.
4b9374a to
74c6f8c
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/Sylius/Bundle/ProductBundle/spec/EventListener/SelectProductAttributeChoiceRemoveListenerSpec.php (1)
56-57: Consider using mock objects for better unit testing.Creating a real repository instance in a unit test might make the test more brittle. Consider using mock objects for better isolation.
- $productAttributeValueRepository = new ProductAttributeValueRepository($entityManager->getWrappedObject(), new ClassMetadata('Sylius\Component\Product\Model\ProductAttributeValue')); + $productAttributeValueRepository = $entityManager->getRepository('Sylius\Component\Product\Model\ProductAttributeValue');src/Sylius/Bundle/ApiBundle/spec/Resolver/UriTemplateParentResourceResolverSpec.php (1)
132-133: Remove commented out code.The commented out code is no longer needed as:
- The changes are tracked in version control
- It can become outdated and misleading
- It adds unnecessary noise to the codebase
- // $entityManager->getRepository(get_class($parentItem))->willReturn($repository); - //$repository->findOneBy(['code' => 'value'])->willReturn($parentItem);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (39)
src/Sylius/Bundle/ApiBundle/spec/CommandHandler/Cart/RemoveItemFromCartHandlerSpec.php(4 hunks)src/Sylius/Bundle/ApiBundle/spec/Doctrine/ORM/QueryExtension/Common/NonArchivedExtensionSpec.php(1 hunks)src/Sylius/Bundle/ApiBundle/spec/Doctrine/ORM/QueryExtension/Shop/Address/ShopUserBasedExtensionSpec.php(2 hunks)src/Sylius/Bundle/ApiBundle/spec/Doctrine/ORM/QueryExtension/Shop/Currency/ChannelBasedExtensionSpec.php(4 hunks)src/Sylius/Bundle/ApiBundle/spec/Doctrine/ORM/QueryExtension/Shop/Customer/ShopUserBasedExtensionSpec.php(4 hunks)src/Sylius/Bundle/ApiBundle/spec/Doctrine/ORM/QueryExtension/Shop/ExchangeRate/ChannelBasedExtensionSpec.php(2 hunks)src/Sylius/Bundle/ApiBundle/spec/Doctrine/ORM/QueryExtension/Shop/Order/ShopUserBasedExtensionSpec.php(3 hunks)src/Sylius/Bundle/ApiBundle/spec/Doctrine/ORM/QueryExtension/Shop/Order/StateBasedExtensionSpec.php(2 hunks)src/Sylius/Bundle/ApiBundle/spec/Doctrine/ORM/QueryExtension/Shop/Order/VisitorBasedExtensionSpec.php(2 hunks)src/Sylius/Bundle/ApiBundle/spec/Doctrine/ORM/QueryExtension/Shop/OrderItem/ShopUserBasedExtensionSpec.php(4 hunks)src/Sylius/Bundle/ApiBundle/spec/Doctrine/ORM/QueryExtension/Shop/OrderItem/VisitorBasedExtensionSpec.php(4 hunks)src/Sylius/Bundle/ApiBundle/spec/Doctrine/ORM/QueryExtension/Shop/Product/TaxonBasedExtensionSpec.php(2 hunks)src/Sylius/Bundle/ApiBundle/spec/Doctrine/ORM/QueryExtension/Shop/ProductReview/AcceptedExtensionSpec.php(1 hunks)src/Sylius/Bundle/ApiBundle/spec/Doctrine/ORM/QueryExtension/Shop/Taxon/ChannelBasedExtensionSpec.php(1 hunks)src/Sylius/Bundle/ApiBundle/spec/Doctrine/ORM/QueryExtension/Shop/Taxon/EnabledChildrenExtensionSpec.php(1 hunks)src/Sylius/Bundle/ApiBundle/spec/Resolver/UriTemplateParentResourceResolverSpec.php(4 hunks)src/Sylius/Bundle/ApiBundle/spec/StateProvider/Shop/Product/ProductAttributeValue/CollectionProviderSpec.php(2 hunks)src/Sylius/Bundle/AttributeBundle/Doctrine/ORM/Subscriber/LoadMetadataSubscriber.php(3 hunks)src/Sylius/Bundle/AttributeBundle/spec/Doctrine/ORM/Subscriber/LoadMetadataSubscriberSpec.php(5 hunks)src/Sylius/Bundle/CoreBundle/Doctrine/DQL/DateFormat.php(1 hunks)src/Sylius/Bundle/CoreBundle/Doctrine/DQL/Day.php(2 hunks)src/Sylius/Bundle/CoreBundle/Doctrine/DQL/Hour.php(2 hunks)src/Sylius/Bundle/CoreBundle/Doctrine/DQL/Month.php(2 hunks)src/Sylius/Bundle/CoreBundle/Doctrine/DQL/Week.php(2 hunks)src/Sylius/Bundle/CoreBundle/Doctrine/DQL/Year.php(2 hunks)src/Sylius/Bundle/CoreBundle/Doctrine/ORM/Handler/ResourceDeleteHandler.php(1 hunks)src/Sylius/Bundle/CoreBundle/Doctrine/ORM/SqlWalker/OrderByIdentifierSqlWalker.php(1 hunks)src/Sylius/Bundle/CoreBundle/Remover/ReviewerReviewsRemover.php(2 hunks)src/Sylius/Bundle/CoreBundle/spec/Doctrine/ORM/Handler/ResourceDeleteHandlerSpec.php(2 hunks)src/Sylius/Bundle/CoreBundle/spec/EventListener/CanonicalizerListenerSpec.php(1 hunks)src/Sylius/Bundle/CoreBundle/spec/Order/NumberGenerator/SequentialOrderNumberGeneratorSpec.php(3 hunks)src/Sylius/Bundle/CoreBundle/spec/Remover/ReviewerReviewsRemoverSpec.php(1 hunks)src/Sylius/Bundle/LocaleBundle/spec/Checker/LocaleUsageCheckerSpec.php(3 hunks)src/Sylius/Bundle/OrderBundle/spec/NumberGenerator/SequentialOrderNumberGeneratorSpec.php(3 hunks)src/Sylius/Bundle/ProductBundle/Doctrine/ORM/Query/AST/Function/AbstractJsonFunctionNode.php(6 hunks)src/Sylius/Bundle/ProductBundle/spec/EventListener/SelectProductAttributeChoiceRemoveListenerSpec.php(3 hunks)src/Sylius/Bundle/ReviewBundle/EventListener/ReviewChangeListener.php(1 hunks)src/Sylius/Bundle/ReviewBundle/spec/EventListener/ReviewChangeListenerSpec.php(1 hunks)src/Sylius/Bundle/UserBundle/spec/EventListener/PasswordUpdaterListenerSpec.php(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (34)
- src/Sylius/Bundle/ApiBundle/spec/Doctrine/ORM/QueryExtension/Shop/Taxon/ChannelBasedExtensionSpec.php
- src/Sylius/Bundle/ApiBundle/spec/Doctrine/ORM/QueryExtension/Shop/Taxon/EnabledChildrenExtensionSpec.php
- src/Sylius/Bundle/ApiBundle/spec/Doctrine/ORM/QueryExtension/Shop/ExchangeRate/ChannelBasedExtensionSpec.php
- src/Sylius/Bundle/CoreBundle/Doctrine/ORM/Handler/ResourceDeleteHandler.php
- src/Sylius/Bundle/CoreBundle/Doctrine/DQL/Year.php
- src/Sylius/Bundle/ApiBundle/spec/Doctrine/ORM/QueryExtension/Shop/Product/TaxonBasedExtensionSpec.php
- src/Sylius/Bundle/ApiBundle/spec/Doctrine/ORM/QueryExtension/Common/NonArchivedExtensionSpec.php
- src/Sylius/Bundle/ApiBundle/spec/Doctrine/ORM/QueryExtension/Shop/Order/StateBasedExtensionSpec.php
- src/Sylius/Bundle/ApiBundle/spec/Doctrine/ORM/QueryExtension/Shop/ProductReview/AcceptedExtensionSpec.php
- src/Sylius/Bundle/CoreBundle/spec/Remover/ReviewerReviewsRemoverSpec.php
- src/Sylius/Bundle/ApiBundle/spec/StateProvider/Shop/Product/ProductAttributeValue/CollectionProviderSpec.php
- src/Sylius/Bundle/ReviewBundle/EventListener/ReviewChangeListener.php
- src/Sylius/Bundle/CoreBundle/spec/EventListener/CanonicalizerListenerSpec.php
- src/Sylius/Bundle/CoreBundle/Doctrine/DQL/Day.php
- src/Sylius/Bundle/ApiBundle/spec/Doctrine/ORM/QueryExtension/Shop/Customer/ShopUserBasedExtensionSpec.php
- src/Sylius/Bundle/ReviewBundle/spec/EventListener/ReviewChangeListenerSpec.php
- src/Sylius/Bundle/CoreBundle/Doctrine/DQL/Month.php
- src/Sylius/Bundle/CoreBundle/Doctrine/DQL/Hour.php
- src/Sylius/Bundle/CoreBundle/Doctrine/DQL/Week.php
- src/Sylius/Bundle/ApiBundle/spec/Doctrine/ORM/QueryExtension/Shop/OrderItem/ShopUserBasedExtensionSpec.php
- src/Sylius/Bundle/UserBundle/spec/EventListener/PasswordUpdaterListenerSpec.php
- src/Sylius/Bundle/CoreBundle/Doctrine/ORM/SqlWalker/OrderByIdentifierSqlWalker.php
- src/Sylius/Bundle/CoreBundle/Doctrine/DQL/DateFormat.php
- src/Sylius/Bundle/CoreBundle/spec/Doctrine/ORM/Handler/ResourceDeleteHandlerSpec.php
- src/Sylius/Bundle/ApiBundle/spec/Doctrine/ORM/QueryExtension/Shop/Currency/ChannelBasedExtensionSpec.php
- src/Sylius/Bundle/CoreBundle/Remover/ReviewerReviewsRemover.php
- src/Sylius/Bundle/ProductBundle/Doctrine/ORM/Query/AST/Function/AbstractJsonFunctionNode.php
- src/Sylius/Bundle/CoreBundle/spec/Order/NumberGenerator/SequentialOrderNumberGeneratorSpec.php
- src/Sylius/Bundle/ApiBundle/spec/Doctrine/ORM/QueryExtension/Shop/Order/ShopUserBasedExtensionSpec.php
- src/Sylius/Bundle/ApiBundle/spec/Doctrine/ORM/QueryExtension/Shop/Address/ShopUserBasedExtensionSpec.php
- src/Sylius/Bundle/OrderBundle/spec/NumberGenerator/SequentialOrderNumberGeneratorSpec.php
- src/Sylius/Bundle/AttributeBundle/Doctrine/ORM/Subscriber/LoadMetadataSubscriber.php
- src/Sylius/Bundle/ApiBundle/spec/CommandHandler/Cart/RemoveItemFromCartHandlerSpec.php
- src/Sylius/Bundle/AttributeBundle/spec/Doctrine/ORM/Subscriber/LoadMetadataSubscriberSpec.php
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Static checks / PHP 8.3, Symfony ^7.1
- GitHub Check: Static checks / PHP 8.2, Symfony ^6.4
🔇 Additional comments (16)
src/Sylius/Bundle/ApiBundle/spec/Doctrine/ORM/QueryExtension/Shop/Order/VisitorBasedExtensionSpec.php (2)
85-87: LGTM! Good use of specific expression types.The addition of specific expression types (
Expr\Comparison,Expr\Andx,Expr\Orx) improves type safety and aligns with Doctrine ORM best practices.
100-107: LGTM! Clean refactor of expression handling.The changes properly update the expression handling to use specific expression types while maintaining the same logical conditions. This aligns with modern Doctrine ORM practices and improves code maintainability.
src/Sylius/Bundle/LocaleBundle/spec/Checker/LocaleUsageCheckerSpec.php (3)
17-19: LGTM: New imports align with ORM deprecation fixes.The added Doctrine ORM imports support the transition to using EntityPersister, which is part of the ORM deprecation fixes.
52-63: LGTM: Improved test setup using EntityPersister.The changes improve the test by:
- Using actual EntityRepository instead of mocks
- Explicitly defining entity persistence operations
- Following Doctrine's recommended practices for entity management
87-98: LGTM: Consistent test setup for negative case.The changes maintain consistency with the positive test case while properly testing the negative scenario using the updated EntityPersister approach.
src/Sylius/Bundle/ProductBundle/spec/EventListener/SelectProductAttributeChoiceRemoveListenerSpec.php (3)
16-29: LGTM! Import changes align with ORM deprecation fixes.The changes correctly update the imports to use the Persistence namespace for event handling and add necessary imports for the new QueryBuilder-based approach.
42-50: LGTM! Method signature updated to support QueryBuilder approach.The updated method signature properly includes the new dependencies required for the QueryBuilder-based implementation.
80-88: LGTM! QueryBuilder setup with proper parameter binding.The implementation:
- Uses QueryBuilder for type-safe query construction
- Implements parameter binding for SQL injection prevention
- Properly mocks the query execution chain
src/Sylius/Bundle/ApiBundle/spec/Doctrine/ORM/QueryExtension/Shop/OrderItem/VisitorBasedExtensionSpec.php (4)
86-88: LGTM! Improved type safety with specific expression types.The change from generic
Exprto specific expression types (Comparison,Andx,Orx) improves type safety and clarity.
103-110: LGTM! Improved expression handling with explicit SQL conditions.The changes improve test clarity by:
- Using explicit SQL condition strings instead of generic expressions
- Properly constructing complex conditions using typed expression objects
164-166: Consistent type improvements across methods.
181-188: Consistent expression handling improvements across methods.src/Sylius/Bundle/ApiBundle/spec/Resolver/UriTemplateParentResourceResolverSpec.php (4)
19-21: LGTM! Import changes align with ORM updates.The shift from high-level repository abstractions to direct ORM components is appropriate for addressing ORM deprecations.
Also applies to: 25-25
100-101: LGTM! Repository instantiation updated to use direct ORM components.The change from
RepositoryInterfacetoEntityRepositorywith explicitEntityManagerandClassMetadatais appropriate for addressing ORM deprecations.
102-106: LGTM! Entity loading mechanism updated to use explicit ORM components.The change from
findOneBytoEntityPersister::loadwith explicit parameters provides better type safety and aligns with modern Doctrine practices.Also applies to: 122-126
129-129: LGTM! Link parameters updated for better type safety.Using
$parentItem::classdirectly for thefromClassparameter improves type safety.
74c6f8c to
f7ac04a
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/Sylius/Bundle/ProductBundle/spec/EventListener/SelectProductAttributeChoiceRemoveListenerSpec.php (1)
84-84: Consider verifying the exact SQL query parameters.The test could be more robust by verifying the exact SQL parameters that will be used in the query.
- $queryBuilder->setParameter('key', '%"1739bc61-9e42-4c80-8b9a-f97f0579cccb"%')->shouldBeCalled()->willReturn($queryBuilder); + $queryBuilder->setParameter('key', '%"1739bc61-9e42-4c80-8b9a-f97f0579cccb"%', \PDO::PARAM_STR)->shouldBeCalled()->willReturn($queryBuilder);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (39)
src/Sylius/Bundle/ApiBundle/spec/CommandHandler/Cart/RemoveItemFromCartHandlerSpec.php(4 hunks)src/Sylius/Bundle/ApiBundle/spec/Doctrine/ORM/QueryExtension/Common/NonArchivedExtensionSpec.php(1 hunks)src/Sylius/Bundle/ApiBundle/spec/Doctrine/ORM/QueryExtension/Shop/Address/ShopUserBasedExtensionSpec.php(2 hunks)src/Sylius/Bundle/ApiBundle/spec/Doctrine/ORM/QueryExtension/Shop/Currency/ChannelBasedExtensionSpec.php(4 hunks)src/Sylius/Bundle/ApiBundle/spec/Doctrine/ORM/QueryExtension/Shop/Customer/ShopUserBasedExtensionSpec.php(4 hunks)src/Sylius/Bundle/ApiBundle/spec/Doctrine/ORM/QueryExtension/Shop/ExchangeRate/ChannelBasedExtensionSpec.php(2 hunks)src/Sylius/Bundle/ApiBundle/spec/Doctrine/ORM/QueryExtension/Shop/Order/ShopUserBasedExtensionSpec.php(3 hunks)src/Sylius/Bundle/ApiBundle/spec/Doctrine/ORM/QueryExtension/Shop/Order/StateBasedExtensionSpec.php(2 hunks)src/Sylius/Bundle/ApiBundle/spec/Doctrine/ORM/QueryExtension/Shop/Order/VisitorBasedExtensionSpec.php(2 hunks)src/Sylius/Bundle/ApiBundle/spec/Doctrine/ORM/QueryExtension/Shop/OrderItem/ShopUserBasedExtensionSpec.php(4 hunks)src/Sylius/Bundle/ApiBundle/spec/Doctrine/ORM/QueryExtension/Shop/OrderItem/VisitorBasedExtensionSpec.php(4 hunks)src/Sylius/Bundle/ApiBundle/spec/Doctrine/ORM/QueryExtension/Shop/Product/TaxonBasedExtensionSpec.php(2 hunks)src/Sylius/Bundle/ApiBundle/spec/Doctrine/ORM/QueryExtension/Shop/ProductReview/AcceptedExtensionSpec.php(1 hunks)src/Sylius/Bundle/ApiBundle/spec/Doctrine/ORM/QueryExtension/Shop/Taxon/ChannelBasedExtensionSpec.php(1 hunks)src/Sylius/Bundle/ApiBundle/spec/Doctrine/ORM/QueryExtension/Shop/Taxon/EnabledChildrenExtensionSpec.php(1 hunks)src/Sylius/Bundle/ApiBundle/spec/Resolver/UriTemplateParentResourceResolverSpec.php(4 hunks)src/Sylius/Bundle/ApiBundle/spec/StateProvider/Shop/Product/ProductAttributeValue/CollectionProviderSpec.php(2 hunks)src/Sylius/Bundle/AttributeBundle/Doctrine/ORM/Subscriber/LoadMetadataSubscriber.php(3 hunks)src/Sylius/Bundle/AttributeBundle/spec/Doctrine/ORM/Subscriber/LoadMetadataSubscriberSpec.php(5 hunks)src/Sylius/Bundle/CoreBundle/Doctrine/DQL/DateFormat.php(1 hunks)src/Sylius/Bundle/CoreBundle/Doctrine/DQL/Day.php(2 hunks)src/Sylius/Bundle/CoreBundle/Doctrine/DQL/Hour.php(2 hunks)src/Sylius/Bundle/CoreBundle/Doctrine/DQL/Month.php(2 hunks)src/Sylius/Bundle/CoreBundle/Doctrine/DQL/Week.php(2 hunks)src/Sylius/Bundle/CoreBundle/Doctrine/DQL/Year.php(2 hunks)src/Sylius/Bundle/CoreBundle/Doctrine/ORM/Handler/ResourceDeleteHandler.php(1 hunks)src/Sylius/Bundle/CoreBundle/Doctrine/ORM/SqlWalker/OrderByIdentifierSqlWalker.php(1 hunks)src/Sylius/Bundle/CoreBundle/Remover/ReviewerReviewsRemover.php(2 hunks)src/Sylius/Bundle/CoreBundle/spec/Doctrine/ORM/Handler/ResourceDeleteHandlerSpec.php(2 hunks)src/Sylius/Bundle/CoreBundle/spec/EventListener/CanonicalizerListenerSpec.php(1 hunks)src/Sylius/Bundle/CoreBundle/spec/Order/NumberGenerator/SequentialOrderNumberGeneratorSpec.php(3 hunks)src/Sylius/Bundle/CoreBundle/spec/Remover/ReviewerReviewsRemoverSpec.php(1 hunks)src/Sylius/Bundle/LocaleBundle/spec/Checker/LocaleUsageCheckerSpec.php(3 hunks)src/Sylius/Bundle/OrderBundle/spec/NumberGenerator/SequentialOrderNumberGeneratorSpec.php(3 hunks)src/Sylius/Bundle/ProductBundle/Doctrine/ORM/Query/AST/Function/AbstractJsonFunctionNode.php(6 hunks)src/Sylius/Bundle/ProductBundle/spec/EventListener/SelectProductAttributeChoiceRemoveListenerSpec.php(3 hunks)src/Sylius/Bundle/ReviewBundle/EventListener/ReviewChangeListener.php(1 hunks)src/Sylius/Bundle/ReviewBundle/spec/EventListener/ReviewChangeListenerSpec.php(1 hunks)src/Sylius/Bundle/UserBundle/spec/EventListener/PasswordUpdaterListenerSpec.php(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (34)
- src/Sylius/Bundle/ApiBundle/spec/Doctrine/ORM/QueryExtension/Shop/ProductReview/AcceptedExtensionSpec.php
- src/Sylius/Bundle/ApiBundle/spec/Doctrine/ORM/QueryExtension/Shop/Taxon/ChannelBasedExtensionSpec.php
- src/Sylius/Bundle/CoreBundle/Doctrine/DQL/Year.php
- src/Sylius/Bundle/ApiBundle/spec/Doctrine/ORM/QueryExtension/Shop/Address/ShopUserBasedExtensionSpec.php
- src/Sylius/Bundle/CoreBundle/Doctrine/DQL/Month.php
- src/Sylius/Bundle/ApiBundle/spec/Doctrine/ORM/QueryExtension/Common/NonArchivedExtensionSpec.php
- src/Sylius/Bundle/ApiBundle/spec/Doctrine/ORM/QueryExtension/Shop/Customer/ShopUserBasedExtensionSpec.php
- src/Sylius/Bundle/ApiBundle/spec/Doctrine/ORM/QueryExtension/Shop/ExchangeRate/ChannelBasedExtensionSpec.php
- src/Sylius/Bundle/CoreBundle/spec/EventListener/CanonicalizerListenerSpec.php
- src/Sylius/Bundle/ApiBundle/spec/Doctrine/ORM/QueryExtension/Shop/Taxon/EnabledChildrenExtensionSpec.php
- src/Sylius/Bundle/ReviewBundle/EventListener/ReviewChangeListener.php
- src/Sylius/Bundle/ReviewBundle/spec/EventListener/ReviewChangeListenerSpec.php
- src/Sylius/Bundle/CoreBundle/Doctrine/DQL/Day.php
- src/Sylius/Bundle/ApiBundle/spec/Doctrine/ORM/QueryExtension/Shop/Product/TaxonBasedExtensionSpec.php
- src/Sylius/Bundle/CoreBundle/Doctrine/ORM/Handler/ResourceDeleteHandler.php
- src/Sylius/Bundle/ApiBundle/spec/StateProvider/Shop/Product/ProductAttributeValue/CollectionProviderSpec.php
- src/Sylius/Bundle/CoreBundle/spec/Remover/ReviewerReviewsRemoverSpec.php
- src/Sylius/Bundle/CoreBundle/Doctrine/ORM/SqlWalker/OrderByIdentifierSqlWalker.php
- src/Sylius/Bundle/LocaleBundle/spec/Checker/LocaleUsageCheckerSpec.php
- src/Sylius/Bundle/ProductBundle/Doctrine/ORM/Query/AST/Function/AbstractJsonFunctionNode.php
- src/Sylius/Bundle/AttributeBundle/Doctrine/ORM/Subscriber/LoadMetadataSubscriber.php
- src/Sylius/Bundle/CoreBundle/Remover/ReviewerReviewsRemover.php
- src/Sylius/Bundle/ApiBundle/spec/Doctrine/ORM/QueryExtension/Shop/Order/ShopUserBasedExtensionSpec.php
- src/Sylius/Bundle/ApiBundle/spec/Doctrine/ORM/QueryExtension/Shop/OrderItem/ShopUserBasedExtensionSpec.php
- src/Sylius/Bundle/CoreBundle/Doctrine/DQL/Hour.php
- src/Sylius/Bundle/UserBundle/spec/EventListener/PasswordUpdaterListenerSpec.php
- src/Sylius/Bundle/ApiBundle/spec/Doctrine/ORM/QueryExtension/Shop/Currency/ChannelBasedExtensionSpec.php
- src/Sylius/Bundle/CoreBundle/spec/Order/NumberGenerator/SequentialOrderNumberGeneratorSpec.php
- src/Sylius/Bundle/CoreBundle/Doctrine/DQL/DateFormat.php
- src/Sylius/Bundle/CoreBundle/Doctrine/DQL/Week.php
- src/Sylius/Bundle/OrderBundle/spec/NumberGenerator/SequentialOrderNumberGeneratorSpec.php
- src/Sylius/Bundle/ApiBundle/spec/CommandHandler/Cart/RemoveItemFromCartHandlerSpec.php
- src/Sylius/Bundle/AttributeBundle/spec/Doctrine/ORM/Subscriber/LoadMetadataSubscriberSpec.php
- src/Sylius/Bundle/ApiBundle/spec/Doctrine/ORM/QueryExtension/Shop/Order/StateBasedExtensionSpec.php
⏰ Context from checks skipped due to timeout of 90000ms (14)
- GitHub Check: Frontend / NodeJS 20.x
- GitHub Check: End-to-end tests (MariaDB) / Non-JS, PHP 8.3, Symfony ^7.1, MariaDB 11.4.3, State Machine Adapter winzou_state_machine
- GitHub Check: End-to-end tests (MariaDB) / Non-JS, PHP 8.3, Symfony ^7.1, MariaDB 11.4.3, State Machine Adapter symfony_workflow
- GitHub Check: End-to-end tests (MariaDB) / Non-JS, PHP 8.2, Symfony ^6.4, MariaDB 10.11.9, State Machine Adapter symfony_workflow
- GitHub Check: End-to-end tests (PostgreSQL) / Non-JS, PHP 8.3, Symfony ^7.1, PostgreSQL 16.4
- GitHub Check: End-to-end tests (PostgreSQL) / Non-JS, PHP 8.2, Symfony ^6.4, PostgreSQL 15.8
- GitHub Check: Packages / PHP 8.3, Symfony ^7.1
- GitHub Check: Packages / PHP 8.2, Symfony ^6.4
- GitHub Check: End-to-end tests (MySQL) / Non-JS, PHP 8.3, Symfony ^7.1 (test_cached), MySQL 8.4, Twig ^3.3
- GitHub Check: End-to-end tests (MySQL) / JS with Chromedriver, PHP 8.3, Symfony ^7.1 (test_cached), MySQL 8.4, Twig ^3.3
- GitHub Check: End-to-end tests (MySQL) / Non-JS, PHP 8.2, Symfony ^6.4 (test_cached), MySQL 8.0, Twig ^3.3
- GitHub Check: End-to-end tests (MySQL) / JS with Panther, PHP 8.3, Symfony ^7.1 (test_cached), MySQL 8.4, Twig ^3.3
- GitHub Check: End-to-end tests (MySQL) / JS with Chromedriver, PHP 8.2, Symfony ^6.4 (test_cached), MySQL 8.0, Twig ^3.3
- GitHub Check: End-to-end tests (MySQL) / JS with Panther, PHP 8.2, Symfony ^6.4 (test_cached), MySQL 8.0, Twig ^3.3
🔇 Additional comments (14)
src/Sylius/Bundle/CoreBundle/spec/Doctrine/ORM/Handler/ResourceDeleteHandlerSpec.php (3)
18-19: LGTM! Proper handling of ORM exception namespace changes.The import statements correctly handle both the legacy and new ORM exception classes, ensuring backward compatibility during the transition.
71-75: LGTM! Well-implemented backward compatibility layer.The conditional block elegantly handles both old and new ORM exception implementations:
- Uses the new interface-based exception when available
- Falls back to the legacy exception class when needed
78-78: LGTM! Proper usage of the compatibility layer.The test correctly uses the dynamically created exception instance, maintaining compatibility with both ORM versions.
src/Sylius/Bundle/ApiBundle/spec/Doctrine/ORM/QueryExtension/Shop/Order/VisitorBasedExtensionSpec.php (2)
85-87: LGTM! Improved type safety with specific expression types.The change from generic
Expr\Functo specific expression types (Expr\Comparison,Expr\Andx,Expr\Orx) improves type safety and makes the code more maintainable.
100-106: LGTM! Enhanced clarity in SQL condition generation.The change to return SQL string conditions directly (e.g., 'user IS NULL') instead of using
Expr\Funcmakes the test more readable and better reflects the actual SQL being generated.src/Sylius/Bundle/ApiBundle/spec/Doctrine/ORM/QueryExtension/Shop/OrderItem/VisitorBasedExtensionSpec.php (3)
86-88: LGTM! Consistent use of specific expression types.The change to use specific expression types (
Expr\Comparison,Expr\Andx,Expr\Orx) aligns with the changes in the Order spec and improves type safety.
103-108: LGTM! Improved SQL condition handling.The change to use string conditions and specific expression types for combining conditions enhances readability and maintainability.
181-186: LGTM! Consistent implementation across methods.The changes in
it_applies_conditions_to_itemmirror those init_applies_conditions_to_collection, maintaining consistency throughout the codebase.src/Sylius/Bundle/ProductBundle/spec/EventListener/SelectProductAttributeChoiceRemoveListenerSpec.php (3)
16-29: LGTM! Import changes align with Doctrine's best practices.The switch to
Doctrine\Persistencenamespace forLifecycleEventArgsand addition of necessary DB-related imports are well-organized and properly support the QueryBuilder implementation.
42-98: LGTM! QueryBuilder implementation improves test maintainability.The refactoring from direct repository calls to QueryBuilder pattern provides better control and visibility of the query construction process.
100-189: LGTM! Negative test cases remain consistent with the new implementation.The unchanged test methods properly verify negative scenarios and maintain correct assertions with
shouldNotBeCalled().src/Sylius/Bundle/ApiBundle/spec/Resolver/UriTemplateParentResourceResolverSpec.php (3)
19-21: LGTM! Import changes align with the implementation updates.The new imports correctly reflect the transition from repository interfaces to concrete Doctrine ORM classes.
Also applies to: 25-25
83-88: LGTM! Method signatures updated to use specific Doctrine ORM components.The changes align with the PR objective of fixing ORM deprecations by transitioning from generic repository interfaces to specific Doctrine ORM components.
Also applies to: 113-119
100-107: LGTM! Implementation correctly updated to use EntityPersister.The changes maintain the same test behavior while transitioning to more specific Doctrine ORM components. The EntityPersister's load method is correctly configured with all necessary parameters.
Also applies to: 120-127
src/Sylius/Bundle/ApiBundle/spec/Resolver/UriTemplateParentResourceResolverSpec.php
Outdated
Show resolved
Hide resolved
f7ac04a to
6f48eb1
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
src/Sylius/Bundle/CoreBundle/spec/Doctrine/ORM/Handler/ResourceDeleteHandlerSpec.php (1)
71-75: LGTM! Well-implemented backward compatibility handling.The conditional creation of the exception instance elegantly handles both old and new Doctrine ORM versions. This approach ensures the tests remain functional during the transition period.
This pattern of using anonymous classes for backward compatibility testing is a good practice that could be applied to other areas dealing with library transitions.
src/Sylius/Bundle/ApiBundle/spec/Doctrine/ORM/QueryExtension/Shop/Order/VisitorBasedExtensionSpec.php (1)
85-87: LGTM! Good transition to typed expressions.The changes correctly replace deprecated
Expr\Funcwith specific expression types (Comparison,Andx,Orx) and use string literals for SQL conditions, which aligns with modern Doctrine practices.Consider extracting the SQL condition strings to constants for better maintainability:
+ private const SQL_USER_IS_NULL = 'user IS NULL'; + private const SQL_CUSTOMER_IS_NULL = 'o.customer IS NULL'; + private const SQL_USER_IS_NOT_NULL = 'user IS NOT NULL'; + function it_applies_conditions_to_item( // ... ): void { // ... - $expr->isNull('user')->willReturn('user IS NULL'); - $expr->isNull('o.customer')->willReturn('o.customer IS NULL'); - $expr->isNotNull('user')->willReturn('user IS NOT NULL'); + $expr->isNull('user')->willReturn(self::SQL_USER_IS_NULL); + $expr->isNull('o.customer')->willReturn(self::SQL_CUSTOMER_IS_NULL); + $expr->isNotNull('user')->willReturn(self::SQL_USER_IS_NOT_NULL);Also applies to: 100-107
src/Sylius/Bundle/ApiBundle/spec/Doctrine/ORM/QueryExtension/Shop/OrderItem/VisitorBasedExtensionSpec.php (1)
86-88: LGTM! Good consistency with Order extension changes.The changes correctly implement the same expression type updates as the Order extension, maintaining consistency across the codebase.
Consider extracting the common SQL condition logic into a shared method to reduce code duplication between
it_applies_conditions_to_collectionandit_applies_conditions_to_item:+ private function setupCommonConditions( + Expr $expr, + string $customerPath, + ): array { + $exprEq = new Expr\Comparison(); + $exprAndx = new Expr\Andx(); + $exprOrx = new Expr\Orx(); + + $expr->isNull('user')->willReturn('user IS NULL'); + $expr->isNull($customerPath)->willReturn("$customerPath IS NULL"); + $expr->isNotNull('user')->willReturn('user IS NOT NULL'); + $expr->eq("$customerPath.createdByGuest", ':createdByGuest')->willReturn($exprEq); + $expr->andX('user IS NOT NULL', $exprEq)->willReturn($exprAndx); + $expr->orX('user IS NULL', "$customerPath IS NULL", $exprAndx)->willReturn($exprOrx); + + return [$exprEq, $exprAndx, $exprOrx]; + } + function it_applies_conditions_to_collection( // ... Expr $expr, - Expr\Comparison $exprEq, - Expr\Andx $exprAndx, - Expr\Orx $exprOrx, ): void { // ... - $expr->isNull('user')->willReturn('user IS NULL'); - $expr->isNull('order.customer')->willReturn('order.customer IS NULL'); - // ... rest of the conditions + [$exprEq, $exprAndx, $exprOrx] = $this->setupCommonConditions($expr, 'order.customer');Also applies to: 103-109, 164-166, 181-187
src/Sylius/Bundle/LocaleBundle/spec/Checker/LocaleUsageCheckerSpec.php (1)
62-63: Consider extracting EntityPersister load parameters to constants.The EntityPersister load parameters are quite specific and could be made more maintainable by extracting them to constants or a data provider.
+ private const DEFAULT_LOAD_PARAMS = [ + 'limit' => 1, + 'orderBy' => null, + 'lockMode' => null, + 'hints' => [], + 'offset' => null, + ]; - $entityPersister->load(['code' => 'en_US'], null, null, [], null, 1, null)->willReturn($locale); + $entityPersister->load(['code' => 'en_US'], ...array_values(self::DEFAULT_LOAD_PARAMS))->willReturn($locale);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (39)
src/Sylius/Bundle/ApiBundle/spec/CommandHandler/Cart/RemoveItemFromCartHandlerSpec.php(4 hunks)src/Sylius/Bundle/ApiBundle/spec/Doctrine/ORM/QueryExtension/Common/NonArchivedExtensionSpec.php(1 hunks)src/Sylius/Bundle/ApiBundle/spec/Doctrine/ORM/QueryExtension/Shop/Address/ShopUserBasedExtensionSpec.php(2 hunks)src/Sylius/Bundle/ApiBundle/spec/Doctrine/ORM/QueryExtension/Shop/Currency/ChannelBasedExtensionSpec.php(4 hunks)src/Sylius/Bundle/ApiBundle/spec/Doctrine/ORM/QueryExtension/Shop/Customer/ShopUserBasedExtensionSpec.php(4 hunks)src/Sylius/Bundle/ApiBundle/spec/Doctrine/ORM/QueryExtension/Shop/ExchangeRate/ChannelBasedExtensionSpec.php(2 hunks)src/Sylius/Bundle/ApiBundle/spec/Doctrine/ORM/QueryExtension/Shop/Order/ShopUserBasedExtensionSpec.php(3 hunks)src/Sylius/Bundle/ApiBundle/spec/Doctrine/ORM/QueryExtension/Shop/Order/StateBasedExtensionSpec.php(2 hunks)src/Sylius/Bundle/ApiBundle/spec/Doctrine/ORM/QueryExtension/Shop/Order/VisitorBasedExtensionSpec.php(2 hunks)src/Sylius/Bundle/ApiBundle/spec/Doctrine/ORM/QueryExtension/Shop/OrderItem/ShopUserBasedExtensionSpec.php(4 hunks)src/Sylius/Bundle/ApiBundle/spec/Doctrine/ORM/QueryExtension/Shop/OrderItem/VisitorBasedExtensionSpec.php(4 hunks)src/Sylius/Bundle/ApiBundle/spec/Doctrine/ORM/QueryExtension/Shop/Product/TaxonBasedExtensionSpec.php(2 hunks)src/Sylius/Bundle/ApiBundle/spec/Doctrine/ORM/QueryExtension/Shop/ProductReview/AcceptedExtensionSpec.php(1 hunks)src/Sylius/Bundle/ApiBundle/spec/Doctrine/ORM/QueryExtension/Shop/Taxon/ChannelBasedExtensionSpec.php(1 hunks)src/Sylius/Bundle/ApiBundle/spec/Doctrine/ORM/QueryExtension/Shop/Taxon/EnabledChildrenExtensionSpec.php(1 hunks)src/Sylius/Bundle/ApiBundle/spec/Resolver/UriTemplateParentResourceResolverSpec.php(4 hunks)src/Sylius/Bundle/ApiBundle/spec/StateProvider/Shop/Product/ProductAttributeValue/CollectionProviderSpec.php(2 hunks)src/Sylius/Bundle/AttributeBundle/Doctrine/ORM/Subscriber/LoadMetadataSubscriber.php(3 hunks)src/Sylius/Bundle/AttributeBundle/spec/Doctrine/ORM/Subscriber/LoadMetadataSubscriberSpec.php(5 hunks)src/Sylius/Bundle/CoreBundle/Doctrine/DQL/DateFormat.php(1 hunks)src/Sylius/Bundle/CoreBundle/Doctrine/DQL/Day.php(2 hunks)src/Sylius/Bundle/CoreBundle/Doctrine/DQL/Hour.php(2 hunks)src/Sylius/Bundle/CoreBundle/Doctrine/DQL/Month.php(2 hunks)src/Sylius/Bundle/CoreBundle/Doctrine/DQL/Week.php(2 hunks)src/Sylius/Bundle/CoreBundle/Doctrine/DQL/Year.php(2 hunks)src/Sylius/Bundle/CoreBundle/Doctrine/ORM/Handler/ResourceDeleteHandler.php(1 hunks)src/Sylius/Bundle/CoreBundle/Doctrine/ORM/SqlWalker/OrderByIdentifierSqlWalker.php(1 hunks)src/Sylius/Bundle/CoreBundle/Remover/ReviewerReviewsRemover.php(2 hunks)src/Sylius/Bundle/CoreBundle/spec/Doctrine/ORM/Handler/ResourceDeleteHandlerSpec.php(2 hunks)src/Sylius/Bundle/CoreBundle/spec/EventListener/CanonicalizerListenerSpec.php(1 hunks)src/Sylius/Bundle/CoreBundle/spec/Order/NumberGenerator/SequentialOrderNumberGeneratorSpec.php(3 hunks)src/Sylius/Bundle/CoreBundle/spec/Remover/ReviewerReviewsRemoverSpec.php(1 hunks)src/Sylius/Bundle/LocaleBundle/spec/Checker/LocaleUsageCheckerSpec.php(3 hunks)src/Sylius/Bundle/OrderBundle/spec/NumberGenerator/SequentialOrderNumberGeneratorSpec.php(3 hunks)src/Sylius/Bundle/ProductBundle/Doctrine/ORM/Query/AST/Function/AbstractJsonFunctionNode.php(6 hunks)src/Sylius/Bundle/ProductBundle/spec/EventListener/SelectProductAttributeChoiceRemoveListenerSpec.php(3 hunks)src/Sylius/Bundle/ReviewBundle/EventListener/ReviewChangeListener.php(1 hunks)src/Sylius/Bundle/ReviewBundle/spec/EventListener/ReviewChangeListenerSpec.php(1 hunks)src/Sylius/Bundle/UserBundle/spec/EventListener/PasswordUpdaterListenerSpec.php(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (33)
- src/Sylius/Bundle/ApiBundle/spec/Doctrine/ORM/QueryExtension/Shop/Taxon/ChannelBasedExtensionSpec.php
- src/Sylius/Bundle/CoreBundle/Doctrine/DQL/Year.php
- src/Sylius/Bundle/ApiBundle/spec/Doctrine/ORM/QueryExtension/Shop/ProductReview/AcceptedExtensionSpec.php
- src/Sylius/Bundle/CoreBundle/spec/EventListener/CanonicalizerListenerSpec.php
- src/Sylius/Bundle/CoreBundle/Doctrine/ORM/Handler/ResourceDeleteHandler.php
- src/Sylius/Bundle/ApiBundle/spec/Doctrine/ORM/QueryExtension/Shop/Order/StateBasedExtensionSpec.php
- src/Sylius/Bundle/ReviewBundle/spec/EventListener/ReviewChangeListenerSpec.php
- src/Sylius/Bundle/ApiBundle/spec/Doctrine/ORM/QueryExtension/Shop/Address/ShopUserBasedExtensionSpec.php
- src/Sylius/Bundle/UserBundle/spec/EventListener/PasswordUpdaterListenerSpec.php
- src/Sylius/Bundle/ApiBundle/spec/Doctrine/ORM/QueryExtension/Common/NonArchivedExtensionSpec.php
- src/Sylius/Bundle/CoreBundle/Doctrine/DQL/Month.php
- src/Sylius/Bundle/ApiBundle/spec/Doctrine/ORM/QueryExtension/Shop/Currency/ChannelBasedExtensionSpec.php
- src/Sylius/Bundle/ApiBundle/spec/Doctrine/ORM/QueryExtension/Shop/Product/TaxonBasedExtensionSpec.php
- src/Sylius/Bundle/CoreBundle/Doctrine/ORM/SqlWalker/OrderByIdentifierSqlWalker.php
- src/Sylius/Bundle/ApiBundle/spec/Doctrine/ORM/QueryExtension/Shop/OrderItem/ShopUserBasedExtensionSpec.php
- src/Sylius/Bundle/ApiBundle/spec/Doctrine/ORM/QueryExtension/Shop/ExchangeRate/ChannelBasedExtensionSpec.php
- src/Sylius/Bundle/ApiBundle/spec/Doctrine/ORM/QueryExtension/Shop/Customer/ShopUserBasedExtensionSpec.php
- src/Sylius/Bundle/ProductBundle/Doctrine/ORM/Query/AST/Function/AbstractJsonFunctionNode.php
- src/Sylius/Bundle/ApiBundle/spec/StateProvider/Shop/Product/ProductAttributeValue/CollectionProviderSpec.php
- src/Sylius/Bundle/CoreBundle/Doctrine/DQL/Hour.php
- src/Sylius/Bundle/CoreBundle/spec/Remover/ReviewerReviewsRemoverSpec.php
- src/Sylius/Bundle/CoreBundle/Remover/ReviewerReviewsRemover.php
- src/Sylius/Bundle/CoreBundle/Doctrine/DQL/DateFormat.php
- src/Sylius/Bundle/ApiBundle/spec/Doctrine/ORM/QueryExtension/Shop/Taxon/EnabledChildrenExtensionSpec.php
- src/Sylius/Bundle/AttributeBundle/Doctrine/ORM/Subscriber/LoadMetadataSubscriber.php
- src/Sylius/Bundle/ReviewBundle/EventListener/ReviewChangeListener.php
- src/Sylius/Bundle/CoreBundle/spec/Order/NumberGenerator/SequentialOrderNumberGeneratorSpec.php
- src/Sylius/Bundle/CoreBundle/Doctrine/DQL/Day.php
- src/Sylius/Bundle/OrderBundle/spec/NumberGenerator/SequentialOrderNumberGeneratorSpec.php
- src/Sylius/Bundle/ApiBundle/spec/Doctrine/ORM/QueryExtension/Shop/Order/ShopUserBasedExtensionSpec.php
- src/Sylius/Bundle/CoreBundle/Doctrine/DQL/Week.php
- src/Sylius/Bundle/ApiBundle/spec/CommandHandler/Cart/RemoveItemFromCartHandlerSpec.php
- src/Sylius/Bundle/AttributeBundle/spec/Doctrine/ORM/Subscriber/LoadMetadataSubscriberSpec.php
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Static checks / PHP 8.3, Symfony ^7.1
- GitHub Check: Static checks / PHP 8.2, Symfony ^6.4
🔇 Additional comments (13)
src/Sylius/Bundle/CoreBundle/spec/Doctrine/ORM/Handler/ResourceDeleteHandlerSpec.php (2)
18-19: LGTM! Proper handling of Doctrine ORM exception namespace changes.The import statements correctly handle the transition in Doctrine ORM's exception organization, maintaining compatibility with both old and new namespace structures.
78-78: LGTM! Consistent use of dynamic exception instance.The test correctly uses the dynamically created exception instance, maintaining consistency with the backward compatibility approach.
src/Sylius/Bundle/ApiBundle/spec/Resolver/UriTemplateParentResourceResolverSpec.php (4)
19-21: LGTM! Import changes align with the ORM deprecation fixes.The new imports support the transition from repository-based to EntityPersister-based entity loading, which is part of addressing ORM deprecations.
Also applies to: 25-25
83-88: LGTM! Method signature changes reflect the new entity loading approach.The updated method signatures properly inject the required dependencies (UnitOfWork and EntityPersister) for the new entity loading approach, aligning with the PR's objective of fixing ORM deprecations.
Also applies to: 113-119
100-101: LGTM! Repository initialization is now more explicit and type-safe.The new initialization approach using EntityRepository with explicit ClassMetadata improves type safety and aligns with modern Doctrine practices.
Also applies to: 120-121
102-106: LGTM! Entity loading now uses the recommended Doctrine approach.The changes properly implement entity loading using EntityPersister through UnitOfWork, which is the recommended approach in Doctrine. The loading parameters are explicit and well-defined.
Also applies to: 122-127
src/Sylius/Bundle/LocaleBundle/spec/Checker/LocaleUsageCheckerSpec.php (3)
17-19: LGTM: New imports align with ORM refactoring.The added Doctrine ORM imports are necessary for the refactored test implementation and align with the PR's objective of fixing ORM deprecations.
55-58: LGTM: Improved test setup using real EntityRepository.The change from mocked repository to real EntityRepository provides a more realistic test environment and better reflects actual usage patterns.
90-98: LGTM: Consistent implementation with positive test case.The changes maintain symmetry with the positive test case and follow the same pattern of using EntityPersister, which is good for maintainability.
src/Sylius/Bundle/ProductBundle/spec/EventListener/SelectProductAttributeChoiceRemoveListenerSpec.php (4)
16-29: LGTM! Import changes align with ORM deprecation fixes.The shift to
Doctrine\Persistencenamespace and addition of specific DB-related imports properly support the ORM deprecation fixes.
42-50: LGTM! Test signature updated to support modern Doctrine patterns.The addition of QueryBuilder, Connection, and Query parameters properly supports the shift from deprecated repository methods to explicit query building.
100-189: LGTM! Comprehensive test coverage maintained.The test suite maintains excellent coverage of edge cases and invalid scenarios, properly verifying:
- Addition of new choices
- Value changes without removals
- Non-product attribute entities
- Non-select type attributes
80-88: Verify DB compatibility of JSON pattern matching.The query parameter
'%"1739bc61-9e42-4c80-8b9a-f97f0579cccb"%'uses JSON pattern matching which might have different behavior across different database engines.❌ Verification inconclusive
🏁 Script executed:
#!/bin/bash # Check if there are any database-specific implementations or tests rg -g '!vendor' -l 'ProductAttributeValueRepository.*json.*where'Length of output: 67
DB Compatibility Check for JSON Pattern Matching
The initial search did not reveal any dedicated database-specific implementations or tests addressing JSON pattern matching for this query parameter. It is therefore recommended to manually verify that the JSON matching pattern (i.e.
%'“1739bc61-9e42-4c80-8b9a-f97f0579cccb"%) works correctly on the databases in use (such as MySQL, PostgreSQL, etc.), as behavior can differ across engines.
...undle/ApiBundle/spec/Doctrine/ORM/QueryExtension/Shop/Currency/ChannelBasedExtensionSpec.php
Show resolved
Hide resolved
.../Bundle/ApiBundle/spec/Doctrine/ORM/QueryExtension/Shop/Order/ShopUserBasedExtensionSpec.php
Outdated
Show resolved
Hide resolved
...s/Bundle/ApiBundle/spec/Doctrine/ORM/QueryExtension/Shop/Product/TaxonBasedExtensionSpec.php
Outdated
Show resolved
Hide resolved
...s/Bundle/ApiBundle/spec/Doctrine/ORM/QueryExtension/Shop/Product/TaxonBasedExtensionSpec.php
Outdated
Show resolved
Hide resolved
src/Sylius/Bundle/ApiBundle/spec/Resolver/UriTemplateParentResourceResolverSpec.php
Show resolved
Hide resolved
src/Sylius/Bundle/CoreBundle/spec/Doctrine/ORM/Handler/ResourceDeleteHandlerSpec.php
Show resolved
Hide resolved
6f48eb1 to
8e5fb79
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
src/Sylius/Bundle/ProductBundle/spec/EventListener/SelectProductAttributeChoiceRemoveListenerSpec.php (1)
42-98: LGTM! Query building approach is more explicit and secure.The shift from direct repository calls to QueryBuilder improves testability and security by using parameterized queries.
Consider extracting the query building logic into a dedicated repository method to maintain consistency with other similar queries in the codebase:
$entityManager->createQueryBuilder()->willReturn($queryBuilder); +$productAttributeValueRepository->createQueryBuilderForChoiceKeySearch()->willReturn($queryBuilder); $queryBuilder->select(Argument::cetera())->willReturn($queryBuilder);src/Sylius/Bundle/ApiBundle/spec/Resolver/UriTemplateParentResourceResolverSpec.php (1)
83-111: LGTM! Consider extracting repository creation to a helper method.The changes correctly implement Doctrine's recommended patterns for entity management. The test maintains its original behavior while being more explicit about entity loading mechanisms.
Consider extracting the repository creation to a private helper method to reduce duplication:
+ private function createRepository( + EntityManagerInterface $entityManager, + string $class + ): EntityRepository { + return new EntityRepository($entityManager->getWrappedObject(), new ClassMetadata($class)); + } + function it_throws_an_exception_if_parent_resource_is_not_found( EntityManagerInterface $entityManager, ResourceInterface $item, UnitOfWork $unitOfWork, EntityPersister $entityPersister, ): void { - $repository = new EntityRepository($entityManager->getWrappedObject(), new ClassMetadata($parentItem::class)); + $repository = $this->createRepository($entityManager, $parentItem::class);src/Sylius/Bundle/LocaleBundle/spec/Checker/LocaleUsageCheckerSpec.php (2)
52-63: Consider trade-offs of using EntityPersister in tests.While the changes align with fixing ORM deprecations, using EntityPersister directly in tests introduces tighter coupling to Doctrine internals and increases test complexity. Consider:
- Whether this level of integration testing is necessary
- If the same test coverage could be achieved with simpler mocks
- Adding comments explaining why this approach was chosen
If the goal is to maintain simpler tests while still fixing ORM deprecations, consider this alternative approach:
- $localeRepository = new EntityRepository($entityManager->getWrappedObject(), new ClassMetadata(LocaleInterface::class)); - - $this->beConstructedWith($localeRepository, $registry, $entityManager); - - $entityManager->getUnitOfWork()->willReturn($unitOfWork); - $unitOfWork->getEntityPersister(LocaleInterface::class)->willReturn($entityPersister); - - $entityPersister->load(['code' => 'en_US'], null, null, [], null, 1, null)->willReturn($locale); - $entityPersister->count(['locale' => 'en_US'])->willReturn(1); + $localeRepository->findOneBy(['code' => 'en_US'])->willReturn($locale); + $localeRepository->count(['locale' => 'en_US'])->willReturn(1);
87-98: Reduce test setup duplication.The EntityPersister setup code is duplicated between test methods. Consider extracting common setup into the
letmethod or a private helper method.Example refactor:
private function setupEntityPersister( EntityManagerInterface $entityManager, UnitOfWork $unitOfWork, EntityPersister $entityPersister, LocaleInterface $locale, int $count ): void { $localeRepository = new EntityRepository( $entityManager->getWrappedObject(), new ClassMetadata(LocaleInterface::class) ); $this->beConstructedWith($localeRepository, $this->registry, $entityManager); $entityManager->getUnitOfWork()->willReturn($unitOfWork); $unitOfWork->getEntityPersister(LocaleInterface::class)->willReturn($entityPersister); $entityPersister->load( ['code' => 'en_US'], null, null, [], null, 1, null )->willReturn($locale); $entityPersister->count(['locale' => 'en_US'])->willReturn($count); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (39)
src/Sylius/Bundle/ApiBundle/spec/CommandHandler/Cart/RemoveItemFromCartHandlerSpec.php(4 hunks)src/Sylius/Bundle/ApiBundle/spec/Doctrine/ORM/QueryExtension/Common/NonArchivedExtensionSpec.php(1 hunks)src/Sylius/Bundle/ApiBundle/spec/Doctrine/ORM/QueryExtension/Shop/Address/ShopUserBasedExtensionSpec.php(2 hunks)src/Sylius/Bundle/ApiBundle/spec/Doctrine/ORM/QueryExtension/Shop/Currency/ChannelBasedExtensionSpec.php(4 hunks)src/Sylius/Bundle/ApiBundle/spec/Doctrine/ORM/QueryExtension/Shop/Customer/ShopUserBasedExtensionSpec.php(4 hunks)src/Sylius/Bundle/ApiBundle/spec/Doctrine/ORM/QueryExtension/Shop/ExchangeRate/ChannelBasedExtensionSpec.php(2 hunks)src/Sylius/Bundle/ApiBundle/spec/Doctrine/ORM/QueryExtension/Shop/Order/ShopUserBasedExtensionSpec.php(3 hunks)src/Sylius/Bundle/ApiBundle/spec/Doctrine/ORM/QueryExtension/Shop/Order/StateBasedExtensionSpec.php(2 hunks)src/Sylius/Bundle/ApiBundle/spec/Doctrine/ORM/QueryExtension/Shop/Order/VisitorBasedExtensionSpec.php(2 hunks)src/Sylius/Bundle/ApiBundle/spec/Doctrine/ORM/QueryExtension/Shop/OrderItem/ShopUserBasedExtensionSpec.php(4 hunks)src/Sylius/Bundle/ApiBundle/spec/Doctrine/ORM/QueryExtension/Shop/OrderItem/VisitorBasedExtensionSpec.php(4 hunks)src/Sylius/Bundle/ApiBundle/spec/Doctrine/ORM/QueryExtension/Shop/Product/TaxonBasedExtensionSpec.php(2 hunks)src/Sylius/Bundle/ApiBundle/spec/Doctrine/ORM/QueryExtension/Shop/ProductReview/AcceptedExtensionSpec.php(1 hunks)src/Sylius/Bundle/ApiBundle/spec/Doctrine/ORM/QueryExtension/Shop/Taxon/ChannelBasedExtensionSpec.php(1 hunks)src/Sylius/Bundle/ApiBundle/spec/Doctrine/ORM/QueryExtension/Shop/Taxon/EnabledChildrenExtensionSpec.php(1 hunks)src/Sylius/Bundle/ApiBundle/spec/Resolver/UriTemplateParentResourceResolverSpec.php(4 hunks)src/Sylius/Bundle/ApiBundle/spec/StateProvider/Shop/Product/ProductAttributeValue/CollectionProviderSpec.php(2 hunks)src/Sylius/Bundle/AttributeBundle/Doctrine/ORM/Subscriber/LoadMetadataSubscriber.php(3 hunks)src/Sylius/Bundle/AttributeBundle/spec/Doctrine/ORM/Subscriber/LoadMetadataSubscriberSpec.php(5 hunks)src/Sylius/Bundle/CoreBundle/Doctrine/DQL/DateFormat.php(1 hunks)src/Sylius/Bundle/CoreBundle/Doctrine/DQL/Day.php(2 hunks)src/Sylius/Bundle/CoreBundle/Doctrine/DQL/Hour.php(2 hunks)src/Sylius/Bundle/CoreBundle/Doctrine/DQL/Month.php(2 hunks)src/Sylius/Bundle/CoreBundle/Doctrine/DQL/Week.php(2 hunks)src/Sylius/Bundle/CoreBundle/Doctrine/DQL/Year.php(2 hunks)src/Sylius/Bundle/CoreBundle/Doctrine/ORM/Handler/ResourceDeleteHandler.php(1 hunks)src/Sylius/Bundle/CoreBundle/Doctrine/ORM/SqlWalker/OrderByIdentifierSqlWalker.php(1 hunks)src/Sylius/Bundle/CoreBundle/Remover/ReviewerReviewsRemover.php(2 hunks)src/Sylius/Bundle/CoreBundle/spec/Doctrine/ORM/Handler/ResourceDeleteHandlerSpec.php(2 hunks)src/Sylius/Bundle/CoreBundle/spec/EventListener/CanonicalizerListenerSpec.php(1 hunks)src/Sylius/Bundle/CoreBundle/spec/Order/NumberGenerator/SequentialOrderNumberGeneratorSpec.php(3 hunks)src/Sylius/Bundle/CoreBundle/spec/Remover/ReviewerReviewsRemoverSpec.php(1 hunks)src/Sylius/Bundle/LocaleBundle/spec/Checker/LocaleUsageCheckerSpec.php(3 hunks)src/Sylius/Bundle/OrderBundle/spec/NumberGenerator/SequentialOrderNumberGeneratorSpec.php(3 hunks)src/Sylius/Bundle/ProductBundle/Doctrine/ORM/Query/AST/Function/AbstractJsonFunctionNode.php(6 hunks)src/Sylius/Bundle/ProductBundle/spec/EventListener/SelectProductAttributeChoiceRemoveListenerSpec.php(3 hunks)src/Sylius/Bundle/ReviewBundle/EventListener/ReviewChangeListener.php(1 hunks)src/Sylius/Bundle/ReviewBundle/spec/EventListener/ReviewChangeListenerSpec.php(1 hunks)src/Sylius/Bundle/UserBundle/spec/EventListener/PasswordUpdaterListenerSpec.php(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (33)
- src/Sylius/Bundle/ApiBundle/spec/Doctrine/ORM/QueryExtension/Shop/Taxon/ChannelBasedExtensionSpec.php
- src/Sylius/Bundle/CoreBundle/spec/EventListener/CanonicalizerListenerSpec.php
- src/Sylius/Bundle/ApiBundle/spec/Doctrine/ORM/QueryExtension/Shop/Order/StateBasedExtensionSpec.php
- src/Sylius/Bundle/ApiBundle/spec/Doctrine/ORM/QueryExtension/Shop/ProductReview/AcceptedExtensionSpec.php
- src/Sylius/Bundle/CoreBundle/Doctrine/DQL/Day.php
- src/Sylius/Bundle/ApiBundle/spec/Doctrine/ORM/QueryExtension/Shop/ExchangeRate/ChannelBasedExtensionSpec.php
- src/Sylius/Bundle/CoreBundle/Doctrine/DQL/Year.php
- src/Sylius/Bundle/ApiBundle/spec/Doctrine/ORM/QueryExtension/Shop/Address/ShopUserBasedExtensionSpec.php
- src/Sylius/Bundle/CoreBundle/Doctrine/DQL/Month.php
- src/Sylius/Bundle/ApiBundle/spec/Doctrine/ORM/QueryExtension/Common/NonArchivedExtensionSpec.php
- src/Sylius/Bundle/CoreBundle/Doctrine/ORM/SqlWalker/OrderByIdentifierSqlWalker.php
- src/Sylius/Bundle/ApiBundle/spec/StateProvider/Shop/Product/ProductAttributeValue/CollectionProviderSpec.php
- src/Sylius/Bundle/ApiBundle/spec/Doctrine/ORM/QueryExtension/Shop/Taxon/EnabledChildrenExtensionSpec.php
- src/Sylius/Bundle/ApiBundle/spec/Doctrine/ORM/QueryExtension/Shop/Product/TaxonBasedExtensionSpec.php
- src/Sylius/Bundle/CoreBundle/Doctrine/DQL/Week.php
- src/Sylius/Bundle/ApiBundle/spec/Doctrine/ORM/QueryExtension/Shop/Customer/ShopUserBasedExtensionSpec.php
- src/Sylius/Bundle/CoreBundle/Doctrine/DQL/Hour.php
- src/Sylius/Bundle/ApiBundle/spec/Doctrine/ORM/QueryExtension/Shop/Currency/ChannelBasedExtensionSpec.php
- src/Sylius/Bundle/ApiBundle/spec/Doctrine/ORM/QueryExtension/Shop/OrderItem/ShopUserBasedExtensionSpec.php
- src/Sylius/Bundle/ReviewBundle/EventListener/ReviewChangeListener.php
- src/Sylius/Bundle/UserBundle/spec/EventListener/PasswordUpdaterListenerSpec.php
- src/Sylius/Bundle/CoreBundle/Doctrine/ORM/Handler/ResourceDeleteHandler.php
- src/Sylius/Bundle/CoreBundle/Remover/ReviewerReviewsRemover.php
- src/Sylius/Bundle/ReviewBundle/spec/EventListener/ReviewChangeListenerSpec.php
- src/Sylius/Bundle/CoreBundle/spec/Order/NumberGenerator/SequentialOrderNumberGeneratorSpec.php
- src/Sylius/Bundle/CoreBundle/spec/Remover/ReviewerReviewsRemoverSpec.php
- src/Sylius/Bundle/ApiBundle/spec/Doctrine/ORM/QueryExtension/Shop/Order/ShopUserBasedExtensionSpec.php
- src/Sylius/Bundle/CoreBundle/Doctrine/DQL/DateFormat.php
- src/Sylius/Bundle/ProductBundle/Doctrine/ORM/Query/AST/Function/AbstractJsonFunctionNode.php
- src/Sylius/Bundle/AttributeBundle/Doctrine/ORM/Subscriber/LoadMetadataSubscriber.php
- src/Sylius/Bundle/OrderBundle/spec/NumberGenerator/SequentialOrderNumberGeneratorSpec.php
- src/Sylius/Bundle/ApiBundle/spec/CommandHandler/Cart/RemoveItemFromCartHandlerSpec.php
- src/Sylius/Bundle/AttributeBundle/spec/Doctrine/ORM/Subscriber/LoadMetadataSubscriberSpec.php
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Static checks / PHP 8.3, Symfony ^7.1
- GitHub Check: Static checks / PHP 8.2, Symfony ^6.4
🔇 Additional comments (10)
src/Sylius/Bundle/CoreBundle/spec/Doctrine/ORM/Handler/ResourceDeleteHandlerSpec.php (3)
18-19: LGTM! Clean handling of backward compatibility.The import statements are well-structured, using appropriate aliasing to handle both legacy and new Doctrine ORM exception classes.
71-75: Add TODO comment for future maintenance.This backward compatibility code block should be documented with a TODO comment to facilitate its removal in future versions.
Add a TODO comment before the conditional block:
+ // TODO: Remove this conditional block when support for older Doctrine ORM versions is dropped if (interface_exists(ORMException::class)) { $ormException = new class() extends \RuntimeException implements ORMException {}; } else { $ormException = new class() extends ORMException {}; }
78-78: LGTM! Consistent exception usage.The dynamically created exception instance is correctly used in the test case.
src/Sylius/Bundle/ProductBundle/spec/EventListener/SelectProductAttributeChoiceRemoveListenerSpec.php (2)
16-29: LGTM! Import changes align with ORM deprecation fixes.The shift from ORM-specific to Persistence namespace and the addition of more specific DB-related imports improve the code's alignment with Doctrine's best practices.
100-189: LGTM! Edge cases and negative scenarios are well covered.The test methods properly verify that the listener doesn't perform unnecessary operations in various scenarios, maintaining good test coverage.
src/Sylius/Bundle/ApiBundle/spec/Doctrine/ORM/QueryExtension/Shop/Order/VisitorBasedExtensionSpec.php (1)
85-87: LGTM! Improved type safety in expression handling.The changes enhance the code by:
- Using specific expression types instead of generic ones
- Employing string literals for NULL checks
- Properly chaining expressions with typed methods
Also applies to: 100-107
src/Sylius/Bundle/ApiBundle/spec/Doctrine/ORM/QueryExtension/Shop/OrderItem/VisitorBasedExtensionSpec.php (1)
86-88: LGTM! Consistent expression handling improvements.The changes maintain consistency with the Order extension and improve type safety by:
- Using specific expression types
- Employing string literals for NULL checks
- Properly chaining expressions with typed methods
Also applies to: 103-109, 164-166, 181-187
src/Sylius/Bundle/ApiBundle/spec/Resolver/UriTemplateParentResourceResolverSpec.php (2)
19-21: LGTM! Import changes align with ORM deprecation fixes.The new imports correctly reflect the transition from generic repository interfaces to specific Doctrine ORM types, which is in line with the PR's objective of fixing ORM deprecations.
Also applies to: 25-25
113-133: LGTM! Changes maintain consistency with exception test.The updates to parent resource resolution maintain consistency with the exception test changes while improving type safety through the use of
::classsyntax in the Link parameter.src/Sylius/Bundle/LocaleBundle/spec/Checker/LocaleUsageCheckerSpec.php (1)
17-19: LGTM! New imports align with ORM refactoring.The added Doctrine ORM imports support the transition to using EntityPersister, which is part of the ORM deprecation fixes.
Extracted from #17635
Summary by CodeRabbit
Refactor
Chore
These updates contribute to a more efficient and reliable user experience throughout the shopping and order processes.