Added the support of wildcard in ignore annotation (addGlobalIgnoredName) #156

Closed
wants to merge 2 commits into
from

Projects

None yet

5 participants

@ezimuel
ezimuel commented Jun 22, 2012

Using the addGlobalIgnoredName() of the AnnotationReader we can specify annotation tags that should not be considered by Doctrine. The ignored tags must be specified as strings. If we have tags specified by namespace we need to list all the possible sub-namespaces in addGlobalIgnoredName().

This PR add the support of wildcard in addGlobalIgnoredName(), for instance you can specify to ignore all the sub-namespaces of an annotation tag using the "*" syntax:
use Doctrine\Common\Annotations\AnnotationReader;
$reader = new AnnotationReader();
AnnotationReader::addGlobalIgnoredName('Foo*');
This example will exclude all the Foo annotation tag and all the sub-namespaces.
Here you can find a complete example on how to use it: https://gist.github.com/2972974

I think this PR can be useful to continue to use the annotation of Doctrine with other PHP projects that use different annotation systems. Please note that the PR take care about the parsing performance of Doctrine, the wildcard syntax is activated only if there are annotations specified.

@Ocramius
Member

Apart of the CS issues, which I'll eventually comment later, I would first like to see if the approach of re-introducing the "ignore non imported annotations" has a better feedback. Let's keep this hanging at least till next week :)

Otherwise, the PR's functionality is ok imo :)

@weierophinney

@ocramius I had @ezimuel go this approach initially as it's the documented approach for whitelisting annotations within Doctrine -- wildcard seemed a good extension of that idea. The problem I see with the "ignore non-imported annotations" is that it can mean typos in annotations leads to the parser silently ignoring something you've written; at least with the whitelist, you'll still get exceptions raised for anything not on the whitelist.

@Ocramius
Member

@weierophinney yeah, as I've already told Enrico on IRC I like this one :)

@schmittjoh
Member

This only works for the top level namespace, no?

@Ocramius
Member

@schmittjoh seems like that

@stof stof commented on the diff Jun 22, 2012
lib/Doctrine/Common/Annotations/AnnotationReader.php
@@ -71,12 +71,22 @@
);
/**
+ * Specify to ignore wildcard namespace (*) in annotation
+ *
+ * @var boolean
+ */
+ private static $ignoreWildcardNamespace = true;
@stof
stof Jun 22, 2012 Member

the naming is crappy. According to the implementation, it will be set to false to ignore them, and to true to disable the feature.

@Ocramius
Ocramius Jun 22, 2012 Member

@stof skip it for now :)

@stof
stof Jun 22, 2012 Member

well, in fact, you should probably keep the name, but change the case where you use true and false

@stof stof commented on the diff Jun 22, 2012
lib/Doctrine/Common/Annotations/AnnotationReader.php
@@ -145,7 +155,8 @@ public function getClassAnnotations(ReflectionClass $class)
$this->parser->setTarget(Target::TARGET_CLASS);
$this->parser->setImports($this->getImports($class));
$this->parser->setIgnoredAnnotationNames($this->getIgnoredAnnotationNames($class));
-
+ $this->parser->setIgnoreWildcardNamespace(self::$ignoreWildcardNamespace);
+
@stof
stof Jun 22, 2012 Member

please don't add trailing whitespaces

@stof stof and 1 other commented on an outdated diff Jun 22, 2012
...trine/Tests/Common/Annotations/AbstractReaderTest.php
@@ -326,6 +326,21 @@ public function testInvalidAnnotationButIgnored()
$this->assertCount(0, $reader->getPropertyAnnotations($class->getProperty('foo')));
}
+ public function testSupportWildcardIgnoredAnnotation()
+ {
+ $reader = $this->getReader();
+ AnnotationReader::addGlobalIgnoredName('Foo\\*');
+
+ $class = new \ReflectionClass('Doctrine\Tests\Common\Annotations\Fixtures\IgnoreAnnotationWithWildcard');
+ $annots = $reader->getClassAnnotations($class);
+
+ if (!empty($annots)) {
+ $this->assertEquals('foo', $annots[0]->bar);
+ } else {
+ $this->markTestSkipped('Skipped support wildcard ignored annotation');
@stof
stof Jun 22, 2012 Member

I don't understand why you have a test skipped conditionally here

@ezimuel
ezimuel Jun 25, 2012

Sorry, I forgot to remove this code, this was a test. I fixed it and moved this test in AnnotationReaderTest.php instead of AbstractReaderTest.php.

@stof
Member
stof commented Jun 22, 2012

I also vote 👍 for this approach, which is better than disabling the validation of annotation when using external libraries.

@Ocramius Ocramius referenced this pull request in zendframework/zendframework Jun 23, 2012
Closed

[WIP] - Reuse doctrine common annotations #1581

@schmittjoh
Member

I'd like to add a separate map with ignored namespaces instead of adding this to the names section and a flag.

This would also better allow to validate that not something like Foo\Bar\* is passed which would not work. Also be aware that the check is currently done on the name as it is used in the file, not the actually resolved class name.


use Foo\Bar as Baz;

/** @Baz\Annotation */
class MyClass { }

AnnotationReader::addGlobalIgnoredName('Foo\\*'); // would not ignore above annotation
@stof
Member
stof commented Jun 25, 2012

@schmittjoh currently, ignored annotations also apply on the unresolved name AFAIK

@schmittjoh
Member

@stof, yes, that is intended, but this feature is a bit different.

@Ocramius
Member

If this is still needed, we should rather move it to https://github.com/doctrine/annotations

@Ocramius Ocramius closed this Dec 23, 2013
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment