On 7/1/11 1:42 AM, Benjamin Eberlei wrote:
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:
It's not too late for two reasons:
* we have added the @Annotation requirement for annotation classes (so
we have already changed the way annotations work recently);
* defining new annotations is not something that many people are doing a
lot. So, the changes will mainly impact the core.
So, we can still make some changes but this should land before the next
release candidate.
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.
agreed
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.
That does not work for Validation (yet) as there is no way to know all
the Constraint classes in AnnotationLoader. The DIC only knows about the
Validator classes:
<parameter
key="doctrine.orm.validator.unique.class">Symfony\Bridge\Doctrine\Validator\Constraints\UniqueEntityValidator</parameter>
<service id="doctrine.orm.validator.unique"
class="%doctrine.orm.validator.unique.class%">
<tag name="validator.constraint_validator"
alias="doctrine.orm.validator.unique" />
<argument type="service" id="doctrine" />
</service>
Thanks for this really good analysis of the problem. I agree with your
points and we should work on a solution before the next RC.
Fabien
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.
greetings, Benjamin
On Thu, 30 Jun 2011 10:48:09 +0200 Jordi Boggiano<[email protected]>
wrote:
On 30.06.2011 08:02, Johannes Schmitt wrote:
Extending a class will introduce a coupling (for example on the
validator component). Another, less intruding approach would be
to require an annotation on annotations itself (e.g.
@Annotation). This seems like the most future-proof approach to
me since it allows us to add more annotations like @Target for
example.
Not sure why everyone seems to have ignored this, but it sounds to
me like a better idea than the other one.
Cheers
-- Jordi Boggiano @seldaek - http://nelm.io/jordi
--
If you want to report a vulnerability issue on symfony, please send it to
security at symfony-project.com
You received this message because you are subscribed to the Google
Groups "symfony developers" group.
To post to this group, send email to [email protected]
To unsubscribe from this group, send email to
[email protected]
For more options, visit this group at
http://groups.google.com/group/symfony-devs?hl=en
