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

[6.x] Fix handleBeginTransactionException method calling pdo property instead of getPdo() method #31233

Merged

Conversation

@kanidjar
Copy link
Contributor

kanidjar commented Jan 24, 2020

Fix handleBeginTransactionException method calling pdo property instead of getPdo() method (#31230)

Description:

When connection to database is lost and transaction is retried, an exception is thrown.

[2020-01-24 15:00:16] production.ERROR: Symfony\Component\Debug\Exception\FatalThrowableError: Call to undefined method Closure::beginTransaction() in /var/www/html/vendor/illuminate/database/Concerns/ManagesTransactions.php:157
Stack trace:
#0 /var/www/html/vendor/illuminate/database/Concerns/ManagesTransactions.php(125): Illuminate\Database\Connection->handleBeginTransactionException(Object(PDOException))
#1 /var/www/html/vendor/illuminate/database/Concerns/ManagesTransactions.php(105): Illuminate\Database\Connection->createTransaction()
#2 /var/www/html/vendor/illuminate/database/Concerns/ManagesTransactions.php(23): Illuminate\Database\Connection->beginTransaction()
#3 /var/www/html/vendor/illuminate/database/DatabaseManager.php(349): Illuminate\Database\Connection->transaction(Object(Closure))
#4 /var/www/html/vendor/illuminate/support/Facades/Facade.php(261): Illuminate\Database\DatabaseManager->__call('transaction', Array)
#5 /var/www/html/app/Services/PriceService.php(55): Illuminate\Support\Facades\Facade::__callStatic('transaction', Array)

The exception is thrown because of line 157 calling $this->pdo instead of $this->getPdo()

Same bug was detected in Laravel 5.3 but was not linked to the same file.

#15927

Steps To Reproduce:

Lose connection to database during transaction creation (hard to reproduce but easy to fix)

…ad of getPdo() method (#31230)
@kanidjar kanidjar changed the title Fix handleBeginTransactionException method calling pdo property instead of getPdo() method (#31230) [6.x] Fix handleBeginTransactionException method calling pdo property instead of getPdo() method (#31230) Jan 24, 2020
@kanidjar kanidjar changed the title [6.x] Fix handleBeginTransactionException method calling pdo property instead of getPdo() method (#31230) [6.x] Fix handleBeginTransactionException method calling pdo property instead of getPdo() method Jan 24, 2020
@GrahamCampbell

This comment has been minimized.

Copy link
Member

GrahamCampbell commented Jan 25, 2020

Please revert the other changes to the file.

@kanidjar

This comment has been minimized.

Copy link
Contributor Author

kanidjar commented Jan 25, 2020

@L3o-pold

This comment has been minimized.

Copy link

L3o-pold commented Jan 27, 2020

@GrahamCampbell can we merge this into the next release?

@GrahamCampbell

This comment has been minimized.

Copy link
Member

GrahamCampbell commented Jan 27, 2020

If this is merged before early afternoon (UK time) tomorrow, then it will likely be included in tomorrow's release.

@GrahamCampbell

This comment has been minimized.

Copy link
Member

GrahamCampbell commented Jan 27, 2020

It is much more likely to be merged if you add tests that cover this code, that fail before this PR, and pass afterwards.

Copy link
Member

driesvints left a comment

Other calls to the pdo property are also covered with getPdo so this seems okay 👍

@taylorotwell taylorotwell merged commit b5f3d22 into laravel:6.x Jan 27, 2020
2 checks passed
2 checks passed
continuous-integration/styleci/pr The analysis has passed
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@kanidjar kanidjar deleted the kanidjar:fix/handle-begin-transaction-exception branch Jan 27, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

5 participants
You can’t perform that action at this time.