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

[Maintenance] Bump plugin to Sylius 2.0 #437

Merged
merged 6 commits into from
Nov 27, 2024

Conversation

mpysiak
Copy link
Member

@mpysiak mpysiak commented Nov 18, 2024

Q A
Branch? 2.0
Bug fix? no
New feature? no
Related tickets n/a

@mpysiak mpysiak requested a review from a team as a code owner November 18, 2024 06:18
@mpysiak mpysiak force-pushed the SYL-3914-bump-plugin-to-2.0 branch from b85699d to b347b9d Compare November 18, 2024 07:23
@mpysiak mpysiak changed the title Syl 3914 bump plugin to 2.0 [Maintenance] Bump plugin to Sylius 2.0 Nov 18, 2024
@mpysiak mpysiak added the Maintenance Configurations, READMEs, releases, etc. label Nov 18, 2024
@mpysiak mpysiak force-pushed the SYL-3914-bump-plugin-to-2.0 branch 3 times, most recently from f34b9c1 to fd5d520 Compare November 25, 2024 07:04
.github/workflows/build.yml Outdated Show resolved Hide resolved
composer.json Outdated Show resolved Hide resolved
private RefundFactoryInterface $refundFactory,
private RemainingTotalProviderInterface $remainingTotalProvider,
private OrderRepositoryInterface $orderRepository,
private ObjectManager $refundManager,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it makes sense to replace every ObjectManager with EntityManagerInterface since odm support is dropped. (There are 12 instances of ObjectManager in the codebase.)

Suggested change
private ObjectManager $refundManager,
private EntityManagerInterface $refundManager,

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@diimpp done in #452

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you plan to rename all templates to snake_case? This one still at camelCase.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this file still exists, but now empty. But I'm not sure if GH shows this right.

If true, then this file as well tests/Application/assets/shop/entry.js

"symfony/messenger": "^5.4.21 || ^6.4"
"php": "^8.2",
"knplabs/knp-snappy-bundle": "^1.10",
"myclabs/php-enum": "^1.8",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would say it's high time to drop pseudo enum support and use sylius/sylius style consts (or php enums)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We would like to do that. There is another task for that. This will be done in another PR

@mpysiak
Copy link
Member Author

mpysiak commented Nov 26, 2024

Thanks @diimpp for your code review! However, this PR is still very much a work in progress, and I didn't mark it as a draft :)

@mpysiak mpysiak marked this pull request as draft November 26, 2024 04:51
@mpysiak mpysiak force-pushed the SYL-3914-bump-plugin-to-2.0 branch 7 times, most recently from f5fe65a to 9dddf97 Compare November 26, 2024 10:14
@diimpp
Copy link
Member

diimpp commented Nov 26, 2024

Thanks @diimpp for your code review! However, this PR is still very much a work in progress, and I didn't mark it as a draft :)

@mpysiak You're welcome. Please ping me when it's done. :)

Do you plan to refactor how refund items entities are structured or it's more of the maintenance PR? There is this issue from 2020 #193, which I think still actual. Described problem is more about letting people develop based of this plugin, rather then solving immediate bug.

tldr: Add "root" entity Refund (Or RefundRequest) to make relationship between RefundUnit, RefundPayment and CreditMemo.

@mpysiak mpysiak force-pushed the SYL-3914-bump-plugin-to-2.0 branch 10 times, most recently from eb2fe2d to 51cc2ca Compare November 26, 2024 18:28
@mpysiak mpysiak marked this pull request as ready for review November 27, 2024 05:19
@mpysiak mpysiak force-pushed the SYL-3914-bump-plugin-to-2.0 branch from 51cc2ca to 92bd78c Compare November 27, 2024 05:34
@mpysiak
Copy link
Member Author

mpysiak commented Nov 27, 2024

@diimpp This PR is mainly Maintenance - just to bump to Sylius 2.0. I've removed deprecations, add some small refactors. But PR is getting huge, so it will be probably merged (after CR from the team). Comments and fixes will be done in other commits, to make it small and more transparent 🖖

Copy link
Member

@GSadee GSadee left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the separate PRs:

  • add an UPGRADE file with all these changes
  • change the structure to the new recommended one by moving directories from src/Resources to main directory

.github/workflows/build.yaml Show resolved Hide resolved
.github/workflows/build.yaml Show resolved Hide resolved
composer.json Show resolved Hide resolved
composer.json Show resolved Hide resolved
composer.json Show resolved Hide resolved
Comment on lines +3 to +9
<tr>
{% hook 'table.header' %}
</tr>
</thead>
<tbody>
{% hook 'table.body' %}
</tbody>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It could be confusing not to have a table hook, I don't know if it wouldn't be better to add an additional template with it and trigger these two there 🤔

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This one should be refactored

@GSadee GSadee merged commit 7245aca into Sylius:2.0 Nov 27, 2024
8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Maintenance Configurations, READMEs, releases, etc.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants