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

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

EclipseGc’s picture

FileSize
26.65 KB

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

Eclipse

dawehner’s picture

+++ 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?

EclipseGc’s picture

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

Eclipse

EclipseGc’s picture

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

Eclipse

EclipseGc’s picture

FileSize
28.01 KB

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

Eclipse

neclimdul’s picture

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.

donquixote’s picture

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

donquixote’s picture

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!

EclipseGc’s picture

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

donquixote’s picture

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

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

donquixote’s picture

Status: Needs work » Needs review
FileSize
30.51 KB
3.63 KB

Status: Needs review » Needs work

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

donquixote’s picture

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

EclipseGc’s picture

FileSize
30.84 KB
3.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

donquixote’s picture

Here is an interdiff #16 vs #15.

donquixote’s picture

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

EclipseGc’s picture

shame on me.

Eclipse

Status: Needs review » Needs work

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

neclimdul’s picture

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.

neclimdul’s picture

Status: Needs review » Needs work

woops xpost with bot.

donquixote’s picture

Status: Needs work » Needs review

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

neclimdul’s picture

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.

donquixote’s picture

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.

EclipseGc’s picture

FileSize
1.36 KB
30.52 KB

so something along these lines?

Eclipse

donquixote’s picture

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.

tim.plunkett’s picture

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

neclimdul’s picture

Issue tags: +API clean-up

tags for clarity.

alexpott’s picture

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
donquixote’s picture

Status: Needs work » Needs review
FileSize
29.56 KB
EclipseGc’s picture

This was just a reroll?

Eclipse

donquixote’s picture

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)

amateescu’s picture

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.

msonnabaum’s picture

Yeah, this seems pretty sane.

webchick’s picture

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

EclipseGc’s picture

FileSize
29.58 KB

BlockManager had some changes. This is just a reroll.

Eclipse

EclipseGc’s picture

Status: Needs work » Needs review

oops

donquixote’s picture

Here is what has changed with the re-roll.

EclipseGc’s picture

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

EclipseGc’s picture

Status: Needs review » Reviewed & tested by the community

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

Eclipse

donquixote’s picture

I approve the RTBC.

webchick’s picture

Status: Reviewed & tested by the community » Fixed

Committed and pushed to 8.x. Thanks!

webchick’s picture

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.

EclipseGc’s picture

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

Change notice here:

https://drupal.org/node/2087153

Eclipse

EclipseGc’s picture

Issue summary: View changes

adding a change notice

EclipseGc’s picture

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

donquixote’s picture

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!

tim.plunkett’s picture

Status: Active » Fixed

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

Anonymous’s picture

Issue summary: View changes

fixing the change notice link.

fgm’s picture

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.