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 Oracle connection string without dbname #908

Closed

Conversation

mRoca
Copy link
Contributor

@mRoca mRoca commented Sep 17, 2015

When using the DoctrineBundle, theCreateDatabaseDoctrineCommand and DropDatabaseDoctrineCommand commands unset the dbname parameter before getting the connection, in order to get a connection to the instance and not to the database which has to be created or removed : https://github.com/doctrine/DoctrineBundle/blob/master/Command/CreateDatabaseDoctrineCommand.php#L76 , and https://github.com/doctrine/DoctrineBundle/blob/master/Command/DropDatabaseDoctrineCommand.php#L78 .

Without this parameter, the connect string can't be created. This PR checks if a dbname parameter is available before setting the CONNECT_DATA value.

@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-1301

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

@deeky666
Copy link
Member

@mRoca AFAIK connecting to an Oracle database without a database instance reference (SID, SERVICE_NAME) isn't possible. So without the $params['dbname'] parameter it is not possible to connect to Oracle which in return means that we cannot open up "temporary" connections for tasks like those used in DoctrineBundle. Oracle does not support these operations via SQL anyways. We should rather require the parameter and throw an InvalidArgumentException instead to make things clear. Then in the DoctrineBundle commands those kind of platforms should be checked and a reasonable Exception thrown there too, to make clear that those commands cannot be used with platforms that don't support it.

@mRoca mRoca force-pushed the feature/oracle-connection-without-dbname branch from 92d9749 to 9f1fe22 Compare December 5, 2015 11:00
@mRoca
Copy link
Contributor Author

mRoca commented Dec 5, 2015

@deeky666 : I've updated the PR, an InvalidArgumentException is now thrown without valid SERVICE_NAME or SID.

$service = 'SERVICE_NAME=' . $serviceName;
$service = '(SERVICE_NAME=' . $serviceName . ')';
} else {
$service = '(SID=' . $serviceName . ')';
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No need to add the parentheses here. Having to add them in multiple statements is cumbersome, when you can add them later on. Please leave it the way it was before.

@deeky666
Copy link
Member

Besides nit picking, looks good to me, but is missing a test. Afterwards can be merged IMO.

@mRoca
Copy link
Contributor Author

mRoca commented Nov 6, 2019

Closing this PR in favor of #3297

@mRoca mRoca closed this Nov 6, 2019
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Aug 6, 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.

3 participants