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

Files: 
CommentFileSizeAuthor
#43 D8-annotations-2084513-37-vs-31.patchdiff.txt1.79 KBdonquixote
#37 2084513-37.patch29.58 KBEclipseGc
PASSED: [[SimpleTest]]: [MySQL] 59,057 pass(es).
[ View ]
#33 D8-annotations-2084513-31-vs-26.patchdiff.txt1.41 KBdonquixote
#31 D8-annotations-2084513-31.patch29.56 KBdonquixote
PASSED: [[SimpleTest]]: [MySQL] 58,682 pass(es).
[ View ]
#26 2084513-4.patch30.52 KBEclipseGc
PASSED: [[SimpleTest]]: [MySQL] 59,006 pass(es).
[ View ]
#26 2084513-interdiff.txt1.36 KBEclipseGc
#17 D8-2084513-16-vs-15.interdiff.txt3.4 KBdonquixote
#16 2084513-interdiff.txt3.65 KBEclipseGc
#16 2084513-3.patch30.84 KBEclipseGc
PASSED: [[SimpleTest]]: [MySQL] 58,503 pass(es).
[ View ]
#15 D8-loadAnnotationClass-2084513-15-vs-12.interdiff.txt3.47 KBdonquixote
#15 D8-loadAnnotationClass-2084513-15.diff31.77 KBdonquixote
PASSED: [[SimpleTest]]: [MySQL] 58,972 pass(es).
[ View ]
#12 D8-loadAnnotationClass-2084513-12-vs-5.interdiff.txt3.63 KBdonquixote
#12 D8-loadAnnotationClass-2084513-12.patch30.51 KBdonquixote
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]
#5 2084513-2.patch28.01 KBEclipseGc
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]
#1 2084513-1.patch26.65 KBEclipseGc
FAILED: [[SimpleTest]]: [MySQL] 58,831 pass(es), 3 fail(s), and 0 exception(s).
[ View ]

Comments

StatusFileSize
new26.65 KB
FAILED: [[SimpleTest]]: [MySQL] 58,831 pass(es), 3 fail(s), and 0 exception(s).
[ View ]

Let's see how testbot feels about this. I might have missed a couple things.

Eclipse

+++ b/core/lib/Drupal/Component/Plugin/Discovery/AnnotatedClassDiscovery.php
@@ -81,7 +81,11 @@ public function getDefinitions() {
+    AnnotationRegistry::registerLoader(
+      function ($className) {
+        return class_exists($className);
+      }
+    );

Is there any reason why Doctrine doesn't do that for themselves?

I picked this up from talking to Doctrine people, so... it's definitely something they do.

Eclipse

specifically, this guy: https://github.com/Ocramius?tab=activity

Eclipse

StatusFileSize
new28.01 KB
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]

oops, didn't change the constructor... :-(

Eclipse

If this works is seems much preferable to the complexity going on in the other issues.

Status:Needs review» Needs work

The last submitted patch, 2084513-2.patch, failed testing.

Won'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 :)

Btw, 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!

Yeah, 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

Just to be sure, what if we do something like this?

<?php
0
=== strpos($class, 'Drupal\\') && FALSE !== strpos($class, '\Annotation\\')
?>

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..

Status:Needs work» Needs review
StatusFileSize
new30.51 KB
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]
new3.63 KB

Status:Needs review» Needs work

The last submitted patch, D8-loadAnnotationClass-2084513-12.patch, failed testing.

Status:Needs work» Needs review

+  public function loadAnnotationClass($class) {
+    return 1
+      && 0 === strpos($class, 'Drupal\\')
+      && FALSE !== strpos($class, '\Annotation\\')
+      && class_exists($class)
+    ;
+  }

Someone is going to complain :)

StatusFileSize
new30.84 KB
PASSED: [[SimpleTest]]: [MySQL] 58,503 pass(es).
[ View ]
new3.65 KB

ok, 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

StatusFileSize
new3.4 KB

Here is an interdiff #16 vs #15.

