gbecchio / addendum

Automatically exported from code.google.com/p/addendum
GNU Lesser General Public License v2.1
0 stars 0 forks source link

Name collision with PHPUnit's "Target" class #21

Open GoogleCodeExporter opened 8 years ago

GoogleCodeExporter commented 8 years ago
What steps will reproduce the problem?
1. Use a @Target annotated class besides a PHPUnit test using its Target class.

What is the expected output? What do you see instead?
PHP fails when redeclaring "Target" over another already declared "Target".

What version of the product are you using? On what operating system?
0.4.0, Linux

Please provide any additional information below.

The correct fix might be to use namespaces to avoid clashes, but it's very 
tricky (tried it).
As a fix I've renamed @Target to @AnnotationTarget locally.

Original issue reported on code.google.com by ramon.p...@gmail.com on 20 Aug 2010 at 9:52

GoogleCodeExporter commented 8 years ago
I have a crazy opinion on this. I don't reckon "Target" should be an annotation 
- it's nice and neat, and a good illustration of exactly what annotations are 
all about, but I think it's waaaay faster just to use flags. For something that 
emulates a feature that should really be language level, I think performance 
has to win over sweetness, although it _is_ kinda sweet using an annotation to 
annotate an annotation :)

I've been doing an awful lot of hacking on Addendum the last few days to see 
what I can do to get the speed up - this was one area (apart from the massive 
one that made almost all the difference - caching) that helped quite a bit. I 
can sit down and do better numbers if you like.

I can see a few caveats that would potentially make this change undesirable:

 * Firstly, it's a massive breaking change for anyone using the current method of restricting target using the Target() annotation, although a few stern warnings and a bump from 0.4 to 0.5 might mitigate that somewhat.
 * I also have not looked in to how to test nested targets, though I suppose adding another flag and passing it in in NestedAnnotationMatcher would work
 * Lots of doc updates would be needed too

My apologies if the below code ideas aren't perfect or even working - I'm kind 
of going off the top of my head, but if there is any interest in going down 
this road I will create a nice patch with proper tests (and hopefully as few 
bogus "newline only" changes as possible!).

You have to tweak ReflectionAnnotated* a bit to have a $target property:

    class ReflectionAnnotatedClass extends ReflectionClass {
        public $target=Annotation::TARGET_CLASS;
        // ...
    }

    class ReflectionAnnotatedMethod extends ReflectionMethod {
        public $target=Annotation::TARGET_METHOD;
        // ...
    }

    class ReflectionAnnotatedProperty extends ReflectionProperty {
        public $target=Annotation::TARGET_PROPERTY;
        // ...
    }

Then change the calls to instantiateAnnotation that pass a target:

    $this->instantiateAnnotation($class, $params, $targetReflection->target);

And finally, add the extra bits to Annotation:

    class Annotation {
        const TARGET_METHOD=1;
        const TARGET_CLASS=2;
        const TARGET_PROPERTY=4;
        const TARGET_ALL=7;

        public static $targetNames = array(
            self::TARGET_METHOD=>'method',
            self::TARGET_CLASS=>'class',
            self::TARGET_PROPERTY=>'property',
        );  

        public function getTarget() {
            return self::TARGET_ANY;
        }

        private function checkTargetConstraints($sourceTarget) {
            if ($sourceTarget != null && !($sourceTarget & $this->getTarget())) {
            $type = self::$targetNames[$sourceTarget];
                throw new Exception("Annotation '{$class}' is not valid on target type '{$type}'");
            }
        }
    }

So when you declare annotations, you just set the target using an OR:

    class Pants extends Annotation {
        public function getTarget() {
            return self::TARGET_METHOD | self::TARGET_CLASS;
        }
    }

Oh, and there's also another way I just figured out for how to make the changes 
to Annotation without wrecking the very good exception text created in the 
current implementation of checkTargetConstraints - the Annotation class itself 
can simply access $reflection->target, rather than it being passed through to 
instantiateAnnotation.

Original comment by shabbyr...@gmail.com on 10 Jan 2011 at 4:49

GoogleCodeExporter commented 8 years ago
Oh, I forgot to mention that the reason why I used a getTarget method and not a 
property was that PHP doesn't let you use bitwise OR in property declarations:

    // Bzzzzt! Nope!
    class Pants extends Annotation {
        public $target = self::TARGET_METHOD | self::TARGET_CLASS;
    }

Original comment by shabbyr...@gmail.com on 10 Jan 2011 at 4:51

GoogleCodeExporter commented 8 years ago
Hang on, I just realised... wouldn't just using @Annotation_Target work?

Original comment by shabbyr...@gmail.com on 11 Jan 2011 at 2:37

GoogleCodeExporter commented 8 years ago
If you look at the issue date, class name was already changd to 
Annotation_Target long ago.

About optimizations, aka the root of all evil, I'd leave it to the instancing 
classes and keep Addendum doing one thing, doing it well and keep being easy to 
read. ;)

Original comment by ramon.p...@gmail.com on 11 Jan 2011 at 2:50