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

fix issue with doctrine/dbal 2.11 and connection option #1226

Merged
merged 1 commit into from
Oct 11, 2020

Conversation

dmaicher
Copy link
Contributor

@dmaicher dmaicher commented Oct 7, 2020

@stof
Copy link
Member

stof commented Oct 7, 2020

The proper fix would be to wire the ConnectionProvider instead.

@dmaicher
Copy link
Contributor Author

dmaicher commented Oct 7, 2020

The proper fix would be to wire the ConnectionProvider instead.

This is more a quick fix to make sure it works with dbal 2.11 until we have a proper fix including ConnectionProvider.

If you think we should not release this fix first then I can also close here. I don't mind 😊

@ostrolucky
Copy link
Member

Why are we again fixing doctrinebundle instead of fixing DBAL? DBAL broke this in in minor version, issue is on their side.

@dmaicher
Copy link
Contributor Author

dmaicher commented Oct 7, 2020

@ostrolucky not sure how to fix this on DBAL's side 😕

This fix is similar to your fix here 86d2469

@stof
Copy link
Member

stof commented Oct 7, 2020

@ostrolucky the issue here is that we were adding new options in a child class. This broke when upstreaming the option.
But I don't think DBAL can be considered responsible for that. If you want to allow child classes to add new command options in a BC way, you have to forbid DBAL from ever changing the available options (the same would be true regarding adding methods in a child class, and that's why Symfony excludes such possibility from safe changes in downstream code).

@stof
Copy link
Member

stof commented Oct 7, 2020

And as DBAL 2.11 is released now, we should not do a weird hack to keep the DoctrineBundle feature working. We should use the new upstream feature instead.

@ostrolucky
Copy link
Member

Yeah, after some more investigation I don't think we should hold DBAL accountable for this.

If you want to allow child classes to add new command options in a BC way, you have to forbid DBAL from ever changing the available options

You are talking about what happened previously, in linked commit. What happened now is that DBAL added exception throw when connection is specified. But I don't personally stand behind opinion that adding exception throws is a BC break in general either.

And as DBAL 2.11 is released now, we should not do a weird hack to keep the DoctrineBundle feature working. We should use the new upstream feature instead.

We will do that, but in 2.2. This is non trivial change which will also include deprecating our commands.

@ostrolucky ostrolucky merged commit 934bdcf into doctrine:1.12.x Oct 11, 2020
@dmaicher dmaicher deleted the fix_connection_name branch October 11, 2020 18:14
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.

3 participants