Fix autocomplete in admin to be case-insensitive#18477
Fix autocomplete in admin to be case-insensitive#18477Glancu wants to merge 1 commit intoSylius:2.1from
Conversation
|
Warning Rate limit exceeded@Rafikooo has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 21 minutes and 8 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 (6)
WalkthroughThe PR introduces case-insensitive parent taxon autocomplete search, modernizes Behat testing infrastructure by replacing JQueryHelper with DriverHelper, refactors CookieSetter with stricter typing and readonly class properties, introduces specialized HTTP exceptions for API cart handlers, removes obsolete helper utilities, and updates CI workflows to target Symfony 7.4 beta. Changes
Sequence Diagram(s)sequenceDiagram
participant User as Test/Admin
participant FormElement
participant AutocompleteHelper
participant TranslatableAutocompleteType
participant Database
User->>FormElement: searchParentTaxon(searchTerm)
FormElement->>FormElement: waitForPageToLoad()
FormElement->>AutocompleteHelper: search(parentFieldXPath, searchTerm)
AutocompleteHelper->>AutocompleteHelper: waitForAsynchronousActionsToFinish()
AutocompleteHelper->>Database: Query with LOWER(field) LIKE LOWER(:query)
Database-->>AutocompleteHelper: Matching results
AutocompleteHelper-->>FormElement: Search results array
FormElement-->>User: Results for assertion
sequenceDiagram
participant PageClass
participant DriverHelper
participant Session as Mink Session
participant Browser as JavaScript Driver
PageClass->>DriverHelper: waitForElement(session, selector)
DriverHelper->>Session: Determine driver type
alt JavaScript Driver
DriverHelper->>Browser: Wait for selector presence
Browser-->>DriverHelper: Element ready
else Non-JavaScript Driver
DriverHelper-->>PageClass: No-op (skip)
end
PageClass->>PageClass: Execute page logic
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes
Possibly related issues
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (2 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 |
❌ Preview Environment deleted from BunnyshellAvailable commands:
|
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (7)
src/Sylius/Bundle/AdminBundle/Form/Type/TranslatableAutocompleteType.php (1)
115-116: Lower the parameter in PHP; compare LOWER(field) to :queryAvoid
LOWER(:query)on the RHS; lower the param once and use:query. Simpler DQL and more portable.Apply:
- ->setParameter('query', '%' . $query . '%') + ->setParameter('query', '%' . mb_strtolower($query, 'UTF-8') . '%')and
- yield $expr->like((string) $expr->lower($field), 'LOWER(:query)'); + yield $expr->like((string) $expr->lower($field), ':query');src/Sylius/Behat/Element/Admin/Taxon/FormElementInterface.php (1)
48-50: Interface addition looks goodSignature matches the implementation.
Optionally document return as
list<string>via PHPDoc for clarity.src/Sylius/Behat/Element/Admin/Taxon/FormElement.php (1)
145-156: Solid implementation of autocomplete searchWait + delegate + normalize is correct.
Add
@return list<string>PHPDoc to communicate expected shape.src/Sylius/Behat/Context/Setup/TaxonomyContext.php (2)
43-48: Add explicit return and parameter types to step methodPer guidelines, add types.
Apply:
- public function storeClassifiesItsProductsAs(...$taxonsNames) + public function storeClassifiesItsProductsAs(string ...$taxonsNames): void
61-71: Type the parameters and return type for this stepKeep consistency with strict typing.
Apply:
- public function theStoreHasTaxonomyNamedInAndIn($firstName, $firstLocale, $secondName, $secondLocale) + public function theStoreHasTaxonomyNamedInAndIn( + string $firstName, + string $firstLocale, + string $secondName, + string $secondLocale, + ): voidsrc/Sylius/Behat/Context/Ui/Admin/ManagingTaxonsContext.php (2)
16-18: Remove unused/incorrect import
use Behat\Step\When;isn’t used here and is not an Attribute. Remove it.-use Behat\Step\When;
500-514: Harden assertion against missing shared storage keyGuard against null to avoid warnings in failure paths.
Apply:
- $autocompleteSearchResults = $this->sharedStorage->get('autocompleteSearchResults'); + $autocompleteSearchResults = $this->sharedStorage->get('autocompleteSearchResults') ?? [];
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
features/admin/taxonomy/managing_taxons/ui/case_insensitive_parent_search.feature(1 hunks)src/Sylius/Behat/Context/Setup/TaxonomyContext.php(8 hunks)src/Sylius/Behat/Context/Ui/Admin/ManagingTaxonsContext.php(4 hunks)src/Sylius/Behat/Element/Admin/Taxon/FormElement.php(1 hunks)src/Sylius/Behat/Element/Admin/Taxon/FormElementInterface.php(1 hunks)src/Sylius/Bundle/AdminBundle/Form/Type/TranslatableAutocompleteType.php(1 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
**/*.{php,yaml,yml,xml,twig}
📄 CodeRabbit inference engine (AGENTS.md)
Use 4 spaces for indentation in PHP, YAML, XML, and Twig files
Files:
src/Sylius/Bundle/AdminBundle/Form/Type/TranslatableAutocompleteType.phpsrc/Sylius/Behat/Element/Admin/Taxon/FormElement.phpsrc/Sylius/Behat/Element/Admin/Taxon/FormElementInterface.phpsrc/Sylius/Behat/Context/Setup/TaxonomyContext.phpsrc/Sylius/Behat/Context/Ui/Admin/ManagingTaxonsContext.php
**/*.php
📄 CodeRabbit inference engine (AGENTS.md)
**/*.php: Use modern PHP 8.2+ syntax and features
Declare strict_types=1 in all PHP files
Follow the Sylius Coding Standard
Do not use deprecated features from PHP, Symfony, or Sylius
Use final for all classes, except entities and repositories
Use readonly for immutable services and value objects
Add type declarations for all properties, arguments, and return values
Use camelCase for variables and method names
Use SCREAMING_SNAKE_CASE for constants
Use fast returns instead of unnecessary nesting
Use trailing commas in multi-line arrays and argument lists
Order array keys alphabetically where applicable
Use PHPDoc only when necessary (e.g., @var Collection)
Group class elements in order: constants, properties, constructor, public, protected, private methods
Group getter and setter methods for the same properties together
Suffix interfaces with Interface and traits with Trait
Use use statements for all non-global classes
Sort use imports alphabetically and group by type (classes, functions, constants)
Files:
src/Sylius/Bundle/AdminBundle/Form/Type/TranslatableAutocompleteType.phpsrc/Sylius/Behat/Element/Admin/Taxon/FormElement.phpsrc/Sylius/Behat/Element/Admin/Taxon/FormElementInterface.phpsrc/Sylius/Behat/Context/Setup/TaxonomyContext.phpsrc/Sylius/Behat/Context/Ui/Admin/ManagingTaxonsContext.php
{features/**/*.feature,behat.yml*}
📄 CodeRabbit inference engine (AGENTS.md)
Use Behat for behavior-driven scenarios
Files:
features/admin/taxonomy/managing_taxons/ui/case_insensitive_parent_search.feature
🧬 Code graph analysis (3)
src/Sylius/Behat/Element/Admin/Taxon/FormElement.php (1)
src/Sylius/Behat/Element/Admin/Taxon/FormElementInterface.php (1)
searchParentTaxon(49-49)
src/Sylius/Behat/Element/Admin/Taxon/FormElementInterface.php (1)
src/Sylius/Behat/Element/Admin/Taxon/FormElement.php (1)
searchParentTaxon(145-156)
src/Sylius/Behat/Context/Ui/Admin/ManagingTaxonsContext.php (2)
src/Sylius/Behat/Element/Admin/Taxon/FormElement.php (1)
searchParentTaxon(145-156)src/Sylius/Behat/Element/Admin/Taxon/FormElementInterface.php (1)
searchParentTaxon(49-49)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: End-to-end tests (MariaDB) / Get matrix
- GitHub Check: Packages / Get matrix
- GitHub Check: Frontend / Get matrix
🔇 Additional comments (5)
src/Sylius/Behat/Context/Ui/Admin/ManagingTaxonsContext.php (1)
234-241: Step stores results correctlyLooks good and aligns with the new element API.
features/admin/taxonomy/managing_taxons/ui/case_insensitive_parent_search.feature (4)
1-13: Well-structured feature setup.The feature declaration clearly describes the business value, and the Background sets up diverse taxon codes with intentional case variations (mixed uppercase/lowercase) to comprehensively test case-insensitive search behavior.
15-31: Well-designed code-based case variation coverage.Scenarios 1–3 test three distinct case transformations (lowercase, uppercase, mixed) and each scenario verifies that the autocomplete correctly finds the taxon regardless of the search term's case.
33-43: Name-based search scenarios add meaningful coverage.Scenarios 4–5 complement the code-based searches by testing case-insensitive matching on the taxon name itself (with spaces), ensuring the feature works across both codes and display names.
45-49: Partial-match scenario provides edge-case coverage.Scenario 6 tests partial code matching (searching "SPORTS" against "SPORTS_equipment"), confirming that case-insensitive search works for partial terms as well.
| use Behat\Step\Given; | ||
| use Doctrine\Persistence\ObjectManager; |
There was a problem hiding this comment.
Incorrect attribute import for step definitions
Behat\Step\Given is not an Attribute. Use the Behat attribute class.
Apply:
-use Behat\Step\Given;
+use Behat\Behat\Context\Attribute\Given;🤖 Prompt for AI Agents
In src/Sylius/Behat/Context/Setup/TaxonomyContext.php around lines 17 to 18, the
file imports Behat\Step\Given which is not the Attribute class; replace that
import with the Behat attribute namespace (use
Behat\Behat\Context\Attribute\Given) so the step definition attribute resolves
correctly, and update any other step attribute imports (When/Then) in this file
to their Attribute equivalents if present.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
CONFLICTS.md (1)
10-15: Wrap URL in markdown link syntax.The bare URL on line 15 should be wrapped in markdown link syntax for proper formatting and consistency with documentation best practices. The static analysis tool correctly flags this as MD034 (no-bare-urls).
Apply this diff:
- References: https://github.com/doctrine/orm/issues/12245 + References: [https://github.com/doctrine/orm/issues/12245](https://github.com/doctrine/orm/issues/12245)Alternatively, use a more concise markdown link format:
- References: https://github.com/doctrine/orm/issues/12245 + References: [GitHub issue #12245](https://github.com/doctrine/orm/issues/12245)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
CONFLICTS.md(1 hunks)composer.json(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
composer.json
📄 CodeRabbit inference engine (AGENTS.md)
composer.json: Ensure compatibility with Symfony and PHP versions defined in composer.json
For API configuration, depend on API Platform 4.x
Files:
composer.json
🪛 markdownlint-cli2 (0.18.1)
CONFLICTS.md
15-15: Bare URL used
(MD034, no-bare-urls)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Packages / Get matrix
- GitHub Check: End-to-end tests (PostgreSQL) / Get matrix
- GitHub Check: End-to-end tests (MySQL) / Get matrix
- GitHub Check: End-to-end tests (MariaDB) / Get matrix
🔇 Additional comments (1)
composer.json (1)
246-246: Add conflict entry for doctrine/orm regression.The conflict entry correctly blocks the two problematic doctrine/orm versions that contain a regression with empty array queries. This allows the rest of the version range (^2.18 || ^3.3) to function properly.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
src/Sylius/Behat/Context/Setup/TaxonomyContext.php (1)
17-17: Incorrect attribute import for step definitions
Behat\Step\Givenis not the correct attribute class. UseBehat\Behat\Context\Attribute\Giveninstead so the step definition attributes resolve correctly.Apply this diff:
-use Behat\Step\Given; +use Behat\Behat\Context\Attribute\Given;
🧹 Nitpick comments (1)
src/Sylius/Behat/Context/Ui/Admin/ManagingTaxonsContext.php (1)
17-17: Remove unused import.The
Behat\Step\Whenimport is not used anywhere in this file.Apply this diff:
-use Behat\Step\When;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/Sylius/Behat/Context/Setup/TaxonomyContext.php(8 hunks)src/Sylius/Behat/Context/Ui/Admin/ManagingTaxonsContext.php(3 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{php,yaml,yml,xml,twig}
📄 CodeRabbit inference engine (AGENTS.md)
Use 4 spaces for indentation in PHP, YAML, XML, and Twig files
Files:
src/Sylius/Behat/Context/Ui/Admin/ManagingTaxonsContext.phpsrc/Sylius/Behat/Context/Setup/TaxonomyContext.php
**/*.php
📄 CodeRabbit inference engine (AGENTS.md)
**/*.php: Use modern PHP 8.2+ syntax and features
Declare strict_types=1 in all PHP files
Follow the Sylius Coding Standard
Do not use deprecated features from PHP, Symfony, or Sylius
Use final for all classes, except entities and repositories
Use readonly for immutable services and value objects
Add type declarations for all properties, arguments, and return values
Use camelCase for variables and method names
Use SCREAMING_SNAKE_CASE for constants
Use fast returns instead of unnecessary nesting
Use trailing commas in multi-line arrays and argument lists
Order array keys alphabetically where applicable
Use PHPDoc only when necessary (e.g., @var Collection)
Group class elements in order: constants, properties, constructor, public, protected, private methods
Group getter and setter methods for the same properties together
Suffix interfaces with Interface and traits with Trait
Use use statements for all non-global classes
Sort use imports alphabetically and group by type (classes, functions, constants)
Files:
src/Sylius/Behat/Context/Ui/Admin/ManagingTaxonsContext.phpsrc/Sylius/Behat/Context/Setup/TaxonomyContext.php
🧠 Learnings (1)
📚 Learning: 2025-09-01T12:06:56.917Z
Learnt from: CR
PR: Sylius/Sylius#0
File: AGENTS.md:0-0
Timestamp: 2025-09-01T12:06:56.917Z
Learning: Applies to **/*.php : Use readonly for immutable services and value objects
Applied to files:
src/Sylius/Behat/Context/Ui/Admin/ManagingTaxonsContext.php
🧬 Code graph analysis (1)
src/Sylius/Behat/Context/Ui/Admin/ManagingTaxonsContext.php (2)
src/Sylius/Behat/Element/Admin/Taxon/FormElementInterface.php (1)
searchParentTaxon(49-49)src/Sylius/Behat/Element/Admin/Taxon/FormElement.php (1)
searchParentTaxon(145-156)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Static checks / PHP 8.4, Symfony ^7.3
🔇 Additional comments (4)
src/Sylius/Behat/Context/Setup/TaxonomyContext.php (4)
43-47: LGTM: Clean migration to PHP 8 attributesThe conversion from PHPDoc annotations to PHP 8 attributes follows modern PHP conventions and is consistently applied across all step definitions.
Also applies to: 61-61, 72-72, 95-95, 112-115, 126-126, 134-134
55-59: LGTM: New step definition follows established patternsThe new method correctly enables creating taxons with explicit codes in Behat scenarios. The implementation follows the existing conventions in this context class.
173-173: LGTM: Return type improves type safetyAdding the explicit
TaxonInterfacereturn type follows the coding guidelines and enhances type safety.
153-162: The original review comment is incorrect and should be dismissed.The feature file
case_insensitive_parent_search.featurereveals thatcreateTaxonWithCode()is intentionally used with non-normalized codes like"TECH_GADGETS","home_DECOR", and"SPORTS_equipment". These mixed-case codes are critical to the test scenario, which verifies that parent search works case-insensitively.If code normalization were applied (converting all to lowercase), the test assertions would fail because the expected codes would no longer match. The method's design—accepting codes as-is without normalization—is correct and deliberate for accommodating test-specific scenarios.
Unlike
createTaxon()which generates a code from a name and needs normalization for consistency,createTaxonWithCode()is designed for explicit, test-controlled codes that may need to preserve specific formatting for edge-case testing.Likely an incorrect or invalid review comment.
| public function iSearchForParentTaxon(string $searchTerm) | ||
| { | ||
| $this->sharedStorage->set('autocompleteSearchResults', $this->formElement->searchParentTaxon($searchTerm)); | ||
|
|
||
| return $this->formElement->searchParentTaxon($searchTerm); | ||
| } |
There was a problem hiding this comment.
Add missing return type and eliminate duplicate call.
The method has two issues:
- Missing return type declaration violates the coding guidelines requiring type declarations for all return values.
searchParentTaxonis called twice—once to store results (line 239) and again to return them (line 241). This is inefficient and could theoretically produce inconsistent results.
Apply this diff:
- public function iSearchForParentTaxon(string $searchTerm)
+ public function iSearchForParentTaxon(string $searchTerm): void
{
- $this->sharedStorage->set('autocompleteSearchResults', $this->formElement->searchParentTaxon($searchTerm));
-
- return $this->formElement->searchParentTaxon($searchTerm);
+ $searchResults = $this->formElement->searchParentTaxon($searchTerm);
+ $this->sharedStorage->set('autocompleteSearchResults', $searchResults);
}As per coding guidelines.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| public function iSearchForParentTaxon(string $searchTerm) | |
| { | |
| $this->sharedStorage->set('autocompleteSearchResults', $this->formElement->searchParentTaxon($searchTerm)); | |
| return $this->formElement->searchParentTaxon($searchTerm); | |
| } | |
| public function iSearchForParentTaxon(string $searchTerm): void | |
| { | |
| $searchResults = $this->formElement->searchParentTaxon($searchTerm); | |
| $this->sharedStorage->set('autocompleteSearchResults', $searchResults); | |
| } |
🤖 Prompt for AI Agents
In src/Sylius/Behat/Context/Ui/Admin/ManagingTaxonsContext.php around lines 237
to 242, the method iSearchForParentTaxon is missing a return type and calls
searchParentTaxon twice; capture the search result in a local variable, set
sharedStorage with that variable, and return it, and add the appropriate return
type declaration (e.g., : array) to the method signature to comply with coding
guidelines.
| public function iShouldSeeInTheFoundResults(string $taxonName) | ||
| { | ||
| $autocompleteSearchResults = $this->sharedStorage->get('autocompleteSearchResults'); | ||
| Assert::true( | ||
| in_array($taxonName, $autocompleteSearchResults, true), | ||
| sprintf( | ||
| 'Expected to see "%s" in autocomplete results, but found: %s', | ||
| $taxonName, | ||
| implode(', ', $autocompleteSearchResults), | ||
| ), | ||
| ); | ||
| } |
There was a problem hiding this comment.
Add missing return type and validate shared storage content.
The method has two issues:
- Missing return type declaration violates the coding guidelines requiring type declarations for all return values.
- No validation that
autocompleteSearchResultsexists in shared storage or that it's an array before using it within_array()andimplode(). This could cause runtime errors if the step runs out of sequence or if the stored value has an unexpected type.
Apply this diff:
- public function iShouldSeeInTheFoundResults(string $taxonName)
+ public function iShouldSeeInTheFoundResults(string $taxonName): void
{
$autocompleteSearchResults = $this->sharedStorage->get('autocompleteSearchResults');
+ Assert::isArray($autocompleteSearchResults, 'Expected autocompleteSearchResults to be an array in shared storage.');
+
Assert::true(
in_array($taxonName, $autocompleteSearchResults, true),
sprintf(
'Expected to see "%s" in autocomplete results, but found: %s',
$taxonName,
implode(', ', $autocompleteSearchResults),
),
);
}As per coding guidelines.
🤖 Prompt for AI Agents
In src/Sylius/Behat/Context/Ui/Admin/ManagingTaxonsContext.php around lines 505
to 516, add a return type declaration to the method (use : void) and validate
the shared storage value before using it: retrieve 'autocompleteSearchResults',
assert it exists and is an array (fail with a clear message if not), then
perform the in_array() check and build the implode() output safely from the
confirmed array; update the Assert call messages accordingly so the method never
calls in_array()/implode() on a non-array.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/Sylius/Bundle/ApiBundle/CommandHandler/Cart/BlameCartHandler.php (1)
1-65: Critical inconsistency: File changes don't match PR objectives.This file changes cart exception handling in the API bundle, but the PR is titled "Fix autocomplete in admin to be case-insensitive" and describes implementing case-insensitive parent-taxon autocomplete. These cart-related exception changes appear completely unrelated to the stated PR objectives.
Please verify:
- Are these the correct files for this PR?
- If these changes belong in this PR, please update the PR description to accurately reflect all changes.
- If these are unrelated changes, they should be in a separate PR.
Technical review: Exception handling changes are correct.
The replacement of generic
InvalidArgumentExceptionwith HTTP-specific exceptions is appropriate:
- Line 47:
NotFoundHttpExceptionfor missing user (404 status) ✓- Line 54:
UnprocessableCartExceptionfor missing cart (422 status) ✓- Line 58:
ConflictHttpExceptionfor already-assigned cart (409 status) ✓The HTTP semantics align with REST API best practices.
🧹 Nitpick comments (1)
src/Sylius/Bundle/ApiBundle/Exception/ProductVariantUnprocessableException.php (1)
1-27: LGTM with optional suggestion: Exception class is well-structured.The exception class follows best practices:
- Extends
UnprocessableEntityHttpExceptionfor 422 status ✓- Uses
finalas per coding guidelines ✓- Declares
strict_types=1✓- Constructor signature aligns with parent class ✓
Optional refinement: Consider a more specific error message.
The current message "Product variant could not be processed." is somewhat vague. Based on the usage in
AddItemToCartHandler.php(line 50-51), this exception is thrown when a product variant is not found. Consider making the message more specific:- parent::__construct('Product variant could not be processed.', $previous, $code, $headers); + parent::__construct('Product variant with given code has not been found.', $previous, $code, $headers);This would be more consistent with
UnprocessableCartException's message and provide clearer feedback to API consumers.Based on coding guidelines.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (23)
.github/workflows/ci_e2e-unstable.yaml(3 hunks).github/workflows/ci_packages-unstable.yaml(1 hunks)features/admin/taxonomy/managing_taxons/editing_taxon_slug.feature(3 hunks)src/Sylius/Behat/Page/Admin/Crud/IndexPage.php(2 hunks)src/Sylius/Behat/Page/Admin/DashboardPage.php(2 hunks)src/Sylius/Behat/Page/Admin/Product/IndexPerTaxonPage.php(2 hunks)src/Sylius/Behat/Page/Shop/Account/AddressBook/CreatePage.php(2 hunks)src/Sylius/Behat/Page/Shop/Account/AddressBook/UpdatePage.php(2 hunks)src/Sylius/Behat/Page/Shop/Checkout/AddressPage.php(1 hunks)src/Sylius/Behat/Service/Accessor/NotificationAccessor.php(2 hunks)src/Sylius/Behat/Service/AutocompleteHelper.php(3 hunks)src/Sylius/Behat/Service/DriverHelper.php(1 hunks)src/Sylius/Behat/Service/JQueryHelper.php(0 hunks)src/Sylius/Behat/Service/Setter/CookieSetter.php(3 hunks)src/Sylius/Behat/Service/Setter/CookieSetterInterface.php(1 hunks)src/Sylius/Behat/Service/SlugGenerationHelper.php(0 hunks)src/Sylius/Bundle/ApiBundle/CommandHandler/Cart/AddItemToCartHandler.php(2 hunks)src/Sylius/Bundle/ApiBundle/CommandHandler/Cart/BlameCartHandler.php(2 hunks)src/Sylius/Bundle/ApiBundle/Exception/ProductVariantUnprocessableException.php(1 hunks)src/Sylius/Bundle/ApiBundle/Exception/UnprocessableCartException.php(1 hunks)src/Sylius/Bundle/ApiBundle/tests/CommandHandler/Cart/AddItemToCartHandlerTest.php(3 hunks)src/Sylius/Bundle/ApiBundle/tests/CommandHandler/Cart/BlameCartHandlerTest.php(3 hunks)tests/Api/Shop/Checkout/CartTest.php(1 hunks)
💤 Files with no reviewable changes (2)
- src/Sylius/Behat/Service/JQueryHelper.php
- src/Sylius/Behat/Service/SlugGenerationHelper.php
🧰 Additional context used
📓 Path-based instructions (4)
**/*.{php,yaml,yml,xml,twig}
📄 CodeRabbit inference engine (AGENTS.md)
Use 4 spaces for indentation in PHP, YAML, XML, and Twig files
Files:
src/Sylius/Bundle/ApiBundle/tests/CommandHandler/Cart/BlameCartHandlerTest.phpsrc/Sylius/Behat/Page/Admin/Crud/IndexPage.phpsrc/Sylius/Behat/Service/Setter/CookieSetterInterface.phpsrc/Sylius/Behat/Page/Admin/DashboardPage.phptests/Api/Shop/Checkout/CartTest.phpsrc/Sylius/Behat/Page/Admin/Product/IndexPerTaxonPage.phpsrc/Sylius/Bundle/ApiBundle/CommandHandler/Cart/AddItemToCartHandler.phpsrc/Sylius/Behat/Service/DriverHelper.phpsrc/Sylius/Behat/Page/Shop/Checkout/AddressPage.phpsrc/Sylius/Bundle/ApiBundle/CommandHandler/Cart/BlameCartHandler.phpsrc/Sylius/Bundle/ApiBundle/Exception/UnprocessableCartException.phpsrc/Sylius/Behat/Page/Shop/Account/AddressBook/CreatePage.phpsrc/Sylius/Behat/Page/Shop/Account/AddressBook/UpdatePage.phpsrc/Sylius/Behat/Service/Accessor/NotificationAccessor.phpsrc/Sylius/Bundle/ApiBundle/tests/CommandHandler/Cart/AddItemToCartHandlerTest.phpsrc/Sylius/Behat/Service/AutocompleteHelper.phpsrc/Sylius/Behat/Service/Setter/CookieSetter.phpsrc/Sylius/Bundle/ApiBundle/Exception/ProductVariantUnprocessableException.php
**/*.php
📄 CodeRabbit inference engine (AGENTS.md)
**/*.php: Use modern PHP 8.2+ syntax and features
Declare strict_types=1 in all PHP files
Follow the Sylius Coding Standard
Do not use deprecated features from PHP, Symfony, or Sylius
Use final for all classes, except entities and repositories
Use readonly for immutable services and value objects
Add type declarations for all properties, arguments, and return values
Use camelCase for variables and method names
Use SCREAMING_SNAKE_CASE for constants
Use fast returns instead of unnecessary nesting
Use trailing commas in multi-line arrays and argument lists
Order array keys alphabetically where applicable
Use PHPDoc only when necessary (e.g., @var Collection)
Group class elements in order: constants, properties, constructor, public, protected, private methods
Group getter and setter methods for the same properties together
Suffix interfaces with Interface and traits with Trait
Use use statements for all non-global classes
Sort use imports alphabetically and group by type (classes, functions, constants)
Files:
src/Sylius/Bundle/ApiBundle/tests/CommandHandler/Cart/BlameCartHandlerTest.phpsrc/Sylius/Behat/Page/Admin/Crud/IndexPage.phpsrc/Sylius/Behat/Service/Setter/CookieSetterInterface.phpsrc/Sylius/Behat/Page/Admin/DashboardPage.phptests/Api/Shop/Checkout/CartTest.phpsrc/Sylius/Behat/Page/Admin/Product/IndexPerTaxonPage.phpsrc/Sylius/Bundle/ApiBundle/CommandHandler/Cart/AddItemToCartHandler.phpsrc/Sylius/Behat/Service/DriverHelper.phpsrc/Sylius/Behat/Page/Shop/Checkout/AddressPage.phpsrc/Sylius/Bundle/ApiBundle/CommandHandler/Cart/BlameCartHandler.phpsrc/Sylius/Bundle/ApiBundle/Exception/UnprocessableCartException.phpsrc/Sylius/Behat/Page/Shop/Account/AddressBook/CreatePage.phpsrc/Sylius/Behat/Page/Shop/Account/AddressBook/UpdatePage.phpsrc/Sylius/Behat/Service/Accessor/NotificationAccessor.phpsrc/Sylius/Bundle/ApiBundle/tests/CommandHandler/Cart/AddItemToCartHandlerTest.phpsrc/Sylius/Behat/Service/AutocompleteHelper.phpsrc/Sylius/Behat/Service/Setter/CookieSetter.phpsrc/Sylius/Bundle/ApiBundle/Exception/ProductVariantUnprocessableException.php
tests/**/*Test.php
📄 CodeRabbit inference engine (AGENTS.md)
Use PHPUnit for unit and functional testing
Files:
tests/Api/Shop/Checkout/CartTest.php
{features/**/*.feature,behat.yml*}
📄 CodeRabbit inference engine (AGENTS.md)
Use Behat for behavior-driven scenarios
Files:
features/admin/taxonomy/managing_taxons/editing_taxon_slug.feature
🧠 Learnings (3)
📚 Learning: 2025-09-01T12:06:56.917Z
Learnt from: CR
Repo: Sylius/Sylius PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-09-01T12:06:56.917Z
Learning: Applies to tests/**/*Api*.php : Use PHPUnit tests to validate API configuration and API responses
Applied to files:
tests/Api/Shop/Checkout/CartTest.php
📚 Learning: 2025-09-01T12:06:56.917Z
Learnt from: CR
Repo: Sylius/Sylius PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-09-01T12:06:56.917Z
Learning: Applies to composer.json : Ensure compatibility with Symfony and PHP versions defined in composer.json
Applied to files:
.github/workflows/ci_packages-unstable.yaml
📚 Learning: 2025-09-01T12:06:56.917Z
Learnt from: CR
Repo: Sylius/Sylius PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-09-01T12:06:56.917Z
Learning: Applies to composer.json : For API configuration, depend on API Platform 4.x
Applied to files:
.github/workflows/ci_packages-unstable.yaml
🧬 Code graph analysis (18)
src/Sylius/Bundle/ApiBundle/tests/CommandHandler/Cart/BlameCartHandlerTest.php (3)
src/Sylius/Bundle/ApiBundle/Exception/UnprocessableCartException.php (1)
UnprocessableCartException(18-27)src/Sylius/Bundle/ApiBundle/CommandHandler/Cart/AddItemToCartHandler.php (1)
__invoke(45-69)src/Sylius/Bundle/ApiBundle/CommandHandler/Cart/BlameCartHandler.php (1)
__invoke(41-64)
src/Sylius/Behat/Page/Admin/Crud/IndexPage.php (1)
src/Sylius/Behat/Service/DriverHelper.php (1)
DriverHelper(22-62)
src/Sylius/Behat/Service/Setter/CookieSetterInterface.php (1)
src/Sylius/Behat/Service/Setter/CookieSetter.php (1)
setCookie(31-47)
src/Sylius/Behat/Page/Admin/DashboardPage.php (1)
src/Sylius/Behat/Page/Shop/Account/AddressBook/UpdatePage.php (1)
waitForElement(120-123)
tests/Api/Shop/Checkout/CartTest.php (1)
tests/Api/JsonApiTestCase.php (2)
setUpDefaultPostHeaders(95-101)requestPost(169-182)
src/Sylius/Behat/Page/Admin/Product/IndexPerTaxonPage.php (1)
src/Sylius/Behat/Service/DriverHelper.php (2)
DriverHelper(22-62)waitForPageToLoad(34-39)
src/Sylius/Bundle/ApiBundle/CommandHandler/Cart/AddItemToCartHandler.php (2)
src/Sylius/Bundle/ApiBundle/Exception/UnprocessableCartException.php (1)
UnprocessableCartException(18-27)src/Sylius/Bundle/ApiBundle/Exception/ProductVariantUnprocessableException.php (1)
ProductVariantUnprocessableException(18-27)
src/Sylius/Behat/Service/DriverHelper.php (2)
src/Sylius/Behat/Page/Shop/Account/AddressBook/UpdatePage.php (2)
waitForElement(120-123)waitForFormToStopLoading(74-77)src/Sylius/Behat/Page/Shop/Checkout/AddressPage.php (1)
waitForFormToStopLoading(284-287)
src/Sylius/Behat/Page/Shop/Checkout/AddressPage.php (2)
src/Sylius/Behat/Service/DriverHelper.php (2)
DriverHelper(22-62)waitForFormToStopLoading(56-61)src/Sylius/Behat/Page/Shop/Account/AddressBook/UpdatePage.php (1)
waitForFormToStopLoading(74-77)
src/Sylius/Bundle/ApiBundle/CommandHandler/Cart/BlameCartHandler.php (1)
src/Sylius/Bundle/ApiBundle/Exception/UnprocessableCartException.php (1)
UnprocessableCartException(18-27)
src/Sylius/Bundle/ApiBundle/Exception/UnprocessableCartException.php (1)
src/Sylius/Bundle/ApiBundle/Exception/ProductVariantUnprocessableException.php (1)
__construct(20-26)
src/Sylius/Behat/Page/Shop/Account/AddressBook/CreatePage.php (4)
src/Sylius/Behat/Service/DriverHelper.php (2)
DriverHelper(22-62)waitForFormToStopLoading(56-61)src/Sylius/Behat/Page/Shop/Account/AddressBook/UpdatePage.php (2)
waitForFormToStopLoading(74-77)selectCountry(64-72)src/Sylius/Behat/Page/Shop/Checkout/AddressPage.php (1)
waitForFormToStopLoading(284-287)src/Sylius/Behat/Page/Shop/Account/AddressBook/CreatePageInterface.php (1)
selectCountry(23-23)
src/Sylius/Behat/Page/Shop/Account/AddressBook/UpdatePage.php (4)
src/Sylius/Behat/Service/DriverHelper.php (2)
DriverHelper(22-62)waitForFormToStopLoading(56-61)src/Sylius/Behat/Page/Shop/Checkout/AddressPage.php (1)
waitForFormToStopLoading(284-287)src/Sylius/Behat/Page/Shop/Account/AddressBook/UpdatePageInterface.php (2)
waitForFormToStopLoading(32-32)saveChanges(34-34)src/Sylius/Behat/Page/Admin/Crud/UpdatePage.php (1)
saveChanges(37-44)
src/Sylius/Behat/Service/Accessor/NotificationAccessor.php (1)
src/Sylius/Behat/Service/DriverHelper.php (2)
DriverHelper(22-62)waitForElement(41-49)
src/Sylius/Bundle/ApiBundle/tests/CommandHandler/Cart/AddItemToCartHandlerTest.php (2)
src/Sylius/Bundle/ApiBundle/Exception/UnprocessableCartException.php (1)
UnprocessableCartException(18-27)src/Sylius/Bundle/ApiBundle/Exception/ProductVariantUnprocessableException.php (1)
ProductVariantUnprocessableException(18-27)
src/Sylius/Behat/Service/AutocompleteHelper.php (1)
src/Sylius/Behat/Service/DriverHelper.php (2)
DriverHelper(22-62)waitForAsynchronousActionsToFinish(51-54)
src/Sylius/Behat/Service/Setter/CookieSetter.php (1)
src/Sylius/Behat/Service/Setter/CookieSetterInterface.php (1)
setCookie(18-18)
src/Sylius/Bundle/ApiBundle/Exception/ProductVariantUnprocessableException.php (1)
src/Sylius/Bundle/ApiBundle/Exception/UnprocessableCartException.php (1)
__construct(20-26)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Static checks / PHP 8.3, Symfony ^6.4
🔇 Additional comments (16)
.github/workflows/ci_packages-unstable.yaml (1)
32-32: Symfony matrix version updated to align with ecosystem upgrade.The bump from
^7.3@betato^7.4@betais consistent with the broader Symfony ecosystem updates across unstable workflows mentioned in the PR context (e.g., ci_e2e-unstable.yaml).Verify that PHP 8.3 + Symfony
^7.4@betaare compatible with the rootcomposer.jsonconstraints for this branch..github/workflows/ci_e2e-unstable.yaml (1)
28-28: Consistent Symfony version bump applied across all test matrices.The version constraint update from
^7.3@betato^7.4@betais applied uniformly to all three job configurations (non-JS, JS with Chromedriver, and JS with Panther), maintaining consistency in test infrastructure.Verify that Symfony 7.4 beta is compatible with the broader test environment and that unstable dependency runs continue to execute successfully. Run the workflow to confirm there are no new failures introduced by this bump.
Also applies to: 101-101, 182-182
src/Sylius/Bundle/ApiBundle/CommandHandler/Cart/AddItemToCartHandler.php (1)
17-18: LGTM: Exception handling improvements are correct.The replacement of
InvalidArgumentExceptionwith domain-specific HTTP exceptions improves API error handling:
- Line 51:
ProductVariantUnprocessableExceptionfor missing product variant- Line 58:
UnprocessableCartExceptionfor missing cartBoth return 422 status, which is semantically appropriate for these scenarios.
Note: The inconsistency between PR objectives and these changes was flagged in the BlameCartHandler.php review.
Also applies to: 51-51, 58-58
src/Sylius/Bundle/ApiBundle/tests/CommandHandler/Cart/AddItemToCartHandlerTest.php (1)
28-29: LGTM: Test expectations correctly updated.The test expectations now match the new exception types thrown by
AddItemToCartHandler:
- Line 96: Expects
ProductVariantUnprocessableExceptionwhen product not found- Line 115: Expects
UnprocessableCartExceptionwhen cart not foundTest logic and assertions remain valid.
Also applies to: 96-96, 115-115
src/Sylius/Bundle/ApiBundle/Exception/UnprocessableCartException.php (1)
1-27: LGTM: Well-structured HTTP exception class.The new exception class follows best practices:
- Extends
UnprocessableEntityHttpExceptionfor 422 status ✓- Uses
finalas per coding guidelines ✓- Declares
strict_types=1✓- Provides clear, fixed error message ✓
- Constructor signature aligns with parent class ✓
The hardcoded message "Cart with given token has not been found." is appropriate since this exception represents a specific error scenario.
Based on coding guidelines.
src/Sylius/Bundle/ApiBundle/tests/CommandHandler/Cart/BlameCartHandlerTest.php (1)
26-28: LGTM: Test expectations align with handler changes.The test expectations correctly reflect the new exception types thrown by
BlameCartHandler:
- Line 85: Expects
ConflictHttpExceptionwhen cart is already assigned- Line 95: Expects
UnprocessableCartExceptionwhen cart not found- Line 101: Expects
NotFoundHttpExceptionwhen user not foundAll three error scenarios are properly tested.
Also applies to: 85-85, 95-95, 101-101
tests/Api/Shop/Checkout/CartTest.php (2)
626-650: LGTM: Comprehensive test for non-existing cart scenario.This test correctly validates the API error handling when adding an item to a non-existing cart:
- Sets up necessary fixtures ✓
- Posts to a non-existing cart token ✓
- Asserts HTTP 422 status ✓
- Verifies error message in
hydra:description✓Follows established API testing patterns.
Based on learnings: API tests should validate API responses.
652-678: LGTM: Comprehensive test for non-existing product variant scenario.This test correctly validates the API error handling when adding a non-existing product variant:
- Creates a valid cart via
pickUpCart()✓- Attempts to add non-existing variant ✓
- Asserts HTTP 422 status ✓
- Verifies error message contains "does not exist" ✓
Complements the previous test by covering the product variant error path.
Based on learnings: API tests should validate API responses.
src/Sylius/Behat/Service/DriverHelper.php (1)
41-49: LGTM: Proper synchronization with JavaScript driver guards.Both
waitForElementandwaitForFormToStopLoadingcorrectly check for JavaScript-enabled drivers before executing waits. The use ofjson_encodefor the selector inwaitForElementproperly escapes special characters.Also applies to: 56-61
src/Sylius/Behat/Service/AutocompleteHelper.php (1)
43-43: LGTM: Successfully migrated from JQueryHelper to DriverHelper.The replacement of
JQueryHelper::waitForAsynchronousActionsToFinishwithDriverHelper::waitForAsynchronousActionsToFinishis correct and aligns with the broader migration strategy.Note: This depends on fixing the critical issue in
DriverHelper::waitForAsynchronousActionsToFinish(missing JavaScript driver check) identified in src/Sylius/Behat/Service/DriverHelper.php.Also applies to: 59-59, 71-71, 75-75
src/Sylius/Behat/Page/Admin/Product/IndexPerTaxonPage.php (1)
60-60: LGTM: Added page load synchronization after saving positions.The additional
waitForPageToLoadcall ensures the page is fully ready after the save button's loading state completes. This defensive synchronization is appropriate for JavaScript-driven interactions.src/Sylius/Behat/Page/Shop/Checkout/AddressPage.php (1)
286-286: LGTM: Correctly migrated to DriverHelper with proper Session parameter.The change from
JQueryHelper::waitForFormToStopLoading($this->getDocument())toDriverHelper::waitForFormToStopLoading($this->getSession())is correct. The parameter change fromgetDocument()togetSession()aligns with DriverHelper's API requirements.src/Sylius/Behat/Service/Accessor/NotificationAccessor.php (1)
30-31: LGTM: Added synchronization to wait for notification elements.Adding
waitForElementbefore querying for notification elements is a good defensive practice. This prevents race conditions where notifications might not yet be rendered in JavaScript-driven scenarios.src/Sylius/Behat/Page/Admin/Crud/IndexPage.php (1)
161-162: LGTM: Added element synchronization before checking filter value.The
waitForElementcall ensures the enabled filter is present before checking its value. This defensive synchronization prevents race conditions in JavaScript-driven scenarios where the filter may load asynchronously.src/Sylius/Behat/Page/Admin/DashboardPage.php (1)
55-55: LGTM: Added LiveComponent synchronization before counting rows.The
waitForElementcalls ensure the order and customer lists have finished loading (busy attribute removed) before counting table rows. This is consistent with the existing synchronization pattern used elsewhere in this class for LiveComponents.Also applies to: 69-69
features/admin/taxonomy/managing_taxons/editing_taxon_slug.feature (1)
60-60: No issues found. Removal of@javascripttag is appropriate.These three scenarios contain only simple form field edits (name and slug entry) followed by save operations. None involve JavaScript-dependent features like slug generation (which uses
@javascriptin other scenarios) or parent taxon selection dropdowns (which use@mink:chromedriver). The removal will improve test performance without sacrificing functionality.
| public static function waitForAsynchronousActionsToFinish(Session $session): void | ||
| { | ||
| $session->wait(1000, "!document.querySelector('[data-live-is-loading]')"); | ||
| } |
There was a problem hiding this comment.
Add JavaScript driver check to prevent errors on non-JS drivers.
Unlike waitForElement and waitForFormToStopLoading, this method doesn't check if the driver supports JavaScript before calling $session->wait(). This will cause errors when used with non-JavaScript drivers.
Apply this diff to add the guard:
public static function waitForAsynchronousActionsToFinish(Session $session): void
{
- $session->wait(1000, "!document.querySelector('[data-live-is-loading]')");
+ if (self::isJavascript($session->getDriver())) {
+ $session->wait(1000, "!document.querySelector('[data-live-is-loading]')");
+ }
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| public static function waitForAsynchronousActionsToFinish(Session $session): void | |
| { | |
| $session->wait(1000, "!document.querySelector('[data-live-is-loading]')"); | |
| } | |
| public static function waitForAsynchronousActionsToFinish(Session $session): void | |
| { | |
| if (self::isJavascript($session->getDriver())) { | |
| $session->wait(1000, "!document.querySelector('[data-live-is-loading]')"); | |
| } | |
| } |
🤖 Prompt for AI Agents
In src/Sylius/Behat/Service/DriverHelper.php around lines 51 to 54, the method
waitForAsynchronousActionsToFinish calls $session->wait() unconditionally which
breaks non-JS drivers; guard the call by checking the driver supports JavaScript
(e.g. $session->getDriver() instanceof \Behat\Mink\Driver\JavascriptDriver) and
only call $session->wait(1000,
"!document.querySelector('[data-live-is-loading]')") when that check passes;
otherwise return/skip so non-JS drivers do not error.
9642b9d to
a1b33c4
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (4)
src/Sylius/Behat/Context/Setup/TaxonomyContext.php (1)
17-17: Incorrect import: attributes will not resolveThis import statement references
Behat\Step\Given, which is not the correct attribute class for Behat 3.22. All#[Given(...)]attributes in this file will fail to resolve until this is corrected toBehat\Behat\Context\Attribute\Given.src/Sylius/Behat/Context/Ui/Admin/ManagingTaxonsContext.php (2)
237-242: Add missing return type and eliminate duplicate call.This issue was previously flagged but remains unresolved. The method:
- Lacks a return type declaration (should be
: arraybased on the return statement)- Calls
searchParentTaxontwice (lines 239 and 241), which is inefficientApply this diff:
- public function iSearchForParentTaxon(string $searchTerm) + public function iSearchForParentTaxon(string $searchTerm): array { - $this->sharedStorage->set('autocompleteSearchResults', $this->formElement->searchParentTaxon($searchTerm)); - - return $this->formElement->searchParentTaxon($searchTerm); + $searchResults = $this->formElement->searchParentTaxon($searchTerm); + $this->sharedStorage->set('autocompleteSearchResults', $searchResults); + return $searchResults; }
505-516: Add missing return type and validate shared storage content.This issue was previously flagged but remains unresolved. The method:
- Lacks a return type declaration (should be
: void)- Doesn't validate that
autocompleteSearchResultsexists or is an array before using it within_array()andimplode()Apply this diff:
- public function iShouldSeeInTheFoundResults(string $taxonName) + public function iShouldSeeInTheFoundResults(string $taxonName): void { $autocompleteSearchResults = $this->sharedStorage->get('autocompleteSearchResults'); + Assert::isArray($autocompleteSearchResults, 'Expected autocompleteSearchResults to be an array in shared storage.'); + Assert::true( in_array($taxonName, $autocompleteSearchResults, true), sprintf( 'Expected to see "%s" in autocomplete results, but found: %s', $taxonName, implode(', ', $autocompleteSearchResults), ), ); }src/Sylius/Behat/Service/DriverHelper.php (1)
51-54: Add JavaScript driver check to prevent errors on non-JS drivers.This issue was previously flagged but remains unresolved. Unlike
waitForElementandwaitForFormToStopLoading, this method unconditionally calls$session->wait()without checking if the driver supports JavaScript, which will cause errors with non-JS drivers.Apply this diff:
public static function waitForAsynchronousActionsToFinish(Session $session): void { - $session->wait(1000, "!document.querySelector('[data-live-is-loading]')"); + if (self::isJavascript($session->getDriver())) { + $session->wait(1000, "!document.querySelector('[data-live-is-loading]')"); + } }
🧹 Nitpick comments (2)
src/Sylius/Bundle/ApiBundle/Exception/ProductVariantUnprocessableException.php (1)
18-27: Exception structure is correct; consider a more specific error message.The class follows the proper pattern with
final,strict_types, and extendsUnprocessableEntityHttpExceptionappropriately. However, the message "Product variant could not be processed" is vague.Since this exception is thrown when a product variant is not found (line 51 in
AddItemToCartHandler.php), consider a more specific message like "Product variant not found" or "Product variant with the given code does not exist."Apply this diff for a clearer error message:
- parent::__construct('Product variant could not be processed.', $previous, $code, $headers); + parent::__construct('Product variant not found.', $previous, $code, $headers);src/Sylius/Behat/Service/Setter/CookieSetter.php (1)
49-54: Consider adding PHPDoc for driver type expectations.The
mixedtype hint is acceptable given the lack of a common interface, but the method specifically expectsChromeDriverorPantherDriverinstances. Consider adding a PHPDoc to document this expectation:/** * @param ChromeDriver|PantherDriver|mixed $driver */ private function ensureDriverStarted(mixed $driver): voidThis would improve maintainability by making the expected types explicit to future developers.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (25)
.github/workflows/ci_e2e-unstable.yaml(3 hunks).github/workflows/ci_packages-unstable.yaml(1 hunks)features/admin/taxonomy/managing_taxons/editing_taxon_slug.feature(3 hunks)src/Sylius/Behat/Context/Setup/TaxonomyContext.php(8 hunks)src/Sylius/Behat/Context/Ui/Admin/ManagingTaxonsContext.php(3 hunks)src/Sylius/Behat/Page/Admin/Crud/IndexPage.php(2 hunks)src/Sylius/Behat/Page/Admin/DashboardPage.php(2 hunks)src/Sylius/Behat/Page/Admin/Product/IndexPerTaxonPage.php(2 hunks)src/Sylius/Behat/Page/Shop/Account/AddressBook/CreatePage.php(2 hunks)src/Sylius/Behat/Page/Shop/Account/AddressBook/UpdatePage.php(2 hunks)src/Sylius/Behat/Page/Shop/Checkout/AddressPage.php(1 hunks)src/Sylius/Behat/Service/Accessor/NotificationAccessor.php(2 hunks)src/Sylius/Behat/Service/AutocompleteHelper.php(3 hunks)src/Sylius/Behat/Service/DriverHelper.php(1 hunks)src/Sylius/Behat/Service/JQueryHelper.php(0 hunks)src/Sylius/Behat/Service/Setter/CookieSetter.php(3 hunks)src/Sylius/Behat/Service/Setter/CookieSetterInterface.php(1 hunks)src/Sylius/Behat/Service/SlugGenerationHelper.php(0 hunks)src/Sylius/Bundle/ApiBundle/CommandHandler/Cart/AddItemToCartHandler.php(2 hunks)src/Sylius/Bundle/ApiBundle/CommandHandler/Cart/BlameCartHandler.php(2 hunks)src/Sylius/Bundle/ApiBundle/Exception/ProductVariantUnprocessableException.php(1 hunks)src/Sylius/Bundle/ApiBundle/Exception/UnprocessableCartException.php(1 hunks)src/Sylius/Bundle/ApiBundle/tests/CommandHandler/Cart/AddItemToCartHandlerTest.php(3 hunks)src/Sylius/Bundle/ApiBundle/tests/CommandHandler/Cart/BlameCartHandlerTest.php(3 hunks)tests/Api/Shop/Checkout/CartTest.php(1 hunks)
💤 Files with no reviewable changes (2)
- src/Sylius/Behat/Service/JQueryHelper.php
- src/Sylius/Behat/Service/SlugGenerationHelper.php
🚧 Files skipped from review as they are similar to previous changes (13)
- src/Sylius/Behat/Page/Admin/Crud/IndexPage.php
- src/Sylius/Bundle/ApiBundle/CommandHandler/Cart/BlameCartHandler.php
- src/Sylius/Bundle/ApiBundle/tests/CommandHandler/Cart/AddItemToCartHandlerTest.php
- tests/Api/Shop/Checkout/CartTest.php
- src/Sylius/Behat/Page/Admin/Product/IndexPerTaxonPage.php
- src/Sylius/Behat/Page/Shop/Checkout/AddressPage.php
- src/Sylius/Bundle/ApiBundle/Exception/UnprocessableCartException.php
- src/Sylius/Behat/Page/Shop/Account/AddressBook/UpdatePage.php
- src/Sylius/Behat/Service/Accessor/NotificationAccessor.php
- features/admin/taxonomy/managing_taxons/editing_taxon_slug.feature
- .github/workflows/ci_packages-unstable.yaml
- src/Sylius/Behat/Service/AutocompleteHelper.php
- src/Sylius/Behat/Page/Admin/DashboardPage.php
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{php,yaml,yml,xml,twig}
📄 CodeRabbit inference engine (AGENTS.md)
Use 4 spaces for indentation in PHP, YAML, XML, and Twig files
Files:
src/Sylius/Bundle/ApiBundle/tests/CommandHandler/Cart/BlameCartHandlerTest.phpsrc/Sylius/Behat/Page/Shop/Account/AddressBook/CreatePage.phpsrc/Sylius/Behat/Service/Setter/CookieSetterInterface.phpsrc/Sylius/Bundle/ApiBundle/Exception/ProductVariantUnprocessableException.phpsrc/Sylius/Behat/Context/Setup/TaxonomyContext.phpsrc/Sylius/Behat/Service/Setter/CookieSetter.phpsrc/Sylius/Bundle/ApiBundle/CommandHandler/Cart/AddItemToCartHandler.phpsrc/Sylius/Behat/Service/DriverHelper.phpsrc/Sylius/Behat/Context/Ui/Admin/ManagingTaxonsContext.php
**/*.php
📄 CodeRabbit inference engine (AGENTS.md)
**/*.php: Use modern PHP 8.2+ syntax and features
Declare strict_types=1 in all PHP files
Follow the Sylius Coding Standard
Do not use deprecated features from PHP, Symfony, or Sylius
Use final for all classes, except entities and repositories
Use readonly for immutable services and value objects
Add type declarations for all properties, arguments, and return values
Use camelCase for variables and method names
Use SCREAMING_SNAKE_CASE for constants
Use fast returns instead of unnecessary nesting
Use trailing commas in multi-line arrays and argument lists
Order array keys alphabetically where applicable
Use PHPDoc only when necessary (e.g., @var Collection)
Group class elements in order: constants, properties, constructor, public, protected, private methods
Group getter and setter methods for the same properties together
Suffix interfaces with Interface and traits with Trait
Use use statements for all non-global classes
Sort use imports alphabetically and group by type (classes, functions, constants)
Files:
src/Sylius/Bundle/ApiBundle/tests/CommandHandler/Cart/BlameCartHandlerTest.phpsrc/Sylius/Behat/Page/Shop/Account/AddressBook/CreatePage.phpsrc/Sylius/Behat/Service/Setter/CookieSetterInterface.phpsrc/Sylius/Bundle/ApiBundle/Exception/ProductVariantUnprocessableException.phpsrc/Sylius/Behat/Context/Setup/TaxonomyContext.phpsrc/Sylius/Behat/Service/Setter/CookieSetter.phpsrc/Sylius/Bundle/ApiBundle/CommandHandler/Cart/AddItemToCartHandler.phpsrc/Sylius/Behat/Service/DriverHelper.phpsrc/Sylius/Behat/Context/Ui/Admin/ManagingTaxonsContext.php
🧠 Learnings (1)
📚 Learning: 2025-09-01T12:06:56.917Z
Learnt from: CR
Repo: Sylius/Sylius PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-09-01T12:06:56.917Z
Learning: Applies to **/*.php : Use readonly for immutable services and value objects
Applied to files:
src/Sylius/Behat/Context/Ui/Admin/ManagingTaxonsContext.php
🧬 Code graph analysis (8)
src/Sylius/Bundle/ApiBundle/tests/CommandHandler/Cart/BlameCartHandlerTest.php (5)
src/Sylius/Bundle/ApiBundle/Exception/UnprocessableCartException.php (1)
UnprocessableCartException(18-27)src/Sylius/Bundle/ApiBundle/CommandHandler/Cart/AddItemToCartHandler.php (1)
__invoke(45-69)src/Sylius/Bundle/ApiBundle/CommandHandler/Cart/BlameCartHandler.php (1)
__invoke(41-64)src/Sylius/Bundle/ApiBundle/tests/CommandHandler/Account/SendShopUserVerificationEmailHandlerTest.php (1)
testThrowsAnExceptionIfUserHasNotBeenFound(75-89)src/Sylius/Bundle/ApiBundle/tests/CommandHandler/Customer/RemoveShopUserHandlerTest.php (1)
testThrowsAnExceptionIfUserHasNotBeenFound(40-45)
src/Sylius/Behat/Page/Shop/Account/AddressBook/CreatePage.php (4)
src/Sylius/Behat/Service/DriverHelper.php (2)
DriverHelper(22-62)waitForFormToStopLoading(56-61)src/Sylius/Behat/Page/Shop/Account/AddressBook/UpdatePage.php (2)
waitForFormToStopLoading(74-77)selectCountry(64-72)src/Sylius/Behat/Page/Shop/Checkout/AddressPage.php (1)
waitForFormToStopLoading(284-287)src/Sylius/Behat/Page/Shop/Account/AddressBook/CreatePageInterface.php (1)
selectCountry(23-23)
src/Sylius/Behat/Service/Setter/CookieSetterInterface.php (1)
src/Sylius/Behat/Service/Setter/CookieSetter.php (1)
setCookie(31-47)
src/Sylius/Bundle/ApiBundle/Exception/ProductVariantUnprocessableException.php (1)
src/Sylius/Bundle/ApiBundle/Exception/UnprocessableCartException.php (1)
__construct(20-26)
src/Sylius/Behat/Service/Setter/CookieSetter.php (1)
src/Sylius/Behat/Service/Setter/CookieSetterInterface.php (1)
setCookie(18-18)
src/Sylius/Bundle/ApiBundle/CommandHandler/Cart/AddItemToCartHandler.php (2)
src/Sylius/Bundle/ApiBundle/Exception/UnprocessableCartException.php (1)
UnprocessableCartException(18-27)src/Sylius/Bundle/ApiBundle/Exception/ProductVariantUnprocessableException.php (1)
ProductVariantUnprocessableException(18-27)
src/Sylius/Behat/Service/DriverHelper.php (3)
src/Sylius/Behat/Page/Admin/DashboardPage.php (1)
waitForElement(214-218)src/Sylius/Behat/Page/Shop/Account/AddressBook/UpdatePage.php (2)
waitForElement(120-123)waitForFormToStopLoading(74-77)src/Sylius/Behat/Page/Shop/Checkout/AddressPage.php (1)
waitForFormToStopLoading(284-287)
src/Sylius/Behat/Context/Ui/Admin/ManagingTaxonsContext.php (2)
src/Sylius/Behat/Element/Admin/Taxon/FormElementInterface.php (1)
searchParentTaxon(49-49)src/Sylius/Behat/Element/Admin/Taxon/FormElement.php (1)
searchParentTaxon(145-156)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (13)
- GitHub Check: Frontend / NodeJS 24.x
- GitHub Check: End-to-end tests (MySQL) / JS with Panther, PHP 8.4, Symfony ^7.3 (test_cached), MySQL 8.4, Twig ^3.3
- GitHub Check: End-to-end tests (MySQL) / JS with Chromedriver, PHP 8.4, Symfony ^7.3 (test_cached), MySQL 8.4, Twig ^3.3
- GitHub Check: End-to-end tests (MySQL) / JS with Panther, PHP 8.3, Symfony ^6.4 (test_cached), MySQL 8.0, Twig ^3.3
- GitHub Check: End-to-end tests (MySQL) / Non-JS, PHP 8.4, Symfony ^7.3 (test_cached), MySQL 8.4, Twig ^3.3
- GitHub Check: End-to-end tests (MySQL) / Non-JS, PHP 8.3, Symfony ^6.4 (test_cached), MySQL 8.0, Twig ^3.3
- GitHub Check: End-to-end tests (MySQL) / JS with Chromedriver, PHP 8.3, Symfony ^6.4 (test_cached), MySQL 8.0, Twig ^3.3
- GitHub Check: End-to-end tests (PostgreSQL) / Non-JS, PHP 8.4, Symfony ^7.3, PostgreSQL 17.5
- GitHub Check: End-to-end tests (PostgreSQL) / Non-JS, PHP 8.3, Symfony ^6.4, PostgreSQL 15.13
- GitHub Check: End-to-end tests (MariaDB) / Non-JS, PHP 8.4, Symfony ^7.3, MariaDB 11.4.7, State Machine Adapter symfony_workflow, ApiPlatform ~4.1.7
- GitHub Check: End-to-end tests (MariaDB) / Non-JS, PHP 8.3, Symfony ^6.4, MariaDB 10.11.13, State Machine Adapter winzou_state_machine
- GitHub Check: End-to-end tests (MariaDB) / Non-JS, PHP 8.4, Symfony ^7.3, MariaDB 11.4.7, State Machine Adapter symfony_workflow
- GitHub Check: Packages / PHP 8.4, Symfony ^7.3
🔇 Additional comments (20)
.github/workflows/ci_e2e-unstable.yaml (1)
28-28: Consistently updating Symfony version in unstable test matrix—looks good.All three unstable E2E test jobs (
behat-no-js-unstable,behat-ui-js-chromedriver-unstable, andbehat-ui-js-panther-unstable) are updated uniformly from Symfony^7.3@betato^7.4@beta. The changes are straightforward and align with the PR objective to modernize CI workflows.Consider verifying that any other CI workflow files (e.g.,
ci.yaml,ci_e2e.yaml, or similar stable branches) are also updated consistently if they follow a similar pattern, to avoid version drift across test suites.Also applies to: 101-101, 182-182
src/Sylius/Bundle/ApiBundle/CommandHandler/Cart/AddItemToCartHandler.php (4)
17-18: LGTM: Appropriate exception imports for API context.The specialized HTTP exceptions provide clearer API responses with proper status codes (422 Unprocessable Entity).
50-52: Exception change improves API response clarity.Replacing
InvalidArgumentExceptionwithProductVariantUnprocessableExceptionensures the API returns HTTP 422 with a domain-specific error message.
57-59: Exception change aligns with API best practices.Using
UnprocessableCartExceptioninstead ofInvalidArgumentExceptionprovides consistent HTTP 422 responses for cart-not-found scenarios.
1-70: Verify these changes align with PR objectives.The PR title indicates "Fix autocomplete in admin to be case-insensitive," but this file modifies API cart handler exceptions. These changes appear unrelated to autocomplete functionality.
src/Sylius/Bundle/ApiBundle/tests/CommandHandler/Cart/BlameCartHandlerTest.php (4)
26-28: LGTM: Test exception imports match handler changes.The imported exception types align with the updated
BlameCartHandlerimplementation.
85-87: Test expectation correctly updated.The test now expects
ConflictHttpExceptionwhen the cart is already occupied, matching the handler's behavior.
95-97: Test expectation correctly updated.The test now expects
UnprocessableCartExceptionwhen the cart is not found, consistent with the API error handling pattern.
101-103: Test expectation correctly updated.The test now expects
NotFoundHttpExceptionwhen the user is not found, providing proper HTTP 404 semantics.src/Sylius/Behat/Context/Setup/TaxonomyContext.php (2)
55-59: LGTM: New code-aware taxon creation stepsThe new
storeClassifiesItsProductsAsWithCodestep and supportingcreateTaxonWithCodehelper are well-implemented. Both methods have proper type declarations, follow established patterns in the file, and correctly integrate with the repository layer.Also applies to: 153-162
43-47: LGTM: Modernization to PHP 8 attributes and improved type safetyThe conversion from PHPDoc annotations to PHP 8 attributes for step definitions follows modern PHP practices, and the added return type on
createTaxonInManyLanguagesimproves type safety. All changes align with the Sylius coding guidelines.Also applies to: 61-61, 72-72, 95-95, 112-115, 126-126, 134-134, 173-173
src/Sylius/Behat/Service/Setter/CookieSetterInterface.php (1)
18-18: LGTM! Strict typing improves type safety.The addition of explicit parameter types and return type declaration aligns with PHP 8.2+ best practices and the coding guidelines. The implementation in
CookieSetter.phpcorrectly matches this signature.src/Sylius/Behat/Service/Setter/CookieSetter.php (5)
23-29: LGTM! Proper use of readonly class.Making this class
readonlyfollows the coding guideline to use readonly for immutable services. The constructor-promoted properties are automatically readonly, which is correct for this immutable service class.
31-31: LGTM! Signature correctly implements the interface.The method signature with strict types matches the updated
CookieSetterInterfaceand follows PHP 8.2+ best practices.
71-77: LGTM! Improved driver checks with better abstraction.The refactored logic using the
isPageNotLoadedhelper improves code readability and follows the DRY principle. The Selenium2Driver check now properly handles both null WebDriver sessions and unloaded pages.
79-80: LGTM! Fallback logic is correct.The negated
str_containscheck correctly identifies when the session needs preparation—returningtruewhen the current URL doesn't contain the base URL.
82-85: LGTM! Well-defined helper method.The
isPageNotLoadedhelper properly encapsulates the logic for detecting unloaded pages with a reasonable set of URL checks. Using strict comparison is correct and follows best practices.src/Sylius/Behat/Page/Shop/Account/AddressBook/CreatePage.php (1)
19-19: LGTM! Clean migration to DriverHelper.The migration from JQueryHelper to DriverHelper is consistent and correct. The switch from passing
$this->getDocument()to$this->getSession()aligns with DriverHelper's API and matches the pattern used in other pages like UpdatePage and checkout AddressPage.Also applies to: 38-38, 45-45
src/Sylius/Behat/Service/DriverHelper.php (1)
41-49: LGTM! Proper JavaScript driver guards in place.Both
waitForElementandwaitForFormToStopLoadingcorrectly check for JavaScript drivers before attempting to wait, preventing errors on non-JS drivers.Also applies to: 56-61
src/Sylius/Behat/Context/Ui/Admin/ManagingTaxonsContext.php (1)
17-17: Remove the unusedBehat\Step\Whenimport on line 17.The search confirms this import is not used anywhere in the code. All occurrences of "When" in the file are docblock annotations (
@When), which Behat processes at runtime through its AnnotationReader and don't require the class to be imported.
a1b33c4 to
ddd2b0b
Compare
Co-authored-by: Mateusz <mateusz@rustle.pl> Co-authored-by: Norbert Glanc <norglanc@gmail.com>
ddd2b0b to
c19c351
Compare
This is a continuation of pull request #18477 | Q | A |-----------------|----- | Branch? | 2.1 | Bug fix? | yes | New feature? | no | BC breaks? | no | Deprecations? | no | Related tickets | #18384 | License | MIT <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Bug Fixes** * Parent taxon autocomplete search is now case-insensitive for more reliable results. * **New Features** * Admin taxon form returns richer autocomplete results for parent selection. * Added UI actions to search and verify parent taxon results. * Support for creating taxons with explicit codes where needed. * **Tests** * New BDD tests covering case-insensitive parent taxon autocomplete and related scenarios. <sub>✏️ Tip: You can customize this high-level summary in your review settings.</sub> <!-- end of auto-generated comment: release notes by coderabbit.ai -->
This is a continuation of pull request Sylius/Sylius#18477 | Q | A |-----------------|----- | Branch? | 2.1 | Bug fix? | yes | New feature? | no | BC breaks? | no | Deprecations? | no | Related tickets | Sylius/Sylius#18384 | License | MIT <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Bug Fixes** * Parent taxon autocomplete search is now case-insensitive for more reliable results. * **New Features** * Admin taxon form returns richer autocomplete results for parent selection. * Added UI actions to search and verify parent taxon results. * Support for creating taxons with explicit codes where needed. * **Tests** * New BDD tests covering case-insensitive parent taxon autocomplete and related scenarios. <sub>✏️ Tip: You can customize this high-level summary in your review settings.</sub> <!-- end of auto-generated comment: release notes by coderabbit.ai -->
|
Closing as it has been replaced by #18572 |
This is a continuation of pull request #18393.
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Tests
Chores