aerospike-community / aerospike-client-php

Aerospike client for PHP7
Apache License 2.0
29 stars 28 forks source link

phpdoc are not precise #30

Open max-matiukhin opened 5 years ago

max-matiukhin commented 5 years ago

I've caught this error using \Aerospike::get() method, but it seems, other methods might have similar issues. The file https://github.com/aerospike/aerospike-client-php/blob/master/doc/phpdoc/aerospike.php gives us this interface for the method get():

public function get(array $key, array &$record, array $select = [], array $options = []) {}

There are 2 problems here. First, it says that the default value for the last 2 parameters is an empty array. However this code gonna fail:

$aerospike = new \Aerospike(['hosts' => [ ['addr' => 'aerospike1.local', 'port' => 4000]]]);
$key = $aerospike->initKey('test', 'test', 'abc')
$response_status = $aerospike->get($key, $result, [], []);

with $response_status === 4 (AS_PROTO_RESULT_FAIL_PARAMETER) It seems that the real default value for the last 2 parameters is null. This code works without errors:

$aerospike = new \Aerospike(['hosts' => [ ['addr' => 'aerospike1.local', 'port' => 4000]]]);
$key = $aerospike->initKey('test', 'test', 'abc')
$response_status = $aerospike->get($key, $result, null, null);

The second problem is about the second parameter - array &$record. AFAIK, in PHP if you create function with such parameter, you should pass an array there, otherwise it fails due to type mismatch

function get(array &$record) { ... }
$my_record = []; 
get($my_record); // this call is ok because my_record is array and function expects an array
get($undeclared_record); // this call gonna fail because $undeclared_record is null but function expects an array

It's not critical, but these errors are visible if you:

  1. use static analyzers
  2. create a wrapper-class around the original Aerospike class and you want to mimic the original class. In this case you might want to copy-paste the interface from the phpdoc and you will get these problems.
aerospikerobertmarks commented 5 years ago

Thanks for noticing this. I'll fix the those documentation issues.

In the second case, we were trying to indicate that, you were passing in a reference to a return value which would end up being turned into an array, but as you correctly point out, the current documentation does not correctly indicate that.

Thanks again for discovering these issues.

max-matiukhin commented 5 years ago

One more issue with constants. In the php file they are string. So, when you run your code and got an error 4, you can't open this file and find what this code means. Also, static analyzers check this file and see that constants are strings and in the real code we compare it with int. So we get messages from static analyzers.

Another issue. Some constants in this file have wrong name. Here is the list:

const ERR_TLS = -9;
const ERR_FORBIDDEN = 22;
const ERR_FAIL_NOT_FOUND = 23;
const ERR_QUERY_END = 50;
const ERR_BATCH_MAX_REQUESTS_EXCEEDED = 151;
const OPT_POLICY_RETRY = 4;
const OPT_SCAN_INCLUDELDT = 11;
const COMPRESSION_THRESHOLD = 19;
const POLICY_RETRY_NONE = 0;
const POLICY_RETRY_ONCE = 1;
const POLICY_REPLICA_PREFER_RACK = 3;
const OPT_TLS_CAFILE = 33;
ondrejmirtes commented 4 years ago

Also there's a problem with ReflectionParameter because for some function+parameter from this extension, ReflectionParameter::getName() returns null which shouldn't be possible.

dwelch-spike commented 4 years ago

Thank you for pointing this out. I'll look into a fix and post an update when done. If you know which functions are causing the issue could you please post them?

Thanks

ramunasd commented 4 years ago

I can confirm this issue. Easy way to list all empty parameter names:

$class = new ReflectionClass('Aerospike');
foreach ($class->getMethods() as $method) {
    $parameters = $method->getParameters();
    foreach ($parameters as $key => $parameter) {
        $name = $parameter->getName();
        if (empty($name)) {
            printf("Empty method %s parameter %u name\n", $method->getName(), $key);
        }
    }
}

in my env PHP7.3, Aerospike client 7.5.2 output is:

Empty method operateOrdered parameter 0 name
Empty method operateOrdered parameter 1 name
Empty method operateOrdered parameter 2 name
Empty method operateOrdered parameter 3 name
Empty method scanInfo parameter 0 name
Empty method scanInfo parameter 1 name
Empty method scanInfo parameter 2 name

Due this is impossible to use Aerospike class directly as Symfony DI lazy service.