Change Notice: https://drupal.org/node/2087153
per #2083335: Annotation namespaces are stupid. & #2033611: Port AnnotatedClassDiscovery to PSR-4, and make it agnostic of directory information. the Annotation discovery has always been a little wonky with regard to how we get the actual annotation class. I've never been happy with this, but I went and spent a little time with some Doctrine people and they showed me what looks to be a WAY better way that should support any autoloader long term. I had to change basically every plugin manager implementation, but it's a very minor and very sane change that I think makes using AnnotatedClassDiscovery WAY better, so I think this is a huge DX win.
Basically this removes the need to know what dir an Annotation class resides in, which is awesome.
Eclipse
Comment | File | Size | Author |
---|---|---|---|
#43 | D8-annotations-2084513-37-vs-31.patchdiff.txt | 1.79 KB | donquixote |
#37 | 2084513-37.patch | 29.58 KB | EclipseGc |
#33 | D8-annotations-2084513-31-vs-26.patchdiff.txt | 1.41 KB | donquixote |
#31 | D8-annotations-2084513-31.patch | 29.56 KB | donquixote |
#26 | 2084513-4.patch | 30.52 KB | EclipseGc |
Comments
Comment #1
EclipseGc CreditAttribution: EclipseGc commentedLet's see how testbot feels about this. I might have missed a couple things.
Eclipse
Comment #2
dawehnerIs there any reason why Doctrine doesn't do that for themselves?
Comment #3
EclipseGc CreditAttribution: EclipseGc commentedI picked this up from talking to Doctrine people, so... it's definitely something they do.
Eclipse
Comment #4
EclipseGc CreditAttribution: EclipseGc commentedspecifically, this guy: https://github.com/Ocramius?tab=activity
Eclipse
Comment #5
EclipseGc CreditAttribution: EclipseGc commentedoops, didn't change the constructor... :-(
Eclipse
Comment #6
neclimdulIf this works is seems much preferable to the complexity going on in the other issues.
Comment #8
donquixote CreditAttribution: donquixote commentedWon't this work too?
AnnotationRegistry::registerLoader('class_exists')
This is just a blind guess, maybe doesn't work.
Also be careful, these loaders do pile up!
You can use AnnotationRegistry::reset() to clean it.
Otherwise:
I think the only reason for passing around those namespaces is if we want to avoid false positives when loading an annotation class. E.g. if there is a class out there that we absolutely don't want to load. Or if there are strings in docblock comments that, if read as class names and fed into class_exists(), would cause unpleasant results.
All of these concerns seem pretty exotic to me, so probably this is going to be ok.
At the time I worked on the PSR-4 patches, I had the idea that this stuff happens for a reason, and I cannot simply reduce it to class_exists(). But I am happy if I was wrong :)
Comment #9
donquixote CreditAttribution: donquixote commentedBtw, this was my own attempt to simplify this:
https://github.com/donquixote/drupal/commit/cb07e08eccac233030c24db1bc77...
(part of https://github.com/donquixote/drupal/compare/D8-psr4-combined-2083547-24)
Here we still do some filtering based on annotation namespaces, but then use class_exists(), so we don't need to pass around directories.
But removing them altogether is much preferred!
Comment #10
EclipseGc CreditAttribution: EclipseGc commentedYeah, we just need to know that the Annotation class can be used, and since we're basically just relying on the autoloader for that in this approach, we should be ok. The only tricky part in any of this is that we don't load the ANNOTATED classes via php unless we're actually using them. So we aren't using reflection to get the annotation itself, but so long as that's preserved and we don't do anything insane, I think we're pretty ok.
Eclipse
Comment #11
donquixote CreditAttribution: donquixote commentedJust to be sure, what if we do something like this?
We could also put an overridable generic loadAnnotationClass() in Component AnnotatedClassDiscovery, and then put this special logic into Core AnnotatedClassDiscovery.
Btw, in the PSR-4 patch I introduced an AbstractAnnotatedClassDiscovery.
https://github.com/donquixote/drupal/commit/baf4615cf989359e53784a75382e...
This way, Core AnnotatedClassDiscovery does not have to inherit the $annotationNamespaces attribute from Component AnnotatedClassDiscovery, which it then not uses.
Would you say this is a good idea?
We can also postpone this to a new issue..
Comment #12
donquixote CreditAttribution: donquixote commentedComment #14
donquixote CreditAttribution: donquixote commentedSomeone is going to complain :)
Comment #15
donquixote CreditAttribution: donquixote commentedhttps://github.com/donquixote/drupal/compare/D8-loadAnnotationClass-2084...
Comment #16
EclipseGc CreditAttribution: EclipseGc commentedok, donquixote and I discussed this in irc some, and I think the following patch presents a pretty good middle ground.
The interdiff is a little off cause I removed the getAnnotationNamespaces() method in a separate commit and didn't feel like trying to get it all sane. The interdiff is also against my last patch, not don's, because I had already continues working on this after the failures last night. I think the full patch should pass.
Eclipse
Comment #17
donquixote CreditAttribution: donquixote commentedHere is an interdiff #16 vs #15.
Comment #18
donquixote CreditAttribution: donquixote commentedHa! Caught you.
Comment #19
EclipseGc CreditAttribution: EclipseGc commentedshame on me.
Eclipse
Comment #21
neclimdulCouple thoughts. I still like this method as a general approach but have some questions now.
1) I'm not sure we need anything more then the closure we had originally. At the least, it should not be public, that muddies the public interface. I'm not sure it needs to be done though since the global could be manipulated outside the code without overriding the method so it doesn't seem a useful interface.
2) The reset got me thinking(a dangerous pastime, I know). How is this global suppose to be handled. It doesn't seem clear from the doctrine code docs. Is the system suppose to be reset and initialized anytime you parse annotations? If so and we are going to handle the global in this class, should we reset the system prior to starting and then on exiting? Alternatively, should we accept that the application calling plugins sets up the global annotation parser correctly? The later sounds more robust and likely trivially faster but also more dangerous. Stupid globals...
No answers just lots of questions sorry.
Comment #22
neclimdulwoops xpost with bot.
Comment #23
donquixote CreditAttribution: donquixote commented#16: 2084513-3.patch queued for re-testing.
Comment #24
neclimdulTalked with don. We seem to be in agreement that we don't need the extra method and can just register 'class_exists' or the anonymous function. Also, we should probably "clear the table" as the analogy went before running through. so a ::reset() before we get going and a ::reset() after to make sure everything is cleaned up.
Comment #25
donquixote CreditAttribution: donquixote commentedAnd if anyone in the future needs to change how annotation namespaces are loaded, we can then discuss a solution as in #15 / #16.
Comment #26
EclipseGc CreditAttribution: EclipseGc commentedso something along these lines?
Eclipse
Comment #27
donquixote CreditAttribution: donquixote commentedI like this and want it to be committed.
It helps a lot to move forward with PSR-4.
Comment #28
tim.plunkettWow, this is much easier on module authors. Huge +1
Comment #29
neclimdultags for clarity.
Comment #30
alexpottComment #31
donquixote CreditAttribution: donquixote commentedEDIT: Here is what caused the conflict:
#2050919: Replace local task plugin discovery with YamlDiscovery
Comment #32
EclipseGc CreditAttribution: EclipseGc commentedThis was just a reroll?
Eclipse
Comment #33
donquixote CreditAttribution: donquixote commentedI had to remove the changes in
core/lib/Drupal/Core/Menu/LocalTaskManager.php
If it helps, here is a diff of the patches.
(interdiff of commits does not help on reroll)
Comment #34
amateescu CreditAttribution: amateescu commentedBack to RTBC I guess. This will make the remaining conversions in #1966246: [meta] Introduce specific annotations for each plugin type a tad bit easier.
Comment #35
msonnabaum CreditAttribution: msonnabaum commentedYeah, this seems pretty sane.
Comment #36
webchickI actually think that given the amount of mental overhead (as well as likely performance) that this saves, we can re-classify this as a task, rather than a feature. Using native PHP functions as opposed to custom stuff is always a win. We talked about this in IRC. Although it's an API break, it is likely something that plugin developers will be thankful for as it eliminates some awfully annoying boilerplate code to have write every time. Tagging accordingly.
However, it doesn't apply for me. :(
Comment #37
EclipseGc CreditAttribution: EclipseGc commentedBlockManager had some changes. This is just a reroll.
Eclipse
Comment #38
EclipseGc CreditAttribution: EclipseGc commentedoops
Comment #43
donquixote CreditAttribution: donquixote commentedHere is what has changed with the re-roll.
Comment #44
EclipseGc CreditAttribution: EclipseGc commentedThat interdiff is weirdly backwards w/o being completely backwards. The block manager introduced some translation manager stuff, my patch does not remove or change it at all, it just changes the two lines with annotation dir and parent::__construct()
Eclipse
Comment #45
EclipseGc CreditAttribution: EclipseGc commentedit was just an RTBC reroll, so I feel comfortable re-rtbcing.
Eclipse
Comment #46
donquixote CreditAttribution: donquixote commentedI approve the RTBC.
Comment #47
webchickCommitted and pushed to 8.x. Thanks!
Comment #48
webchickOops, I meant this.
Comment #49
EclipseGc CreditAttribution: EclipseGc commentedChange notice here:
https://drupal.org/node/2087153
Eclipse
Comment #49.0
EclipseGc CreditAttribution: EclipseGc commentedadding a change notice
Comment #50
EclipseGc CreditAttribution: EclipseGc commentedShould have changed the title, my bad.
Eclipse
Comment #51
donquixote CreditAttribution: donquixote commentedGreat, thanks everyone!
EclipseGc for the patch and change notice, webchick for committing, everyone for participating.
I am super happy this is in, and we can move forward with PSR-4!
Comment #52
tim.plunkettComment #53.0
(not verified) CreditAttribution: commentedfixing the change notice link.
Comment #54
fgmAbout #2, the reason why this is done is because standard PSR-0 autoloaders may throw exceptions on failures (discussion about this was ignored at the time of PSR-0, but somehow heated for PSR-4), which breaks discovery, and Doctrine tries to be agnostic about its environment, so it includes its own autoloader to avoid potential error throwing by autoloaders.
FWIW, I wrote a bit of explanation about this (and another solution) on http://blog.riff.org/2014_02_16_reducing_redundancy_in_doctrine_annotati... as I hadn't noticed the problem had been fixed in D8.