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

Closed
wants to merge 27 commits into
from

Projects

None yet

2 participants

@tomphp
tomphp commented Jul 23, 2013

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
tomphp commented Jul 23, 2013

Update: Also done this for MethodReflection

@weierophinney
Member

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

@tomphp
tomphp commented Aug 20, 2013

@weierophinney Apologies, unit tests now included.

@tomphp
tomphp commented Aug 20, 2013

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
Member

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
tomphp commented Aug 20, 2013

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
Member

@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
tomphp commented Aug 22, 2013

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
Member

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
tomphp commented Sep 6, 2013

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
Member

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

@tomphp
tomphp commented Sep 13, 2013

@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
Member

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

@tomphp
tomphp commented Oct 22, 2013

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

@tomphp
tomphp commented Oct 25, 2013

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

@weierophinney weierophinney added a commit that closed this pull request Oct 28, 2013
@weierophinney weierophinney Merge branch 'hotfix/4874'
Close #4874
52c0bc9
@weierophinney weierophinney added a commit that referenced this pull request Oct 28, 2013
@weierophinney weierophinney Merge branch 'hotfix/4874' into develop
Forward port #4874

Conflicts:
	tests/ZendTest/Code/Reflection/MethodReflectionTest.php
ca2199b
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment