Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[FIX] Default variant #15548

Merged
merged 7 commits into from Nov 22, 2023
Merged

[FIX] Default variant #15548

merged 7 commits into from Nov 22, 2023

Conversation

Prometee
Copy link
Contributor

@Prometee Prometee commented Nov 20, 2023

Q A
Branch? 1.12
Bug fix? yes
New feature? yes
BC breaks? no
Deprecations? yes
Related tickets fixes #15547
License MIT

This PR fix an issue introduced by #15273.
Variants are by default ordered by position via a specific Doctrine resource configuration :
https://github.com/Sylius/Sylius/blob/1.12/src/Sylius/Bundle/ProductBundle/Resources/config/doctrine/model/Product.orm.xml#L37-L38
This behaviour has to be cloned to retrieve the right variant.

@Prometee Prometee requested a review from a team as a code owner November 20, 2023 16:11
@probot-autolabeler probot-autolabeler bot added the Maintenance CI configurations, READMEs, releases, etc. label Nov 20, 2023
UPGRADE-1.12.md Outdated Show resolved Hide resolved
Copy link

github-actions bot commented Nov 20, 2023

Bunnyshell Preview Environment deleted

Available commands:

  • /bns:deploy to redeploy the environment

@diimpp diimpp added Bug Confirmed bugs or bugfixes. and removed Maintenance CI configurations, READMEs, releases, etc. labels Nov 20, 2023
@vvasiloi
Copy link
Contributor

So it was a regression. We need a test to prevent that from happening again.

@probot-autolabeler probot-autolabeler bot added the Maintenance CI configurations, READMEs, releases, etc. label Nov 20, 2023
@Prometee
Copy link
Contributor Author

So it was a regression. We need a test to prevent that from happening again.

@vvasiloi Scenario added 😉

@Prometee
Copy link
Contributor Author

Do I have to silent the PSALM error ? Any other way to circle around this error ?
Or do I have to (re-)insert a BC break by adding a new Product Variant repository method ?

@vvasiloi
Copy link
Contributor

Do I have to silent the PSALM error ? Any other way to circle around this error ? Or do I have to (re-)insert a BC break by adding a new Product Variant repository method ?

Maybe just use findBy instead. 🤷‍♂️ It doesn't matter if we also add a custom method in 2.0

@vvasiloi
Copy link
Contributor

Or we remove psalm 😆 /cc @jakubtobiasz

…olver.php

Co-authored-by: Victor Vasiloi <victor.vasiloi@gmail.com>
vvasiloi
vvasiloi previously approved these changes Nov 20, 2023
@lchrusciel lchrusciel merged commit 8c6ad41 into Sylius:1.12 Nov 22, 2023
25 checks passed
@lchrusciel
Copy link
Member

Thank you, @Prometee!

@Prometee Prometee deleted the fix-default-variant branch November 22, 2023 11:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Confirmed bugs or bugfixes. Maintenance CI configurations, READMEs, releases, etc.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants