Skip to content

Deprecate AnnotationRegistry in favor of class_exists() #29

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
kingcrunch wants to merge 1 commit into from
Closed

Deprecate AnnotationRegistry in favor of class_exists() #29

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion lib/Doctrine/Common/Annotations/AnnotationReader.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -160,7 +160,7 @@ public function __construct()
throw AnnotationException::optimizerPlusSaveComments();
}

AnnotationRegistry::registerFile(__DIR__ . '/Annotation/IgnoreAnnotation.php');
require_once __DIR__ . '/Annotation/IgnoreAnnotation.php';

$this->parser = new DocParser;
$this->preParser = new DocParser;
Expand Down
2 changes: 2 additions & 0 deletions lib/Doctrine/Common/Annotations/AnnotationRegistry.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,8 @@

/**
* AnnotationRegistry
*
* @deprecated Rely on spl_register_autoloader() autoloaders instead
*/
final class AnnotationRegistry
{
Expand Down
17 changes: 6 additions & 11 deletions lib/Doctrine/Common/Annotations/DocParser.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -401,13 +401,8 @@ private function classExists($fqcn)
return $this->classExists[$fqcn];
}

// first check if the class already exists, maybe loaded through another AnnotationReader
if (class_exists($fqcn, false)) {
return $this->classExists[$fqcn] = true;
}

// final check, does this class exist?
return $this->classExists[$fqcn] = AnnotationRegistry::loadAnnotationClass($fqcn);
return $this->classExists[$fqcn] = class_exists($fqcn) || AnnotationRegistry::loadAnnotationClass($fqcn);
Copy link
Copy Markdown
Author

@kingcrunch kingcrunch Apr 14, 2014

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not entirely sure, that there is no project out there, which makes use of a separate autoloader (means: Not registered with spl_autoload_register()) and relies on the AnnotationRegistry

Copy link
Copy Markdown
Member

@Ocramius Ocramius Apr 14, 2014

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, no, the parser should still rely on the AnnotationRegistry for autoloading - this was done especially to disallow any problem with collisions or not imported annotations.

Copy link
Copy Markdown
Author

@kingcrunch kingcrunch Apr 14, 2014

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It still relies on it, but I wonder: What does it change? I mean, internally loadAnnotationClass() looks pretty much similar to a regular PSR-loader. So it shouldn't feel any different from class_exists() at all, should it?

Copy link
Copy Markdown
Member

@Ocramius Ocramius Apr 14, 2014

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The idea is that AnnotationRegistry keeps the annotations autoloader and the normal class loaders insulated. What can be improved is AnnotationRegistry::registerLoader() so that it also accepts "use the global loader" (like in https://github.com/doctrine/DoctrineModule/blob/cef5366f4293adeb5b391e9852cdcee4c4dffc70/src/DoctrineModule/Module.php#L55-L59 )

Copy link
Copy Markdown
Author

@kingcrunch kingcrunch Apr 14, 2014

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The idea is that AnnotationRegistry keeps the annotations autoloader and the normal class loaders insulated.

And whats the benefit? 😕

I am not entirely sure, if I understand, what you suggest 😟

Copy link
Copy Markdown
Member

@stof stof Apr 15, 2014

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Ocramius the real reason is that non-silent autoloaders (for instance the Doctrine one) are breaking class_exists by throwing an error when trying to autoload a non-existent class.

An autoloader should always be silent to preserve class_exists, but unfortunately, some autoloaders (ZF1 and Doctrine for instance) made the opposite choice

Copy link
Copy Markdown
Author

@kingcrunch kingcrunch Apr 15, 2014

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the real reason is that non-silent autoloaders (for instance the Doctrine one) are breaking class_exists by throwing an error when trying to autoload a non-existent class.

That is somehow ironic, isn't? ...

An autoloader should always be silent to preserve class_exists, but unfortunately, some
autoloaders (ZF1 and Doctrine for instance) made the opposite choice

So any chance to get Doctrine somehow de-facto-standard compliant? And regarding ZF1 I have no idea .. Actually the whole PR seems pointless as long as there are autoloaders, that don't work as expected.

Copy link
Copy Markdown
Member

@stof stof Apr 15, 2014

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@kingcrunch the Doctrine autoloader is deprecated

}

/**
Expand All @@ -429,10 +424,10 @@ private function collectAnnotationMetadata($name)
'attributes' => 'Doctrine\Common\Annotations\Annotation\Attributes'
));

AnnotationRegistry::registerFile(__DIR__ . '/Annotation/Enum.php');
AnnotationRegistry::registerFile(__DIR__ . '/Annotation/Target.php');
AnnotationRegistry::registerFile(__DIR__ . '/Annotation/Attribute.php');
AnnotationRegistry::registerFile(__DIR__ . '/Annotation/Attributes.php');
require_once __DIR__ . '/Annotation/Enum.php';
require_once __DIR__ . '/Annotation/Target.php';
require_once __DIR__ . '/Annotation/Attribute.php';
require_once __DIR__ . '/Annotation/Attributes.php';
Copy link
Copy Markdown
Author

@kingcrunch kingcrunch Sep 16, 2014

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Member

@Ocramius Ocramius Sep 16, 2014

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

}

$class = new \ReflectionClass($name);
Expand Down Expand Up @@ -474,7 +469,7 @@ private function collectAnnotationMetadata($name)
// collect all public properties
foreach ($class->getProperties(\ReflectionProperty::IS_PUBLIC) as $property) {
$metadata['properties'][$property->name] = $property->name;

Copy link
Copy Markdown
Author

@kingcrunch kingcrunch Apr 14, 2014

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did my IDE automatically....

if (false === ($propertyComment = $property->getDocComment())) {
continue;
}
Expand Down