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 alternative connection/entity-manager via --em or --conn option #1023

Merged
merged 3 commits into from
Sep 5, 2020

Conversation

goetas
Copy link
Member

@goetas goetas commented Jun 25, 2020

Q A
Type bug/feature/improvement
BC Break no
Fixed issues #1021 doctrine/DoctrineMigrationsBundle#341

@goetas goetas force-pushed the allow-custom-em-conn branch from 11010c3 to aea58d2 Compare June 25, 2020 08:24
@goetas
Copy link
Member Author

goetas commented Jun 27, 2020

Note for my self: there should be also a config parameter in the configurations allowing you to choose the entity manager or the connection

@goetas goetas force-pushed the allow-custom-em-conn branch 3 times, most recently from 78299d3 to 3f17e20 Compare June 28, 2020 06:43
docs/en/reference/configuration.rst Outdated Show resolved Hide resolved
docs/en/reference/configuration.rst Show resolved Hide resolved
docs/en/reference/custom-configuration.rst Outdated Show resolved Hide resolved
docs/en/reference/custom-configuration.rst Outdated Show resolved Hide resolved
docs/en/reference/custom-configuration.rst Outdated Show resolved Hide resolved
docs/en/reference/custom-configuration.rst Outdated Show resolved Hide resolved
@AndyDune
Copy link

AndyDune commented Jul 3, 2020

It is very important feature for me.

@goetas
Copy link
Member Author

goetas commented Jul 6, 2020

@SenseException

It's like ConnectionRegistryConnection isn't supposed to be a ConnectionLoader. And the DependencyFactory is already too crowded for implementing new methods there. I have currently no good ideas for this.

Yeah, ideally the connection ConnectionLoader should have a "name", passed as parameter.... I'm not sure if it is backward compatible (yes?) it feels as if the connection should not be part to the dependency factory...

@SenseException
Copy link
Member

I'm not sure if it is backward compatible (yes?)

I guess it's not backward compatible, but ConnectionLoader is marked as internal (and probably out of BC promise?) and the classes that are implementing it are declared final. Is there a guide line for such a case?

@goetas
Copy link
Member Author

goetas commented Jul 9, 2020

Hmm... that is a good point... (in reality that makes it confusing, ConnectionLoader probably should not be internal.... since that is how ppl can specify custom connections...)

@goetas
Copy link
Member Author

goetas commented Jul 18, 2020

With 9f75d67 i have refactored how connections are loaded.

The biggest change is the following:

before

/**
 * @internal
 */
interface ConnectionLoader
{
    public function getConnection() : Connection;
}

after

interface ConnectionLoader
{
    public function getConnection(?string $name = null) : Connection;
}

The change is not backward compatible, but given:

I think that we could discuss the possibility to release it as 3.1 instead of making a new major release just for this change.

What do you think?

cc @greg0ire @SenseException @alcaeus

@greg0ire
Copy link
Member

If the changed API was marked as @internal, why are you saying the change isn't backward compatible?

@SenseException
Copy link
Member

I guess this is a fitting change for the interface.

ConnectionLoader probably should not be internal.... since that is how ppl can specify custom connections...)

Should the interface stay @internal?

@goetas
Copy link
Member Author

goetas commented Jul 23, 2020

If the changed API was marked as @internal, why are you saying the change isn't backward compatible?

to be honest i got lost and i did not notice that was marked as @internal. IMO it should not have been marked so, but now is convenient.

Should the interface stay @internal?

I think not, if ppl have weird connection setups, that interface is the way how they can provide the connection to the library.

It seems that both of you agree with proceeding with this, I will update tests and documentation by using the currently proposed approach.

@SenseException
Copy link
Member

It looks like it.

@goetas goetas force-pushed the allow-custom-em-conn branch 2 times, most recently from 9a8543d to 27bfcd6 Compare August 1, 2020 08:14
@goetas goetas marked this pull request as ready for review August 1, 2020 08:14
@goetas goetas added this to the 3.1.0 milestone Aug 1, 2020
@goetas goetas force-pushed the allow-custom-em-conn branch from 27bfcd6 to 008d9ed Compare August 1, 2020 08:20
@goetas
Copy link
Member Author

goetas commented Aug 1, 2020

After a lot of work, this is ready for review!

@goetas goetas force-pushed the allow-custom-em-conn branch from 008d9ed to 1b0ff32 Compare August 1, 2020 08:25
@goetas goetas closed this Aug 1, 2020
@goetas goetas reopened this Aug 1, 2020
@goetas goetas force-pushed the allow-custom-em-conn branch from 1b0ff32 to 27248a8 Compare August 1, 2020 14:20
@goetas
Copy link
Member Author

goetas commented Aug 1, 2020

@andrew-demb @greg0ire You are right, i somehow missed that. Thanks!

@goetas goetas force-pushed the allow-custom-em-conn branch from 27248a8 to a83b35f Compare August 1, 2020 14:34
@greg0ire greg0ire changed the title Allow alternative connection/entity-manger via --em or --conn option Allow alternative connection/entity-manager via --em or --conn option Aug 1, 2020
@goetas goetas force-pushed the allow-custom-em-conn branch from 26a9a72 to 107a313 Compare August 4, 2020 09:02
@goetas
Copy link
Member Author

goetas commented Aug 4, 2020

All suggestions have been applied.

@goetas goetas requested a review from greg0ire September 2, 2020 07:29
Co-authored-by: Grégoire Paris <[email protected]>
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.

5 participants