mamuz / PhpDependencyAnalysis

Static code analysis to find violations in a dependency graph
http://mamuz.github.io/PhpDependencyAnalysis/
MIT License
564 stars 45 forks source link

Catch the exception thrown if a DocBlock cannot be read. #1

Closed otruffer closed 10 years ago

otruffer commented 10 years ago

If

An \InvalidArgumentException was thrown and not catched which broke the whole process of building dependencies.

The solution in this pull request tries to ignore the Exception as it is a problem of the phpDocumentor library. At the same time it logs the occurrence. This looks quite ugly if used in the command line though. Do you have any suggestions on that?

coveralls commented 10 years ago

Coverage Status

Coverage decreased (-0.12%) when pulling 7829dbb30dce05bce84912bd4cf4e1ffd5772ece on otruffer:tag_visitor_exception_fix into 3b277d42d916ca5e36e9e34a1ae17ad446724d33 on mamuz:master.

mamuz commented 10 years ago

Hi,

thanks a lot for the first PR.

Please catch Exception instead of InvalidArgumentException, and throw a new PhpParser\Error, like..

} catch (\Exception $e) {
    throw new Error($e->getMessage(), $node->getLine());
}

These type of exceptions are catched to skip analysis for current php-file. After analysis you will get a list of catched errors on command line.

See PhpDA\Parser\Visitor\Required for instance.

..and would be nice to have a test to cover this bug.

coveralls commented 10 years ago

Coverage Status

Coverage increased (+0.0%) when pulling 73114b1954019aee42bc5a8ca64fe4a0d7a9841b on otruffer:tag_visitor_exception_fix into 3b277d42d916ca5e36e9e34a1ae17ad446724d33 on mamuz:master.

otruffer commented 10 years ago

Hi Thanks for the quick response. I added the Test and implemented it the way you mentioned above. If it is skipping the whole file it seems like a slight overkill as it's only a problem with the DocBlock. And the DocBlock is not per se invalid but PhpDocumentor has a bug when encountering an @ element with a following whitespace anywhere in the doc.

mamuz commented 10 years ago

Thanks for contributing, i will merge it. And you are right. We should add a new type of exception for that. Maybe Parser\Warning as a derivate from Parser\Error. Those exceptions should be collectable at the ADT entity and should be also listed at the end of analysis.