Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with
or
.
Download ZIP

Loading…

Reflection parameter type check #3241

Closed
wants to merge 3 commits into from

2 participants

@pierredup

When checking the type of the parameter in a method using the ParameterReflection, check if the parameter is type-hinted to get the type.

E.G

use Some\Library\ClassName;

class Foo {
    public function bar(array $var1, ClassName $var2)
    {
    }
}

The method ParameterReflection::getType would then return array and Some\Library\ClassName for the different parameters

Edit: Updated description

@weierophinney

There's a problem with this: often, developers will be more specific in the docblock tag. As an example, the default value might be an array, but the param annotation might indicate string|int|array. As such, your solution only provides a partial story.

I'd argue that we should only look at the parameter value in the case that no param annotation is present.

@pierredup

If a parameter is type-hinted to an array or object, then the value can't be anything but that.
So if you have a method public function foo(array $bar), then the parameter can only be an array, so specifying the annotation as E.G string|int|array is technically incorrect, as the parameter won't accept anything but an array.
The same goes for objects.

The only place where it makes a difference, is if you specify a default value of null. E.G public function foo(array $bar = null), then the parameter can accept an array or null, but we can check for that as well without looking at the annotations.

Then there is also the rare case of typos in the annotations, or annotations that are defined incorrectly.

@pierredup

@weierophinney My original description might have been a bit misleading, my apologies. This fix only checks the type of parameter if it is type-hinted.

I created a different solution, that checks the default value of the parameter as well. If the parameter isn't type-hinted, and no annotation is available, then it will use the default value if it is defined.

The changes is here: pierredup@3460576

Let me know what you think

@weierophinney

@pierredup Okay, makes sense. However:

  • It's not currently picking up the default value and adding "|null" if that's been declared.
  • No unit tests

Correct those, and I'll merge.

@weierophinney

@pierredup Looking good! Travis reports some test errors -- you can see the results here: https://travis-ci.org/zendframework/zf2/jobs/3741879/#L158

@pierredup

Not sure why the tests are failing. It all passes on my machine. Will keep digging around

@pierredup

So it seems in some versions of PHP (specifically php < 5.3.16 and php < 5.4.7), if you have a parameter that doesn't have a default value after parameters with a default value, then the isDefaultValueAvailable always returns false.

E.G:

public function foo($bar = null, $baz)
{
}

isDefaultValueAvailable will always return false.

Test case: http://3v4l.org/1oAQX

Hopefully no developer specifies methods like this, but it might happen.

So I think either:
1. Check PHP version to determine if we should check for default value
2. Leave the check for a default value, and only check type-hints and param annotations.

1 would offer the best case in many scenarios, but feels a bit bloated. So I think to rather go with 2 and trust the developer to specify the annotations correctly.

Opinions?

@weierophinney

I agree with (2). Go for it, @pierredup

@weierophinney weierophinney commented on the diff
library/Zend/Code/Reflection/ParameterReflection.php
((19 lines not shown))
}
- return null;
+ return $type;
@weierophinney Owner

Reviewing for merge; I'll likely make the following changes when I do.

First, import ReflectionClass.

Second, the can be simplified and branched a little cleaner.

if ($this->isArray()) {
    return 'array';
}

if (($class = $this->getClass()) instanceof ReflectionClass) {
    return $class->getName();
}

$docBlock = $this->getDeclaringFunction()->getDocBlock();

if (!$docBlock instanceof DocBlock) { // or whatever the type is
    return null;
}

$params = $docBlock->getTags('param');
if (isset($params[$this->getPosition()])) {
    return $params[$this->getPosition()]->getType();
}

return null;

The reason I suggest this approach is that it becomes easier to pair the conditions to the return type, which makes it a bit simpler to read, as well as to identify the specific behaviors.
if ($docBlock = $this->getDeclaringFunction()->getDocBlock()) {

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@weierophinney weierophinney referenced this pull request from a commit
@weierophinney weierophinney [#3241] Minor logic cleanup
- Cleaned up logic in ParameterReflection::getType() to better pair return
  values with conditions that cause them.
6bf16d2
@weierophinney

All changes suggested made on merge.

@ghost Unknown referenced this pull request from a commit
@weierophinney weierophinney [#3241] Minor logic cleanup
- Cleaned up logic in ParameterReflection::getType() to better pair return
  values with conditions that cause them.
6f696e0
@ghost Unknown referenced this pull request from a commit
@weierophinney weierophinney Merge branch 'hotfix/3241'
Close #3241
0cb5f0a
@ghost Unknown referenced this pull request from a commit
@weierophinney weierophinney Merge branch 'hotfix/3241' into develop
Forward port #3241
578ac8e
@weierophinney weierophinney referenced this pull request from a commit
Commit has since been removed from the repository and is no longer available.
@weierophinney weierophinney referenced this pull request from a commit
Commit has since been removed from the repository and is no longer available.
@weierophinney weierophinney referenced this pull request from a commit
Commit has since been removed from the repository and is no longer available.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
This page is out of date. Refresh to see the latest.
View
13 library/Zend/Code/Reflection/ParameterReflection.php
@@ -80,16 +80,21 @@ public function getDeclaringFunction($reflectionClass = null)
*/
public function getType()
{
- if ($docBlock = $this->getDeclaringFunction()->getDocBlock()) {
+ $type = null;
+
+ if ($this->isArray()) {
+ $type = 'array';
+ } elseif (($class = $this->getClass()) instanceof \ReflectionClass) {
+ $type = $class->getName();
+ } elseif ($docBlock = $this->getDeclaringFunction()->getDocBlock()) {
$params = $docBlock->getTags('param');
if (isset($params[$this->getPosition()])) {
- return $params[$this->getPosition()]->getType();
+ $type = $params[$this->getPosition()]->getType();
}
-
}
- return null;
+ return $type;
@weierophinney Owner

Reviewing for merge; I'll likely make the following changes when I do.

First, import ReflectionClass.

Second, the can be simplified and branched a little cleaner.

if ($this->isArray()) {
    return 'array';
}

if (($class = $this->getClass()) instanceof ReflectionClass) {
    return $class->getName();
}

$docBlock = $this->getDeclaringFunction()->getDocBlock();

if (!$docBlock instanceof DocBlock) { // or whatever the type is
    return null;
}

$params = $docBlock->getTags('param');
if (isset($params[$this->getPosition()])) {
    return $params[$this->getPosition()]->getType();
}

return null;

The reason I suggest this approach is that it becomes easier to pair the conditions to the return type, which makes it a bit simpler to read, as well as to identify the specific behaviors.
if ($docBlock = $this->getDeclaringFunction()->getDocBlock()) {

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
}
public function toString()
View
2  tests/ZendTest/Code/Reflection/ParameterReflectionTest.php
@@ -54,6 +54,8 @@ public function paramTypeTestProvider()
array('one','int'),
array('two','int'),
array('three','string'),
+ array('array','array'),
+ array('class','ZendTest\Code\Reflection\TestAsset\TestSampleClass')
);
}
}
View
3  tests/ZendTest/Code/Reflection/TestAsset/TestSampleClass5.php
@@ -34,7 +34,7 @@ class TestSampleClass5
* which spans multiple lines
* @return mixed Some return descr
*/
- public function doSomething($one, $two = 2, $three = 'three')
+ public function doSomething($one, $two = 2, $three = 'three', array $array = array(), TestSampleClass $class = null)
{
return 'mixedValue';
}
@@ -52,5 +52,4 @@ public function doSomethingElse($one, $two = 2, $three = 'three')
{
return 'mixedValue';
}
-
}
Something went wrong with that request. Please try again.