goaop / framework

:gem: Go! AOP PHP - modern aspect-oriented framework for the new level of software development
go.aopphp.com
MIT License
1.66k stars 163 forks source link

Non-variadic parameter causes 'Variadic parameter cannot have a default value' exception #476

Closed tolgadevsec closed 7 months ago

tolgadevsec commented 3 years ago

Hi! I'm using the Go! AOP framework ("^3.0") in my Laravel project ("^8.40"), however, I'm running into the following exception thrown from the Laminas ParameterGenerator class when the following line is called:

https://github.com/laminas/laminas-code/blob/17fd2af36804f3f61788573cb70196454d6ee1d8/src/Generator/ParameterGenerator.php#L260

https://github.com/goaop/framework/blob/501d013c78e92057601c17ca0df1a59ab984f210/src/Proxy/Part/FunctionParameterList.php#L66

It seems that the $defaultValue passed to the constructor of the ParameterGenerator in line 62 really needs to be null for optional and non-variadic parameters. This is not the case due to: https://github.com/goaop/framework/blob/501d013c78e92057601c17ca0df1a59ab984f210/src/Proxy/Part/FunctionParameterList.php#L46

I also have one case where I have a controller method with a default parameter value, this is a non-variadic parameter as well and runs into the same Exception. However, I was able to resolve both issues by explicitly setting the default value after the setVariadic call instead of doing it before. This has the reason that setVariadic is checking whether its default value attribute is set (and throws an Exception if thats the case) before it actually sets the variadic attribute to true or false.

Here is a snippet of what I have changed:

   // ...

  $defaultValue = null;

  // Move default value detection code to the bottom (after setVariadic)

  $parameterTypeName = null;
  if (!$useTypeWidening && $reflectionParameter->hasType()) {
      $parameterReflectionType = $reflectionParameter->getType();
      if ($parameterReflectionType instanceof ReflectionNamedType) {
          $parameterTypeName = $parameterReflectionType->getName();
      } else {
          $parameterTypeName = (string) $parameterReflectionType;
      }
  }

  $generatedParameter = new ParameterGenerator(
      $reflectionParameter->getName(),
      $parameterTypeName,
      $defaultValue, // We are passing null here for now
      $reflectionParameter->getPosition(),
      $reflectionParameter->isPassedByReference()
  );
  $generatedParameter->setVariadic($reflectionParameter->isVariadic());
  // The isset($this->defaultValue) wont throw a Exception now

  // Check if parameter is non-variadic and call setDefaultValue 
  if(!$reflectionParameter->isVariadic()){
      if ($reflectionParameter->isDefaultValueAvailable()) {
          $defaultValue = new ValueGenerator($reflectionParameter->getDefaultValue());
      } elseif ($reflectionParameter->isOptional()) {
          $defaultValue = new ValueGenerator(null);
      }

      if($defaultValue instanceof ValueGenerator) {
          $generatedParameter->setDefaultValue($defaultValue);
      }
  }

  // ...

Hope this can be of use to anyone running into the same issue. Let me know if further information are required for reproduction - the Laravel project that I'm working on is pretty much an empty project with one HTTP controller.

Kind regards, Tolga

func0der commented 2 years ago

Same problem with a laminas project here. It probably worked before this PR got merged: https://github.com/laminas/laminas-code/pull/72/files which was merged into 4.2.0.

Can we find a fix for this, please? Maybe @tolgadevsec can make a PR for his changes so they can be evaluated against the tests.

lisachenko commented 2 years ago

Hey, @mchekin, stop breaking the laminas-code API ) Your change has introduced a BC break in minor version and affected my framework.

sakhunzai commented 1 year ago

@lisachenko the issue in upstream lib is fixed so this issue is also resolved in laminas/laminas-code v4.9.0