Added support to override the protocol used when connecting to Oracle. #6593
Conversation
| @@ -70,14 +70,21 @@ public static function fromConnectionParameters(array $params): self | |||
| $connectData['INSTANCE_NAME'] = $params['instancename']; | |||
| } | |||
|
|
|||
| $protocol = 'TCP'; | |||
| if (isset($params['protocol'])) { | |||
There was a problem hiding this comment.
I believe we should stop putting driver-specific parameters into the driver-agnostic $params. Please try moving it to driver options.
There was a problem hiding this comment.
Hey @morozov - It's an Oracle parameter rather than a driver-specific function. You'll find it used when you generate a connection string from within OCI Cloud.
It's not dissimilar to how it's done in java - as much as it's driven within an oracle namespace.
Where else would you suggest I put this if not in an AbstractOracleDriver/EasyConnectString.php ?
Also noting that 'TCP' as the protocol is already set statically in this portion of code?
Cheers!
There was a problem hiding this comment.
It's an Oracle parameter rather than a driver-specific function.
So… it's a parameter that we only ever need when connecting through the Oracle drivers which makes it very driver-specific.
There was a problem hiding this comment.
Hi @derrabus - and the change is in AbstractOracleDriver - am I missing something?
There was a problem hiding this comment.
As @morozov said, $params is driver-agnostic. That's regardless of which class receives it.
There was a problem hiding this comment.
Ah now I follow what was being said.
I’ll look at the other drivers to see how specific database settings are made available
tests/Driver/OCI8/DriverTest.php
Outdated
| (new Driver())->connect([ | ||
| 'driverOptions' => ['protocol' => 'TCPS'], | ||
| ]); |
There was a problem hiding this comment.
It would be better to have a functional test instead. Is it possible to query some system view and get the protocol of the current connection? Otherwise, this test doesn't prove anything.
There was a problem hiding this comment.
@morozov I can check, but if you got the protocol wrong, it would not connect in the first place, so would I not be creating a test that would in theory never succeed?
There was a problem hiding this comment.
I'm talking about testing the happy path. If the user provides a valid value, the connection should be established using this protocol.
There was a problem hiding this comment.
@morozov OK. looking into it.
There was a problem hiding this comment.
@morozov The closest I can find in Oracle is via v$session_conn_info:
select * from V$SESSION_CONNECT_INFO;
(DESCRIPTION=(address=(protocol=tcps)(port=1521)(host=1.1.1.1))(CONNECT_DATA=(CID=(PROGRAM=DataGrip)(HOST=__jdbc__)(USER=xxxx))(service_name=xxxxx.oraclecloud.com)(CONNECTION_ID=xxxxxxx)(SERVER=dedicated)(INSTANCE_NAME=xxxx))(ADDRESS=(PROTOCOL=TCP)(HOST=1.1.1.1)(PORT=1521)(publish_service_acl=true))(SOURCE_ROUTE=YES)(HOP_COUNT=2))
I would have to parse that string then to get the answer unless a pattern match would be OK. I'm working on the 'existence' of protocol=tcps - because there may be some magic in Oracle cloud where it is likely passing via TCP internally.
What are your thoughts?
There was a problem hiding this comment.
@greg0ire I believe I have it this time - although the message is a bit skewiff with the note about 'combination of 2 commits'. Says 1 commit now :)
There was a problem hiding this comment.
Regarding the number of commits, we're good now, congrats!
Regarding the message itself, I don't know how you managed to do that 🤣
Can tell me what git config --get 'commit.cleanup' prints? I suspect an unexpected configuration on your end.
If I run git commit --amend, the comments disappear, as expected.
Can you try git commit --amend --no-edit --cleanup=strip && git push --force?
There was a problem hiding this comment.
@greg0ire, thanks for guiding @martinedge here. If it takes too much effort on your end, and if the author doesn't mind, we could amend the commit ourselves.
There was a problem hiding this comment.
@morozov I don't mind, if you want me to work through the process, I'm happy to either way, as it might save me mucking it up in the future :-) Up to you all, thankyou for your support!
There was a problem hiding this comment.
I did the regular git commit --amend && git push --force.
docs/en/reference/configuration.rst
Outdated
| @@ -312,6 +312,7 @@ pdo_oci / oci8 | |||
| connection pooling. | |||
| - ``charset`` (string): The charset used when connecting to the | |||
| database. | |||
|
|
|||
There was a problem hiding this comment.
Please remove the extra line you've introduced.
martinedge
force-pushed
the
oracle-connect-via-tcps
branch
from
00955cf to
e877129
Compare
This protocol is generally required when connecting to Oracle Cloud's Autonomous Databases.
morozov
force-pushed
the
oracle-connect-via-tcps
branch
from
e877129 to
8e4a40a
Compare
|
Thanks, @martinedge. |
|
Actually, it should have gone to 4.3.x since it's a new feature but it looks safe. Maintainers, please let me know if you think we should revert and retarget. |
|
It looks safe indeed. I think one way to notice that kind of mistake is to form the habit of assigning the milestone right before merging. |
TCPS is required to be passed in the EasyConnectString to connect to Autonomous databases. Tested against an Oracle Cloud instance using TLS authentication.
Fixes #6593.
Summary
Attempted to connect to an autonomous database within Oracle OCI Cloud. Identified the protocol needs to be changed within the connection string to match their requirements. Added a parameter to permit this override.
TCP remains the default, can be overridden by TCPS and TCPS only.