aerospike-community / aerospike-client-php

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

Methods do not have correct ZEND_ARG_INFO #36

Closed sustmi closed 5 years ago

sustmi commented 5 years ago

It seems that lot of the \Aerospike class methods do not have correct definition of ZEND_ARG_INFO. Take the \Aerospike::__construct() method as an example: https://github.com/aerospike/aerospike-client-php/blob/bd13d50d04eb69c2fa35d592c45d8c87c58f3fd5/src/include/aerospike_class.h#L35-L37

It should actually look like this:

PHP_METHOD(Aerospike, __construct); 
ZEND_BEGIN_ARG_INFO_EX(construct_arg_info, 0, 0, 1) 
        ZEND_ARG_ARRAY_INFO(0, config, 0)
        ZEND_ARG_INFO(0, persistent_connection)
        ZEND_ARG_ARRAY_INFO(0, options, 1)
ZEND_END_ARG_INFO(); 

(Note: I am not sure if options should have allow_null or not.)

See all the ZEND_ARG_... macros that ZEND API provides: https://github.com/php/php-src/blob/7686b0b88906e2522300b9e631ddde2051de839f/Zend/zend_API.h#L97 . Also see examples how are method arguments defined: https://github.com/php/php-src/search?q=ZEND_BEGIN_ARG_INFO_EX&unscoped_q=ZEND_BEGIN_ARG_INFO_EX .

The problem I am facing is that I am using PHPStan static analyzer and it reports that:

Class Aerospike constructor invoked with 1 parameter, 0 required.

The "problematic" code in our codebase that the static analyzer reports is a simple constructor call:

new Aerospike($hosts);

It looks like PHPStan relies on the number of arguments that reflection returns.

I have created a simple script that uses reflection in order to check whether total number of method arguments is equal or greater than number of required arguments.

<?php

$reflectionExtension = new ReflectionExtension('aerospike');

foreach ($reflectionExtension->getClasses() as $reflectionClass) {
    foreach ($reflectionClass->getMethods() as $reflectionMethod) {
        if ($reflectionMethod->getNumberOfParameters() < $reflectionMethod->getNumberOfRequiredParameters()) {
            echo sprintf(
                "Method %s::%s is defined with %d parameters, but %d parameters are defined as required.\n",
                $reflectionClass->getName(),
                $reflectionMethod->getName(),
                $reflectionMethod->getNumberOfParameters(),
                $reflectionMethod->getNumberOfRequiredParameters()
            );
        }
    }
}

The script writes the following output:

Method Aerospike::__construct is defined with 0 parameters, but 1 parameters are defined as required.
Method Aerospike::addIndex is defined with 0 parameters, but 6 parameters are defined as required.
Method Aerospike::append is defined with 0 parameters, but 3 parameters are defined as required.
Method Aerospike::deregister is defined with 0 parameters, but 1 parameters are defined as required.
Method Aerospike::dropIndex is defined with 0 parameters, but 2 parameters are defined as required.
Method Aerospike::increment is defined with 0 parameters, but 3 parameters are defined as required.
Method Aerospike::initKey is defined with 0 parameters, but 3 parameters are defined as required.
Method Aerospike::listAppend is defined with 0 parameters, but 3 parameters are defined as required.
Method Aerospike::listClear is defined with 0 parameters, but 2 parameters are defined as required.
Method Aerospike::listInsert is defined with 0 parameters, but 4 parameters are defined as required.
Method Aerospike::listInsertItems is defined with 0 parameters, but 4 parameters are defined as required.
Method Aerospike::listMerge is defined with 0 parameters, but 3 parameters are defined as required.
Method Aerospike::listRemove is defined with 0 parameters, but 3 parameters are defined as required.
Method Aerospike::listRemoveRange is defined with 0 parameters, but 4 parameters are defined as required.
Method Aerospike::listSet is defined with 0 parameters, but 4 parameters are defined as required.
Method Aerospike::listTrim is defined with 0 parameters, but 4 parameters are defined as required.
Method Aerospike::put is defined with 0 parameters, but 2 parameters are defined as required.
Method Aerospike::prepend is defined with 0 parameters, but 3 parameters are defined as required.
Method Aerospike::register is defined with 0 parameters, but 2 parameters are defined as required.
Method Aerospike::remove is defined with 0 parameters, but 1 parameters are defined as required.
Method Aerospike::setDeserializer is defined with 0 parameters, but 1 parameters are defined as required.
Method Aerospike::setSerializer is defined with 0 parameters, but 1 parameters are defined as required.
Method Aerospike::touch is defined with 0 parameters, but 1 parameters are defined as required.
Method Aerospike::predicateEquals is defined with 0 parameters, but 2 parameters are defined as required.
Method Aerospike::predicateGeoContainsGeoJSONPoint is defined with 0 parameters, but 2 parameters are defined as required.
Method Aerospike::predicateGeoContainsPoint is defined with 0 parameters, but 3 parameters are defined as required.
Method Aerospike::predicateGeoWithinRadius is defined with 0 parameters, but 4 parameters are defined as required.
Method Aerospike::predicateGeoWithinGeoJSONRegion is defined with 0 parameters, but 2 parameters are defined as required.
Method Aerospike::predicateRange is defined with 0 parameters, but 4 parameters are defined as required.
Method Aerospike::predicateBetween is defined with 0 parameters, but 3 parameters are defined as required.
Method Aerospike::predicateContains is defined with 0 parameters, but 2 parameters are defined as required.
Method Aerospike::setLogLevel is defined with 0 parameters, but 1 parameters are defined as required.
Method Aerospike::setLogHandler is defined with 0 parameters, but 1 parameters are defined as required.
Method Aerospike::scan is defined with 0 parameters, but 3 parameters are defined as required.
Method Aerospike::query is defined with 0 parameters, but 4 parameters are defined as required.

Do you want me to create a pull request adding the method argument definitions?

aerospikerobertmarks commented 5 years ago

Hi @sustmi ,

Thanks for bringing this up. If you are willing to submit a pull request for those changes, that would be awesome.