Skip to content

Fix Memcache/MemcachePool get method signature #2323

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

Closed
wants to merge 0 commits into from

Conversation

dravnic
Copy link
Contributor

@dravnic dravnic commented Apr 4, 2023

@@ -6414,7 +6414,7 @@
'Memcache::decrement' => ['int', 'key'=>'string', 'value='=>'int'],
'Memcache::delete' => ['bool', 'key'=>'string', 'timeout='=>'int'],
'Memcache::flush' => ['bool'],
'Memcache::get' => ['string|array|false', 'key'=>'string', '&flags='=>'array', '&keys='=>'array'],
'Memcache::get' => ['string|array|false', 'key'=>'string|array', '&flags='=>'int|array'],
Copy link
Contributor

Choose a reason for hiding this comment

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

Could we get more precise, e.g. string[] ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@staabm Good idea. Just added commit with string[] for key and int[] for flags

Copy link
Member

@ondrejmirtes ondrejmirtes left a comment

Choose a reason for hiding this comment

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

Can you please add these as separate variants? Because they are separate variants at https://www.php.net/manual/en/memcache.get.php

What I mean by that - look at the top of the file and see the function abs():

'abs' => ['0|positive-int', 'number'=>'int'],
'abs\'1' => ['float', 'number'=>'float'],
'abs\'2' => ['float|0|positive-int', 'number'=>'string'],

You can also test that in NodeScopeResolverTest - passing string should return string, passing array should return array.

@dravnic
Copy link
Contributor Author

dravnic commented Apr 4, 2023

@ondrejmirtes Sure, I didn't know it was possible to define variants. makes more sense.

@dravnic
Copy link
Contributor Author

dravnic commented Apr 4, 2023

Using variants in the latest commit.

Also fixed return value. Memcache::get deserializes so return value should be mixed i.e. mixed[]|false

@ondrejmirtes
Copy link
Member

You can also test that in NodeScopeResolverTest - passing string should return string, passing array should return array.

@dravnic
Copy link
Contributor Author

dravnic commented Apr 5, 2023

It seems that PHP documentation isn't correct regarding Memcache::get return type. It doesn't return string|array, but mixed|mixed[] type depending on value. Below is a simple script that checks this.

How to test this in NodeScopeResolverTest? Is there a stub/mock of Memcache class that I could use? I didn't find any.

<?php
declare(strict_types=1);

$memcache = new Memcache();
$memcache->connect('localhost');

$memcache->set('key-number', 123);
$memcache->set('key-string', "abc");
$memcache->set('key-array', array(1,2,3));
$memcache->set('key-object', (object) array(1,2,3));

echo "\n---- Memcache::get (single) ----\n";
echo "key-number type -> " . gettype($memcache->get('key-number')) . "\n";
echo "key-string type -> " . gettype($memcache->get('key-string')) . "\n";
echo "key-array type -> " . gettype($memcache->get('key-array')) . "\n";
echo "key-object type -> " . gettype($memcache->get('key-object')) . "\n";

echo "\n---- Memcache::get (multi)  ----\n";
$multi = $memcache->get(array('key-number', 'key-string', 'key-array', 'key-object'));
foreach ($multi as $key => $value) {
    echo "$key type -> " . gettype($value) . "\n";
}

and here is the output of running it (php 8.1):

---- Memcache::get (single) ----
key-number type -> integer
key-string type -> string
key-array type -> array
key-object type -> object

---- Memcache::get (multi)  ----
key-number type -> integer
key-string type -> string
key-array type -> array
key-object type -> object

@ondrejmirtes
Copy link
Member

Here's the documentation about testing: https://phpstan.org/developing-extensions/testing#type-inference

You can also check out all the files linked in NodeScopeResolverTest to get the idea how it works.

@dravnic dravnic changed the base branch from 1.10.x to 1.11.x April 17, 2023 06:47
@dravnic
Copy link
Contributor Author

dravnic commented Apr 17, 2023

@ondrejmirtes just added tests to the NodeScopeResolverTest. hope it's ok now.

@dravnic dravnic force-pushed the memcache-get-signature branch from e92b685 to 8fce724 Compare April 17, 2023 07:14
@ondrejmirtes
Copy link
Member

@dravnic 1) Please base it on top of 1.10.x.
2) Rebase it so that it contains only your own commits and no merge commits.

@dravnic dravnic closed this Apr 17, 2023
@dravnic dravnic force-pushed the memcache-get-signature branch from 8fce724 to 0745fbd Compare April 17, 2023 07:16
@dravnic dravnic deleted the memcache-get-signature branch April 17, 2023 07:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants