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

Add a schema_provider option that enables the diff command for DBAL #1050

Merged
merged 1 commit into from
Dec 3, 2020

Conversation

veewee
Copy link
Contributor

@veewee veewee commented Aug 28, 2020

Q A
Type feature
BC Break no
Fixed issues #1047

Summary

This change makes it possible to configure a schema_provider that enables the diff command for doctrine DBAL as well.

Currently the PR allows you to do:

$dependencyFactory = DependencyFactory::fromConnection(
    new ConfigurationArray([...]),
    new ExistingConnection($connection)
);
$dependencyFactory->setService(SchemaProvider::class, new MySchemaProvider());
MigrationsConsoleRunner::addCommands($application, $dependencyFactory);

In which, MySchemaProvider is a Doctrine\Migrations\Provider\SchemaProvider.

@SenseException SenseException requested a review from a team September 1, 2020 21:13
@SenseException
Copy link
Member

The code looks good for me so far. For the feature itself @doctrine/team-migrations should take a look first.

@goetas
Copy link
Member

goetas commented Sep 2, 2020

Hello. Thanks for working on this.

IMO a schema provider should not be a configuration option but something passed to the dependency container.

Having a callback as configuration value makes configuration non cache-able and impossible to express in XML, JSON or YAML.

What about marking as public getSchemaProvider in the dep-factory and make the needed adjustments to make it work?
I'm also in favor of deprecating/marking-private the getEntityManager, since that method should not be used anywhere outside of the dependency factory.

@goetas
Copy link
Member

goetas commented Sep 2, 2020

Actually looking better, things are "almost" working.

$di->setService(SchemaProvider::class, $someSchemaProvider);

will already do the job.
the only thing that needs to be done is to expose the diff command even if the entity manager is not available

@veewee
Copy link
Contributor Author

veewee commented Sep 2, 2020

@goetas :

If you don't add it through configuration - it has this double way of configuring the tool. It needs proper documentation then.

Shall I give the configuration of the dependency directly another try in this PR or do you want to discuss it first?

@goetas
Copy link
Member

goetas commented Sep 2, 2020

If you don't add it through configuration - it has this double way of configuring the tool. It needs proper documentation then.

Indeed it should be documented, I've added in the past how to provide custom entity managers but did not think on the possibility of providing only the schema provider (and I think it is a really good idea for doing so).

I want to distinguish things that are configurations and things that are dependencies. Anything that can not be expressed in all the configuration formats (yaml, xml, json and php) can not go in the configurations pool.
that is the reason why you can not provide a connection via configuration (creating a connection might need access to php dependencies that can not be expressed in the configuration itself).
Later framework integrations (that are able to manage dependencies) can provide a custom dependencies in the form that each frameworks uses to express them (symfony uses its own dependency injection configured with xml or yaml,but laravel or laminas use just plain php)

@goetas
Copy link
Member

goetas commented Sep 2, 2020

In 2.x, the configuration object was huge.... mainly because was managing both dependencies and configurations

@veewee
Copy link
Contributor Author

veewee commented Sep 2, 2020

Ok, thanks for the side information! Will give it a try soon :)

@veewee
Copy link
Contributor Author

veewee commented Sep 2, 2020

@goetas : I removed the configuration part. Where do you want me to document how to configure a custom schema provider?

( FYI : I've kept the entity manager public, since it would be a BC break. If you want to make it private, you can do it in another PR, so that this new feature can be released in a minor? )

@goetas goetas self-assigned this Sep 17, 2020
@veewee
Copy link
Contributor Author

veewee commented Oct 9, 2020

Is there anything I can do in order to get this merged? Thanks!

@goetas
Copy link
Member

goetas commented Nov 28, 2020

@veewee i'm sorry for the delay, I got some time back to look into this. Could you please rebase it?
It looks good to me, and i hope that this PR makes its way already in 3.1

@veewee veewee force-pushed the diff-on-schema branch 2 times, most recently from 5d13b8f to e200ae3 Compare November 30, 2020 07:31
@veewee
Copy link
Contributor Author

veewee commented Nov 30, 2020

@goetas : rebased ;)

@goetas goetas changed the base branch from 3.0.x to 3.1.x December 2, 2020 08:58
@goetas goetas changed the base branch from 3.1.x to 3.0.x December 2, 2020 08:59
@goetas goetas changed the base branch from 3.0.x to 3.1.x December 2, 2020 14:03
@goetas goetas added this to the 3.1.0 milestone Dec 2, 2020
@goetas
Copy link
Member

goetas commented Dec 2, 2020

I've changed the base branch for this to 3.1.x since it adds a new feature.

Can @SenseException or @greg0ire review this please?

@goetas goetas requested a review from greg0ire December 2, 2020 14:05
@goetas goetas requested a review from SenseException December 2, 2020 14:05
@goetas goetas merged commit 2af8596 into doctrine:3.1.x Dec 3, 2020
@goetas
Copy link
Member

goetas commented Dec 3, 2020

Thanks for your work

@veewee veewee mentioned this pull request Dec 4, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants