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

Enhance exception message #677

Merged
merged 1 commit into from
Jul 25, 2017
Merged

Conversation

sebastianfeldmann
Copy link
Contributor

The error occures if doctrine needs to establish a connection to
determine the platform version to register the types.
The idea is to provide some more information so the user can
understand what and why this is happening.

This is discussed at:
#673

@mikeSimonson
Copy link
Contributor

@sebastianfeldmann Nice
Thanks

@mikeSimonson
Copy link
Contributor

Can you add a test for this that trigger the exception ?

@sebastianfeldmann
Copy link
Contributor Author

I can definitely try ;)

@sebastianfeldmann
Copy link
Contributor Author

I added a unit test, found and fixed an error, thats good, but now I face a different problem.

The exception we are trying to catch is of type DriverException.
The minimum composer requirement for Doctrine DBAL is 2.3. The DriverException which I'm trying to catch and enhance was added in Doctrine DBAL 2.4.

List of options

  1. Bump up the version requirement to am minimum of 2.4, I don't know if that's a good Idee. I think changes like that require a major version update :/
  2. Catch a more generic exception type, but then we would change exceptions we definitely don't want to change, so not a good idea either.
  3. Search for some specific exception messages and only change those, but that's definitely not a good idea.

So I'm kind of stuck. Any suggestions?

@Ocramius
Copy link
Member

Ocramius commented Jul 2, 2017

Bump up the version requirement to am minimum of 2.4, I don't know if that's a good Idee. I think changes like that require a major version update :/

Feel free to do that - bumping dependencies is NOT a BC break

Catch a more generic exception type, but then we would change exceptions we definitely don't want to change, so not a good idea either.

Would avoid that - too easy to cause other bugs

Search for some specific exception messages and only change those, but that's definitely not a good idea.

This makes the BC boundary too complex to tackle.

I would suggest bumping to "doctrine/dbal": "^2.5.12"

The error we want to improve occures if doctrine needs to establish
a connection to determine the platform version to register the types.
The idea is to provide some more information so the user can
understand what and why this is happening.

You can follow up on the discussions here:
doctrine#673
doctrine#677

Attention:
The composer doctrine/dbal dependency had to be updated to "^2.5.12".
@sebastianfeldmann
Copy link
Contributor Author

I followed your advice and bumped the dependency to "doctrine/dbal": "^2.5.12".

To "mock" the DriverManager::getConnection call I had to create a "FakeDriver" class.
I put the class at the bottom of the ConnectionFactoryTest file. Hope that's ok.

Additionally I had to change the exception type.
The exception we catch is a DriverException. To create a DriverException the previous exception has to implement the DriverExceptionInterface. Sadly the DriverException we catch doesn't implement the DriverExceptionInterface itself. So it is not possible to put a DriverException into a DriverException.
So I endet up throwing a Doctrine\DBALException to pass the DriverException as the previous exception.

Hope that wasn't to confusing ;)

Copy link
Member

@Ocramius Ocramius left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@kimhemsoe can you check this?

@Ocramius Ocramius requested a review from kimhemsoe July 12, 2017 07:47
@mikeSimonson mikeSimonson modified the milestones: 1.8.0, 1.7.0 Jul 25, 2017
@mikeSimonson mikeSimonson self-requested a review July 25, 2017 10:16
@kimhemsoe kimhemsoe merged commit 4f603e1 into doctrine:master Jul 25, 2017
@kimhemsoe
Copy link
Member

@sebastianfeldmann Thanks! very nice

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants