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

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

Open
PabloKowalczyk opened this issue Sep 11, 2023 · 10 comments
Open

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

PabloKowalczyk opened this issue Sep 11, 2023 · 10 comments
Labels
enhancement major BC breaking / next major

Comments

@PabloKowalczyk
Copy link

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
Copy link
Member

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?

@dereuromark dereuromark added enhancement major BC breaking / next major labels Sep 11, 2023
@PabloKowalczyk
Copy link
Author

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
Copy link
Member

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
Copy link
Author

@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
Copy link
Member

MasterOdin commented Sep 11, 2023

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
Copy link
Author

PabloKowalczyk commented Sep 11, 2023

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
Copy link
Member

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
Copy link
Author

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

@othercorey
Copy link
Member

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
Copy link
Author

It turns out that better option is to move forward #1754 and then this issue can be closed, because there will be no gain in changing underlying library for query builder.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement major BC breaking / next major
Projects
None yet
Development

No branches or pull requests

4 participants