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

[Core] Reorganize namespaces of services related with catalog promotions #13623

Merged

Conversation

GSadee
Copy link
Member

@GSadee GSadee commented Feb 9, 2022

Q A
Branch? 1.11
Bug fix? no
New feature? no
BC breaks? no
Deprecations? no
Related tickets
License MIT

@GSadee GSadee added the DX Issues and PRs aimed at improving Developer eXperience. label Feb 9, 2022
@GSadee GSadee requested a review from a team as a code owner February 9, 2022 12:32
@probot-autolabeler probot-autolabeler bot added the Documentation Documentation related issues and PRs - requests, fixes, proposals. label Feb 9, 2022
@GSadee GSadee force-pushed the core-catalog-promotions-reorganization branch 3 times, most recently from 16d2b8e to 7ac5440 Compare February 9, 2022 19:57
@probot-autolabeler probot-autolabeler bot added the API APIs related issues and PRs. label Feb 9, 2022
@GSadee GSadee force-pushed the core-catalog-promotions-reorganization branch 3 times, most recently from 3f3c5d5 to 1a98be5 Compare February 10, 2022 06:22
@GSadee GSadee force-pushed the core-catalog-promotions-reorganization branch from 1a98be5 to 7cf8b7b Compare February 10, 2022 09:03
@@ -11,9 +11,9 @@

declare(strict_types=1);

namespace Sylius\Bundle\CoreBundle\Validator\Constraints;
namespace Sylius\Bundle\CoreBundle\CatalogPromotion\Validator\Constraints;
Copy link
Member

Choose a reason for hiding this comment

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

I wonder, can we move it to PromotionBundle? It's almost identical, we could maybe even unify these services and register them under 2 different names for 2 different tags

Copy link
Member

Choose a reason for hiding this comment

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

It also somehow completes this PR

Copy link
Member Author

Choose a reason for hiding this comment

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

It's almost identical to.. which service? I agree that the interface should be moved to the PromotionBundle, but I wonder if this validator also as it is connected with entities not related with promotions 🤔 However, even if this one could be moved to the bundle, what about the constraint, moving it there doesn't make sense (because of validation messages) and separating them in 2 different bundles also? I would be for considering it in your separate PR as this one only changes the namespaces in CoreBundle

Copy link
Member

Choose a reason for hiding this comment

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

Almost identical to CatalogPromotionActionValidator, sorry, I've lost the part of the sentence 🖖 I would probably be for moving all of these into the PromotionBundle, as this functionality is not related to products or variants at all (specific validators are) and drop the validation messages in constraint and just use translation keys in the validators (we're anyway doing this e.g. here). But I agree it can be done in the separate PR 🚀

@Zales0123 Zales0123 merged commit 9b2960f into Sylius:1.11 Feb 10, 2022
@Zales0123
Copy link
Member

Thank you, Grzegorz! 🎉

@GSadee GSadee deleted the core-catalog-promotions-reorganization branch February 10, 2022 11:05
Zales0123 added a commit that referenced this pull request Feb 10, 2022
…t to PromotionBundle (GSadee)

This PR was merged into the 1.11 branch.

Discussion
----------

| Q               | A
| --------------- | -----
| Branch?         | 1.11
| Bug fix?        | no
| New feature?    | no
| BC breaks?      | no
| Deprecations?   | no
| Related tickets | fixes #13623 (comment)
| License         | MIT

<!--
 - Bug fixes must be submitted against the 1.10 or 1.11 branch(the lowest possible)
 - Features and deprecations must be submitted against the master branch
 - Make sure that the correct base branch is set

 To be sure you are not breaking any Backward Compatibilities, check the documentation:
 https://docs.sylius.com/en/latest/book/organization/backward-compatibility-promise.html
-->


Commits
-------

60496db [CatalogPromotion] Move scope validator and constraint to PromotionBundle
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
API APIs related issues and PRs. Documentation Documentation related issues and PRs - requests, fixes, proposals. DX Issues and PRs aimed at improving Developer eXperience.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants