-
Notifications
You must be signed in to change notification settings - Fork 11k
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] Error out when detecting incompatible DBAL version #38543
Conversation
3599fe0
to
8c64ed5
Compare
👎 from me here. People can install doctrine 3 and not use that part of laravel, so the conflict is artifical. Also, composer will just load an older version of Laravel 6 to get around this. It would be bad for people to miss security update because this conflict stopped them from being installed. |
Hey @greg0ire, as @GrahamCampbell indicates, people could still be using Doctrine v3 in situations other than just the column changing functionality of the migrations. I think we should thread carefully here and not force people to only use Doctrine v2. |
@GrahamCampbell good point, I hadn't consider the case of people deliberately using DBAL 3, but not through Laravel. The right thing to do would probably to error out in |
@greg0ire a more expressive exception when Doctrine v3 is used is definitely an option yes. Good call. |
8c64ed5
to
5425d6d
Compare
I pushed a new commit doing just that, please review and then I will do the same for other connection classes 🙂 |
5425d6d
to
196f14f
Compare
@@ -85,6 +86,12 @@ protected function getDefaultPostProcessor() | |||
*/ | |||
protected function getDoctrineDriver() | |||
{ | |||
if (! class_exists(DoctrineDriver::class)) { | |||
throw new LogicException( | |||
'Laravel v6 is only compatible with doctrine/dbal 2, lock it to that version with composer require doctrine/dbal "^2.0"' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe In order to use this feature you must require the package "doctrine/dba:^2.6".
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure 👍
@greg0ire that definitely looks good to me. Just want to remind you that Taylor needs to approve this and he only reviews non-draft PR's. So feel free to add it to the other connections and mark this as ready for review. Thanks for sending this in 🙂 |
Yes, I marked as draft to avoid a merge before I had a chance to do the same for other connections 😛 |
doctrine/dbal is an optional dependency of laravel/framework, and v3 of the DBAL has been released recently, but obviously, Laravel 6 was never meant to be compatible with that version. Users still using Laravel 6 and updating their dependencies get confused and file invalid reports on the DBAL, see for instance doctrine/dbal#4439 or doctrine/dbal#4757 Fixes laravel#24271
196f14f
to
55a5da5
Compare
doctrine/dbal is an optional dependency of laravel/framework, and v3 of the DBAL has been released recently, but obviously, Laravel 6 was never meant to be compatible with that version. Users still using Laravel 6 and updating their dependencies get confused and file invalid reports on the DBAL, see for instance doctrine/dbal#4439 or doctrine/dbal#4757 Fixes laravel#24271
doctrine/dbal
is an optional dependency oflaravel/framework
, and v3 ofthe DBAL has been released recently, but obviously, Laravel 6 was never
meant to be compatible with that version.
Users still using Laravel 6 and updating their dependencies get confused
and file invalid reports on the DBAL, see for instance
doctrine/dbal#4439
or
doctrine/dbal#4757
Fixes #24271