PSR-0 annotation reader, try #2 #154

Merged
merged 32 commits into from Jun 30, 2012

Conversation

Projects
None yet
4 participants
Contributor

chx commented Jun 18, 2012

This is a radically different approach to the same goal: use less memory. See #152 for some discussion but most of it is irrelevant to this code that's why I opened a new pull request.

Note how most files are new and only Psr0Parser needed to change and even that only in very small ways.

Usage:

$parser = new Psr0Parser('path/to/your/lib', 'Drupal\\block\\Plugins\\owner\\type\\test');
$reader = new AnnotationReader();
var_dump($reader->getPropertyAnnotations($parser->getPropertyReflection('foo')));

If we decide that this is an approach worth pursuing, adding tests will be very easy because Doctrine is PSR-0 itself.

@chx chx commented on an outdated diff Jun 18, 2012

lib/Doctrine/Common/Annotations/Psr0ClassReflection.php
+ public function getName() {
+ return $this->psr0Parser->getClassName();
+ }
+
+ public function getDocComment() {
+ return $this->psr0Parser->getClassDoxygen();
+ }
+
+ public function getNamespaceName() {
+ return $this->psr0Parser->getNamespaceName();
+ }
+
+ public function getUseStatements() {
+ return $this->psr0Parser->getUseStatements();
+ }
+ static function export($argument, $return = FALSE) { throw new ReflectionException('Method not implemented'); }
@chx

chx Jun 18, 2012

Contributor

Yes this is ugly but otherwise PHP fatals. And, some of these could actually be implemented if we wanted to. I didn't :) How much we want this to be generally useable? I voted on the minimal viable approach even if the implementation would be an empty function body like for getExtension.

Member

stof commented Jun 18, 2012

Please follow the Doctrine coding standards (which are more or less PSR-2) for the code you contribute

Contributor

chx commented Jun 18, 2012

