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

Allow serverVersion to be explicitly unspecified (null) #814

Closed
wants to merge 2 commits into from
Closed

Allow serverVersion to be explicitly unspecified (null) #814

wants to merge 2 commits into from

Conversation

BreiteSeite
Copy link

Hi,

this PR allows serverVersionto be nullable.

We use doctrine/dbal in Integration-Tests and to prevent the unit-tests from connecting to a database, we specify serverVersion with something made-up (like 5.6).

However, i'm not comfortable set serverVersion to a made-up server-version to prevent connections from being made, when there even isn't a server to connect to. With this PR merged, we could specify serverVersion to be null instead of something like 5.6 and still prevent connections from being made. null is a valid return value for getDatabasePlatformVersion().

@doctrinebot
Copy link

Hello,

thank you for creating this pull request. I have automatically opened an issue
on our Jira Bug Tracker for you. See the issue link:

http://www.doctrine-project.org/jira/browse/DBAL-1167

We use Jira to track the state of pull requests and the versions they got
included in.

@BreiteSeite
Copy link
Author

Can somebody please explain to me, why the PHP 5.3 travis builds for commit e74caf9 pass, even though i used short-array-syntax in my unit-test?

Thank you.

@deeky666
Copy link
Member

@BreiteSeite can you point me to some example code and or use case to evaluate whether this change makes sense? Thanks.

@BreiteSeite
Copy link
Author

@deeky666 yes.. the point is config (im)mutability. So we read our config via environment variables, which leads to a config like this:

<?php

return [
    // ...
    'serverVersion' => false !== getenv('SQL_SERVER_VERSION') ? getenv('SQL_SERVER_VERSION) : null
];

We now have a config per environment. For example, for phpunit, we do not set SQL_SERVER_VERSION as our test suite obv. should/will not make queries to the database (we have also an integration suite which loads the whole application and mocks the persistence layer/services).

Till now, if we initialize the application and serverVersion is null, doctrine will make queries to the database to figure out, which mysql version is running. This crashes the app/test as we do not have a connection configured.

With this change, null would be allowed and would prevent doctrine from trying to fetch the sql server version from the server.

An easy workaround of course is to specify as fake version in the phpunit environment (we make this at the moment) but it doesn't feel right to load SQL_SERVER_VERSION=5.6 even if we do not connect to the database at all or even have one available.

Hope this makes things a bit more clear, if you still have any questions or feedback i would be happy to hear from you.

@BreiteSeite
Copy link
Author

Rebased onto master.

@deeky666
Copy link
Member

@BreiteSeite so if I get you right you are facing issues when running unit tests and mocking DBAL? I agree that in such a scenario, DBAL should not try to connect implicity, but it sounds to me as if you were not mocking out DBAL correctly then. I'd still like to see some kind of snippet or pseudo-code that triggers the issue so that I can see if it might be misuage in the tests.

@Ocramius
Copy link
Member

Handled in #2671

@Ocramius Ocramius changed the title allow serverVersion to be unspecified allow serverVersion to be explicitly unspecified (null) Jul 22, 2017
@Ocramius Ocramius changed the title allow serverVersion to be explicitly unspecified (null) Allow serverVersion to be explicitly unspecified (null) Jul 22, 2017
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Aug 15, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants