Skip AnnotationScanner if class name information can't be found.

[tomphp]

Currently when ClassReflection::getAnnotations() is called on class which uses a trait and error occurs when the AnnotationScanner constructor gets called with false for the $nameInformation parameter.

In this PR I have simply changed it so that getAnnotations() returns false instead of creating a new AnnotationScanner if FileScanner::getClassNameInformation() returns false.

This simple causes traits to be skipped, maybe instead more needs to be done here to scan for traits in the scanner, if so let me know.

[tomphp]

Update: Also done this for MethodReflection

[weierophinney]

Please add unit tests, @tomphp -- and thanks!

[tomphp]

@weierophinney Apologies, unit tests now included.

[tomphp]

I've just seen the 5.3.x travis builds failed to you the traits in the test suite. Is there a suggested method to deal with this in the ZF2 test suite? Is it just a matter of check the php version in the test and return if it's pre 5.4?

[weierophinney]

Is there a suggested method to deal with this in the ZF2 test suite? Is it just a matter of check the php version in the test and return if it's pre 5.4?

Yes -- do something like this in the individual test methods and/or the setUp() (if all tests can be skipped).

if (version_compare(PHP_VERSION, '5.4', 'lt')) {
    $this->markTestSkipped('Skipping; PHP 5.4 or greater is needed');
}

[tomphp]

I've added pre 5.4 version skip tests but I've also just noticed PR #4989 which should stop there being a problem with using traits. However still I think FileScanner::getClassNameInformation() failing to return information should be handled, if not by returning false maybe an exception should be thrown instead?

[weierophinney]

@tomphp I'm attempting to merge this to develop, as it implements new functionality for Zend\Code\Reflection. However, when I do so, I get a failure in ZendTest\Code\Reflection\ClassReflectionTest::testGetAnnotationsWithNoNameInformation -- it says:

Failed asserting that Zend\Code\Scanner\AnnotationScanner Object () is false.

Can you look into it, please?

[tomphp]

Hi @weierophinney,

I've taken a look and as I expected PR #4989 has been merged into the develop branch, this has resulted in the TokenArrayScanner::getClassNameInformation() method now successfully returning decent name information with traits so my test with a trait no longer fails.

After looking through the code it would appear that so long as TokenArrayScanner continues to work with classes, interfaces and traits, and PHP doesn't introduce a new class like structure, then it is impossible for this situation to occur.

However null and false are both valid return values for TokenArrayScanner::getClassNameInformation() even through they cannot be produced in the current situation due to previous tests.

Therefore my question is; should this be taken as a situation which can't arise because of the tight dependency on FileScanner and the other tests that occur in the method. And therefore my original error was down to a problem which was not caught in the TokenArrayScanner class rather than here.

Or should we say that because TokenArrayScanner::getClassNameInformation() could potentially return null/false (although not in this current usage) that it should be handled.

The only way I can think to test it would be to allow the injection of a FileScanner like so https://gist.github.com/6305709 rather than creating a new one inline but is that just overkill?

Any thoughts?

[weierophinney]

Or should we say that because TokenArrayScanner::getClassNameInformation() could potentially return null/false (although not in this current usage) that it should be handled.

I'd say if they can return something else, we should handle that. We often put in such defensive code even without testing it as a precautionary measure when testing it would be to roundabout.

[tomphp]

Ooops, very sorry about the delay here, I got distracted and forgot to respond. Shall I just remove the tests then and submit without them since they are just a guard or shall we just close the PR?

[weierophinney]

@tomphp Let's remove the tests, but keep the defensive code.

[tomphp]

@weierophinney I have removed the tests.

However I have been reading this rather fantastic book http://pragprog.com/book/lotdd/modern-c-programming-with-test-driven-development and discovered the override factory method pattern which might be a way to deal with this sort of situation.

This would involve refactoring Zend\Code\Reflection\ClassReflection::getAnnotations() like so:

    protected function createFileScanner($fileName)
    {
        return new FileScanner($fileName);
    }

    public function getAnnotations(AnnotationManager $annotationManager)
    {
        $docComment = $this->getDocComment();

        if ($docComment == '') {
            return false;
        }

        if ($this->annotations) {
            return $this->annotations;
        }

        $fileScanner       = $this->createFileScanner($this->getFileName());
        $nameInformation   = $fileScanner->getClassNameInformation($this->getName());

        if (!$nameInformation) {
            return false;
        }

        $this->annotations = new AnnotationScanner($annotationManager, $docComment, $nameInformation);

        return $this->annotations;
    }

Then the Zend\Code\Reflection\ClassReflection could be extended in the test suite like so:

use Zend\Code\Reflection\ClassReflection;

class ClassReflectionExposedFileScanner extends ClassReflection
{
    protected $fileScanner;

    public function setFileScanner(FileScanner $fileScanner)
    {
        $this->fileScanner = $fileScanner;
    }

    public function createFileScanner($filename)
    {
        return $this->fileScanner;
    }
}

This sub class could then be used in the test with an injected FileScanner mock.

Just a thought?

[weierophinney]

@tomphp The override-factory-pattern usage you show looks great! Want to include it in this one, or no?

[tomphp]

May as well do things the best as well as possible so I'll get this done tomorrow ;)

[tomphp]

Finally got there but made a bit of a mess of the commits... do you want me to squash and resubmit the PR?