Sorry. I am old fashioned and still use two spaces. (I wonder however how we managed to end up with four when it's completely unreadable. Oh well.)

The bigger question is, are we happy with this direction? I will add tests, fix coding style, whatnot if we are. I already abandoned one branch so no point in wasting time with that if this won't be merged.

Thanks, that's very useful! Good that we have the use statements parsed already.

@akkie akkie and 2 others commented on an outdated diff Jun 18, 2012

lib/Doctrine/Common/Annotations/Psr0Parser.php
+ }
+ // No break.
+ case T_FUNCTION:
+ // The next string after function is the name, but
+ // there can be & before the function name so find the
+ // string.
+ while (($token = $this->next()) && $token[0] !== T_STRING);
+ $methodName = $token[1];
+ $this->methodDoxygen[$methodName] = $doxygen;
+ $doxygen = '';
+ break;
+ case T_EXTENDS:
+ $token = $this->next();
+ $this->parentClassName = $token[1];
+ if ($this->parentClassName[0] !== '\\') {
+ $this->parentClassName = $this->ns . '\\' . $this->parentClass;
@akkie

akkie Jun 18, 2012

Contributor

This doesn't work. You can also write:

<?php

namespace Doctrine\Common\Annotations;

use Doctrine\Common as CommonNS;

class Parser extends CommonNS\ParentClass {}

To resolve class names you need all use statements first and then you must resolve the FQN with the help of PHPs name resolution rules.
http://de.php.net/manual/en/language.namespaces.rules.php

@chx

chx Jun 18, 2012

Contributor

Thanks, that's very useful! Good that we have the use statements parsed already.

@stof

stof Jun 18, 2012

Member

but it is still not fixed

@beberlei beberlei and 2 others commented on an outdated diff Jun 18, 2012

lib/Doctrine/Common/Annotations/Psr0Parser.php
+ /**
+ * The parent PSR-0 Parser.
+ *
+ * @var \Doctrine\Common\Annotations\Psr0Parser
+ */
+ protected $parentPsr0Parser;
+
+ /**
+ * Parses a class residing in a PSR-0 hierarchy.
+ *
+ * @param string $includePath Base include path for class files.
+ * @param string $class The full class name.
+ * @param boolean $classAnnotationOptimize Only retrieve the class doxygen. Presumes there is only one statement per line.
+ */
+ public function __construct($includePath, $className, $classAnnotationOptimize = FALSE)
+ {
@beberlei

beberlei Jun 18, 2012

Owner

please move logic out of the constructor into another method. Constructors are only about assignments.

@chx

chx Jun 18, 2012

Contributor

That would require every single method to check whether the parser has ran already ran and call it if not. Either I inline that (three LoC per method) or create another method and call it from every other method. That's just ugly. This object is utterly useless if you don't run the parser. It's not like some part of the object is useful and some of it isn't -- the whole object is useless w/o the parser. I could argue that it indeed does just that: assigns the proper values to the class properties.

@stof

stof Jun 18, 2012

Member

the constructor should not be responsible of the logic as it would make instantiating the object very consuming even if you don't really use it later

@stof

stof Jun 18, 2012

Member

and no, it does not only assign values. It does all the logic of the parser

@chx

chx Jun 18, 2012

Contributor

It seems to me you are talking in terms of theory ("a constructor should do X") versus my rather practical argument -- namely, it would complicate the class needlessly because of how this specific class is. I fail to see why on earth would anyone instantiate an object without calling any methods on it. For this edge case you want me to complicate the class? If you insist, I of course will do it -- although I still haven't heard an affirmative "yes the approach is something we can live with".

@stof

stof Jun 18, 2012

Member

Well, the issue here is that the construcotr does the logic even if you never call a getter to get the value, making the initialization of the object a consuming task, thus making it unusable when your code follows the DI pattern

@chx

chx Jun 18, 2012

Contributor

Is this OK now?

@stof stof and 1 other commented on an outdated diff Jun 18, 2012

lib/Doctrine/Common/Annotations/Psr0Parser.php
+ *
+ * @var \Doctrine\Common\Annotations\Psr0Parser
+ */
+ protected $parentPsr0Parser;
+
+ /**
+ * Parses a class residing in a PSR-0 hierarchy.
+ *
+ * @param string $includePath Base include path for class files.
+ * @param string $class The full class name.
+ * @param boolean $classAnnotationOptimize Only retrieve the class doxygen. Presumes there is only one statement per line.
+ */
+ public function __construct($includePath, $className, $classAnnotationOptimize = FALSE)
+ {
+ $this->className = ltrim($className, '\\');
+ $this->fileName = $includePath . DIRECTORY_SEPARATOR . str_replace('\\', DIRECTORY_SEPARATOR, $className) . '.php';
@stof

stof Jun 18, 2012

Member

this would mean that all your libraries have to live in the same folder, which is absolutely not a requirement in PSR-0, and will be wrong as soon as you use some vendor libraries in your code (as you will not mix the repositories together)

@chx

chx Jun 18, 2012

Contributor

I do not get this. You can pass 'vendor/Symfony' or 'vendor/Doctrine' or whatever else. That's why $includePath is an argument.

@stof stof commented on an outdated diff Jun 18, 2012

lib/Doctrine/Common/Annotations/Psr0Parser.php
+
+ /**
+ * The parent PSR-0 Parser.
+ *
+ * @var \Doctrine\Common\Annotations\Psr0Parser
+ */
+ protected $parentPsr0Parser;
+
+ /**
+ * Parses a class residing in a PSR-0 hierarchy.
+ *
+ * @param string $includePath Base include path for class files.
+ * @param string $class The full class name.
+ * @param boolean $classAnnotationOptimize Only retrieve the class doxygen. Presumes there is only one statement per line.
+ */
+ public function __construct($includePath, $className, $classAnnotationOptimize = FALSE)
@stof

stof Jun 18, 2012

Member

booleans should be written lowercased according to the Doctrine coding standards

@stof stof commented on an outdated diff Jun 18, 2012

lib/Doctrine/Common/Annotations/Psr0Parser.php
+ break;
+ case T_EXTENDS:
+ $token = $this->next();
+ $this->parentClassName = $token[1];
+ if ($this->parentClassName[0] !== '\\') {
+ $this->parentClassName = $this->ns . '\\' . $this->parentClass;
+ }
+ break;
+ }
+ }
+ }
+ // Drop the tokens to save memory.
+ $this->tokens = array();
+ }
+
+ protected function getParentPsr0Parser() {
@stof

stof Jun 18, 2012

Member

please fix the coding standards. The curly brace should be on its own line for methods and classes

@stof stof and 1 other commented on an outdated diff Jun 18, 2012

...octrine/Common/Annotations/Psr0PropertyReflection.php
@@ -0,0 +1,25 @@
+<?php
+
+namespace Doctrine\Common\Annotations;
+
+use ReflectionProperty;
+use ReflectionException;
@stof

stof Jun 18, 2012

Member

ReflectionException is not used

@chx

chx Jun 18, 2012

Contributor

Yet :) it will look like Psr0ClassReflection. Same for Psr0MethodReflection.

