Skip to content

Sylius 2.x compatibility #122

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

Open
wants to merge 9 commits into
base: 3.x
Choose a base branch
from

Conversation

shochdoerfer
Copy link

Updated dependencies and the code for Sylius 2.0 compatibility.

I was able to remove the JS logic, but not sure if the TwigComponent event logic is a bit too messy. Feels hackish, but I could not find a better way to hook into the Live Components for the Product and Taxon forms.

@shochdoerfer
Copy link
Author

The only thing "missing": I don't know how to trigger a change event in the Live component when the input was changed manually.

@shochdoerfer
Copy link
Author

Still need some more time to fix the build issues, will report back once it's done.

@loevgaard
Copy link
Member

Hi, @shochdoerfer

I have the feeling it would make more sense to take such steps as this PR suggestions in more PRs, so we can get some changes into the 2.x branch and create a new branch, 3.x, for the undoubtedly breaking changes the shift to 2.0 will give us. Wdyt?

@shochdoerfer
Copy link
Author

You mean, have a 3.x branch just for Sylius 2.0 and keep the 2.x branch for Sylius 1.x? I guess that makes sense if you want to support Sylius 1.x for a while.

The downside might be that some future PRs need to be added twice if they touch code that is fundamentally different in Sylius 1.x and 2.x (most likely the Twig stuff).

The same should probably apply for the Terms Plugin, right?

@loevgaard loevgaard changed the base branch from 2.x to 3.x May 12, 2025 11:44
@shochdoerfer shochdoerfer force-pushed the feature/sylius_v2 branch 4 times, most recently from 3fd1853 to 920e726 Compare May 28, 2025 17:58
Copy link

codecov bot commented May 28, 2025

Codecov Report

Attention: Patch coverage is 0% with 49 lines in your changes missing coverage. Please review.

Project coverage is 4.51%. Comparing base (510fb82) to head (82edf4b).

Files with missing lines Patch % Lines
...EventSubscriber/ProductFormComponentSubscriber.php 0.00% 22 Missing ⚠️
...g/EventSubscriber/TaxonFormComponentSubscriber.php 0.00% 22 Missing ⚠️
src/EventListener/ControllerSubscriber.php 0.00% 1 Missing ⚠️
src/EventListener/NotFoundSubscriber.php 0.00% 1 Missing ⚠️
src/EventListener/RedirectResponseTrait.php 0.00% 1 Missing ⚠️
src/Repository/RedirectRepository.php 0.00% 1 Missing ⚠️
src/Resolver/RedirectionPathResolver.php 0.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##               3.x    #122      +/-   ##
==========================================
- Coverage     4.90%   4.51%   -0.39%     
- Complexity     163     183      +20     
==========================================
  Files           29      31       +2     
  Lines          510     554      +44     
==========================================
  Hits            25      25              
- Misses         485     529      +44     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@shochdoerfer shochdoerfer force-pushed the feature/sylius_v2 branch 2 times, most recently from 1c3aefb to e632afa Compare May 28, 2025 18:41
@shochdoerfer shochdoerfer force-pushed the feature/sylius_v2 branch 5 times, most recently from 481522b to 1abc93a Compare June 22, 2025 15:20
@shochdoerfer
Copy link
Author

@loevgaard I managed to fix all dependencies issues that broke CI, except for one: psalm/plugin-symfony

The issue is that the composer.json of psalm/plugin-symfony says it depends on doctrine/annotations version ^1.8 or ^2 - but the code clearly needs 1.8 as deprecated functionality got removed in version 2, and Psalm Symfony depends on it. Seems no one noticed that so far.

So we have 2 options: Either remove psalm/plugin-symfony from the dev dependencies and fix the few remaining issues Psalm complains about, or move over to PHPStan. Both would be fine for me. Not sure how emotionally attached you are to Psalm :)

I guess I'll run into the same issue fixing the build for the Terms plugin.

@loevgaard
Copy link
Member

The issue is that the composer.json of psalm/plugin-symfony says it depends on doctrine/annotations version ^1.8 or ^2 - but the code clearly needs 1.8 as deprecated functionality got removed in version 2, and Psalm Symfony depends on it. Seems no one noticed that so far.

https://packagist.org/packages/psalm/plugin-symfony => It doesn't require doctrine/annotations

@shochdoerfer
Copy link
Author

You are right, but still the code depends on it, see here https://github.com/psalm/psalm-plugin-symfony/blob/e5f97e1257a28602ecf6871a74d361fc43ae12e4/src/Plugin.php#L56

@shochdoerfer
Copy link
Author

Which direction should we go? Temporarily remove the Psalm Symfony plugin until the issue is fixed?

@loevgaard
Copy link
Member

Which direction should we go? Temporarily remove the Psalm Symfony plugin until the issue is fixed?

Yes :-)

@shochdoerfer
Copy link
Author

@loevgaard build is green for me.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants