coding-standard
coding-standard copied to clipboard
doctrine
Rework Exception Naming Conventions
Until now the SuperfluousExceptionNaming sniff was enabled in Doctrine Coding Standard, but every of our projects disabled this sniff in their local phpcs.xml rule.
Doctrine does not actually follow this rule, instead we provide the context of the exception in the name as a "prefix" similar to PHPs own RuntimeException and so on.
This is done, because exceptions are used soley in the context of code that reads like:
} catch (DBALException $e) {
}
If we allow classes named "Exception", then we introduce a developer experience problem, because it potentially requires the user to make an alias/renaming decision, increasing their mental load:
use OtherLibrary\Exception as OtherException;
use Doctrine\DBAL\Exception as DBALException;
use Exception;
} catch (OtherException $e) {
} catch (DBALException $e) {
} catch (Exception $e) {
}
Additionally it makes it hard for developers understanding catch clauses when they cannot rely on the fact that "Exception" is the global one.
} catch (Exception $e) { // don't mess with user expectations
I think it can make sense to have interfaces named like the package that have the suffix, while still forbidding for concrete classes. Not 100% sure if that sniff also forbids the suffix on interfaces.
Another reason why we don't want to go away from our convention right now to enforce SuperfluousExceptionNamingSniff is that the resulting BCBreaks in renaming and moving thinks like NotFoundException to Exception\NotFound is a pointless BC break that adds no value and only pain to users.
NotFound is more readable. NotFoundException is the same nonsense as UserClass or $userVariable.
@morozov I agree with you completely, if we were to start from scratch then this would be something we can agree on to do. But we don't start from scratch and changing this now has more downsides than benefits, even if we do it in a major version where BC breaks are allowed, it does not pass the sniff test for a BC break that provides benefits. Other than cleaning up it serves no purpose. As such we should better stick with the convention we have. It has no cost sticking to it.
We added the original sniff because we wanted for new projects to follow that and for us to eventually migrate existing code.
We saw no issue in having local overrides to allow for the migration process back then, and I still don't think this is an issue.
I'd prefer to keep the standard as it was and focus on the migration path to adhere to it.
It's also fine to use inheritance and ignore local occurrences of the violation (//phpcs:ignore sniff) as part of the migration IMHO.
Also, any change to the provided list of sniffs is a BC-break in this project, so the targeted version here should be 9.0.x instead.
The change in https://github.com/doctrine/dbal/pull/4253 is not about the coding standard. It's about removing the meaningless DBAL prefix from the class name. All exceptions under the Doctrine\DBAL namespace are DBAL exceptions but none of them have the DBAL prefix. Being part of the name, it doesn't convey a single bit of information.
[…] this now has more downsides than benefits […]
I think you're overdramatizing things. In my experience, there are two types of DBAL exceptions: the logical ones are not supposed to happen at all (e.g. table already exists), and the runtime ones that are often handled implicitly at the presentation layer (e.g. converted to a 500 response) or retried in case of a connection failure (no longer needed as of https://github.com/doctrine/dbal/pull/4119).
If a project implements a model layer on top of DBAL and converts the DBAL exception into a higher-level one, it's a matter of find/replace in the model layer.
If a project has to deal with exceptions thrown by different packages from one method call, then the naming collision is not the biggest problem there. Even in this case, those exceptions can be handled in a clean way:
try {
// ...
} catch (Pkg1\Exception $e) {
// ...
} catch (Pkg2\Exception $e) {
// ...
}
Solving the name collision problem (that PHP solves by means of namespaces) that may occur in presence of other libraries is definitely not the responsibility of a given library.
I would rather this was contributed to different package. Maybe even as an option SuperfluousExceptionNaming. This is the first sniff we have here.
@ostrolucky i agree, i was just adding the sniff here so that we have a base to work off, I am considering to see where it should go from there.
@morozov your argument misses point, the DBALException is the base exception of the public API of DBAL. All exceptions extend from it. All these Exception have a name that is not Exception, so they can't be mixed up with the global Exception or any other libraries Exception. It does not make sense to prefix DBALTableNotFoundException, because TableNotFoundException is already unique enough.
It is also not really important that DBALException is named what its named and its being used many times. Maybe some 10 years ago we could have named it DBAL\BaseException, but we didn't.
As for renaming DBALException, take open source code, https://grep.app/search?q=DBALException spread around quite a bit. I checked a few private projects, all of them catch DBALException at some low level points a few times. Its a class that really shouldn't be renamed anymore at this point.
@lcobucci My undrestanding of a coding standard is, that is doesn't affect the outside API a lot. Essentially as a user you wouldn't care about the coding standard and if its changing, because it doesn't change the public API. However this rule changes the convention of the Doctrine project in a way that affects outside users quite massively. As such i would say, we should re-consider having it, and instead enforcing the rule that we already follow implicitly
@beberlei I honestly think that everything can be improved but do agree that user impact is important and must be considered when changing stuff.
We accepted this sniff back then because we believed it to be better than the implicit convention the codebase follows at the moment. Going back on this would be unfortunate IMHO.
Also, in case we decide that a class will never be changed because reasons, we can simply ignore that rule for that particular class.
@lcobucci I'd rather have it that the standard grants no room for interpretation, and override rules point at a code smell, instead of the other way around. Ultimtaely the doctrine/coding-standard is for Doctrine projects. So if external users of our standard would need to disable one of our rules and enable one of Slevomat to get to the old behavior, then I am fine with that compared to our projects maintaining large lists of excemptions in their phpcs.xml
Ultimtaely the doctrine/coding-standard is for Doctrine projects.
Nack.
It became much more than that: https://github.com/doctrine/coding-standard/network/dependents
I use it in almost all private projects because it represents a good standard for striving for quality.
@Ocramius Doctrine is in the game of providing a database abstraction and ORM to users, not in quality assurance and static analysis. The documentation even says The Doctrine Coding Standard is a set of rules for PHP_CodeSniffer and applies to all Doctrine projects.
Sure, but the ship has sailed long ago, and I've been endorsing this project around the world (literally) since very early on.
As it currently stands, it provides tremendous value for the ecosystem relying on it, and in my projects, even more value than the DB abstractions.
On Sat, Sep 12, 2020, 12:22 Benjamin Eberlei [email protected] wrote:
@Ocramius https://github.com/Ocramius Doctrine is in the game of providing a database abstraction and ORM to users, not in quality assurance and static analysis. The documentation even says The Doctrine Coding Standard is a set of rules for PHP_CodeSniffer and applies to all Doctrine projects.
— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/doctrine/coding-standard/pull/219#issuecomment-691463807, or unsubscribe https://github.com/notifications/unsubscribe-auth/AABFVEEQ4FN7D7V5MXTSUF3SFNDWHANCNFSM4RBT4ZIA .
You are arguing that its too late to break BC on a coding standard that is 5 years old, with a deterministic 2 lines of XML workaround, and that its better to BC breaks in actual code of Doctrine projects that are 10 years old, with non deterministic code changes?
I fail to see how a coding standard that is unrelated to the primary goal of Doctrine trumps actual code.
Yes, and I've already explained why it is a relevant project.
It's fine though: can be forked and maintained elsewhere.
On Sat, Sep 12, 2020, 12:28 Benjamin Eberlei [email protected] wrote:
You are arguing that its too late to break BC on a coding standard that is 5 years old, with a deterministic 2 lines of XML workaround, and that its better to BC breaks in actual code of Doctrine projects that are 10 years old, with non deterministic code changes?
I fail to see how a coding standard that is unrelated to the primary goal of Doctrine trumps actual code.
— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/doctrine/coding-standard/pull/219#issuecomment-691464748, or unsubscribe https://github.com/notifications/unsubscribe-auth/AABFVEDUXSARSRAE5ZKQZ2LSFNENJANCNFSM4RBT4ZIA .
Doctrine is in the game of providing a database abstraction and ORM to users, not in quality assurance and static analysis.
It became much more than that. Doctrine libraries seem to appear in many vendor-directories as dependencies of projects or as dependencies of project-dependencies. Hidden, but still a part of value for all of these projects out there. Everytime someone talks about a "Doctrine problem", I ask which Doctrine project exactly. I know many of them are referring to ORM, because as this description suggests, it is the most famous one of Doctrine, but there are still so many valuable libraries which is why Doctrine isn't just an organisation that provides entities and database abstractions. A prominent example would be the Instantiator used by PHPUnit. The Doctrine coding-standard was mentioned by some speakers at user groups who speak fond of it and like the direction it went over time. People already appreciate the value it provides today.
The documentation even says
The Doctrine Coding Standard is a set of rules for PHP_CodeSniffer and applies to all Doctrine projects.
This can be applied to the existing Doctrine projects if approached properly. E.g. in DBAL we made all driver-level exception classes internal essentially turning them in factory methods. At this point, it's safe to rename them properly. All new classes are also named with this rule in mind.
Just because changing the exception name is a breaking change, it doesn't mean that this rule should be excluded from the standard.
Throwing my 2 cents in: I agree with what @Ocramius is saying. The coding standard was meant for us but people liked it (a lot) and we ended up with a popular package. I see no harm having a rule in the standard but disabled locally for our existing repositories.
