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

Class naming blunder #4436

Closed
Bilge opened this issue Nov 17, 2020 · 4 comments
Closed

Class naming blunder #4436

Bilge opened this issue Nov 17, 2020 · 4 comments

Comments

@Bilge
Copy link
Contributor

Bilge commented Nov 17, 2020

Most of the 3.0 changes look good, except for this:

The Doctrine\DBAL\DBALException and Doctrine\DBAL\Driver\DriverException have been renamed to Doctrine\DBAL\Exception and Doctrine\DBAL\Driver\Exception respectively.

This is absolutely the wrong way to name classes: all first-party class names should have strong names, which means their name should be unique within the first-party code ecosystem. To be clear, when I say class name, in this instance I refer to the leaf node of the fully qualified class name (FQCN); the portion following the final backslash (\). This class name must be unique when all the other namespace components fall away.

This is particularly important to avoid naming conflicts. If all exceptions are simply named Exception, importing more than one in any given file results in a name conflict and must then be resolved with aliasing to become usable. Aliasing first-party classes is an anti-pattern indicating naming failure. You should not be placing the burden on the consumer to correctly name your classes in every file where this conflict occurs. It is therefore common, and correct, to include 2-3 portions of the preceding namespace nodes in the class name (the leaf node) to disambiguate classes. e.g. MyCompany\MyProduct\Http\Client should be MyCompany\MyProduct\Http\HttpClient, otherwise we cannot determine whether code occurrences of Client refer to an HTTP client or an FTP client just by its name, without referring back to the list of imports at the top of the file.

Ergo, DBALException (or DbalException), and DriverException are correct. Exception and Exception are incorrect. Do not use namespaces as an excuse to duplicate class names; if all namespaces fall away and you're left looking at just a bunch of class names lying on the ground, it should still be easy to differentiate and reorganize them just by looking at the class (leaf) names alone, without opening up the code file.


Moreover, and this is a side-issue to this particular point, but this is a breaking change for no good reason. This is true even before considering this is a breaking change making things worse rather than better.


Aside, this particular library exhibits this misnaming trait frequently; for example, it has many classes just called Driver, which should actually be MySqlDriver, PgSqlDriver, etc. Of course, this has been a long-standing problem for literally a decade, with roots in the original anti-patterns of the now-deprecated PSR-0, so I would not expect this to change, but it is a missed opportunity to improve naming in this particular major version release. Instead, we've regressed.

@greg0ire
Copy link
Member

@morozov
Copy link
Member

morozov commented Jul 25, 2021

Closing as non-actionable.

@morozov morozov closed this as completed Jul 25, 2021
@Bilge
Copy link
Contributor Author

Bilge commented Jul 25, 2021

The action is to rename. It's that simple.

@github-actions
Copy link

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.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jul 26, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants