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

Bug: The SQLSRV driver ignores the port value from the config. #6036

Merged
merged 2 commits into from
May 27, 2022

Conversation

iRedds
Copy link
Collaborator

@iRedds iRedds commented May 27, 2022

Description
Fixes #6032

The SQLSRV driver ignores the port value from the config.

This PR fixes a bug.
Also, if a port is already specified in the hostname, the port from the configuration will be ignored.

It seems to me that the default tests are enough, because the database connection settings for the tests use the default port, which was ignored when configuring the connection.

Checklist:

  • Securely signed commits

Signed-off-by: Andrey Pyzhikov <5071@mail.ru>
@kenjis kenjis added bug Verified issues on the current code behavior or pull requests that will fix them database Issues or pull requests that affect the database layer labels May 27, 2022
Signed-off-by: Andrey Pyzhikov <5071@mail.ru>
@kenjis
Copy link
Member

kenjis commented May 27, 2022

@kenjis
Copy link
Member

kenjis commented May 27, 2022

It seems to me that the default tests are enough, because the database connection settings for the tests use the default port, which was ignored when configuring the connection.

This bug was not detected by the existing tests, so tests are not enough.
But to test this, we build another SQL Server with the different port.
It costs a lot.

I've confirmed Port 1444 server can be connected with this PR.

@kenjis kenjis merged commit 7b42a12 into codeigniter4:develop May 27, 2022
@iRedds
Copy link
Collaborator Author

iRedds commented May 27, 2022

@kenjis I don't see the point in an additional server.
We can use non-standard ports for each DBMS for github actions and in test configs. Or at least change the ports in the docker containers to listen.

@iRedds iRedds deleted the fix-sqlsrv-port branch May 27, 2022 08:08
@kenjis
Copy link
Member

kenjis commented May 27, 2022

@iRedds Okay, you are correct.
Changing the port of the docker containers to listen is enough.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Verified issues on the current code behavior or pull requests that will fix them database Issues or pull requests that affect the database layer
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Bug: SQLSRV with specified port over network
3 participants