[Maintenance] Require precise packages instead of generic sylius/sylius #982
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
jakubtobiasz
approved these changes
Apr 29, 2024
diimpp
approved these changes
Apr 29, 2024
Prometee
requested changes
Apr 29, 2024
composer.json
Outdated
| @@ -26,7 +26,10 @@ | |||
| "require": { | |||
| "php": "^8.1", | |||
| "sylius/paypal-plugin": "^1.6", | |||
| "sylius/sylius": "~1.13.0", | |||
| "sylius/core-bundle": "~1.13.0", | |||
There was a problem hiding this comment.
Suggested change
| "sylius/core-bundle": "~1.13.0", |
Like @diimpp mentioned it, core is already required by shop, admin or api bundle. Then it could be omitted.
There was a problem hiding this comment.
To be honest, yes it is not required. However, I think it is good practice to have explicit composer requirements for the things that you use. Now, we can argue that the same is true for all the other sub-dependencies of sylius/core-bundle. And here I would go pragmatic and just assume that core-bundle is an aggregator for all of them.
To sum up, I guess we would be fine with leaving core-bundle as it is. But removing it will also not change too much
lchrusciel
force-pushed
the
conver-sylius-dependencies
branch
from
e00d484
to
a2f9c94
Compare
Prometee
approved these changes
May 3, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
I propose switching from the generic "sylius/sylius" dependency to a set of high-level, more explicit ones. Of course, I would like to require "sylius/core-bundle" - as it is the core and heart of Sylius itself. In addition, with this change, we will require:
These packages should be considered as a default stack of Sylius at the moment. What is missing from this list? Deprecated admin-api, but that should be considered as a benefit of this change.
In addition what we are earning with it is the fact that if in the future we will add a new layer to Sylius it won't be automatically propagated to all existing shops. Also, we are making it easier for people to turn off one of the accessing layers to Sylius