@stof stof commented on an outdated diff Jun 18, 2012

...octrine/Common/Annotations/Psr0PropertyReflection.php
@@ -0,0 +1,25 @@
+<?php
+
+namespace Doctrine\Common\Annotations;
+
+use ReflectionProperty;
+use ReflectionException;
+
+class Psr0PropertyReflection extends ReflectionProperty {
+ function __construct($psr0Parser, $propertyName) {
@stof

stof Jun 18, 2012

Member

the visibility is missing for methods

@stof stof and 1 other commented on an outdated diff Jun 18, 2012

lib/Doctrine/Common/Annotations/PhpParser.php
@@ -58,6 +58,10 @@
*/
public function parseClass(\ReflectionClass $class)
{
+ if (method_exists($class, 'getUseStatements')) {
+ return $class->getUseStatements();
@stof

stof Jun 18, 2012

Member

you should use 4 spaces per level for the indentation, not 2 spaces or tabs

@chx

chx Jun 18, 2012

Contributor

I am aware. I am trying and most are fixed by now. Sorry :(

@stof

stof Jun 18, 2012

Member

this one is still not fixed

@chx

chx Jun 18, 2012

Contributor

Sorry -- I have rewritten that one after checking the file out from master for the TokenObject rewrite so the fix got lost :(

Member

stof commented Jun 18, 2012

I see an issue: the original PhpParser is a service object: you create one instance and you use it everywhere you need it. And for instance, the AnnotationReader create one PhpParser and uses it for all classes. Your Psr0Parser on the other hand is not such a service object: you can use it only once and have to create a new one for each class.

Member

stof commented Jun 18, 2012

and btw, it will not be usable as a drop-in replacement of the PhpParser: if you inject create a Psr0Parser, the code using the parser would still call the logic of the parent parser.
So your Psr0Parser cannot be used to read the annotations with the Doctrine annotation reader

Contributor

chx commented Jun 18, 2012

Sorry I do not understand why this can not be used with the Doctrine annotation reader. Please see the original post on a usage example -- it's working already or at least it seemed to me.

Member

stof commented Jun 18, 2012

Well, you use your parser as a service responsible to instantiate the reflection, not as a parser in the reader. So it is really weird to extend from the parser.

Contributor

chx commented Jun 18, 2012

Also, all the Psr0Parser has to do with PhpParser is code reuse , nothing else. It's not a drop-in replacement or anything like that. I just found that PhpParser already has next() and parseUseStatement() so I reused that.

Contributor

chx commented Jun 18, 2012

OK, I have moved the common code into a base class to extend from.

Contributor

chx commented Jun 18, 2012

Oh and if you don't like that both extend from TokenParser I can just store a TokenParser object in both.

Member

stof commented Jun 18, 2012

Btw, for the Cs issue, PHP-CS-Fixer should be able to fix most of them if you run it on these files

Contributor

chx commented Jun 19, 2012

I will write the parent class name resolver next. With the current code I can do $parser = new Psr0Parser(drupal_classloader()->getNameSpaces(), 'Drupal\\block\\Plugins\\owner\\type\\test'); where drupal_classloader returns Symfony\Component\ClassLoader\UniversalClassLoader and in fact the file finder code itself is a straight copy out of UniversalClassLoader.

Contributor

chx commented Jun 22, 2012

Please note that while PHP does not support annotations on class constants due to missing reflection / missing getDocComment on them, my code is now extremely close to be able to do that. Also, should we go PHP 5.4, the meaty parts of Psr0MethodReflection and Psr0PropertyReflection should be a trait. There is no difference between the two classes.

Contributor

chx commented Jun 22, 2012

Now, what's necessary to get this merged? I can convert more tests if you want but I think that's not really necessary. Tell me.

@stof stof and 1 other commented on an outdated diff Jun 22, 2012

lib/Doctrine/Common/Annotations/Psr0Parser.php
+ */
+ protected $includePaths;
+
+ /**
+ * TRUE if the caller only wants class annotations.
+ *
+ * @var boolean.
+ */
+ protected $classAnnotationOptimize;
+
+ /**
+ * TRUE when the parser has ran.
+ *
+ * @var boolean
+ */
+ protected $parsed = FALSE;
@stof

stof Jun 22, 2012

Member

coding standard issue: booleans should be written lowercased (same everywhere in your code)

@chx

chx Jun 22, 2012

Contributor

sniff. I ran php-cs-fix. Fixing!

@stof stof and 1 other commented on an outdated diff Jun 22, 2012

...trine/Tests/Common/Annotations/AbstractReaderTest.php
- $this->assertTrue($joinTableAnnot->joinColumns[0] instanceof DummyJoinColumn);
- $this->assertTrue($joinTableAnnot->inverseJoinColumns[0] instanceof DummyJoinColumn);
- $this->assertEquals('col1', $joinTableAnnot->joinColumns[0]->name);
- $this->assertEquals('col2', $joinTableAnnot->joinColumns[0]->referencedColumnName);
- $this->assertEquals('col3', $joinTableAnnot->inverseJoinColumns[0]->name);
- $this->assertEquals('col4', $joinTableAnnot->inverseJoinColumns[0]->referencedColumnName);
-
- $dummyAnnot = $reader->getMethodAnnotation($class->getMethod('getField1'), 'Doctrine\Tests\Common\Annotations\DummyAnnotation');
- $this->assertEquals('', $dummyAnnot->dummyValue);
- $this->assertEquals(array(1, 2, 'three'), $dummyAnnot->value);
-
- $dummyAnnot = $reader->getPropertyAnnotation($class->getProperty('field1'), 'Doctrine\Tests\Common\Annotations\DummyAnnotation');
- $this->assertEquals('fieldHello', $dummyAnnot->dummyValue);
-
- $classAnnot = $reader->getClassAnnotation($class, 'Doctrine\Tests\Common\Annotations\DummyAnnotation');
- $this->assertEquals('hello', $classAnnot->dummyValue);
@stof

stof Jun 22, 2012

Member

why removing the old tests ? you should add new tests for your code, not remove existing tests

@chx

chx Jun 22, 2012

Contributor

I have indented them, that's all. I didn't remove them. I reused them.

@stof

stof Jun 22, 2012

Member

hmm, you should use a @dataProvider annotation instead. Currently, if one of the implementation fails, it will be really hard to debug the ailing test as assertions are used twice in the same test. And if the first implementation fails, the tests will not be run for the other implementation (as it is a single test).

<?php

public function getReflectionClass()
{
    // initialize the Psr0Parser

    return array(
        'native' => array(new ReflectionClass($className)),
        'parser' => array($psr0Parser->getClassReflection()),
    );
}

/**
 * @dataProvider getReflectionClass
 */
public function testAnnotations($class)
{
    // the existing method without change except the instantiation of the reflection class.
}

This way, the 2 implementations will actually be 2 tests, which can fail independently. And PHPUnit will give us the name of the dataset when it fails (which is why I specified a key as the objects are not fine to identify the dataset)

@chx chx commented on an outdated diff Jun 22, 2012

...trine/Tests/Common/Annotations/AbstractReaderTest.php
- $dummyAnnot = $reader->getPropertyAnnotation($class->getProperty('field1'), 'Doctrine\Tests\Common\Annotations\DummyAnnotation');
- $this->assertEquals('fieldHello', $dummyAnnot->dummyValue);
-
- $classAnnot = $reader->getClassAnnotation($class, 'Doctrine\Tests\Common\Annotations\DummyAnnotation');
- $this->assertEquals('hello', $classAnnot->dummyValue);
+ $className = 'Doctrine\Tests\Common\Annotations\DummyClass';
+ $testsRoot = substr(__DIR__, 0, -strlen(__NAMESPACE__) - 1);
+ $paths = array(
+ 'Doctrine\\Tests' => array($testsRoot),
+ );
+ $psr0Parser = new Psr0Parser($className, $paths);
+ $reflections = array(
+ new ReflectionClass($className),
+ $psr0Parser->getClassReflection(),
+ );
+ foreach ($reflections as $class) {
@chx

chx Jun 22, 2012

Contributor

The code upwards is the new code. Down from here is the actual test which I have not touched.

Contributor

chx commented Jun 25, 2012

Should TokenParser be injected instead of extended? I am not sure because that would expose methods currently not public. If it stays like this at least I guess we want to make it abstract? All in all, what's needed to get this merged?

@beberlei beberlei commented on an outdated diff Jun 25, 2012

lib/Doctrine/Common/Annotations/TokenParser.php
+ */
+ protected $numTokens = 0;
+
+ /**
+ * The current array pointer.
+ *
+ * @var int
+ */
+ protected $pointer = 0;
+
+ /**
+ * Gets the next non whitespace and non comment token.
+ *
+ * @return array The token if exists, null otherwise.
+ */
+ protected function next($skipDoxygen = TRUE)
@beberlei

beberlei Jun 25, 2012

Owner

what does this mean? skipDoxygen?

@stof stof commented on an outdated diff Jun 25, 2012

lib/Doctrine/Common/Annotations/TokenParser.php
+ * The current array pointer.
+ *
+ * @var int
+ */
+ protected $pointer = 0;
+
+ public function __construct($contents) {
+ $this->tokens = token_get_all($contents);
+ $this->numTokens = count($this->tokens);
+ $this->pointer = 0;
+ }
+
+ /**
+ * Gets the next non whitespace and non comment token.
+ *
+ * @param $skipDoxygen
@stof

stof Jun 25, 2012

Member

you should name it docComment, not doxygen, to give it its propername

@stof

stof Jun 25, 2012

Member

and btw, the phpdoc and the code are not in sync as you renamed it.

@stof stof commented on an outdated diff Jun 25, 2012

lib/Doctrine/Common/Annotations/TokenParser.php
+ */
+ protected $tokens;
+
+ /**
+ * The number of tokens.
+ *
+ * @var int
+ */
+ protected $numTokens = 0;
+
+ /**
+ * The current array pointer.
+ *
+ * @var int
+ */
+ protected $pointer = 0;
@stof

stof Jun 25, 2012

Member

you should use private properties here. Making them protected would mean that they are potential extension points, which would require taking care of changes done on them. For instance, if the PhpParser was using protected methods previously (and not be final), moving the methods to an external class would have been a BC break for some users whereas now it is fine to move them out of the parser

@stof stof commented on an outdated diff Jun 25, 2012

lib/Doctrine/Common/Annotations/TokenParser.php
+
+ /**
+ * The number of tokens.
+ *
+ * @var int
+ */
+ protected $numTokens = 0;
+
+ /**
+ * The current array pointer.
+ *
+ * @var int
+ */
+ protected $pointer = 0;
+
+ public function __construct($contents) {
@stof

stof Jun 25, 2012

Member

CS issue here for the curly brace :)

@stof stof commented on an outdated diff Jun 25, 2012

lib/Doctrine/Common/Annotations/TokenParser.php
+ * @var int
+ */
+ protected $pointer = 0;
+
+ public function __construct($contents) {
+ $this->tokens = token_get_all($contents);
+ $this->numTokens = count($this->tokens);
+ $this->pointer = 0;
+ }
+
+ /**
+ * Gets the next non whitespace and non comment token.
+ *
+ * @param $skipDoxygen
+ * If TRUE then doxygen is considered a comment and skipped.
+ * If FALSE then only whitespace and normal comment is skipped.
@stof

stof Jun 25, 2012

Member

typo: are skipped

@stof stof and 1 other commented on an outdated diff Jun 25, 2012

lib/Doctrine/Common/Annotations/TokenParser.php
+ }
+
+ /**
+ * Get all use statements.
+ *
+ * @param string $namespaceName The namespace name of the reflected class.
+ * @return array A list with all found use statements.
+ */
+ public function parseUseStatements($namespaceName)
+ {
+ $statements = array();
+ while (($token = $this->next())) {
+ if ($token[0] === T_USE) {
+ $statements = array_merge($statements, $this->parseUseStatement());
+ continue;
+ } else if ($token[0] !== T_NAMESPACE || $this->parseNamespace() != $namespaceName) {
@stof

stof Jun 25, 2012

Member

you could simply use if instead of elseif here as the previous if leaves the execution of this loop.

@chx

chx Jun 25, 2012

Contributor

Hey, not my code :) sure, fixing it.

@stof stof commented on an outdated diff Jun 25, 2012

lib/Doctrine/Common/Annotations/TokenParser.php
+ public function parseNamespace()
+ {
+ $name = '';
+ while (($token = $this->next()) && ($token[0] === T_STRING || $token[0] === T_NS_SEPARATOR)) {
+ $name .= $token[1];
+ }
+
+ return $name;
+ }
+
+ /**
+ * Get the class name.
+ *
+ * @return string The foundclass name.
+ */
+ public function parseClass() {
@stof

stof Jun 25, 2012

Member

CS issue here too

@stof stof commented on the diff Jun 25, 2012

lib/Doctrine/Common/Reflection/StaticReflectionClass.php
@@ -0,0 +1,86 @@
+<?php
+
@stof

stof Jun 25, 2012

Member

please add the Doctrine license header

@stof stof and 1 other commented on an outdated diff Jun 25, 2012

lib/Doctrine/Common/Reflection/StaticReflectionClass.php
@@ -0,0 +1,86 @@
+<?php
+
+namespace Doctrine\Common\Reflection;
+
+use ReflectionClass;
+use ReflectionException;
+
+class StaticReflectionClass extends ReflectionClass
+{
+ public function __construct($psr0Parser)
+ {
+ $this->psr0Parser = $psr0Parser;
@stof

stof Jun 25, 2012

Member

you should rename the property, and declare it (undeclared properties have a performance impact as PHP 5.4 optimized the property access for known properties)

@chx

chx Jun 25, 2012

Contributor

Yes I know about that in PHP 5.4 and I really tried hard to define all, sorry skipping this one.

@stof stof commented on an outdated diff Jun 25, 2012

...Doctrine/Common/Reflection/StaticReflectionMethod.php
+{
+ /**
+ * The PSR-0 parser object.
+ *
+ * @var StaticReflectionParser
+ */
+ protected $staticReflectionParser;
+
+ /**
+ * The name of the method.
+ *
+ * @var string
+ */
+ protected $methodName;
+
+ public function __construct($StaticReflectionParser, $methodName)
@stof

stof Jun 25, 2012

Member

the first letter of the variable name should be lowercased

@stof stof and 1 other commented on an outdated diff Jun 25, 2012

...Doctrine/Common/Reflection/StaticReflectionParser.php
+ $fullySpecified = true;
+ }
+ }
+ }
+ if (!$fullySpecified) {
+ $this->parentClassName = '\\' . $this->ns . '\\' . $this->parentClassName;
+ }
+ break;
+ }
+ }
+ }
+ }
+
+ protected function findClassFile($includePaths, $namespace, $classShortName)
+ {
+ $normalizedClass = str_replace('\\', DIRECTORY_SEPARATOR, $namespace).DIRECTORY_SEPARATOR.$classShortName.'.php';
@stof

stof Jun 25, 2012

Member

this does not follow PSR-0. The handling of underscores in the short class name is missing

@chx

chx Jun 25, 2012

Contributor

Well, that's a bit unfortunate but that would require an additional parameter for finding the underscored roots like UniversalClassLoader has registerNamespace and registerPrefix. Can this be a followup?

@stof

stof Jun 25, 2012

Member

there is no need to do any special logic. UniversalClassLoader was adding an artificial distinction between namespaced classes and unnamespaced ones whereas PSR-0 does not. And Symfony 2.1 introduced a ClassLoader which has a simpler API by avoiding this distinction (just like Composer avoids it too)

@stof stof and 1 other commented on an outdated diff Jun 25, 2012

...Doctrine/Common/Reflection/StaticReflectionParser.php
+ if (strpos($namespace, $ns) === 0) {
+ foreach ($dirs as $dir) {
+ $file = $dir.DIRECTORY_SEPARATOR.$normalizedClass;
+ if (is_file($file)) {
+ return $file;
+ }
+ }
+ }
+ }
+ }
+
+ protected function getParentStaticReflectionParser()
+ {
+ if (empty($this->parentStaticReflectionParser)) {
+ $class = get_class($this);
+ $this->parentStaticReflectionParser = new $class($this->parentClassName, $this->includePaths);
@stof

stof Jun 25, 2012

Member

$this->parentStaticReflectionParser = new static($this->parentClassName, $this->includePaths);

@chx

chx Jun 25, 2012

Contributor

That's spiffy! http://php.net/manual/en/language.oop5.late-static-bindings.php only comments mention that construct. (I will fix it tomorrow)

@stof stof commented on an outdated diff Jun 25, 2012

...Doctrine/Common/Reflection/StaticReflectionParser.php
+ public function getUseStatements()
+ {
+ $this->parse();
+
+ return $this->useStatements;
+ }
+
+ /**
+ * Get doxygen.
+ *
+ * @param string $type class, property or method.
+ * @param string $name Name of the property or method, not needed for class.
+ *
+ * @return string the doxygen or empty string if none.
+ */
+ public function getDoxygen($type = 'class', $name = '')
@stof

stof Jun 25, 2012

Member

this should be renamed to getDocComment

@stof stof and 1 other commented on an outdated diff Jun 25, 2012

...trine/Tests/Common/Annotations/AbstractReaderTest.php
{
- $reader = $this->getReader();
+ $className = 'Doctrine\Tests\Common\Annotations\DummyClass';
+ $testsRoot = substr(__DIR__, 0, -strlen(__NAMESPACE__) - 1);
+ $paths = array(
+ 'Doctrine\\Tests' => array($testsRoot),
+ );
+ $staticReflectionParser = new StaticReflectionParser($className, $paths);
+ return array(
+ 'native' => array(new ReflectionClass($className)),
+ 'parser' => array($staticReflectionParser->getReflectionClass()),
@stof

stof Jun 25, 2012

Member

I would name it static instead of parser. And you should rename the $psr0Parser variable

@chx

chx Jun 25, 2012

Contributor

Renamed both.

Contributor

chx commented Jun 28, 2012

So now I have the interface we discussed. Anything else?

@stof stof commented on an outdated diff Jun 28, 2012

lib/Doctrine/Common/Reflection/ReflectionInterface.php
+ * OWNER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL,
+ * SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT
+ * LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE,
+ * DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY
+ * THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
+ * (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE
+ * OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
+ *
+ * This software consists of voluntary contributions made by many individuals
+ * and is licensed under the MIT license. For more information, see
+ * <http://www.doctrine-project.org>.
+ */
+
+namespace Doctrine\Common\Reflection;
+
+interface ReflectionInterface {
@stof

stof Jun 28, 2012

Member

the curly brace should be on its own line (PHP-CS-Fixer FTW 😄)

@stof stof commented on an outdated diff Jun 28, 2012

lib/Doctrine/Common/Reflection/StaticReflectionClass.php
+
+namespace Doctrine\Common\Reflection;
+
+use ReflectionClass;
+use ReflectionException;
+
+class StaticReflectionClass extends ReflectionClass
+{
+ /**
+ * The static reflection parser object.
+ *
+ * @var StaticReflectionParser
+ */
+ private $staticReflectionParser;
+
+ public function __construct($staticReflectionParser)
@stof

stof Jun 28, 2012

Member

you should typehint the argument

@stof stof commented on an outdated diff Jun 28, 2012

lib/Doctrine/Common/Reflection/StaticReflectionClass.php
+ public function getDocComment()
+ {
+ return $this->staticReflectionParser->getDocComment();
+ }
+
+ public function getNamespaceName()
+ {
+ return $this->staticReflectionParser->getNamespaceName();
+ }
+
+ public function getUseStatements()
+ {
+ return $this->staticReflectionParser->getUseStatements();
+ }
+
+ public function getMethod($name) {
@stof

stof Jun 28, 2012

Member

CS issue

@stof stof commented on an outdated diff Jun 28, 2012

...Doctrine/Common/Reflection/StaticReflectionMethod.php
+{
+ /**
+ * The PSR-0 parser object.
+ *
+ * @var StaticReflectionParser
+ */
+ protected $staticReflectionParser;
+
+ /**
+ * The name of the method.
+ *
+ * @var string
+ */
+ protected $methodName;
+
+ public function __construct($staticReflectionParser, $methodName)
@stof

stof Jun 28, 2012

Member

you should typehint the argument

@stof stof commented on an outdated diff Jun 28, 2012

...Doctrine/Common/Reflection/StaticReflectionParser.php
+ if ($alias == $prefix) {
+ $this->parentClassName = '\\' . $use . $postfix;
+ $fullySpecified = true;
+ }
+ }
+ }
+ if (!$fullySpecified) {
+ $this->parentClassName = '\\' . $this->ns . '\\' . $this->parentClassName;
+ }
+ break;
+ }
+ }
+ }
+ }
+
+ public function findFile()
@stof

stof Jun 28, 2012

Member

@beberlei asked you to move this logic to another class implementing an interface and injected inside the parser, so that people can replace it by another implementation if they don't follow PSR-0

@stof stof commented on an outdated diff Jun 28, 2012

lib/Doctrine/Common/Reflection/ReflectionInterface.php
+ * OWNER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL,
+ * SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT
+ * LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE,
+ * DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY
+ * THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
+ * (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE
+ * OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
+ *
+ * This software consists of voluntary contributions made by many individuals
+ * and is licensed under the MIT license. For more information, see
+ * <http://www.doctrine-project.org>.
+ */
+
+namespace Doctrine\Common\Reflection;
+
+interface ReflectionInterface
@stof

stof Jun 28, 2012

Member

I'm not sure about the name of this interface. It looks more like a ReflectionProviderInterface

@stof stof commented on an outdated diff Jun 28, 2012

...Doctrine/Common/Reflection/StaticReflectionParser.php
+ */
+ protected $classAnnotationOptimize;
+
+ /**
+ * TRUE when the parser has ran.
+ *
+ * @var boolean
+ */
+ protected $parsed = false;
+
+ /**
+ * The namespace of the class
+ *
+ * @var string
+ */
+ protected $ns = '';
@stof

stof Jun 28, 2012

Member

could you name it namespace instead of ns ? It would be more readable

@stof stof commented on an outdated diff Jun 28, 2012

...Doctrine/Common/Reflection/StaticReflectionParser.php
+ * @var \Doctrine\Common\Annotations\StaticReflectionParser
+ */
+ protected $parentStaticReflectionParser;
+
+ /**
+ * Parses a class residing in a PSR-0 hierarchy.
+ *
+ * @param string $class
+ * The full, namespaced class name.
+ * @param ClassFinder $finder
+ * A ClassFinder object which finds the class.
+ * @param boolean $classAnnotationOptimize
+ * Only retrieve the class docComment. Presumes there is only one
+ * statement per line.
+ */
+ public function __construct($className, $finder, $classAnnotationOptimize = FALSE)
@stof

stof Jun 28, 2012

Member

false should be written lowercased (same for true and null but I'm not sure they appear in the PR). The PHP-CS-Fixer does not handle this currently so it has to be fixed by hand

beberlei merged commit d2f8a36 into doctrine:master Jun 30, 2012

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment