Skip to content

Deprecate AnnotationRegistry in favor of class_exists()#61

Closed
kingcrunch wants to merge 1 commit intodoctrine:masterfrom
kingcrunch:class-exists
Closed

Deprecate AnnotationRegistry in favor of class_exists()#61
kingcrunch wants to merge 1 commit intodoctrine:masterfrom
kingcrunch:class-exists

Conversation

@kingcrunch
Copy link
Copy Markdown

Replacement for #29. See over there for discussion

I've removed the fork and therefore the PR got unlinked and unmanageable somehow. The previous PR had merge conflicts, but I wanted to keep it, so I've re-created it.

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.

@DavidBadura
Copy link
Copy Markdown

a big +1 for this! in each lib from me i have to add

\Doctrine\Common\Annotations\AnnotationRegistry::registerLoader('class_exists');

or i need to document it.

@Ocramius Ocramius added this to the v2.0.0 milestone Aug 31, 2015
@Ocramius Ocramius self-assigned this Aug 31, 2015
@Ocramius
Copy link
Copy Markdown
Member

I think this can be applied once http://www.doctrine-project.org/jira/browse/DCOM-295 is done.

@Ocramius
Copy link
Copy Markdown
Member

Personally fully agree with the patch.

As per discussions with internals, the autoloader was mainly meant for:

  • fixing problems with non-silent autoloaders (autoloaders that cry loud when trying to load non existing classes)
  • ignoring some classes (specifically)

I don't think non-silent autoloaders are a thing anymore, since @stof pretty much helped in killing them gradually.

As for the "ignoring" functionality, if DCOM-295 lands, then there is no need for such features, as the imports decide what annotations should be loaded or not.

Still needs some discussion with the others at @doctrine/team-doctrine2

@stof
Copy link
Copy Markdown
Member

stof commented Aug 31, 2015

I don't think non-silent autoloaders are a thing anymore, since @stof pretty much helped in killing them gradually.

Most of the work has been done by the composer adoption here actually: the composer autoloader is silent when it cannot find the class, and most projects are now using it.

@Ocramius
Copy link
Copy Markdown
Member

Yup, but also other autoloaders (including ours) were simply "fixed"

@Jalle19
Copy link
Copy Markdown

Jalle19 commented Dec 15, 2015

Just wanted to say that this doesn't work if you use this in a ZF1 project because its autoloader doesn't fail silently. I've resolved this by using this instead:

AnnotationRegistry::registerLoader(function($fqcn) {
    return @class_exists($fqcn);
});

@stof
Copy link
Copy Markdown
Member

stof commented Dec 15, 2015

@Jalle19 an autoloader breaking class_exists should die IMO. I suggest you to ask the ZF team if they could fix the ZF1 autoloader (as it seems ZF1 is still a thing)

@Ocramius
Copy link
Copy Markdown
Member

Fully agree: the ZF1 class loader should be fixed instead. Note that you may also just use the composer class loader also for ZF1 projects.

@Jalle19
Copy link
Copy Markdown

Jalle19 commented Dec 15, 2015

I don't really care, I wouldn't touch ZF1 unless I had to (working on a legacy application). Any solution that means people don't have to register a loader manually anymore is good in my book.

@Ocramius
Copy link
Copy Markdown
Member

@Jalle19 for new projects, you wouldn't pick something like ZF1 anyway. This is why we're discussing about deprecation and removal in next major versions.

@hboomsma
Copy link
Copy Markdown
Contributor

👍

@Majkl578
Copy link
Copy Markdown
Contributor

Majkl578 commented May 9, 2018

@kingcrunch Hi, I reapplied your changes onto current master as part of #205, let's make this happen. 😎
Meanwhile closing here, let's handle it in the referenced PR.

@Majkl578 Majkl578 closed this May 9, 2018
@Majkl578 Majkl578 assigned Majkl578 and unassigned Ocramius May 9, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants