Make classes final where applicable. #3590

jwage · 2019-06-04T17:22:47Z

Q	A
Type	improvement
BC Break	yes
Fixed issues	fixes #3585

Summary

lib/Doctrine/DBAL/Driver/Mysqli/MysqliStatement.php

tests/Doctrine/Tests/DBAL/Driver/StatementIteratorTest.php

lib/Doctrine/DBAL/Driver/PDOSqlite/Driver.php

morozov · 2019-06-04T22:45:35Z

FWIW, the current state is:

$ finalizer finalizer:check-final-classes lib
Following classes need to be made final:
+--------------------------------------+
| Doctrine\DBAL\Driver\Mysqli\Driver   |
| Doctrine\DBAL\Driver\PDOPgSql\Driver |
+--------------------------------------+

jwage · 2019-06-04T23:02:41Z

@morozov fixed. Should we add that finalizer check to ci so it fails if a new class is introduced that should be final but isn't?

morozov · 2019-06-04T23:50:22Z

This is the first time I'm using it but it looks more suitable for development, not for CI. Specifically, it also fails the build if it finds the classes mistakenly marked as final which is not what we need. I don't think we're introducing new classes that often that we need it enforced on CI.

jwage · 2019-06-05T02:00:36Z

I've got a few more tests to fix tomorrow.

lib/Doctrine/DBAL/Driver/PDOSqlite/Driver.php

…implementation detail Testing the implementations of this method requires partial mocking of the implementing class which makes it impossible to make them `final` (doctrine#3590). Additionally, this API breaks the encapsulation of the driver layer: instead of exposing the fact of whether The rationale behind introducing this method (doctrine#487) is really questionable: > This is also required for drivers that cannot return the database server version without an additional query (performance reasons). 1. There's no evidence that an underlying driver that exposes the server version via its API doesn't make a request of any kind to the server. 2. For an application that works with any realistic database, a query like `SELECT VERSION()` wouldn't be a performance bottleneck. 3. Even if it was, it's always possible to specify the platform version upfront. In addition to the above, the only driver that doesn't support the platform version detection via the underlying driver API is `sqsql` which is barely supported.

…implementation detail Testing the implementations of this method requires partial mocking of the implementing class which makes it impossible to make them `final` (doctrine#3590). Additionally, this API breaks the encapsulation of the driver layer: instead of exposing the fact of whether the connection will perform a query to detect the server version, the driver should just instantiate a platform corresponding to a connection. The rationale behind introducing this method (doctrine#487) is really questionable: > This is also required for drivers that cannot return the database server version without an additional query (performance reasons). 1. There's no evidence that an underlying driver that exposes the server version via its API doesn't make a request of any kind to the server. 2. For an application that works with any realistic database, a query like `SELECT VERSION()` wouldn't be a performance bottleneck. 3. Even if it was, it's always possible to specify the platform version upfront. In addition to the above, the only driver that doesn't support the platform version detection via the underlying driver API is `sqsql` which is barely supported.

…implementation detail Testing the implementations of this method requires partial mocking of the implementing class which makes it impossible to make them `final` (doctrine#3590). Additionally, this API breaks the encapsulation of the driver layer: instead of exposing the fact of whether the connection will perform a query to detect the server version, the driver should just instantiate a platform corresponding to a connection. The rationale behind introducing this method (doctrine#487) is really questionable: > This is also required for drivers that cannot return the database server version without an additional query (performance reasons). 1. There's no evidence that an underlying driver that exposes the server version via its API doesn't make a request of any kind to the server. 2. For an application that works with any realistic database, a query like `SELECT VERSION()` wouldn't be a performance bottleneck. 3. Even if it was, it's always possible to specify the platform version upfront. Otherwise, the current logic of falling back to a default platform may cause undefined behavior of the application (we don't test the compatibility of the lowest level of the DBAL platform with all supported server versions). Remember, “If it doesn’t work, it doesn’t matter how fast it doesn’t work.” In addition to the above, the only driver that doesn't support the platform version detection via the underlying driver API is `sqsql` which is barely supported.

morozov · 2020-01-03T06:02:14Z

In addition to the already finalized classes, the following driver-level classes are meant to be final but currently cannot be declared as such:

The following classes have not been automatically finalized:

MysqliConnection because of its getWrappedResourceHandle() which is not used anywhere and is not part of any API convention (see Expose the underlying statement in DBAL statements #3534 for a similar feature request).
OCI8Connection because of its getExecuteMode() which is used by OCI8Statement::execute(). It indicates a violation of the ISP principle (the statement should be given an object that represents the execution mode, not the entire connection).
OCI8Statement because of its static convertPositionalToNamedPlaceholders() which is public only because it's tested directly which indicates a violation of the SRP. It could be extracted to a utility class:

dbal/lib/Doctrine/DBAL/Driver/OCI8/OCI8Statement.php

Line 130 in f37a88e

* @todo extract into utility class in Doctrine\DBAL\Util namespace
PDOConnection because it has child classes and the createStatement() method.
PDOSqlsrv\Connection because of its createStatement().

The existing relationship between the connection and its statement violate the ISP principle. Instead of having access only to the execution mode of the connection, statements have access to the entire connection API. Having a method which is not defined in the driver connection interface makes it impossible to mark the method `final` (doctrine#3590).

…implementation detail Testing the implementations of this method requires partial mocking of the implementing class which makes it impossible to make them `final` (doctrine#3590). Additionally, this API breaks the encapsulation of the driver layer: instead of exposing the fact of whether the connection will perform a query to detect the server version, the driver should just instantiate a platform corresponding to a connection. The rationale behind introducing this method (doctrine#487) is really questionable: > This is also required for drivers that cannot return the database server version without an additional query (performance reasons). 1. There's no evidence that an underlying driver that exposes the server version via its API doesn't make a request of any kind to the server. 2. For an application that works with any realistic database, a query like `SELECT VERSION()` wouldn't be a performance bottleneck. 3. Even if it was, it's always possible to specify the platform version upfront. Otherwise, the current logic of falling back to a default platform may cause undefined behavior of the application (we don't test the compatibility of the lowest level of the DBAL platform with all supported server versions). Remember, “If it doesn’t work, it doesn’t matter how fast it doesn’t work.” In addition to the above, the only driver that doesn't support the platform version detection via the underlying driver API is `sqsql` which is barely supported.

The concern has been addressed

morozov · 2020-01-12T01:46:06Z

Thank you @jwage 👍

The existing relationship between the connection and its statement violate the ISP principle. Instead of having access only to the execution mode of the connection, statements have access to the entire connection API. Having a method which is not defined in the driver connection interface makes it impossible to mark the method `final` (doctrine#3590).

…n implementation detail Testing the implementations of this method requires partial mocking of the implementing class which makes it impossible to make them `final` (doctrine#3590). Additionally, this API breaks the encapsulation of the driver layer: instead of exposing the fact of whether the connection will perform a query to detect the server version, the driver should just instantiate a platform corresponding to a connection. The rationale behind introducing this method (doctrine#487) is really questionable: > This is also required for drivers that cannot return the database server version without an additional query (performance reasons). 1. There's no evidence that an underlying driver that exposes the server version via its API doesn't make a request of any kind to the server. 2. For an application that works with any realistic database, a query like `SELECT VERSION()` wouldn't be a performance bottleneck. 3. Even if it was, it's always possible to specify the platform version upfront. Otherwise, the current logic of falling back to a default platform may cause undefined behavior of the application (we don't test the compatibility of the lowest level of the DBAL platform with all supported server versions). Remember, “If it doesn’t work, it doesn’t matter how fast it doesn’t work.”

jwage added Improvement BC Break labels Jun 4, 2019

jwage added this to the 3.0.0 milestone Jun 4, 2019

jwage force-pushed the final-classes branch from 7a7c72a to 3e142e4 Compare June 4, 2019 17:24

morozov reviewed Jun 4, 2019

View reviewed changes

lib/Doctrine/DBAL/Driver/Mysqli/MysqliStatement.php Outdated Show resolved Hide resolved

morozov reviewed Jun 4, 2019

View reviewed changes

tests/Doctrine/Tests/DBAL/Driver/StatementIteratorTest.php Show resolved Hide resolved

SenseException requested changes Jun 4, 2019

View reviewed changes

lib/Doctrine/DBAL/Driver/PDOSqlite/Driver.php Outdated Show resolved Hide resolved

jwage force-pushed the final-classes branch from aeb1a88 to 74e2a3d Compare June 4, 2019 23:02

SenseException previously requested changes Jun 7, 2019

View reviewed changes

lib/Doctrine/DBAL/Driver/PDOSqlite/Driver.php Outdated Show resolved Hide resolved

morozov force-pushed the develop branch from 4255137 to b4c2cf3 Compare June 13, 2019 04:43

morozov force-pushed the develop branch 3 times, most recently from 10890d6 to 3836750 Compare June 27, 2019 06:29

morozov force-pushed the develop branch from 3836750 to 7d0eb29 Compare August 27, 2019 05:08

morozov force-pushed the develop branch from 7d0eb29 to acf1cc0 Compare November 2, 2019 21:45

morozov closed this Nov 3, 2019

morozov reopened this Nov 4, 2019

morozov changed the base branch from develop to master November 4, 2019 14:40

morozov force-pushed the final-classes branch from 74e2a3d to 32cdec3 Compare January 3, 2020 01:49

morozov mentioned this pull request Jan 3, 2020

Removed ServerInfoAwareConnection#requiresQueryForServerVersion() as implementation detail #3807

Merged

morozov mentioned this pull request Jan 3, 2020

Removed the OCI8Connection::getExecuteMode() method #3808

Merged

morozov force-pushed the final-classes branch from 32cdec3 to f47a322 Compare January 8, 2020 19:53

morozov mentioned this pull request Jan 10, 2020

Fix phpstan errors #3801

Merged

jwage added 2 commits January 11, 2020 08:32

Make classes final where applicable.

6e63c7f

Use bound instead of binded.

5bcd356

morozov force-pushed the final-classes branch from f47a322 to 5bcd356 Compare January 11, 2020 16:34

morozov mentioned this pull request Jan 11, 2020

Refactored MySQLiStatement::$columnNames #3817

Merged

morozov approved these changes Jan 12, 2020

View reviewed changes

morozov merged commit cc38688 into doctrine:master Jan 12, 2020

morozov self-assigned this Jan 12, 2020

morozov mentioned this pull request Jan 12, 2020

Finalize remaining Connection and Statement classes #3818

Closed

6 tasks

morozov mentioned this pull request Jun 23, 2020

Deprecate inconsistently and ambiguously named driver-level classes #4100

Merged

morozov mentioned this pull request Jun 27, 2020

Fixes for the recently introduced driver-level deprecations #4117

Merged

github-actions bot locked as resolved and limited conversation to collaborators Aug 5, 2022

morozov removed this from the 4.0.0 milestone Aug 11, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make classes final where applicable. #3590

Make classes final where applicable. #3590

jwage commented Jun 4, 2019

morozov commented Jun 4, 2019

jwage commented Jun 4, 2019

morozov commented Jun 4, 2019

jwage commented Jun 5, 2019

morozov commented Jan 3, 2020 •

edited

Loading

morozov commented Jan 12, 2020

Make classes final where applicable. #3590

Make classes final where applicable. #3590

Conversation

jwage commented Jun 4, 2019

Summary

morozov commented Jun 4, 2019

jwage commented Jun 4, 2019

morozov commented Jun 4, 2019

jwage commented Jun 5, 2019

morozov commented Jan 3, 2020 • edited Loading

morozov commented Jan 12, 2020

morozov commented Jan 3, 2020 •

edited

Loading