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 ignored port in getConnectionString() #93

Merged
merged 1 commit into from
Dec 13, 2019
Merged

Fix ignored port in getConnectionString() #93

merged 1 commit into from
Dec 13, 2019

Conversation

bzikarsky
Copy link

Quick and obvious bugfix.

@bzikarsky
Copy link
Author

It looks like the CI failed because the database sometimes failed to bootstrap. Unrelated to my change.

@staabm
Copy link
Member

staabm commented Dec 9, 2019

you should provide a unit test

@bzikarsky
Copy link
Author

Possibly, but:

  • The repo's unit-tests require a locally available mysqld binary without there being a Dockerfile or similar to run the tests in
  • ConnectionConfigTest is not even tested at all in v1.x

If there is test-suite which can both run and changed easily I will gladly provide test cases. In this case, this is too much of a hustle.

@kelunik
Copy link
Member

kelunik commented Dec 10, 2019

Maybe I'm missing something, but it seems pretty weird to parse the port from the host here at all if we always have a host and a port in the object.

@bzikarsky
Copy link
Author

I think the main issue is the API design - having both a dedicated $port property and supporting <host>:<port> syntax on $host. That's redundant. Aynway: Ignoring $port in all varaints of getConnectionString() is just wrong. $port will be initalized to 3360 by default, so the code works the same, if the constructor argument is not used. In case it is, and there is no additional port hidden in $host, this patch will make sure, it is used.

@bzikarsky
Copy link
Author

Is there something blocking this PR from my side?

@kelunik
Copy link
Member

kelunik commented Dec 13, 2019

Nope.

@kelunik
Copy link
Member

kelunik commented Dec 13, 2019

Thanks!

@kelunik kelunik merged commit 739710a into amphp:v1.x Dec 13, 2019
@bzikarsky
Copy link
Author

Thanks for merging - when can we expect a patch-release?

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

Successfully merging this pull request may close these issues.

None yet

3 participants