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

Allow multiple doctrine ORM instance #96

Closed
wants to merge 2 commits into from

Conversation

jdeniau
Copy link

@jdeniau jdeniau commented Aug 1, 2018

Fixes #90

@jdeniau jdeniau force-pushed the jd-feat-multipleDoctrineOrm branch from 75d71d2 to 6dedf04 Compare August 1, 2018 08:36
@jdeniau
Copy link
Author

jdeniau commented Aug 1, 2018

I don't really know why the tests fails. It does not seems related to this PR

@jdeniau jdeniau force-pushed the jd-feat-multipleDoctrineOrm branch from 6dedf04 to 24b4792 Compare August 1, 2018 08:55
@jdeniau jdeniau force-pushed the jd-feat-multipleDoctrineOrm branch from f4ea795 to 8b6fe42 Compare August 1, 2018 13:05
@theofidry
Copy link
Owner

Thanks for the PR!

The tests seem to fail due to a new release of the Doctrine packages which introduces more deprecation notices. No biggie, but that would need to be fixed in a separate PR.

Regarding the PR itself, I'm having a bit of trouble to understand: so the strategy is to "duplicate" the current services per entity manager and then you leave the responsibility to the user to pick the right one?

@jdeniau
Copy link
Author

jdeniau commented Aug 3, 2018

@theofidry Yes, I used the same strategy that doctrine uses.

Imagine you have the following doctrine configuration:

doctrine:
  orm:
    default_entity_manager: foo
    entity_managers:
      foo: 
        connection: xxx
        # etc.
      bar: 
        connection: xxx
        # etc.

Then doctrine bundle will generate:

  • two services named:
    • doctrine.orm.foo_entity_manager
    • doctrine.orm.bar_entity_manager
  • an alias named doctrine.orm.entity_manager for the service doctrine.orm.foo_entity_manager.

(and it will also generate 2 services for each cache, entity_listener, etc.).

I applied the same strategy here:

  • two services named:
    • fidry_alice_data_fixtures.doctrine.foo_purger_loader
    • fidry_alice_data_fixtures.doctrine.bar_purger_loader
  • the old service fidry_alice_data_fixtures.doctrine.purger_loader is now an alias on fidry_alice_data_fixtures.doctrine.foo_purger_loader

@theofidry
Copy link
Owner

The difference is that when you persist an object with Doctrine, provided you have multiple connections/managers, you want to specify which one as wanting to persist it for all of them is much less likely. In alice however, I think it would be more common the other way around: if you have an entity managed by a manager and another entity manager by another, I think most people would like Alice to figure out which manager should be used for which entity.

So I think a different approach that would work better for most cases would be as @dkarlovi mentioned on Slack:

  • fetching all managers
  • asking if they are managing the class in question
  • if so, call persist() and mark them dirty
  • repeat for each fixture loaded
  • flush all dirty managers (edited)

Maybe @dkarlovi can also help you out with the implementation

jdeniau added a commit to jdeniau/AliceDataFixtures that referenced this pull request Aug 29, 2018
@Zayon
Copy link

Zayon commented Sep 10, 2018

Any updates on this ? This fix would be appreciated 😄

edit : I did some work (still WIP, need to test) here : #99

@jdeniau
Copy link
Author

jdeniau commented Sep 10, 2018

@Zayon I worked on this here too, but I did not have time to finish this.

Feel free to compare

@theofidry
Copy link
Owner

theofidry commented Sep 10, 2018

Closing as I think @Zayon follow up (#99) is a good take on this. Hope this one gets in :)

@theofidry theofidry closed this Sep 10, 2018
@dkarlovi
Copy link
Contributor

Sorry all, I should really queue these tasks better, I should have done a follow-up here, my bad.

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

Successfully merging this pull request may close these issues.

4 participants