Deprecate AnnotationRegistry in favor of class_exists()

[kingcrunch]

See also: doctrine/common#321

Actually it looks like there is no benefit in using a separate autoloading mechanism, which itself only makes (manual) use of "real" autoloaders. Instead class_exists() serves the same purpose and prevents one from the need to manually add an autoloader for every loader, that should load annotations.

This is especially interesting now, that composer took over most of the autoloading related issues in many projects.

I don't know, which deprecation-rules apply here, so I just added the tag and kept the class as it is, but remove the use wherever useful.

[kingcrunch]

Did my IDE automatically....

[kingcrunch]

bump So, what now? 🐈

[Ocramius]

@kingcrunch I'd have the AnnotationRegistry register class_exists as a default annotation autoloader instead (can be replaced/disabled by setting another autoloader manually).

@guilhermeblanco thoughts?

[stof]

@Ocramius class_exists works only when your autoloaders are failing silently (if your autoloader throws an error when loading a non-existent class, class_exists cannot be used to check whether the class exists as you will get the autoloading error).
Given that the Doctrine ClassLoader is not silent, we cannot kill the AnnotationRegistry until we kill it (the other non-silent autoloader I know of is Zend_Loader in ZF1)

[Ocramius]

@Ocramius class_exists works only when your autoloaders are failing silently

I think we can assume that in 2014 ;-)

[kingcrunch]

@stof Sooner or later it makes sense to discourage legacy autoloaders, especially when there are compatible drop-in-alternatives (at least for PSR-1 compatible, which are suitable for for example ZF1 as well). But anyway, as an alternative, until there is a decision, I could also change the order of the calls from

class_exists($fqcn) || AnnotationRegistry::loadAnnotationClass($fqcn);

to

AnnotationRegistry::loadAnnotationClass($fqcn) || class_exists($fqcn);

This would be backward-compatible to the old behaviour and when I look at AnnotationRegistry:115 it seems, that it would (in my use-case) iterate over two empty arrays, so the impact is negligible.

[stof]

no, because if the AnnotationRegistry cannot find a class whcih does not exists, so it would still go through class_exists and break. As soon as you introduce class_exists in it, it cannot be BC.

[kingcrunch]

@stof When I try to use an annotation, that does not exists, so loadAnnotationClass($fqcn) couldn't load it from anywhere, wouldn't that break even without the class_exists() (maybe with a different message)?

On the other hand when do you think it makes sense to intentionally break the bc here [1]? Because actually throwing exceptions in autoloaders where never a good idea.

But lets take symfony-standard once more

<?php

use Doctrine\Common\Annotations\AnnotationRegistry;
use Composer\Autoload\ClassLoader;

/**
 * @var ClassLoader $loader
 */
$loader = require __DIR__.'/../vendor/autoload.php';

AnnotationRegistry::registerLoader(array($loader, 'loadClass'));

return $loader;

The registerLoader()-call is the only thing, that prevents one from using the common composer autoloading, but on the other hand there is no workaround for that. And actually that is only to workaround broken autoloaders, that I and I'd even say most other projects don't even use :(

(The exmaple given by @Ocramius for the ZF module looks similar superfluous)

[1] Of course "not in a minor version". Question is more like "any plans?" 😉

[Ocramius]

Feedback on this: let's add a method like autoloadAllZeThingz and we're done with it. Thoughts? :)

[BenMorel]

+1 for dropping the AnnotationRegistry entirely.

I've always wondered why this class existed at all, and even though I now understand the silent autoloading failure problem, I think this should be the autoloader's problem, not Doctrine Annotation's problem. Let's just add one line to the doc: your autoloader must not throw an error when a class is not found.

Plus, this registry only supports PSR-0 autoloaders. I've recently updated my libraries to PSR-4, and that broke my annotations. I fixed this using a loader that returns class_exists(), but that left me with a bitter taste, wondering why I had to do that at all.

I love the Doctrine Annotations library, but please, get rid of the ugly thing! Just stop worrying about legacy autoloaders. Although silent autoloaders, as rightfully reported in the docs, are not part of the PSR-0 specification, it is arguably the only sensitive approach to autoloading, and is becoming a de-facto standard, especially with many projects now using the Composer autoloader.

Final note: a global AnnotationRegistry is a dangerous thing anyway. Even if we add this autoloadAllZeThingz method, @Ocramius, what if two libraries using annotations in the same project have contradicting views on the thing?

If that's a BC break, then maybe it's time for a 2.x branch?

My 2 cents.

[stof]

Plus, this registry only supports PSR-0 autoloaders

nope. It can support any autoloader, if you register an autoloader rather than a namespace

Note that if all your registered autoloaders are silent, you can register class_exists as the autoloader in the AnnotationRegistry (as class_exists triggers the SPL autoloader stack)

[BenMorel]

@stof That's exactly what I've done:

I fixed this using a loader that returns class_exists(), but that left me with a bitter taste, wondering why I had to do that at all.

What I wanted to emphasize is:

• We should not have, by default, to register a custom autoloader for annotations if we already have one that works fine for any class
• Having a separate mechanism for annotation autoloading is confusing and cumbersome: if each library depending on doctrine/annotations registers its own autoloader based on class_exists(), we end up with a stack of autoloaders all doing the same thing.
• Registering autoloaders in a global registry is a potentially dangerous thing: if two libraries depending on doctrine/annotations have conflicting views on the subject (imagine one of them not being a "silent" autoloader an throwing an exception if the annotation is not known to it), we can end up with an unexpected mess

IMHO, the only sane thing to do is to drop the registry and force silent autoloading. If this requirement is properly documented, I think it will do much more good than harm.

How do you feel about releasing a 2.x branch?

[kingcrunch]

👍 for a 2.x-branch with the radical approach (means: dropping the AnnotationRegistry-class completely)

The question now: Are there other BC-breaking issues, that would make a 2.x more reasonable? Right now it looks like this would be the only change and the library seems quite "complete" (there are quite few new commits in the last months). Publishing a version 2 just to remove this class feels strange 😕

Or (I feel dirty...) release a new minor version with bc-break: Keep the AnnotationRegistry, but use class_exists() as pre-registered default/fallback (see #29 (comment)) internally.

I wonder, how many platforms are really affected. The only examples of bad autloaders I heard are the (deprecated) dotrine loader, ZF1 and I myself know Yii, that throws an Exception in one specific case. To me it seems, that support of mostly outdated (Yii is the exception (hihi)) autoloaders is the only reason, that prevents one from using vendor/autoload.php in every situation

[Ocramius]

I think that to start a 2.x branch, we'd need a few issues like this one (suggestions about things to kill or refactor).

My personal suggestion is to avoid static properties in any of the library paths, as they are really problematic right now.

[kingcrunch]

I wonder, why I havent thought about this in april, but shouldn't with this PR the autoloader(s) load this classes too?

[Ocramius]

Those are usually not imported, they are just put in classes as @Annotation, without any namespace.

[kingcrunch]

See #61