XOOPS / XoopsCore25

XOOPS Core 2.5.x (current release is 2.5.11: https://github.com/XOOPS/XoopsCore25/releases)
GNU General Public License v2.0
71 stars 59 forks source link

strict_types for initVar() #1143

Open mambax7 opened 2 years ago

mambax7 commented 2 years ago

I want to set the default to 1:

$this->initVar('active', \XOBJ_DTYPE_INT, 1, true);

But XOOPS Kernel Object requires null:

phpStan: Parameter #3 $value of method XoopsObject::initVar() expects null, int given.

/**
* initialize variables for the object
*
* YOU SHOULD NOT USE THE $enumeration PARAMETER
*
* @access   public
*
* @param string $key
* @param int    $data_type set to one of XOBJ_DTYPE_XXX constants (set to XOBJ_DTYPE_OTHER if no data type checking nor text sanitizing is required)
* @param null   $value
* @param bool   $required  require html form input?
* @param int    $maxlength for XOBJ_DTYPE_TXTBOX type only
* @param string $options
* @param string $enumerations
*
* @return void
*/
public function initVar($key, $data_type, $value = null, $required = false, $maxlength = null, $options = '', $enumerations = '')
{
   $this->vars[$key] = array(
       'value'       => $value,
       'required'    => $required,
       'data_type'   => $data_type,
       'maxlength'   => $maxlength,
       'changed'     => false,
       'options'     => $options,
       'enumeration' => $enumerations);
}

Should we change

 * @param null   $value

to

 * @param string|int|null  $value 

Could we also make similar adjustments for all other cases like this one?

geekwright commented 2 years ago

This is purely an incorrect docblock. If we try to create a list, we'll be forever twiddling it. The appropriate fix is:

* @param mixed $value

mambax7 commented 2 years ago

Are you going to do it, or should I make the adjustments for all the cases with "null" and submit PR?

geekwright commented 2 years ago

Any * @param null lines are obviously wrong. Why have a parameter that only accepts null? I count about 43 occurances.

They are not all 'mixed' so some investigation is required. There are a lot that should be string|null and some array|null.

If you would like to do make the adjustments, please do. Otherwise I'll do it.

mambax7 commented 2 years ago

Whatever works better for you - I'm happy to help, but if you prefer to do it yourself because of potential errors on my part, I perfectly fine with that. So just let me know...