doctrine / dbal

Doctrine Database Abstraction Layer
https://www.doctrine-project.org/projects/dbal.html
MIT License
9.48k stars 1.34k forks source link

Mark eligible classes final #3585

Closed morozov closed 4 years ago

morozov commented 5 years ago

The issue was originally raised in https://github.com/doctrine/dbal/pull/3007#issuecomment-362630921.

A quick check with Ocramius/Finalizer shows:

$ finalizer finalizer:check-final-classes lib/
Following classes need to be made final:
+-----------------------------------------------------------------+
| Doctrine\DBAL\Sharding\ShardChoser\MultiTenantShardChoser       |
| Doctrine\DBAL\Sharding\SQLAzure\Schema\MultiTenantVisitor       |
| Doctrine\DBAL\Sharding\SQLAzure\SQLAzureFederationsSynchronizer |
| Doctrine\DBAL\Sharding\PoolingShardManager                      |
| Doctrine\DBAL\Id\TableGeneratorSchemaVisitor                    |
| Doctrine\DBAL\Driver\OCI8\Driver                                |
| Doctrine\DBAL\Driver\Mysqli\Driver                              |
| Doctrine\DBAL\Driver\Mysqli\MysqliStatement                     |
| Doctrine\DBAL\Driver\PDOSqlsrv\Driver                           |
| Doctrine\DBAL\Driver\PDOSqlsrv\Statement                        |
| Doctrine\DBAL\Driver\PDOMySql\Driver                            |
| Doctrine\DBAL\Driver\IBMDB2\DB2Connection                       |
| Doctrine\DBAL\Driver\IBMDB2\DB2Statement                        |
| Doctrine\DBAL\Driver\IBMDB2\DB2Driver                           |
| Doctrine\DBAL\Driver\SQLSrv\SQLSrvStatement                     |
| Doctrine\DBAL\Driver\SQLSrv\Driver                              |
| Doctrine\DBAL\Driver\SQLSrv\SQLSrvConnection                    |
| Doctrine\DBAL\Driver\SQLAnywhere\SQLAnywhereConnection          |
| Doctrine\DBAL\Driver\SQLAnywhere\Driver                         |
| Doctrine\DBAL\Driver\SQLAnywhere\SQLAnywhereStatement           |
| Doctrine\DBAL\Driver\PDOPgSql\Driver                            |
| Doctrine\DBAL\Driver\PDOOracle\Driver                           |
| Doctrine\DBAL\Driver\PDOSqlite\Driver                           |
| Doctrine\DBAL\Driver\StatementIterator                          |
| Doctrine\DBAL\Cache\ResultCacheStatement                        |
| Doctrine\DBAL\Cache\ArrayStatement                              |
| Doctrine\DBAL\Schema\Synchronizer\SingleDatabaseSynchronizer    |
| Doctrine\DBAL\Schema\Visitor\RemoveNamespacedAssets             |
| Doctrine\DBAL\Portability\Statement                             |
+-----------------------------------------------------------------+

For all finalized classes, all their properties and protected methods should be declard private and renamed if they violate the PSR-2 naming conventions.

mvorisek commented 2 years ago

The world is not perfect nor bug free.

Currently, in the atk4/data project, I fixed a lot of issues by extending the specific DB platform. It works well. Most of the issues, I reported here or even proposed a PR.

Such fixing is impossible when the classes are declared final. Would it be possible to remove the final from the driver classes so the oppurtunity to fixing the impl. from the target project side is still possible?

derrabus commented 2 years ago

I fixed a lot of issues by extending the specific DB platform.

The platform classes are not final, so you can continue to do this.

Would it be possible to remove the final from the driver classes so the oppurtunity to fixing the impl. from the target project side is still possible?

I don't believe that this is really necessary. The DBAL has a well defined driver and middleware architecture that should allow you to easily implement your own driver or hook into whatever the bundled drivers are doing.

If you feel like you need to patch the driver classes, these are your options:

mvorisek commented 2 years ago

One example are driver ExceptionConverter classes. They should not be final.

derrabus commented 2 years ago

ExceptionConverter implementations can be decorated. You gain nothing by extending them.

github-actions[bot] commented 2 years ago

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.