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

#40119 broke how dbal array in config/database.php is handled. #40297

Closed
canvural opened this issue Jan 7, 2022 · 4 comments · Fixed by #40303
Closed

#40119 broke how dbal array in config/database.php is handled. #40297

canvural opened this issue Jan 7, 2022 · 4 comments · Fixed by #40303
Labels

Comments

@canvural
Copy link
Contributor

canvural commented Jan 7, 2022

  • Laravel Version: 8.78.1
  • PHP Version: 8.1.0
  • Database Driver & Version: MySQL 8

Description:

#40119 introduced an improvement for handling custom Doctrine types. But for some people it broke the test suites. (#40119 (comment) and #40119 (comment))

For sequential test runs issue happens when there is only the dbal array in config/database.php And the issue is gone when adding DB::registerDoctrineType call to AppServiceProvider. Which is not a good solution because it causes the application to make DB connection while the app is booting.

For parallel test runs issue happens in both cases.

Steps To Reproduce:

This repo shows the bug.

@skollro
Copy link
Contributor

skollro commented Jan 7, 2022

@canvural Thank you for creating an issue out of our previous discussion!

And the issue is gone when adding DB::registerDoctrineType call to AppServiceProvider.

This only works for sequential test runners. As soon as you run php artisan test --parallel both ways of custom DBAL type registration are broken.

@driesvints
Copy link
Member

@bakerkretzmar @TomHAnderson if we can't find a solution right now it might be best that we revert the PR.

@bakerkretzmar
Copy link
Contributor

@canvural thanks a lot for reproducing it in a repo, I'll dig into this right now. @driesvints agreed, I can do that later today if we can't figure this out.

@TomHAnderson
Copy link
Contributor

TomHAnderson commented Jan 7, 2022

I've been working with @bakerkretzmar all day. I think his PR #40303 addresses 1) problems with running unit tests in series and 2) accounts for unit tests in parallel and makes concessions for them.

I'm not certain parallel testing is 100% because I've been unable to run such tests locally. I don't feel reverting PR #40119 is the right move.

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

Successfully merging a pull request may close this issue.

5 participants