+++ b/core/lib/Drupal/Component/Plugin/Discovery/AnnotatedClassDiscovery.phpundefined
@@ -118,7 +115,7 @@ public function getDefinitions() {
   public function loadAnnotationClass($class) {
-    return class_exists($class);
+      return class_exists($class);

Ha! Caught you.

shame on me.

Eclipse

Status:Needs review» Needs work

The last submitted patch, 2084513-3.patch, failed testing.

Status:Needs work» Needs review

Couple 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.

Status:Needs review» Needs work

woops xpost with bot.

Status:Needs work» Needs review

#16: 2084513-3.patch queued for re-testing.

Talked 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.

We seem to be in agreement that we don't need the extra method and can just register 'class_exists' or the anonymous function.

And if anyone in the future needs to change how annotation namespaces are loaded, we can then discuss a solution as in #15 / #16.

StatusFileSize
new1.36 KB
new30.52 KB
PASSED: [[SimpleTest]]: [MySQL] 59,006 pass(es).
[ View ]

so something along these lines?

Eclipse

Status:Needs review» Reviewed & tested by the community

I like this and want it to be committed.

It helps a lot to move forward with PSR-4.

+++ b/core/modules/block/lib/Drupal/block/Plugin/Type/BlockManager.php
@@ -35,8 +35,7 @@ class BlockManager extends DefaultPluginManager {
-    $annotation_namespaces = array('Drupal\block\Annotation' => $namespaces['Drupal\block']);
-    parent::__construct('Plugin/Block', $namespaces, $annotation_namespaces, 'Drupal\block\Annotation\Block');
+    parent::__construct('Plugin/Block', $namespaces, 'Drupal\block\Annotation\Block');

Wow, this is much easier on module authors. Huge +1

Issue tags:+API clean-up

tags for clarity.

Status:Reviewed & tested by the community» Needs work
Issue tags:+Needs reroll

git ac https://drupal.org/files/2084513-4.patch
  % Total    % Received % Xferd  Average Speed   Time    Time     Time  Current
                                 Dload  Upload   Total   Spent    Left  Speed
100 31251  100 31251    0     0  20145      0  0:00:01  0:00:01 --:--:-- 23286
error: patch failed: core/lib/Drupal/Core/Menu/LocalTaskManager.php:73
error: core/lib/Drupal/Core/Menu/LocalTaskManager.php: patch does not apply

Status:Needs work» Needs review
StatusFileSize
new29.56 KB
PASSED: [[SimpleTest]]: [MySQL] 58,682 pass(es).
[ View ]

This was just a reroll?

Eclipse

I 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)

Status:Needs review» Reviewed & tested by the community

Back to RTBC I guess. This will make the remaining conversions in #1966246: [meta] Introduce specific annotations for each plugin type a tad bit easier.

Yeah, this seems pretty sane.

Category:feature» task
Status:Reviewed & tested by the community» Needs work
Issue tags:+Approved API change

I 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. :(

StatusFileSize
new29.58 KB
PASSED: [[SimpleTest]]: [MySQL] 59,057 pass(es).
[ View ]

BlockManager had some changes. This is just a reroll.

Eclipse

Status:Needs work» Needs review

oops

Here is what has changed with the re-roll.

That 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

Status:Needs review» Reviewed & tested by the community

it was just an RTBC reroll, so I feel comfortable re-rtbcing.

Eclipse

I approve the RTBC.

Status:Reviewed & tested by the community» Fixed

Committed and pushed to 8.x. Thanks!

Title:Annotation class loading could be more elegant.Change notice: Annotation class loading could be more elegant.
Priority:Normal» Major
Status:Fixed» Active
Issue tags:-Needs reroll+Needs change record

Oops, I meant this.

Status:Active» Fixed
Issue tags:-Needs change record

Change notice here:

https://drupal.org/node/2087153

Eclipse

Issue summary:View changes

adding a change notice

Title:Change notice: Annotation class loading could be more elegant.Annotation class loading could be more elegant.
Status:Fixed» Active

Should have changed the title, my bad.

Eclipse

Great, 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!

Status:Active» Fixed

Automatically closed -- issue fixed for 2 weeks with no activity.

Issue summary:View changes

fixing the change notice link.

Issue summary:View changes

About #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.