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 signature for openssl_open under PHP >= 8.0 #6987

Merged
merged 1 commit into from Nov 29, 2021

Conversation

pilif
Copy link
Contributor

@pilif pilif commented Nov 25, 2021

fixes the false InvalidArgument here: https://psalm.dev/r/d84f1672b7

@psalm-github-bot
Copy link

I found these snippets:

https://psalm.dev/r/d84f1672b7
<?php

$k = openssl_pkey_get_private('some file', 'very secret');
openssl_open("flupp", $decoded, "foobar", $k, "RC4");
Psalm output (using commit 55b2b6b):

ERROR: InvalidArgument - 4:43 - Argument 4 of openssl_open expects array<array-key, mixed>|resource|string, OpenSSLAsymmetricKey|false provided

@weirdan weirdan added the release:fix The PR will be included in 'Fixes' section of the release notes label Nov 26, 2021
@weirdan weirdan marked this pull request as draft November 26, 2021 01:50
@pilif
Copy link
Contributor Author

pilif commented Nov 26, 2021

I have force-pushed a new commit which fixes all openssl_* signatures for PHP >= 8.0.

In the end, I went with the long signature for that array keys argument: Even if some of the arguments as they are allowed now make no sense, they are allowed by PHP and we should not complain about valid PHP code, no matter how nonsensical.

One thing: there is openssl_pkey_get_details which returns an associative array whose members depend on the key passed in as the argument and none of the types of these key-dependent values are documented in the manual.

The existing return type was declared as array|false and this PR leaves it at that. If somebody wants to go through all of the combinations and figure out their types, that's a substantial additional task which I think should be subject to a future PR as with this PR, at least all openssl_* functions are useable with psalm under PHP8, where prior to this PR they weren't.

@pilif pilif marked this pull request as ready for review November 26, 2021 09:23
@pilif
Copy link
Contributor Author

pilif commented Nov 26, 2021

also - wow - what a minefield the openssl_* functions are.

stuff like

Screenshot 2021-11-26 at 09 35 51

or

Screenshot 2021-11-26 at 09 38 23

in functions related to crypto...

Copy link
Collaborator

@weirdan weirdan left a comment

Choose a reason for hiding this comment

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

There's a number of nullability nitpicks, but otherwise this looks really good.

dictionaries/CallMap.php Show resolved Hide resolved
dictionaries/CallMap.php Show resolved Hide resolved
dictionaries/CallMap.php Show resolved Hide resolved
dictionaries/CallMap.php Show resolved Hide resolved
dictionaries/CallMap.php Show resolved Hide resolved
dictionaries/CallMap.php Show resolved Hide resolved
dictionaries/CallMap.php Show resolved Hide resolved
while some signatures were already updated in the past, many others were
not.

This commits fixes all signatures to match what the PHP manual
specifies.
@pilif
Copy link
Contributor Author

pilif commented Nov 29, 2021

thank you very much for your additional remarks. When I went through the functions, I mostly looked at the resource-typed parameters and didn't bother re-checking all the nullabilities. Now everything is up to date with the manual.

The windows tests fail before they even start testing with some missing infrastructure to install PHP. I don't know what's going on there, but I don't think it's related to the updated call map.

@weirdan
Copy link
Collaborator

weirdan commented Nov 29, 2021

I don't know what's going on there

Seems to be a temporary glitch. I've restarted the job and it seems to be working now.

@weirdan weirdan merged commit 5197a68 into vimeo:master Nov 29, 2021
@weirdan
Copy link
Collaborator

weirdan commented Nov 29, 2021

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release:fix The PR will be included in 'Fixes' section of the release notes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants