Fix/autocomplete choice value removal#1
Conversation
DhirenMhatre
commented
Jun 6, 2026
DhirenMhatre
commented
| Q | A |
|---|---|
| Branch? | 2.2 or 2.3 |
| Bug fix? | no/yes |
| New feature? | no/yes |
| BC breaks? | no/yes |
| Deprecations? | no/yes |
| Related tickets | fixes #X, partially #Y, mentioned in #Z |
| License | MIT |
…e issue When choice_value is set to 'code' on Symfony UX Autocomplete types, DoctrineChoiceLoader cannot use optimized getEntitiesByIds() query, causing ALL entities to be loaded instead of just the selected ones. This change removes choice_value from all UX Autocomplete types, letting Symfony use the default entity ID which enables the optimized WHERE id IN (...) query. Refs: Sylius#17953
…complete After removing choice_value from autocomplete types, the form values are now entity IDs instead of codes. This commit updates Behat tests to: - Use selectByName/removeByName instead of selectByValue/removeByValue - Use getSelectedItems() or option[selected] text for assertions - Compare with entity names instead of codes - Add JS/non-JS driver handling for parent taxon selection Refs: Sylius#17953
|
@codity review |
PR SummaryWhat Changed
Key Changes by AreaAutocomplete Forms: Removed Taxon Slug Generation: Fixed Test Infrastructure: Updated helpers and assertions to use name-based selection ( Files Changed
Review Focus Areas
ArchitectureDesign Decisions: The shift from codes to names/descriptors for autocomplete values improves UX (human-readable selection) but relies on name uniqueness. The Risks: Name-based selection assumes unique, stable names. If names change or duplicate, autocomplete behavior may break. This is an intentional tradeoff for usability over code-based stability. The taxon parent lookup fix corrects a bug where codes were incorrectly used as IDs, but verify no other code-paths depend on the old behavior. Merge StatusMERGEABLE — PR Score 62/100, above threshold (50). All gates passed. |
Workflow DiagramsAutomatically generated sequence diagrams showing the workflows in this PR 1. Workflow AnalysisMedium complexity • Components: AutocompleteHelper, CatalogPromotion FormElement, Product AssociationsFormElement sequenceDiagram
title Autocomplete Selection Refactoring: Code to Name Based Selection
participant BehatContext as Behat Context
participant FormElement as Form Element
participant AutocompleteHelper as Autocomplete Helper
participant AutocompleteType as Autocomplete Form Type
participant Browser as Browser/JS
participant Entity as Entity Repository
Note over BehatContext,Entity: OLD FLOW: Selection by Code
Note over BehatContext,Entity: NEW FLOW: Selection by Name
rect rgb(230, 245, 255)
Note over BehatContext,Entity: Phase 1: Test Context Preparation
BehatContext->>BehatContext: Extract entity names instead of codes
Note right of BehatContext: variant.getName replaces variant.getCode
BehatContext->>BehatContext: taxon.getName() replaces taxon.getCode()
BehatContext->>BehatContext: product.getName() replaces product.getCode()
end
rect rgb(255, 245, 230)
Note over BehatContext,Entity: Phase 2: Form Element Interaction
BehatContext->>FormElement: selectScopeOption(variantNames)
Note right of BehatContext: Previously passed variantCodes
FormElement->>AutocompleteHelper: selectByName(name)
Note right of FormElement: Previously called selectByValuecode
alt Removal Operation
FormElement->>AutocompleteHelper: removeByName(name)
Note right of FormElement: Previously called removeByValuecode
end
end
rect rgb(230, 255, 230)
Note over BehatContext,Entity: Phase 3: Form Type Configuration
AutocompleteHelper->>AutocompleteType: Configure autocomplete field
Note right of AutocompleteHelper: choice_value set to name property
AutocompleteType->>AutocompleteType: setChoiceValue('name')
Note right of AutocompleteType: Previously set to 'code'
end
rect rgb(255, 230, 245)
Note over BehatContext,Entity: Phase 4: Browser Execution
AutocompleteHelper->>Browser: Type entity name in search field
Browser->>Entity: Search by name pattern
Entity-->>Browser: Return matching entities
Browser->>Browser: Display name-based results
Browser->>AutocompleteHelper: Confirm selection by name
end
rect rgb(245, 255, 230)
Note over BehatContext,Entity: Phase 5: Verification
BehatContext->>FormElement: getLastScopeNames()
Note right of BehatContext: Previously getLastScopeCodes
FormElement-->>BehatContext: Return array of selected names
BehatContext->>BehatContext: Assert expected name in selection
Note right of BehatContext: Previously asserted codes
end
Note over BehatContext,Entity: Affected Components
Note over BehatContext: ManagingCatalogPromotionsContext
Note over BehatContext: ManagingProductsContext
Note over BehatContext: ManagingTaxonsContext
Note over FormElement: CatalogPromotion FormElement
Note over FormElement: Product AssociationsFormElement
Note over FormElement: MediaFormElement
Note over FormElement: Taxon FormElement
Note over AutocompleteType: ProductAutocompleteType
Note over AutocompleteType: ProductVariantAutocompleteType
Note over AutocompleteType: ProductAttributeAutocompleteType
Note: Diagrams show detected patterns only. Complex workflows may require manual review. |
![codity-chait[bot]](https://avatars.githubusercontent.com/u/228176587?s=60&v=4){kind=small}
| public function hasImageWithVariant(ProductVariantInterface $productVariant): bool | ||
| { | ||
| $selectedVariantName = $this->getFirstImageSelectedVariantName(); | ||
|
|
||
| if (null === $selectedVariantName) { | ||
| return false; | ||
| } | ||
|
|
||
| return str_contains($selectedVariantName, $productVariant->getName()); | ||
| } |
There was a problem hiding this comment.
The hasImageWithVariant method only checks the first image's selected variant instead of searching all images in the gallery for a matching variant. This causes the method to incorrectly return false when a variant is attached to any image other than the first one. The original implementation used a CSS selector to search the entire images container, but the refactored version delegates to getFirstImageSelectedVariantName which explicitly retrieves only the first image subform.
Suggested fix
public function hasImageWithVariant(ProductVariantInterface $productVariant): bool
{
$this->changeTab();
$images = $this->getElement('images');
$variantElements = $images->findAll('css', '[data-test-product-variant*="' . $productVariant->getName() . '"]');
return count($variantElements) > 0;
}Prompt for AI assistance
Copy the prompt below and paste it into ChatGPT, Claude, or any LLM:
You are an expert bash developer with deep knowledge of security, performance, and best practices.
### Context
File: src/Sylius/Behat/Element/Admin/Product/MediaFormElement.php
Lines: 108-117
Issue Type: functional-high
Severity: high
Issue Description:
The `hasImageWithVariant` method only checks the first image's selected variant instead of searching all images in the gallery for a matching variant. This causes the method to incorrectly return `false` when a variant is attached to any image other than the first one. The original implementation used a CSS selector to search the entire images container, but the refactored version delegates to `getFirstImageSelectedVariantName` which explicitly retrieves only the first image subform.
Current Code:
public function hasImageWithVariant(ProductVariantInterface $productVariant): bool
{
$selectedVariantName = $this->getFirstImageSelectedVariantName();
if (null === $selectedVariantName) {
return false;
}
return str_contains($selectedVariantName, $productVariant->getName());
}
---
### Instructions
1. Fix the issue described above
2. Maintain the exact indentation and code style from the original
3. Follow bash best practices and language-specific idioms
4. Ensure the fix addresses the root cause, not just the symptoms
5. Add brief inline comments explaining the fix if needed
### Constraints
- Do not change functionality beyond fixing the identified issue
- Preserve existing variable names and function signatures unless they are part of the problem
- Ensure the fix is production-ready
---
| return false; | ||
| } | ||
|
|
||
| return str_contains($selectedVariantName, $productVariant->getName()); |
There was a problem hiding this comment.
The hasImageWithVariant method uses str_contains() to check if a product variant name matches the selected variant, but this performs a substring match rather than an exact match. This causes false positives where checking for variant "Small" incorrectly returns true when "Extra Small" is selected, since "Small" is a substring of "Extra Small". This leads to unreliable test assertions in image-variant validation.
| return str_contains($selectedVariantName, $productVariant->getName()); | |
| return $selectedVariantName === $productVariant->getName(); |
Prompt for AI assistance
Copy the prompt below and paste it into ChatGPT, Claude, or any LLM:
You are an expert bash developer with deep knowledge of security, performance, and best practices.
### Context
File: src/Sylius/Behat/Element/Admin/Product/MediaFormElement.php
Lines: 116-116
Issue Type: functional-high
Severity: high
Issue Description:
The `hasImageWithVariant` method uses `str_contains()` to check if a product variant name matches the selected variant, but this performs a substring match rather than an exact match. This causes false positives where checking for variant "Small" incorrectly returns true when "Extra Small" is selected, since "Small" is a substring of "Extra Small". This leads to unreliable test assertions in image-variant validation.
Current Code:
return str_contains($selectedVariantName, $productVariant->getName());
---
### Instructions
1. Fix the issue described above
2. Maintain the exact indentation and code style from the original
3. Follow bash best practices and language-specific idioms
4. Ensure the fix addresses the root cause, not just the symptoms
5. Add brief inline comments explaining the fix if needed
### Constraints
- Do not change functionality beyond fixing the identified issue
- Preserve existing variable names and function signatures unless they are part of the problem
- Ensure the fix is production-ready
---
Security Scan Summary
No critical security issues detected Scan completed in 35.9sSecurity scan powered by Codity.ai |
License Compliance Scan
Some packages have unknown licenses - manual review required Unknown Licenses - 166 packages
...and 146 more Low Risk Licenses - Top licenses from 22 packagesApache-2.0 (2 packages) Powered by Codity.ai · Docs |
Code Quality Report — test-org-codity/Sylius · PR #1Scanned: 2026-06-06 17:29 UTC | Score: 56/100 | Provider: github Executive Summary
Top Findings[CQ-LLM-003]
|
| File | Critical | High | Medium | Low | Total |
|---|---|---|---|---|---|
src/Sylius/Behat/Context/Ui/Admin/ManagingCatalogPromotionsContext.php |
0 | 1 | 2 | 0 | 3 |
src/Sylius/Behat/Context/Ui/Admin/ManagingProductsContext.php |
0 | 0 | 0 | 1 | 1 |
src/Sylius/Behat/Context/Ui/Admin/ManagingTaxonsContext.php |
0 | 0 | 1 | 0 | 1 |
src/Sylius/Behat/Element/Admin/CatalogPromotion/FormElement.php |
0 | 0 | 2 | 0 | 2 |
Recommendations
- Resolve High severity issues, especially error handling gaps and performance bottlenecks.
- Run automated tests after applying fixes to verify no regressions.
|
@codity review |
PR SummaryWhat Changed
Key Changes by AreaForm Types: Removed explicit Taxon Slug Generation: Fixed Behat Tests: Updated test helpers and contexts to use Files Changed
Review Focus Areas
ArchitectureDesign Decisions: The switch from code-based to name-based values in autocomplete fields aligns with how the underlying autocomplete component handles selection internally. This fixes item removal bugs but trades code stability (unique, immutable) for name readability. The taxon slug fix corrects a mismatch between form value format (ID) and repository lookup method (code). Risks: Name-based values may cause issues if entity names are not unique or change over time. This is an intentional tradeoff for UX improvement. The Merge StatusNOT MERGEABLE — PR Score 44/100, below threshold (50)
|
Workflow DiagramsAutomatically generated sequence diagrams showing the workflows in this PR 1. AnalysisMedium complexity • Components: AutocompleteHelper, CatalogPromotion FormElement, Product AssociationsFormElement sequenceDiagram
title Refactoring Autocomplete from Code-Based to Name-Based Selection
participant BehatContext as Behat Context
participant FormElement as Form Element
participant AutocompleteHelper as Autocomplete Helper
participant FormType as Form Type
participant EntityRepository as Entity Repository
participant Database as Database
Note over BehatContext,Database: NEW FLOW: Name-based autocomplete selection
BehatContext->>BehatContext: Extract entity names instead of codes
Note right of BehatContext: variant.getName replaces variant.getCode
BehatContext->>FormElement: selectScopeOption(variantNames)
activate FormElement
FormElement->>AutocompleteHelper: selectByName(field, names)
Note right of AutocompleteHelper: NEW METHOD replaces selectByValue
AutocompleteHelper->>FormType: Search by name pattern
FormType->>EntityRepository: findByNameLike(name)
EntityRepository->>Database: SELECT * FROM entity WHERE name LIKE
Database-->>EntityRepository: matching entities
EntityRepository-->>FormType: entity results
FormType-->>AutocompleteHelper: name-value pairs
AutocompleteHelper->>AutocompleteHelper: select matching options by name
AutocompleteHelper-->>FormElement: selection confirmed
FormElement-->>BehatContext: scope option selected
BehatContext->>FormElement: getLastScopeNames()
Note right of FormElement: NEW METHOD replaces getLastScopeCodes
FormElement->>AutocompleteHelper: getSelectedItems(field)
Note right of AutocompleteHelper: NEW METHOD for verification
AutocompleteHelper-->>FormElement: selected names array
FormElement-->>BehatContext: names for assertion
BehatContext->>BehatContext: Assert entity name in selected names
deactivate FormElement
Note over BehatContext,Database: REMOVAL FLOW: Name-based option removal
BehatContext->>FormElement: removeScopeOption(variantNames)
FormElement->>AutocompleteHelper: removeByName(field, names)
Note right of AutocompleteHelper: NEW METHOD replaces removeByValue
AutocompleteHelper->>FormType: Remove matching name entries
AutocompleteHelper-->>FormElement: removal confirmed
FormElement-->>BehatContext: scope option removed
Note: Diagrams show detected patterns only. Complex workflows may require manual review. |
| @@ -33,7 +33,6 @@ public function configureOptions(OptionsResolver $resolver): void | |||
| $resolver->setDefaults([ | |||
| 'class' => $this->productAttributeClass, | |||
| 'choice_name' => 'name', | |||
There was a problem hiding this comment.
The ProductAttributeAutocompleteType is missing the 'choice_value' => 'code' configuration option. Without it, the autocomplete API endpoint returns entity IDs instead of attribute codes as option values. This causes the product form to send numeric IDs to Product/FormComponent::addAttributes(), which then fails to find attributes by code and throws a TypeError when calling isTranslatable() on a null object.
Suggested fix
$resolver->setDefaults([
'class' => $this->productAttributeClass,
'choice_name' => 'name',
'choice_value' => 'code',
]);Prompt for AI assistance
Copy the prompt below and paste it into ChatGPT, Claude, or any LLM:
You are an expert bash developer with deep knowledge of security, performance, and best practices.
### Context
File: src/Sylius/Bundle/AdminBundle/Form/Type/ProductAttributeAutocompleteType.php
Lines: 35-35
Issue Type: functional-critical
Severity: critical
Issue Description:
The `ProductAttributeAutocompleteType` is missing the `'choice_value' => 'code'` configuration option. Without it, the autocomplete API endpoint returns entity IDs instead of attribute codes as option values. This causes the product form to send numeric IDs to `Product/FormComponent::addAttributes()`, which then fails to find attributes by code and throws a TypeError when calling `isTranslatable()` on a null object.
Current Code:
'choice_name' => 'name',
---
### Instructions
1. Fix the issue described above
2. Maintain the exact indentation and code style from the original
3. Follow bash best practices and language-specific idioms
4. Ensure the fix addresses the root cause, not just the symptoms
5. Add brief inline comments explaining the fix if needed
### Constraints
- Do not change functionality beyond fixing the identified issue
- Preserve existing variable names and function signatures unless they are part of the problem
- Ensure the fix is production-ready
---
| $selectedVariantName = $this->getFirstImageSelectedVariantName(); | ||
|
|
||
| if (null === $selectedVariantName) { | ||
| return false; | ||
| } | ||
|
|
||
| return str_contains($selectedVariantName, $productVariant->getName()); |
There was a problem hiding this comment.
The hasImageWithVariant() method has two bugs: (1) it only checks the first image subform via getFirstImageSelectedVariantName(), missing variants assigned to other images, and (2) it uses str_contains() for substring matching instead of exact comparison, causing false positives (e.g., 'Red' matches 'Red Large'). The fix must iterate through all image subforms and use exact string comparison.
Suggested fix
public function hasImageWithVariant(ProductVariantInterface $productVariant): bool
{
$this->changeTab();
$images = $this->getElement('images');
$imageSubforms = $images->findAll('css', '[data-test-image-subform]');
foreach ($imageSubforms as $imageSubform) {
$variantField = $imageSubform->find('css', '[data-test-product-variant]');
if (null === $variantField) {
continue;
}
$selectedOption = $variantField->find('css', 'option[selected]');
if (null === $selectedOption) {
continue;
}
if ($selectedOption->getText() === $productVariant->getName()) {
return true;
}
}
return false;
}Prompt for AI assistance
Copy the prompt below and paste it into ChatGPT, Claude, or any LLM:
You are an expert bash developer with deep knowledge of security, performance, and best practices.
### Context
File: src/Sylius/Behat/Element/Admin/Product/MediaFormElement.php
Lines: 110-116
Issue Type: functional-high
Severity: high
Issue Description:
The `hasImageWithVariant()` method has two bugs: (1) it only checks the first image subform via `getFirstImageSelectedVariantName()`, missing variants assigned to other images, and (2) it uses `str_contains()` for substring matching instead of exact comparison, causing false positives (e.g., 'Red' matches 'Red Large'). The fix must iterate through all image subforms and use exact string comparison.
Current Code:
$selectedVariantName = $this->getFirstImageSelectedVariantName();
if (null === $selectedVariantName) {
return false;
}
return str_contains($selectedVariantName, $productVariant->getName());
---
### Instructions
1. Fix the issue described above
2. Maintain the exact indentation and code style from the original
3. Follow bash best practices and language-specific idioms
4. Ensure the fix addresses the root cause, not just the symptoms
5. Add brief inline comments explaining the fix if needed
### Constraints
- Do not change functionality beyond fixing the identified issue
- Preserve existing variable names and function signatures unless they are part of the problem
- Ensure the fix is production-ready
---
Security Scan Summary
No critical security issues detected Scan completed in 36.2sSecurity scan powered by Codity.ai |
License Compliance Scan
Some packages have unknown licenses - manual review required Unknown Licenses - 166 packages
...and 146 more Low Risk Licenses - Top licenses from 22 packagesApache-2.0 (2 packages) Powered by Codity.ai · Docs |
Code Quality Report — test-org-codity/Sylius · PR #1Scanned: 2026-06-06 18:17 UTC | Score: 56/100 | Provider: github Executive Summary
Top Findings[CQ-LLM-003]
|
| File | Critical | High | Medium | Low | Total |
|---|---|---|---|---|---|
src/Sylius/Behat/Context/Ui/Admin/ManagingCatalogPromotionsContext.php |
0 | 1 | 4 | 0 | 5 |
src/Sylius/Behat/Element/Admin/CatalogPromotion/FormElement.php |
0 | 0 | 1 | 1 | 2 |
Recommendations
- Resolve High severity issues, especially error handling gaps and performance bottlenecks.
- Run automated tests after applying fixes to verify no regressions.
3 participants
