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

[5.7] Detect connection errors also #22905

Closed
wants to merge 19 commits into from

Conversation

ollielowson
Copy link
Contributor

I've had an issue where a project has dropped a database connection and thrown a FatalThrowableError. Looking at the createTransaction() function in https://github.com/laravel/framework/blob/5.5/src/Illuminate/Database/Concerns/ManagesTransactions.php and src/Illuminate/Database/DetectsLostConnections.php, this only detects instances of the Exception class.

I was able to resolve this issue in my project by changing the functions concerned to accept an instance of Throwable instead of Exception, so this is a Pull Request implementing those changes.

It's my first PR on a big project though, so I would appreciate any feedback :)

@ollielowson ollielowson changed the base branch from 5.5 to master January 24, 2018 14:19
@ollielowson
Copy link
Contributor Author

Following a discussion on larachat.slack.com (with @36864), I am planning to implement a test for this, but I will need to do some careful thinking about how to do it. Essentially, I will need to null the PDO object at the point of being about to beginTransaction() on it...

@tillkruss tillkruss changed the title Detect connection errors also [5.6] Detect connection errors also Jan 25, 2018
@tillkruss tillkruss changed the title [5.6] Detect connection errors also [5.7] Detect connection errors also Jan 25, 2018
@GrahamCampbell
Copy link
Member

5.6 is probably a better target for this?

@taylorotwell taylorotwell changed the base branch from master to 5.6 January 26, 2018 13:37
@taylorotwell
Copy link
Member

Please re-send to 5.6 branch.

@ollielowson
Copy link
Contributor Author

Sorry for the delay - now re-sent to 5.6 in #22948

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

Successfully merging this pull request may close these issues.

None yet

3 participants