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 DSN detection in the IBM DB2 driver #4066

Merged
merged 2 commits into from Jun 12, 2020
Merged

Conversation

morozov
Copy link
Member

@morozov morozov commented Jun 11, 2020

Q A
Type bug, improvement
BC Break no

Fixes #4065.

$params['protocol'] = 'TCPIP';
}
$params['user'] = $username;
$params['password'] = $password;
Copy link
Member Author

@morozov morozov Jun 11, 2020

Choose a reason for hiding this comment

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

User credentials and driver options are passed to the driver both via individual parameters and as part of $params which is awkward:

dbal/src/Connection.php

Lines 352 to 356 in 48625f1

$driverOptions = $this->params['driverOptions'] ?? [];
$user = $this->params['user'] ?? null;
$password = $this->params['password'] ?? null;
$this->_conn = $this->_driver->connect($this->params, $user, $password, $driverOptions);

This can be improved by simplifying the signature of Driver::connect() to just Driver::connect(array $params) (a separate issue is to be filed). Once it's done, the lines above will be removed.

@morozov morozov force-pushed the issues/4065 branch 2 times, most recently from 2e6bd40 to bb2f529 Compare June 11, 2020 23:13
@codecov-commenter
Copy link

codecov-commenter commented Jun 11, 2020

Codecov Report

Merging #4066 into 2.10.x will increase coverage by 0.08%.
The diff coverage is 96.29%.

Impacted file tree graph

@@             Coverage Diff              @@
##             2.10.x    #4066      +/-   ##
============================================
+ Coverage     73.30%   73.39%   +0.08%     
- Complexity     5031     5036       +5     
============================================
  Files           215      216       +1     
  Lines         12825    12838      +13     
============================================
+ Hits           9401     9422      +21     
+ Misses         3424     3416       -8     
Impacted Files Coverage Δ Complexity Δ
lib/Doctrine/DBAL/Driver/IBMDB2/DB2Driver.php 90.90% <87.50%> (+49.73%) 2.00 <0.00> (-4.00) ⬆️
lib/Doctrine/DBAL/Driver/IBMDB2/DataSourceName.php 100.00% <100.00%> (ø) 9.00 <9.00> (?)
lib/Doctrine/DBAL/Driver/PDOSqlsrv/Driver.php 88.88% <0.00%> (-3.71%) 13.00% <0.00%> (ø%)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3745877...747b042. Read the comment docs.

@morozov morozov requested a review from greg0ire June 11, 2020 23:29
/**
* Creates the object from an array representation
*
* @param array<string,mixed> $params
Copy link
Member

Choose a reason for hiding this comment

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

Let's document the possible keys with an array shape?

Copy link
Member Author

Choose a reason for hiding this comment

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

This method doesn't limit the keys that can be used.

Copy link
Member

@greg0ire greg0ire Jun 12, 2020

Choose a reason for hiding this comment

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

Array shapes do not limit the keys that can be used either, it will allow you to document what keys are mandatory or optional and the type they should have if present.

Copy link
Member Author

Choose a reason for hiding this comment

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

So what will be the behavior if the key being passed is not mandatory or optional?

In this specific case, it's still a map of any string to any value that can be cast to a string. But instead of defining it as an array shape, I'd rather have an object/structure. If we don't do that soon, we may define an array shape later when bumping the Psalm level and it demands it. This way, we'll make sure that our type annotations are valid.

Copy link
Member

@greg0ire greg0ire Jun 12, 2020

Choose a reason for hiding this comment

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

So what will be the behavior if the key being passed is not mandatory or optional?

Not the one I thought apparently… I could have sworn this was OK: https://psalm.dev/r/cd931bdda1

Sorry for polluting you with this nonsense 🤦

Copy link
Member Author

Choose a reason for hiding this comment

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

No worries. I wasn't sure about how Psalm works either, so now we're on the same page 👍

@morozov morozov merged commit e5e3abf into doctrine:2.10.x Jun 12, 2020
@morozov morozov deleted the issues/4065 branch June 12, 2020 19:19
@morozov morozov self-assigned this Jun 12, 2020
@morozov morozov added this to the 2.10.3 milestone Jun 12, 2020
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Aug 2, 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.

The IBM DB2 driver uses the dbname connection parameter improperly
3 participants