[DCOM-56] Remote autoloading from annotations completly, replacing it with its own loading mechanism Created: 01/Jul/11  Updated: 02/Jul/11  Resolved: 02/Jul/11

Status: Resolved
Project: Doctrine Common
Component/s: Annotations
Affects Version/s: 2.1
Fix Version/s: 2.1

Type: Improvement Priority: Major
Reporter: Benjamin Eberlei Assignee: Benjamin Eberlei
Resolution: Fixed Votes: 0
Labels: None


 Description   

This is a mail from me to the symfony-dev mailing-list:

I am sorry it is very late to bring this up again, but given the nasty problems I faced today with the way annotationreader works with autoload = true I have to share them (and blow of some steam). While i guess its to late to change this again before 2.0, we might still have to discuss to go away from autoload = true for 2.1. Now for the reasons:

AnnotationReader with autoload = true (which Symfony2 uses) will not only require a silent autoloader, it requires ALL autoloaders used to be silent. While this is the case for the Symfony Universal loader its not the case for the Doctrine one, and not for many others - and its not even a PSR-0 requirement. For a simple reason: Supporting silent failure means using file_exists, means a file stat, which means apc.stat = 0 is useless. While I don't care so much about it in Doctrine context, because the AnnotationReader default is NOT to autoload annotations this will cause problems for Symfony: Not every library in any Symfony2 app will be included through a silent autoloader. That means given the context users might run into problems using annotations that they have absolutely no way of fixing. And since the AnnotationReader does not know this upfront, potential failures become very real issue.

Example: I use SecurityExtraBundle and happen to have my SuperDuperLibrary XYZ. That library was developed by my company and contains tons of important business logic but unfortunately uses a non-silent autoloader (for whatever reasons). However i use Symfony to secure access to it by generating secure proxies. Now what happens if an annotation reader runs over that library? Because the current namespace is always considered, for every @var _NAMESPACE."
var" is class_exists'ed and then your code fatal errors. I didn't see this problem before, because i don't use annotations myself so much and always in the BC Doctrine 2.0.x way and that slipped my mind. Same goes for Validation with Annotations on external code, or maybe in the future services definitions through annotations.

All this trouble we are getting into with autoloading annotations is just because we wanted to validate that the annotations used actually exist. But since we changed to a use/import approach rather then re-using namespaces we don't even have the clashing problem anymore that got us into the trouble with class_exists before. The autoloading however also got us and users into other problems:

1. We have to maintain a rather large map of "blacklisted" annotations that never throw failure exceptions because they are actually used as doc documentations. As with blacklists this is never a complete list.
2. Users with intensive doc-block usage are punished by Symfony having to maintain their own blacklist of annotations that never throw exceptions so that their code actually works.
3. Libraries have to handle the case that a reader is either using autoloading or not through special code.

While I do think it would have been nice to offer validation for annotation this is a task that should be put on IDEs and developers, since it turns out that its not possible flawlessly within the library itself. The AnnotationReader should always use class_exists($name, false). Docblocks are still comments and the code shouldn't fail because classes are not available that are not even relevant in the context of the current AnnotationReader. Each part of the code that uses an AnnotationReader should require_once the annotations that it can potentially need upfront. That even works for Validation as you can grab the tags from the DIC. That way we have a single mode of operation for the AnnotationReader and not two different ones that are 180 degrees different. OF course one could argue ALWAYS to use class_exists($name, true) instead, but i hope my mail showed why this is not a good idea.

If for some reason we do want autoloading for annotations then it should be a mechanism different from the PHP one, because they are both not compatible. The reader could have hooks for autoloading and validation mechanisms. Nothing we want to add for Doctrine Common 2.1, but something we could easily play around with for Common 3.

Next a mail with implementation details:

I propose to change the following on AnnotationReader:

1. Add a method to register annotation classes:
$reader->registerAnnotation("Symfony\Bridge\Doctrine\Validator\UniqueEntity");
and $reader->registerAnnotations(array(..));
2. Add a method to register files that contain annotations:
$reader->registerAnnotationFile(".."), directly loading them via
require_once;
3. Add a method to register annotation namespaces:
$reader->registerAnnotationNamespace("Symfony\Component\Validator\Constraint",
$dir). This is used to create a silently failing PSR-0 "autoloader" for
this path + directories. It can be automatically populated from data in
the UniversalClassLoader.
4. Change the class_exists check to never use autoload = true. If a
class is part of a registered annotation namespace try load the file based on a silent
PSR-0 autoload for it using an autoloader impl. contained in the
AnnotationReader (not the php autoloading mechanism), before triggering
the class_exists($name, false)

This way for symfony only the UniversalClassLoader data has to be used
to populate the registered annotation namespaces.



 Comments   
Comment by Benjamin Eberlei [ 02/Jul/11 ]

Implemented





[DCOM-55] Ugly problems with default import __NAMESPACE__ when it contains non-annotation classes that are docblock props Created: 29/Jun/11  Updated: 29/Jun/11  Resolved: 29/Jun/11

Status: Resolved
Project: Doctrine Common
Component/s: None
Affects Version/s: 2.1
Fix Version/s: 2.1

Type: Bug Priority: Major
Reporter: Benjamin Eberlei Assignee: Benjamin Eberlei
Resolution: Fixed Votes: 0
Labels: None


 Description   

Say you have a class "Entity".

Then /** @Entity */ on a class in the same namespace will try to instantiate Entity.

