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

Remove redundant phpstan param from DriverManager::getConnection() #4403

Merged
merged 1 commit into from
Nov 14, 2020

Conversation

simPod
Copy link
Contributor

@simPod simPod commented Nov 2, 2020

Q A
Type bug
BC Break no
Fixed issues

Summary

This effectively prevented phpstan from inferring type of T template.

Unable to resolve the template type T in call to method static method Doctrine\DBAL\DriverManager::getConnection()

@greg0ire
Copy link
Member

greg0ire commented Nov 2, 2020

This effectively prevented phpstan from inferring type of T template.

IIRC that was the goal, it did not understand the annotations I added when I added them. Glad to see that has been fixed :)

greg0ire
greg0ire previously approved these changes Nov 2, 2020
@simPod
Copy link
Contributor Author

simPod commented Nov 2, 2020

it did not understand the annotations I added when I added them

Sounds legit, I also often type differently for psalm and phpstan

@morozov
Copy link
Member

morozov commented Nov 5, 2020

Thanks, @simPod. Please squash and double-check if the same will work on 3.0.x (strict PHPStan rules) and master, e.g. cherry-pick your commit on top of those branches and run both PHPStan and Psalm locally.

@morozov morozov added this to the 2.12.1 milestone Nov 5, 2020
This effectively prevented phpstan from inferring type of `T` template.

> Unable to resolve the template type T in call to method static method Doctrine\DBAL\DriverManager::getConnection()
@simPod
Copy link
Contributor Author

simPod commented Nov 6, 2020

There are some errors but I'd rather fix them individually on each branch.

@morozov
Copy link
Member

morozov commented Nov 7, 2020

There are some errors but I'd rather fix them individually on each branch.

It doesn't work this way. We regularly merge the changes from the older branch up to master. If the build fails during the up merge, it will have to be fixed in an emergency way. We need to be sure that when we apply certain changes in 2.12, we know how to handle it in the newer ones.

@simPod
Copy link
Contributor Author

simPod commented Nov 11, 2020

I did this for master as well #4419. Don't know how to do it in one PR properly, same as in #4413

@morozov morozov merged commit c2c8488 into doctrine:2.12.x Nov 14, 2020
@morozov
Copy link
Member

morozov commented Nov 14, 2020

FYI: I reverted it during the merge of 3.0.x to master (#4432).

This change opens a can of worms: the connection parameters array contains a lot of different keys of different types which may or may not be used in certain scenarios. The array{?key: type} notation implies that there are no other keys than "key" which is not true.

@simPod
Copy link
Contributor Author

simPod commented Nov 15, 2020

This change opens a can of worms: the connection parameters array contains a lot of different keys of different types which may or may not be used in certain scenarios. The array{?key: type} notation implies that there are no other keys than "key" which is not true.

I agree, if I was the one who introduced it I'd definitely list all possible keys.

I reverted it during the merge of 3.0.x to master

Should I do something? Not sure what it implies.

@simPod simPod deleted the patch-1 branch November 15, 2020 09:26
@morozov
Copy link
Member

morozov commented Nov 15, 2020

I agree, if I was the one who introduced it I'd definitely list all possible keys.

If only it was that easy :-) There are quite a lot of keys in the connection parameters, and while Psalm allows specifying named array types, it doesn't allow sharing them across files. See some WIP for example: 3.0.x...morozov:psalm-params.

Should I do something? Not sure what it implies.

It was just a note for your information.

@andrew-demb
Copy link
Contributor

while Psalm allows specifying named array types, it doesn't allow sharing them across files

@morozov Psalm allows import and reuse them - please see https://psalm.dev/docs/annotating_code/supported_annotations/#psalm-import-type

@morozov
Copy link
Member

morozov commented Nov 15, 2020

@andrew-demb that's new to me. Thanks for the pointer!

rgrellmann added a commit to Rossmann-IT/dbal that referenced this pull request Mar 7, 2021
Release [2.12.1](https://github.com/doctrine/dbal/milestone/84)

2.12.1
======

- Total issues resolved: **2**
- Total pull requests resolved: **11**
- Total contributors: **7**

Documentation,Prepared Statements
---------------------------------

 - [4424: Mark SQLParserUtils internal](doctrine#4424) thanks to @morozov

Packaging
---------

 - [4416: Update .gitattributes](doctrine#4416) thanks to @bytestream

Bug,Cache
---------

 - [4414: ResultCacheStatement::fetchAllAssociative does not store results in cache](doctrine#4414) thanks to @morozov and @dFayet

Deprecation,Prepared Statements
-------------------------------

 - [4411: Deprecate inappropriate usage of prepared statement parameters](doctrine#4411) thanks to @morozov
 - [4407: Deprecate colon prefix for prepared statement parameters](doctrine#4407) thanks to @morozov

Static Analysis
---------------

 - [4403: Remove redundant phpstan param from DriverManager::getConnection()](doctrine#4403) thanks to @simPod

Bug,Locking,Transactions
------------------------

 - [4400: LockMode::NONE should not set WITH (NOLOCK)](doctrine#4400) thanks to @BenMorel

Code Style,PHP
--------------

 - [4398: Update PHP&doctrine#95;CodeSniffer to 3.5.8](doctrine#4398) thanks to @morozov

PDO,PHP,Test Suite
------------------

 - [4396: Fix php8 mysql mariadb](doctrine#4396) thanks to @greg0ire

Documentation
-------------

 - [4390: Fix headline in the upgrade docs](doctrine#4390) thanks to @jdreesen

Documentation,Testing
---------------------

 - [4356: Testing Guidelines](doctrine#4356) thanks to @morozov

# gpg: Signature made Sat Nov 14 21:50:01 2020
# gpg:                using DSA key 1BEDEE0A820BC30D858F9F0C2C3A645671828132
# gpg: Can't check signature: No public key

# Conflicts:
#	README.md
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jul 26, 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.

4 participants