Switch to Doctrine/Migrations 3.0 #205
Merged
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
pamil
added
Enhancement
13 tasks
pamil
added a commit
to Sylius/Sylius
that referenced
this pull request
Aug 24, 2020
This PR was merged into the 1.8-dev branch.
Discussion
----------
### Customisations
- [x] Decorated migration factory to support migrations implementing `ContainerAwareInterface` - done in CoreBundle
- [x] Added topology-based version comparator in CoreBundle to achieve the following order:
- Sylius/Sylius
- Plugins (Sylius/PluginSkeleton: Sylius/InvoicingPlugin, Sylius/RefundPlugin)
- Sylius/Plus
- End-user application (Sylius/Sylius-Standard)
### Concerns
- [x] **Q:** Currently, migrations are doubled in `app/migrations` and `src/Sylius/Bundle/CoreBundle/Migrations` - could we safely delete the ones in `app/migrations`?
**A:** It should be possible to delete them in the next minor release - while migrating migrations database, it correctly assigns FQCN of the migration by its timestamp. However, we need to figure out how to upgrade existing apps, especially after we recommended copying migrations to the end-user's project before.
- [x] **Q:** All the migrations exist in CoreBundle, even though some of them contain SQL queries related to AdminApiBundle - what's the best way to tackle that (one might not use AdminApiBundle but only CoreBundle)? Should we split them into two bundles? If so, how to do it and allow for a seamless upgrade?
**A:** They can be safely extracted to AdminApiBundle as long as there are no migrations mixing both CoreBundle and AdminApiBundle. In that case, I'm not sure what's the best solution - probably skipping some of them if AdminApiBundle is not installed.
### Up next for upgrade to migrations 3.0-alpha
- [ ] Sylius-Standard
- [ ] PluginSkeleton
- [x] InvoicingPlugin - Sylius/InvoicingPlugin#175
- [x] RefundPlugin - Sylius/RefundPlugin#205
- [x] Plus (requiring InvoicingPlugin and RefundPugin) - https://github.com/Sylius/Plus/pull/575
### TODO
1. [x] Extract related utilities (comparator and factory) to a standalone package (SyliusLabs/DoctrineMigrationsExtraBundle?) with a following configuration schema:
```yaml
sylius_labs_doctrine_migrations_extra:
migrations:
"Sylius\InvoicingPlugin\Migrations": ["Sylius\Bundle\CoreBundle\Migrations"]
```
Tests need to be added for both those services, configuration and DI extension.
2. [x] Use bundle notation for specifying migrations instead of prepending `doctrine_migrations` with relative paths, see doctrine/DoctrineMigrationsBundle#318.
3. [x] Decouple CoreBundle migrations from AdminApiBundle (either conditionally add them or move somehow to AdminApiBundle, while maintaining BC - after running all migrations on `master` branch, checking in this branch should result in no additional migrations)
4. [ ] Spread those changes to all the repositories mentioned above
### References
- https://www.goetas.com/blog/multi-namespace-migrations-with-doctrinemigrations-30/
- https://www.goetas.com/blog/released-doctrinemigrations-30-alpha/
- doctrine/migrations#967
- doctrine/DoctrineMigrationsBundle#314
Commits
-------
0bf2c7e Move existing migrations to Sylius\Bundle\CoreBundle\Migrations
66ef7dd Allow injecting container to the migrations & implement a topology-based sorter
aca4fd2 Split migrations between CoreBundle & AdminApiBundle
0d22ea0 Improve SyliusCoreExtension prepend logic
GSadee
force-pushed
the
migrations-alpha
branch
from
e8b6743 to
b45f3e2
Compare
GSadee
changed the title
GSadee
force-pushed
the
migrations-alpha
branch
from
b45f3e2 to
3efec33
Compare
GSadee
force-pushed
the
migrations-alpha
branch
from
3efec33 to
fc797bf
Compare
GSadee
force-pushed
the
migrations-alpha
branch
from
fe31b08 to
dd26289
Compare
GSadee
force-pushed
the
migrations-alpha
branch
from
8d96963 to
62ad4b9
Compare
GSadee
changed the title
AdamKasp
approved these changes
Sep 22, 2020
Tomanhez
approved these changes
Sep 22, 2020
lchrusciel
approved these changes
Sep 22, 2020
| @@ -17,7 +17,7 @@ public function __construct(RemainingTotalProviderInterface $remainingTotalProvi | |||
| $this->remainingTotalProvider = $remainingTotalProvider; | |||
| } | |||
|
|
|||
| public function calculateForUnitWithIdAndType(int $id, RefundType $refundType, ?float $amount = null) | |||
| public function calculateForUnitWithIdAndType(int $id, RefundType $refundType, ?float $amount = null): int | |||
| @@ -8,5 +8,5 @@ | |||
|
|
|||
| interface UnitRefundTotalCalculatorInterface | |||
| { | |||
| public function calculateForUnitWithIdAndType(int $id, RefundType $refundType, ?float $amount = null); | |||
| public function calculateForUnitWithIdAndType(int $id, RefundType $refundType, ?float $amount = null): int; | |||
|
Thanks, Kamil and Grzegorz! 🎉 |
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.
Related to Sylius/Sylius#11389.