We should avoid this by testing if Entity is a subclass of \Doctrine\Common\Annotations\Annotation



 Comments   
Comment by Benjamin Eberlei [ 29/Jun/11 ]

Actually this problem is more problematic, we have to enforce extending \Doctrine\Common\Annotations\Annotation for ALL annotations.

Comment by Benjamin Eberlei [ 29/Jun/11 ]

Fixed.





[DCOM-51] Support parsing of single quotes Created: 03/May/11  Updated: 05/May/11  Resolved: 05/May/11

Status: Resolved
Project: Doctrine Common
Component/s: Annotations
Affects Version/s: 2.0.2
Fix Version/s: 2.1

Type: New Feature Priority: Major
Reporter: Jordi Boggiano Assignee: Guilherme Blanco
Resolution: Fixed Votes: 0
Labels: None


 Description   

It'd be great to support single quotes, this limitation doesn't make sense from a user point of view and in php context single quotes are so commonly used that it's easy to slip and then get some funny Exception message out of it.



 Comments   
Comment by Jordi Boggiano [ 03/May/11 ]

Patch against master is at https://github.com/doctrine/common/pull/17

Comment by Guilherme Blanco [ 05/May/11 ]

Merged the pull request.

Thanks!





[DCOM-46] Make annotation index configurable Created: 07/Apr/11  Updated: 13/May/11  Resolved: 13/May/11

Status: Resolved
Project: Doctrine Common
Component/s: Annotations
Affects Version/s: 2.1
Fix Version/s: 2.1

Type: Improvement Priority: Major
Reporter: Johannes Schmitt Assignee: Guilherme Blanco
Resolution: Fixed Votes: 0
Labels: None


 Description   

So, this is another improvement that I'd like to make. Right now all annotations are indexed by their name which has the limitation that on the top level annotations with the same name are only gathered once.

/**
 * @param mixed $a
 * @param mixed $a
 * @param mixed $a
 */
function ($a, $b, $c) { }

In the above case the annotation parser would only pick up one "param" annotation. My guess is that this was done for fast lookups, but I think we need to make this configurable (I know you hate this word ) as the workaround here is needlessly bloated.

/**
 * @params({@param, @param, @param})
 */
function($a, $b, $c) {}

This only requires two lines to be changed/made conditional, see
https://github.com/schmittjoh/SecurityExtraBundle/blob/master/Mapping/Driver/AnnotationParser.php#L63
https://github.com/schmittjoh/SecurityExtraBundle/blob/master/Mapping/Driver/AnnotationParser.php#L72

p.s. If you want me to provide a patch for this, just tell me.



 Comments   
Comment by Guilherme Blanco [ 13/May/11 ]

Implemented on master.

https://github.com/doctrine/common/commit/59910f53fad7ce08a1ec840d9874a74cefcf32b8





[DCOM-45] Allow different namespaces for the same alias Created: 07/Apr/11  Updated: 26/Apr/11  Resolved: 26/Apr/11

Status: Resolved
Project: Doctrine Common
Component/s: Annotations
Affects Version/s: 2.1
Fix Version/s: 2.1

Type: Improvement Priority: Major
Reporter: Johannes Schmitt Assignee: Benjamin Eberlei
Resolution: Won't Fix Votes: 0
Labels: None


 Description   

Right now, it is only possible to register one namespace per annotation alias. This might lead to problems since we now throw exceptions if an annotation is not found in that namespace (https://github.com/doctrine/common/commit/8967f476ddcdb7b9017a8be7f774979ca4c72247).

This improvement would be a necessary first step in order to overcome the problem we talked about on IRC (FrameworkExtra/SecurityExtra both using the "extra" alias).



 Comments   
Comment by Johannes Schmitt [ 26/Apr/11 ]

I'll close this as there is a better solution now.





[DCOM-44] Throw exception if known namespaced annotation is not found Created: 05/Apr/11  Updated: 06/Apr/11  Resolved: 05/Apr/11

Status: Resolved
Project: Doctrine Common
Component/s: None
Affects Version/s: None
Fix Version/s: 2.1

Type: Improvement Priority: Major
Reporter: Benjamin Eberlei Assignee: Benjamin Eberlei
Resolution: Fixed Votes: 0
Labels: None


 Description   

If we know that an annotation is namespaced and that the namespace exists we should throw an exception if the annotation specified does not exist, currently its just skipped which can be very nasty to debug.



 Comments   
Comment by Benjamin Eberlei [ 05/Apr/11 ]

Implemented

Comment by Gediminas Morkevicius [ 06/Apr/11 ]

For example my extensions are using same alias[vendor name] for all annotations. In this case each extension can be used independetly from others. And after this "fix" it couples all extensions and forces to autoload all annotations because in other case it will simply throw a fatal error, since class_exists now uses require without even checking if file exists.
Instead of fatal error ClassLoader maybe could throw an exception.

Comment by Benjamin Eberlei [ 06/Apr/11 ]

This change should not fatal through class_exists(). I dont change the autoloading behavior at all, i just throw an exception if the class was not found and annotation is namesapced

Johannes complained about this for the same reason (reusing vendor prefixes).

I find this quite annoying, because i constantly misstype annotations and get no error messages at all, the number of bug reports in this regard is also quite high. We have to find a solution for this





Generated at Wed Dec 17 23:15:47 UTC 2014 using JIRA 6.2.3#6260-sha1:63ef1d6dac3f4f4d7db4c1effd405ba38ccdc558.