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

[3.0-alpha] Container-aware migrations #314

Closed
pamil opened this issue Apr 22, 2020 · 9 comments
Closed

[3.0-alpha] Container-aware migrations #314

pamil opened this issue Apr 22, 2020 · 9 comments
Milestone

Comments

@pamil
Copy link

pamil commented Apr 22, 2020

I'm currently trying to run Sylius with a couple of plugins using 3.0-alpha and existing migrations extending ContainerAwareInterface stopped working.

There are a few cases in which we need to access a parameter or a service from the container. An example:

  1. We have name on product_variant
  2. We want to make it translatable by adding product_variant_translation table (id, product_variant_id, locale, name) and removing name from product_variant
  3. We need to create rows in product_variant_translation with name taken from related product_variant and locale being %locale% parameter taken from the container

In these cases, being able to access the container or having some other way to inject parameters and services is really helpful.

@goetas
Copy link
Member

goetas commented Apr 22, 2020

You can provide a custom migration factories, that can provide the container or any other needed dependency. I have blogged about it in https://www.goetas.com/blog/multi-namespace-migrations-with-doctrinemigrations-30/#extra%3A-custom-migration-factories-aka-decorating-services (see Extra: Custom migration factories (aka Decorating services) section)

@alcaeus
Copy link
Member

alcaeus commented Apr 22, 2020

We might want to provide such a factory for convenience sake in the bundle 🤔

@goetas
Copy link
Member

goetas commented Apr 22, 2020

hmmm... here the question comes. Is it better to have ContainerAwareInterface migrations or to allow proper dependency injection any pass the dependencies via migration consturctor?

Wasn't ContainerAwareInterface deprecated?

@alcaeus
Copy link
Member

alcaeus commented Apr 22, 2020

If you can make proper dependency injection work, that's even better. That would require making migration services as was done for fixtures a while back.

@pamil
Copy link
Author

pamil commented Apr 22, 2020

Thanks a lot for the quick reply! Migration factory sounds good and I will help me move forward with testing.

As for making it built-in, dependency injection sounds like the best solution, I try to avoid ContainerAwareInterface as much as I can.

In FriendsOfBehat/SymfonyExtension we have a similar logic - if a service with id equal to context class exists, it uses the service; if it doesn't, it creates an instance based on the class name.

@goetas
Copy link
Member

goetas commented Apr 22, 2020

Dependency injection is already possibile, is just about providing a proper factory.
I guess that some built in factories can be provided in future feature releases

@pamil
Copy link
Author

pamil commented Apr 22, 2020

Btw. the blogpost snippet for container-aware factory needs some adjustments to work:

App\Core\ProjectVersionFactory.inner:
        class: Doctrine\Migrations\Version\MigrationFactory
        factory: ['@doctrine.migrations.dependency_factory', 'getMigrationFactory']

Added class option and @ in the first factory argument.

@goetas
Copy link
Member

goetas commented Apr 22, 2020

Thanks, will fix that

@goetas goetas added this to the 3.0.0 milestone Apr 25, 2020
@goetas
Copy link
Member

goetas commented May 9, 2020

Closing as this feature is already available. and the issue reported on my blog has been fixed.

As a note, #323 should improve even more the possibility to provide more external services into the migrations library.

@goetas goetas closed this as completed May 9, 2020
pamil added a commit to Sylius/Sylius that referenced this issue 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
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

No branches or pull requests

3 participants