cakephp / phinx

PHP Database Migrations for Everyone
https://phinx.org
MIT License
4.47k stars 891 forks source link

Depend on doctrine/dbal instead of cakephp/database #2217

Open PabloKowalczyk opened 1 year ago

PabloKowalczyk commented 1 year ago

Hello guys, I would like to propose change database abstraction to doctrine/dbal.

Look at cakephp/database's (v5) dependency tree:

/var/www/html $ composer show cakephp/database --tree
cakephp/database 5.0.0 Flexible and powerful Database abstraction library with a familiar PDO-like API
├──cakephp/chronos ^3.0
│  └──php >=8.1
├──cakephp/core ^5.0
│  ├──cakephp/utility ^5.0
│  │  ├──cakephp/core ^5.0 (circular dependency aborted here)
│  │  └──php >=8.1
│  ├──league/container ^4.2
│  │  ├──php ^7.2 || ^8.0
│  │  └──psr/container ^1.1 || ^2.0
│  │     └──php >=7.4.0
│  ├──php >=8.1
│  └──psr/container ^2.0
│     └──php >=7.4.0
├──cakephp/datasource ^5.0
│  ├──cakephp/core ^5.0
│  │  ├──cakephp/utility ^5.0
│  │  │  ├──cakephp/core ^5.0 (circular dependency aborted here)
│  │  │  └──php >=8.1
│  │  ├──league/container ^4.2
│  │  │  ├──php ^7.2 || ^8.0
│  │  │  └──psr/container ^1.1 || ^2.0
│  │  │     └──php >=7.4.0
│  │  ├──php >=8.1
│  │  └──psr/container ^2.0
│  │     └──php >=7.4.0
│  ├──php >=8.1
│  └──psr/simple-cache ^2.0 || ^3.0
│     └──php >=8.0.0
├──php >=8.1
└──psr/log ^3.0
   └──php >=8.0.0

vs DBAL's:

/var/www/html $ composer show doctrine/dbal --tree
doctrine/dbal 3.6.6 Powerful PHP database abstraction layer (DBAL) with many features for database schema introspection and management.
├──composer-runtime-api ^2
├──doctrine/cache ^1.11|^2.0
│  └──php ~7.1 || ^8.0
├──doctrine/deprecations ^0.5.3|^1
│  └──php ^7.1 || ^8.0
├──doctrine/event-manager ^1|^2
│  └──php ^8.1
├──php ^7.4 || ^8.0
├──psr/cache ^1|^2|^3
│  └──php >=8.0.0
└──psr/log ^1|^2|^3
   └──php >=8.0.0

For me, it looks like too much dependencies for a database abstraction (for example Cake's core which needs DI container), while DBAL has only what is needed: small deprecations wrapper and lib for events.

Would You consider changing db abstraction to DBAL? I can do a PR.

dereuromark commented 1 year ago

Currently, phinx serves as a direct internal dependency of https://github.com/cakephp/migrations and CakePHP apps. So how would this be affected by such a change? Right now, the benefit of this being the same code is that migrations are quite easy to write and understand, as well as customize. I wonder how this would look like with such a change under the hood. Would the abstraction be good enough that all cases needed can still be covered with it just as it was so far?

PabloKowalczyk commented 1 year ago

Currently, phinx serves as a direct internal dependency of https://github.com/cakephp/migrations and CakePHP apps.

On the other hand I use Phinx in Symfony-based app and probalby some users uses Phinx without any framework.

So how would this be affected by such a change?

IMO only dependencies will (should) change, not behavior of Phinx, so no change from user's POV.

Would the abstraction be good enough that all cases needed can still be covered with it just as it was so far?

Actually I don't know now, because I don't started PR yet, but DBAL is quite mature and popular, so there should not be any problems.

othercorey commented 1 year ago

I don't think a PR is welcome here. I don't think the change would be merged. There isn't any value for the maintainers.

PabloKowalczyk commented 1 year ago

@othercorey what about value for users? Less dependencies is better.

I think that there is option to do another abstraction layer, Phinx's abstraction will provide interface which can be implemented by Cake's db abstraction, DBAL and actually any lib that will be interested. Will it be accepted?

MasterOdin commented 1 year ago

Right now, the link to cakephp/database is purely for the query builder, and isn't really a critical part of the rest of the project. I'm still of the opinion that we should make cakephp/database completely optional, and then could additionally add support for other DBALs assuming such an interface isn't overly complex and adds much maintenance overhead (and that if said overhead grows, can just remove support for offending package).

For reference, I use phinx with a codebase that doesn't use any framework nor do I use the query builder feature, and so installing doctrine or cakephp is a waste for me

PabloKowalczyk commented 1 year ago

For me, Phinx without any db abstraction dependency is even better. I found that there is aura/sqlquery which states that:

Provides query builders for MySQL, Postgres, SQLite, and Microsoft SQL Server. These builders are independent of any particular database connection library, although PDO in general is recommended.

Maybe using it (or any other lib that provides "only" query builder) instead of cakephp/database is a way?

MasterOdin commented 1 year ago

To be clear, cakephp/database will always remain the principally "blessed" library for the query builder available to users, given this project's owernship by cakephp, and also its heavy coupling into cakephp/migrations. I just want to make it optional for people to make it possible to bring their own (or none at all).

PabloKowalczyk commented 1 year ago

Do you think it will be easier to make cakephp/database optional than switching it to other lib?

othercorey commented 1 year ago

I'm dropping any objections to this change. I assume it will be part of a (correct this time) 1.0 release.

Feel free to discuss any goals that are best for phinx without me chiming in on cake dev compatibility :).

PabloKowalczyk commented 1 year ago

It turns out that better option is to move forward https://github.com/cakephp/phinx/issues/1754 and then this issue can be closed, because there will be no gain in changing underlying library for query builder.