Revert Exception Class renames

[beberlei]

Q	A
Type	bugfix
BC Break	yes
Related issues	#4253

Summary

This reverts the rename of DBALException. This is based on discussions in Slack and doctrine/coding-standard#219 that keeps the Doctrine convention of having the component level exception prefixed by the component name, not using Exceptions without a prefix to avoid confusion with the global Exception or other namespaces using Exception.

Foremost, the DBALException rename is reverted, because its use is widespread in open source and private projects and Exception renames are an easy source for nasty bugs.

[morozov]

As I said, I'm strongly against this change. Especially the insane name at the driver level that 99% of the users don't deal with.

[morozov]

I am not going to approve this change. The DBAL prefix doesn't add any value because the exception is already part of the DBAL package. It might make sense to have a DBALException in a package that uses DBAL to indicate where it comes from but it's not the case here.

It's one of the smallest deprecations among 34 total deprecations that 2.11.x introduces. As I said earlier, DBAL exceptions are either logical exceptions that are not supposed to be handled or the runtime ones that are usually handled at the presentation layer. So the impact of this change on real projects should be negligible.

I do not share the opinion that the DBAL prefix of the class name should solve the problem of the non-fully-qualified Exception name conflicting with other Exception classes. It's up to the end-user to factor their code in the way that it doesn't deal with multiple generic exceptions in the same class or use partially-qualified class names or aliases.

[…] its use is widespread in open source

I wouldn't call the 704 occurrences in ~3700 projects widespread. It's ~1 occurrence per 5 projects which proves the point.

[beberlei]

@morozov The only argument that counts is that its a convention across all Doctrine projects that a base exception exists that has the name of the component. We haven't changed this convention even though the coding standard was defined in defiance of it.

In addition to that argument, the deprecation seems the smallest, but is actually very nasty, because a catch (DBALException $e) does not trigger the autoloader, as such no deprecation can be triggered. only when the excepiton is actually triggered can we do that. Since that means the code is not running in the happy path, its much more likely that you oversee this while migrating from DBAL 2 to 3. (Edit:) If you forget it then you don't get a fatal error as method does not exist anymore, instead the exception bubbles up further. Especially with DBALException might be used for transaction rollback this could be really bad.

Most of the other deprecations are either in the factory related code, or directly in execution and fetching. Honestly I don't think we should deprecate those either, we could keep them and just not document them, but it will be much more obvious to users to see once we trigger the deprecations.

[morozov]

its much more likely that you oversee this while migrating from DBAL 2 to 3.

How can one oversee that?

$ git grep DBALException

Especially with DBALException might be used for transaction rollback this could be really bad.

Any exception can happen in a transaction:

dbal/src/Connection.php

Lines 1140 to 1144 in 789d900

	} catch (Throwable $e) {
	$this->rollBack();
	throw $e;
	}

Catching only a specific type looks like a bad idea in the first place.

[greg0ire]

I see that this is a pain point so here is my opinion in case it helps: the DBAL has only ever had one major version since its inception if I believe packagist, so I think people will know that the upgrade is not to be taken lightly, and read the upgrade guide. Changing the name of the exception is OK in my opinion, but I do agree that this could be dangerous? Maybe we could focus on making sure they don't miss that one part of the migration since it's more likely to slip through, by simply moving it up on top the list it's already top of the list, and adding helpful commands to be run to detect the need to migrate such as the git grep above?

[SenseException]

What do you think about tools like rector/rector to ease a migration of the exception changes? Adding a new tool or at least the config file of the tool isn't a trivial decision, but maybe a help for developers.

[greg0ire]

It looks like there is already something BTW: https://github.com/rectorphp/rector/blob/master/config/set/doctrine-dbal-30.php
That was initiated in rectorphp/rector#2514, and might be completed.

[beberlei]

@morozov as per our discussion we decided to roll this back for now. How do we proceed? Can you approve the PR?

[morozov]

as per our discussion we decided to roll this back for now.

I don't remember making this decision. I don't approve of this change.