[3.0] Reorganize exceptions (part 1 - specialized exceptions) #3131

Majkl578 · 2018-05-04T23:39:32Z

Extracting all exception factory methods to specific sub-classes, keeping the type hiearchy intact. These methods are now removed (thus BC break).
No test changes except mocks or specific factory tests.

In the next PR(s) these (now empty) exception classes will be converted to interfaces and moved to their appropriate namespaces.

First part to complete #3124.
UPGRADE update will come with the 2nd part.

Majkl578 · 2018-05-12T00:36:20Z

@morozov Please take a look at this, currently one extracted/removed method per commit. ~~Once reviewed, I'll squash them.~~ Done already.

morozov

Structure-wise, the changes look fine but therу are cases (I just commented on a few) where exceptions could be eliminated by introducing proper APIs.

Not sure how we want to proceed with that but I feel that without doing so we'll just introduce a bunch of useless classes.

Let me know what you think.

morozov · 2018-05-12T01:40:55Z

lib/Doctrine/DBAL/Connection.php

@@ -211,7 +218,7 @@ public function __construct(array $params, Driver $driver, Configuration $config

        if (isset($params["platform"])) {
            if ( ! $params['platform'] instanceof Platforms\AbstractPlatform) {
-                throw DBALException::invalidPlatformType($params['platform']);
+                throw InvalidPlatformType::new($params['platform']);


Would it make sense to change the constructor signature and get rid of the exception?

moved to #3163

morozov · 2018-05-12T01:43:25Z

lib/Doctrine/DBAL/Connection.php

@@ -656,7 +663,7 @@ private function gatherConditions(array $identifiers)
    public function delete($tableExpression, array $identifier, array $types = [])
    {
        if (empty($identifier)) {
-            throw InvalidArgumentException::fromEmptyCriteria();
+            throw EmptyCriteriaNotAllowed::new();


Personally, I'd better not have this check at all. DELETE FROM table is a valid SQL expression. Sometimes you just need to remove all records from a cache/projection table. Why should we not allow it? Probably a topic for another discussion.

moved to #3164

morozov · 2018-05-12T01:48:36Z

lib/Doctrine/DBAL/Connection.php

@@ -937,7 +944,7 @@ public function executeCacheQuery($query, $params, $types, QueryCacheProfile $qc
    {
        $resultCache = $qcp->getResultCacheDriver() ?: $this->_config->getResultCacheImpl();
        if ( ! $resultCache) {


It would be simpler if enforced by the interfaces of $qcp and the config. E.g they both are required to have an instance of the cache driver which defaults to a null-implementation which throws exceptions from any method.

moved to #3165

Majkl578 · 2018-05-12T02:13:32Z

there are cases (I just commented on a few) where exceptions could be eliminated by introducing proper APIs.

Agreed.

Not sure how we want to proceed with that

At this moment I think it's easier to keep it as is and refactor later, either as part of introducing strict types or as separate improvements. Also when an exception becomes useless during that process, it can simply be removed, it's not going to a stable release anytime soon. (And we may eventually mark them as @internal before 3.0 anyway.)
Do you want me to open issues to track those possible improvements?

morozov · 2018-05-12T18:09:20Z

A few issues with the current structure (not necessarily affected by this patch):

All transaction-related exceptions extend ConnectionException which probably could renamed to TransactionException.
NonUniqueFieldNameException directly extending ServerException is weird.
TableExistsException extending ServerException is weird too.
ServerException extending DriverException
More like these.

@Majkl578 please file one or two tiсkets for 3.0 abour cleaning up and restructuring exceptions. Besides that, it looks good!

Majkl578 · 2018-05-12T18:54:07Z

@morozov At this moment I only extracted the methods to specific classes.
The plan for next PR is to make them interfaces and move them around to their appropriate namespaces, also split some of them or slightly change their hiearchy as we discussed earlier. Looking forward to discussing this then. 👍

morozov · 2018-05-13T03:03:08Z

@Majkl578 what do you say about the Scrutinizer issues like the one in CommitFailedRollbackOnly? There are indeed two classes with the same name within the same hierarchy:

↪ find -name ConnectionException.php
./lib/Doctrine/DBAL/Exception/ConnectionException.php
./lib/Doctrine/DBAL/ConnectionException.php

Also, please squash.

…m (DBAL)

Majkl578 added WIP Error Handling BC Break labels May 4, 2018

Majkl578 added this to the 3.0.0 milestone May 4, 2018

Majkl578 self-assigned this May 4, 2018

jwage mentioned this pull request May 5, 2018

Split up monster MigrationException in to smaller exceptions doctrine/migrations#621

Closed

Majkl578 force-pushed the exceptions branch from 7fc4df2 to ac8412f Compare May 5, 2018 20:44

Majkl578 changed the title ~~[WIP 3.0] Reorganize exceptions~~ [WIP 3.0] Reorganize exceptions (part 1 - specialized exceptions) May 5, 2018

Majkl578 changed the title ~~[WIP 3.0] Reorganize exceptions (part 1 - specialized exceptions)~~ [3.0] Reorganize exceptions (part 1 - specialized exceptions) May 12, 2018

Majkl578 force-pushed the exceptions branch from c936977 to 6e0221b Compare May 12, 2018 00:28

Majkl578 removed the WIP label May 12, 2018

Majkl578 requested a review from morozov May 12, 2018 00:35

Majkl578 force-pushed the exceptions branch 3 times, most recently from 6c10ed8 to 7b5f2f8 Compare May 12, 2018 01:49

morozov reviewed May 12, 2018

View reviewed changes

morozov approved these changes May 12, 2018

View reviewed changes

guilhermeblanco and others added 8 commits May 13, 2018 12:16

Decouple unique index from unique constraint

8b974e7

Added flag support to unique constraint

29a660f

Fixed tests

b9f2476

Removed identifier generator type from schema table definition

c008d12

Fixing rebase with master

13eefe3

Fixed undefined result of rebase

7159855

Moved getSequencePrefix() from ClassMetadata (ORM) to AbstractPlatfor…

3ab6f62

…m (DBAL)

Remove copyright headers

834c2cd

morozov force-pushed the develop branch 2 times, most recently from 72cddfb to d8f1786 Compare June 6, 2018 21:46

morozov force-pushed the develop branch 3 times, most recently from 67bd64c to 7e5fb71 Compare July 2, 2018 00:52

Majkl578 mentioned this pull request Jul 9, 2018

Add throws phpdoc #3208

Closed

morozov force-pushed the develop branch from 7e5fb71 to a598800 Compare July 25, 2018 04:49

morozov force-pushed the develop branch from e646ba5 to 9f9d347 Compare August 7, 2018 16:26

morozov force-pushed the develop branch from 40c4f38 to ed7ad19 Compare September 29, 2018 06:41

morozov force-pushed the develop branch 2 times, most recently from c4478a0 to e5a586e Compare October 7, 2018 22:01

morozov force-pushed the develop branch 2 times, most recently from 0a11c71 to fa42c10 Compare November 14, 2018 19:23

morozov mentioned this pull request Nov 16, 2018

DBAL 3.0: suggestions to improve the Driver\Connection interface #3335

Closed

morozov force-pushed the develop branch from fa42c10 to 0ef7d47 Compare November 25, 2018 02:58

Majkl578 mentioned this pull request Dec 2, 2018

[3.0] Rework driver exceptions #3367

Closed

morozov force-pushed the develop branch from 7764294 to 7c38e80 Compare December 6, 2018 07:42

morozov force-pushed the develop branch from 7c38e80 to 152bc9e Compare January 20, 2019 04:12

morozov force-pushed the develop branch from 152bc9e to e39bc08 Compare February 7, 2019 06:30

morozov force-pushed the develop branch 2 times, most recently from 3850154 to 4fbe91a Compare February 22, 2019 00:01

morozov force-pushed the develop branch 3 times, most recently from a640b82 to e7b6c16 Compare March 18, 2019 06:13

morozov force-pushed the develop branch from 09565d5 to 6daf933 Compare April 16, 2019 04:11

Majkl578 closed this Apr 16, 2019

Majkl578 deleted the exceptions branch April 16, 2019 19:48

jwage mentioned this pull request Apr 19, 2019

Extract exception factory methods into specific exceptions #3525

Merged

morozov removed this from the 3.0.0 milestone May 23, 2019

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[3.0] Reorganize exceptions (part 1 - specialized exceptions) #3131

[3.0] Reorganize exceptions (part 1 - specialized exceptions) #3131

Majkl578 commented May 4, 2018 •

edited

Loading

Majkl578 commented May 12, 2018 •

edited

Loading

morozov left a comment

morozov May 12, 2018

Majkl578 May 26, 2018

morozov May 12, 2018

Majkl578 May 26, 2018 •

edited

Loading

morozov May 12, 2018

Majkl578 May 26, 2018

Majkl578 commented May 12, 2018

morozov commented May 12, 2018

Majkl578 commented May 12, 2018

morozov commented May 13, 2018

[3.0] Reorganize exceptions (part 1 - specialized exceptions) #3131

[3.0] Reorganize exceptions (part 1 - specialized exceptions) #3131

Conversation

Majkl578 commented May 4, 2018 • edited Loading

Majkl578 commented May 12, 2018 • edited Loading

morozov left a comment

Choose a reason for hiding this comment

morozov May 12, 2018

Choose a reason for hiding this comment

Majkl578 May 26, 2018

Choose a reason for hiding this comment

morozov May 12, 2018

Choose a reason for hiding this comment

Majkl578 May 26, 2018 • edited Loading

Choose a reason for hiding this comment

morozov May 12, 2018

Choose a reason for hiding this comment

Majkl578 May 26, 2018

Choose a reason for hiding this comment

Majkl578 commented May 12, 2018

morozov commented May 12, 2018

Majkl578 commented May 12, 2018

morozov commented May 13, 2018

Majkl578 commented May 4, 2018 •

edited

Loading

Majkl578 commented May 12, 2018 •

edited

Loading

Majkl578 May 26, 2018 •

edited

Loading