Use Annotations for plugin discovery

+    AnnotationRegistry::registerAutoloadNamespace('Drupal\Core\Annotation', array(DRUPAL_ROOT . '/core/lib'));

You can probably stick a AnnotationRegistry::registerLoader(array($loader, 'loadClass')); in drupal_classloader() (that's what Symfony does).

     // Register PEAR-style vendor namespaces.
     $loader->registerPrefixes(array(
       // All Twig-borrowed code lives in /core/vendor/twig.
       'Twig' => DRUPAL_ROOT . '/core/vendor/twig/twig/lib',
+      // All Symfony-borrowed code lives in /core/vendor/Symfony.
+      'Symfony' => DRUPAL_ROOT . '/core/vendor',
+      'Doctrine' => DRUPAL_ROOT . '/core/vendor',
     ));

Why is that necessary?

+    $namespaces = drupal_classloader()->getNamespaces();
+    foreach ($namespaces as $ns => $namespace_dirs) {
+      $ns = str_replace('\\', DIRECTORY_SEPARATOR, $ns);
+      foreach ($namespace_dirs as $dir) {
+        $prefix = implode(DIRECTORY_SEPARATOR, array(
+          $ns,
+          'Plugins',
+          $this->owner,
+          $this->type
+        ));
+        $dir .= DIRECTORY_SEPARATOR . $prefix;
+        if (file_exists($dir)) {
+          foreach (new DirectoryIterator($dir) as $fileinfo) {
+            if ($fileinfo->getExtension() == 'php') {
+              $reflectionClass = new ReflectionClass(str_replace(DIRECTORY_SEPARATOR, '\\', "$prefix/". $fileinfo->getBasename('.php')));
+              if ($annotation = $reader->getClassAnnotation($reflectionClass, 'Drupal\Core\Annotation\Plugin')) {
+                $definition = $annotation->getDefinition();
+                $definition['class'] = $reflectionClass->name;
+                $definitions[$definition['plugin_id']] = $definition;
+              }
+            }
+          }
+        }
+      }
+    }

This hurts a little :) It requires us to do assumptions that are not in the PSR-0 specification. This kind of introspection should really be provided by the autoloader itself. The composer ClassLoader for example doesn't even have a ->getNamespaces()...

Not sure what the way forward could be. One approach would be to implement a proper NamespaceIterator (an iterator that allows to navigate through the namespace hierarchy and returns Reflection* objects) and register it in the same way we register the class loader.

One, I sincerely doubt AnnotationRegistry::registerAutoloadNamespace('Drupal\Core\Annotation', array(DRUPAL_ROOT . '/core/lib')); is necessary any more, files using annotations need use statements on the top, this registerAutoloadNamespace IMO does nothing useful. Two, we do not need namespaces but we do need a way to get the dirs we fed into the classloader back if we are going down my route.

This hurts a little :) It requires us to do assumptions that are not in the PSR-0 specification. This kind of introspection should really be provided by the autoloader itself. The composer ClassLoader for example doesn't even have a ->getNamespaces()...

We currently are not using the Composer classloader. If we were, we'd use ->getPrefixes and ->getFallbackDirs instead.

Why is that necessary?

We won't need that block once #1663404: Use Composer's defined namespaces to ease maintenance is in.

I, too, prefer annotations, but it is hard for me to prioritize this when I think rationally about it; it seems to introduce timing risk and consistency/usability risk.

Annotations feels like a nice-to-have optimization that could be postponed to Drupal 9, or late in the Drupal 8 cycle in the "polish phase". Annotations are not currently in the critical path for converting blocks to plugins, nor for realizing things like layouts, web services, or any of the features that will actually make Drupal 8 popular. We have limited time, and a lot to do, do we really have to spend time on this and introduce more timing risk? Blocks need to be converted before the feature freeze, annotations not necessarily.

I also prefer to do it in Drupal 9 because then we can consider to apply annotations consistently across core. If we use annotations for plugins, we probably still have non-plugin _info hooks. Also, some of these things currently lives in .info files. In other words, if we want to switch to annotations, I think we should use them consistently across core/Drupal. Annotations, when implemented partially, could actually hurt learnability. As a result, people will actually try to apply it to other parts of Drupal 8 and it becomes a big distraction.

Long story short, annotations feel a bit like a false dependency in order to make progess. It would be smarter to focus on converting blocks to plugins with _info hooks, and to introduce annotations after we done that work. Let's discuss, but I'm tempted to postpone this for now.

I think it's a bit early to be postponing things to Drupal 9, we're not that near near code freeze (yet).

I don't have a strong opinion either way on this particular issue yet though, still not caught up with things and won't be for a while at the current rate.

Just a note Dries said _info() hooks, not .info files.

One of the big wins of annotations is keeping the object behaviour and configuration in the object. .info files or _info files are somewhere else. That the API documentation contains them instead of needing to fish out of variious other files is pure win. Not to mention that unlike info hooks and info files annotations are a fairly standard way of doing things (Symfony and phpunit both use them) and didn't we want such standardization in Drupal 8?

Well I think the question isn't "is it cool" but "does it complicate development"

It seems like the core of Dries' argument is that by using it just for plugins we're introducing the need to understand different things depending on what you're interacting with. We should look at using it everywhere and do that in D9.

I think the counter argument to that is that we will never figure out how to use it across core without using it somewhere first. Plugins provide a well contained place for us to start using them and since its a new system for people already its just one thing to learn.

Also, I don't think annotations are a silver bullet that will just replace info hooks or other forms of metadata. They work great for a class of problem but sometimes just using some functions and an info hook might make sense. Or maybe your info hook has no code associated with it, just some files or something.

My point is that annotations are simple and easy to understand; well fit for the common plugin use case; and are likely easier to use then info hooks. And since its self contained and a new system for everyone its something everyone will be learning to some degree. We should continue looking at using them in favor of pushing definitions back into a legacy system and let it be our test case for annotations and see how people accept it.

The problem with hooks they are too late. We need to load quite a few things before modules (the php writing subsystem from #1675260: Implement PHP reading/writing secured against "leaky" script, cache, session and perhaps config itself?) and they perhaps should be plugins but you can't use hooks for them.

One of the big wins of annotations is keeping the object behaviour and configuration in the object.

Wouldn't a static "getInfo" method on the object (just like we already do with simpletests) accomplish the same thing?

Embedding functionality inside code comments is something that has been considered before and rejected for a number of reasons (see, for example, #211182: Updates run in unpredictable order)... at first glance, I would think those reasons still apply but don't have the time to think about it in detail right now.

Thanks David. Static methods would accomplish mostly the same thing. Annotations have the advantage over static methods specifically of being fairly common outside of Drupal and can be parsed without loading code into memory(thanks to chx's tokenizer PR to Doctrine). They're also pretty straight-forward to read and develop and they group the information with the implementation but any difference from static methods there is mostly opinion.

So I skimmed the update function issue and don't see any concerns that do apply anymore. It was hard to parse though as there was one mention of annotations(positive) and about a billion mentions of comment referring to the comment module. This was the only comment I saw directly dissenting annotations: http://drupal.org/node/211182#comment-2409218

"I do not feel that using a regex to parse a file looking for doxygen style comments is the best (or even a good) way"
We're suggesting using a much better system from Doctrine that largely based on php's tokenizer. Alternatively we'd be looking at using Doctrine's reflection version but that requires the loading of code into memory so we loose that advantage. Neither are regex based.

"Is there precedent? Possibly. Does it work? Probably. Is that still a good enough reason to change program flow based on the content of comments? Highly doubtfully. Not trying to derail the issue, but personal, I consider this very bad form."
There is precedent, it does work, and it is changing program flow based on metadata no matter where that data lives. That is actually the core of the discovery interface in plugins. The rest is mostly opinion but Annotations are becoming more and more popular and have been used quite a bit in other languages for a long time as Damien points out in that thread so I disagree.

The rest of the dissent in the follow ups seems to be "We're not sure and it would be too much before the D7 release, lets go with what we know and have a patch for(hooks)." That's reasonable but we're past that and we're not writing the annotation implementation and the patch to use it mostly exists so I don't see that as relevant.

Are there other concerns I'm missing? Do you have other examples?

Yes -- the thought was that EclipseGc's Blocks initative will lead to the presence of exactly 1 Bazillion Megaton of plugins in a typical Drupal install and I patched up Doctrine so that it can read the annotations without PHP-including the class for reflection. It already had a tokenizer for the use statements so I started from there. That's your problem with static getinfo methods: god knows whether you have the memory to load 'em all. Also, API module would have a reasonably hard time reading a static method compared to annotations.

This is good to go as a first step.

I'd like to see this, too, and agree with the arguments that have been stated so far.

However, the code that is being added here has almost zero documentation :-/ - neither on the high file/class level, nor on the code/inline level. Looking at the code, it is entirely not clear to me how these classes interact, what the properties are/contain/mean, what the functional code is actually doing, and what logical conditions are being handled differently, and most importantly, why.

Talked some more at the Developer Summit and agreed to proceed with Annotations. Reviewed the code and found a couple of things that need clean-up.

+++ b/core/lib/Drupal/Core/Annotation/Access.phpundefined
@@ -0,0 +1,20 @@
+  protected $callable;

I'd like to see these variables documented. Wouldn't hurt to have a one or two line description for the class too.

+++ b/core/lib/Drupal/Core/Annotation/Plugin.phpundefined
@@ -0,0 +1,31 @@
+      if ($value instanceof Translation) {
+        $this->definition[$key] = $value->getTranslation();
+      }
+      elseif ($value instanceof Access) {
+        $this->definition[$key] = $value->getAccess();
+      }
+      else {
+        $this->definition[$key] = $value;

This could be made more generic by checking for instanceof AnnotationInterface or something. It would make these annotations extensible.

+++ b/core/lib/Drupal/Core/Plugin/Discovery/DirectoryDiscovery.phpundefined
@@ -0,0 +1,69 @@
+/**
+ * A discovery mechanism finds annotated plugins in PSR-0 namespaces.
+ */

I think this should be renamed to 'AnnotationDiscovery' or 'AnnotatedClassDiscovery', especially because it lives in 'Drupal/Core/Plugin/Discovery'. It should have 'Annotation' in its name.

+++ b/core/lib/Drupal/Core/Plugin/Discovery/DirectoryDiscovery.phpundefined
@@ -0,0 +1,69 @@
+  /**
+   * Implements DerivativeAwareDiscovery::getBasePluginDefinition().
+   */
+  public function getDefinition($plugin_id) {

Code comment looks incorrect. Copy-paste error?

+++ b/core/lib/Drupal/Core/Plugin/Discovery/DirectoryDiscovery.phpundefined
@@ -0,0 +1,69 @@
+  /**
+   * Implements DerivativeAwareDiscovery::getBasePluginDefinitions().
+   */
+  public function getDefinitions() {

Code comment looks incorrect. Copy-paste error?

+++ b/core/lib/Drupal/Core/Annotation/Plugin.phpundefined
@@ -0,0 +1,34 @@
+class Plugin extends DrupalAnnotation {

Just wanted to actual use that and got an php fatal. It should implement the Interface ...

So I skimmed the update function issue and don't see any concerns that do apply anymore.... Are there other concerns I'm missing? Do you have other examples?

Yeah, sorry for dumping that comment and running - I didn't have too much time when I wrote it :) I don't have much time now either, but here are a few specific concerns I remember:

1. One was philosophical opposition (as you noted), @webchick being a big proponent of that - basically that this goes against the separation between code and comments that people expect as soon as they learn to program. I realize some other systems are using this for certain things, but are any actually using it in a similar way we're talking about here? I mean, we're basically saying that a typo in a code comment could cause blocks to disappear from the admin UI (or similar)... That seems really odd. How would someone even debug that?
2. Second was a concern that this could cause problems with opcode caches in some configurations (if they strip out or ignore code comments). No idea if it's a problem in practice, but it was mentioned by Dries in the above issue as a possible concern.
3. Third concern was that this hurts flexibility. You can't embed logic in a code comment. So in this issue, for example, it seems like we'd prevent people from ever having dynamic text in their plugin descriptions?

Annotations have the advantage over static methods specifically of being fairly common outside of Drupal and can be parsed without loading code into memory(thanks to chx's tokenizer PR to Doctrine).
....
Yes -- the thought was that EclipseGc's Blocks initative will lead to the presence of exactly 1 Bazillion Megaton of plugins in a typical Drupal install and I patched up Doctrine so that it can read the annotations without PHP-including the class for reflection.

Maybe so, but doesn't the above patch call ReflectionClass() before that, so won't that wind up including the class anyway?

BTW this is my simple argument in favor of annotations:

http://api.drupal.org/api/views/includes%21plugins.inc/function/views_vi...

After spending the time to convert all of the 210 Views plugins and handlers, we only had two that didn't fit directly into the annotation paradigm, and they were both hook_views_plugin_alter().

Any typo in the annotation blows up, with fairly clear error messages. Which is a good thing.

Also, as far as flexibility, if it works for Views it will likely work for everything else :)

http://drupal.org/sandbox/damiankloip/1685040

I meant to mark this RTBC.

Since the technical argument in favor of annotations is that they may be able to reduce memory usage, but the patch as written doesn't do that yet, I don't think this can be RTBC.... I think we really need to see how that works and get an idea of how the memory usage actually turns out.

In general I'm curious how much memory we're even talking about in practice. (Sure, you might have 50,000 nodes on your site that are exposed via block plugins or something, but presumably those are all using derivatives... so doesn't it actually reduce to a relatively small number of classes/files in the end, on a typical site? Would we really be using a lot of extra memory if those files got loaded on an admin page?)

I want to look into some of the other comments above also but didn't have a chance yet.

In addition to the technical arguments, I can attest that there is a huge benefit as far as DX goes.

Nope. The memory argument is that we have code done already to save from a memory doom should that come, the memory savings have been measured already to get the Doctrine patch accepted but that's basically a failsafe, an advantage of having annotations but that's a corollary of having annotations not the cause to have them. Sorry if I wasn't clear about that. If we find it's necessary we will need to carefully evaluate the performance vs memory differences, see what caching options are there and so on.

#14 and #44 are some of the real arguments for this: easy documentation, standard way of doing things, better DX. Having info hooks or info files store the metadata separate from classes is not as good as having them in place.

I think we're talking past each other here. I'm not comparing annotations to info hooks... I agree that info hooks are poor DX in this case.

I'm comparing them to static methods on the class, for which the above arguments (DX, etc.) are equally true if not more so (plus the other things I mentioned above). I don't know how to measure DX (wish I could), but anecdotally, at least:

• I've never seen anyone struggle at all with the static getInfo() methods in Simpletest.
• Because we already use them in Simpletest it's already kind of a Drupal standard (and with a little work we could make them consistent, or perhaps just go all the way and make simpletests plugins).
• And do you really think it's good DX to go from a world where we make people decide between t() and st() (bad enough as it is) to one where we now add a third option, @Translation(), to the mix? :)

So in comparing annotations to static methods, if we're not worrying about memory for now, what other advantage is there?

So you're proposing some kind of "getInfo" discovery?

It should be possible to get some benchmarks/profiling of the tokenizer version of annotations vs. current Docrine vs. getInfo() just to get an idea, even if it's not possible to use the tokenizer stuff in the patch yet.

We can do that but I warn you: the tokenizer is slow. When writing the Doctrine patch I created a gigantic file with 455 classes in it, a total of 2.5MB (concatted from Drupal 8 core). It takes about 640ms to run token_get_all on it, to tokenize and iterate over them was altogether 1371ms altogether. Now, include takes about 130ms. Include eats 33M RAM, the tokenizer peak is 342M. This, however, means that one class needs 700KB peak on average and after that, obviously, none because the tokens are dropped. Performance vs memory, as usual. And yes: this might prove that we are worried about the wrong thing.

Here's a test.php file (extension renamed due to d.o. security) that you can place in Drupal root. Also, checkout Views 8.x-3.x into site/all/modules/views.

Running this test shows peak memory usage of loading the 225 Views plugin classes. On my machine, with APC disabled:

Memory after full bootstrap: 23MB
Memory after additionally loading those classes: 41MB

That's 18MB with core Views plugins alone. Lots of contrib modules add more.

For SimpleTest, we accept that to run tests, you need more memory. But we shouldn't require more memory just to use the Views UI.

More CPU time to parse annotations isn't as much a problem, because we'll cache the result, so it's CPU time only needed rarely. But with an empty cache, running out of memory on an initial visit to the Views UI on a server with a low PHP memory limit is a problem.

[Edit: After doing this test, I saw that #41 links to a sandbox that may allow us to get a more accurate result as well as compare CPU time. Is that necessary, or is this proximate result enough to demonstrate the essential point?]

Does anotations mean we have to require once every plugin? (aka put every plugin into memory)
If not the test looks kinda wrong to me...

The 8.x-3.x branch of Views is functionally identical to D7, in that it uses its own plugin system.

The 8.x-plugins branch of http://drupal.org/sandbox/damiankloip/1685040 has the annotation based plugins and requires this patch.

Does anotations mean we have to require once every plugin? (aka put every plugin into memory)

No. Calling PluginFoo::getInfo() requires putting the plugin in memory (and keeping it there, because PHP has no mechanism to free code from memory). Whereas annotations allow for reading the code file as a string, and after getting the desired info from it, freeing that memory before moving on to the next file (PHP's normal behavior when assigning a new value to an iterating variable).

So, the claim in this issue is that info hooks are bad DX because they create a separation between the class file (buried deep in a PSR-0 path) and the info about the class, and that a static getInfo() method on the class is bad for memory usage, and that annotations solve both of those problems.

Also: there never was any scrutiny over whether going with Symfony HTTP Kernel makes sense or not. We fairly blindly accepted that instead of going with a Drupalism, going with a Symfony-ism is good. We could have a fairly endless debate over it and not having it was good. Watch what happened when we tried to write just our context stack. Endless debates are not good so not having them was good.

We have stopped applying this argument when not just Symfony but Doctrine and Zend uses the same annotation reader and PHPUnit while not (yet?) employing the same reader it also uses annotations. Are we seriously defending a Drupalism vs. what everyone else does? Don't we just accept that by now others have figured out most things better than we did?

Trying once more...

+++ b/core/modules/aggregator/lib/Drupal/aggregator/Plugin/aggregator/fetcher/DefaultFetcher.php
@@ -8,12 +8,22 @@
+use Drupal\Core\Annotation\Plugin;
+use Drupal\Core\Annotation\Translation;

Will this be able to go away once we update to Doctrine 2.3 and use the tokenizer instead of reflection? If so, then does it make sense to postpone this until after #1698108: Update Drupal's dependencies? The only reason to commit this sooner rather than later is to facilitate the bulk conversion of Views plugins and Block plugins, but seems like needing to add these use statements to every class and then remove them later negates the benefit?

Also, once we switch to the tokenizer, why would eAccelerator or any other opcode cache be incompatible? Wouldn't we at that point be reading the files from disk regardless of whether their opcode is cached?

Not so fast. The tokenizer feeds the annotation reader fairly the same as ReflectionClass does. What is inside an annotation is not affected. So the use statements stay because the annotation reader needs them. In fact it was this need that sparked the tokenizer work because Doctrine was already tokenizing the files containing the class to find use statements.

I do not know how APC fits into this picture.

Could we rename the interface 'Annotation' or 'AnnotationInterface; instead of 'DrupalAnnotation'? We already have Drupal in the namespace, and we don't add Drupal to the class name elsewhere. Other than that, this patch seems RTBC

Rerolled for #62.

That had trailing whitespace and some other oddities.

+++ b/core/lib/Drupal/Core/Plugin/Discovery/AnnotatedClassDiscovery.php
@@ -0,0 +1,70 @@
+  public function getDefinitions() {
+    $definitions = array();
+    $reader = new AnnotationReader();
+    AnnotationRegistry::registerAutoloadNamespace('Drupal\Core\Annotation', array(DRUPAL_ROOT . '/core/lib'));
+    $namespaces = drupal_classloader()->getNamespaces();
+    foreach ($namespaces as $ns => $namespace_dirs) {
+      $ns = str_replace('\\', DIRECTORY_SEPARATOR, $ns);
+      foreach ($namespace_dirs as $dir) {
+        $prefix = implode(DIRECTORY_SEPARATOR, array(
+          $ns,
+          'Plugin',
+          $this->owner,
+          $this->type
+        ));
+        $dir .= DIRECTORY_SEPARATOR . $prefix;
+        if (file_exists($dir)) {
+          $directories = new DirectoryIterator($dir);
+          foreach ($directories as $fileinfo) {
+            if ($fileinfo->getExtension() == 'php') {
+              $reflectionClass = new ReflectionClass(str_replace(DIRECTORY_SEPARATOR, '\\', "$prefix/". $fileinfo->getBasename('.php')));
+              if ($annotation = $reader->getClassAnnotation($reflectionClass, 'Drupal\Core\Annotation\Plugin')) {
+                $definition = $annotation->get();
+                $definition['class'] = $reflectionClass->name;
+                $definitions[$definition['plugin_id']] = $definition;
+              }
+            }
+          }
+        }
+      }
+    }
+    return $definitions;
+  }

This code (and some of the other added code) is still a giant black hole for me.

I mean, no comments at all...? :)

I already asked for comments that explain what all this code is doing (and most importantly, why it is doing what it is doing) way earlier in #22 of this issue. Since there are still no comments, does that mean we do not understand what this code is doing after all? ;)

+++ b/core/modules/aggregator/lib/Drupal/aggregator/Plugin/aggregator/fetcher/DefaultFetcher.php
@@ -8,12 +8,22 @@
+ * @Plugin(

I like the general idea of annotations.

But honestly, I did not really expect to see us porting, migrating, and declaring our previous huge Arrays of Doom™ with them.

Is there any particular/strong reason for why we do not use individual annotations that lead to a phpDoc block of; e.g.:

@PluginID aggregator
@PluginTitle @t('Default fetcher')
@PluginDescription @t("...")

?

Wouldn't that even make the discovery mechanism faster, since we can omit all of the other, possibly huge definitions in case we're just collating candidates? (which I assume is the >90% case?)

+++ b/core/modules/aggregator/lib/Drupal/aggregator/Plugin/aggregator/fetcher/DefaultFetcher.php
@@ -8,12 +8,22 @@
+ *   plugin_id = "aggregator",

I didn't really expect to see a plugin ID of "aggregator" here.

I expected "AggregatorFetcher" or similar, since Aggregator module defines multiple plugin types.

Looks like I need to study the new plugin system code some more...

+++ b/core/modules/aggregator/lib/Drupal/aggregator/Plugin/aggregator/fetcher/DefaultFetcher.php
@@ -8,12 +8,22 @@
+ *   title = @Translation("Default fetcher"),
+ *   description = @Translation("Downloads data from a URL using Drupal's HTTP request handler.")

Since @webchick is on vacation...

Let me try to assume and channel what I think she'd have to say about this:

WAT? After t(), st(), get_t(), and whatnot you want to make Drupal devs learn a gazillion-th way of dealing with translatable strings...? No way.

Can't we at least make this @t() ?

;-)

Note: Please take this with a solid salt of humor — I do not pretend to be webchick, nor "replace" her. I'm just reviewing this once more and trying to think of what she might say...

+++ b/core/modules/aggregator/lib/Drupal/aggregator/Plugin/aggregator/fetcher/DefaultFetcher.php
@@ -8,12 +8,22 @@
+ *   description = @Translation("Downloads data from a URL using Drupal's HTTP request handler.")
+ * )

Curious... does the annotation parser support Drupal's coding style of "trailing commas for the last array element, even if it is technically optional" ?

Sure, more docs would be good.

I'm not sure about redefining how annotations work to that extent (adding more @ conventions to follow) and I'm not sure that's a good idea.

Defining a custom plugin_id seems like a feature, not a bug.

Sure, @Translation is a bit bizarre, but it matches the CamelCase standards for class names.

Trailing commas aren't allowed AFAIK.

@tim.plunkett just pointed me to Drupal\views\Plugin\views\display\Block.

That's a massively larger extent of using annotations than I expected. I'm not sure whether that is the intended/endorsed way of using them, but this example looks and feels concerning to me... (given that I've just seen this for the first time, I'm not yet sure what exactly bugs me, but something feels "wrong" about that extent)

+++ b/core/modules/aggregator/lib/Drupal/aggregator/Plugin/aggregator/fetcher/DefaultFetcher.php
@@ -8,12 +8,22 @@
 /**
  * Defines a default fetcher implementation.
  *
  * Uses drupal_http_request() to download the feed.
  */
+
+/**
+ * @Plugin(
+ *   plugin_id = "aggregator",
+ *   title = @Translation("Default fetcher"),
+ *   description = @Translation("Downloads data from a URL using Drupal's HTTP request handler.")
+ * )
+ */
 class DefaultFetcher implements FetcherInterface {

Apparently, the separated phpDoc does not only appear in this patch, but also in that Views plugin example.

What's up with that?

This change violates our coding standards, since the actual phpDoc for the class is no longer directly above the class that it documents.

I can only guess this breaks API module's code parser (and probably every IDE out there). (I'd also have no idea how to tell the parser that the first, distant phpDoc block actually belongs to the class below the second phpDoc block - without inherently breaking the code parser for the other existing consecutive phpDoc blocks that happen to define to entirely different things [e.g., @defgroup and similar blocks].)

I'm not sure whether that is the intended/endorsed way of using them

My assumption has been that plugin definitions (the stuff inside the @Plugin annotation) should only contain key/values needed by a UI that lists plugins. The idea being to be able to list all possible plugins without loading each class into memory. Any information that is only needed after a plugin has been selected should probably be part of the class (i.e., a getInfo() method or specific getFoo() methods). Given that, I don't know why 'theme' or 'use_more' would be needed in the annotations. @tim.plunkett: does Views UI need that info during plugin listing, or is that an artifact of CTools plugin history where more stuff was added to plugin definitions than what I'm suggesting?

This change violates our coding standards, since the actual phpDoc for the class is no longer directly above the class that it documents.

My top preference would be for @Plugin to appear immediately below @file. But I don't know if Doctrine requires such annotations to come after the "use" statements that define the full class name that the annotation maps to. If it does, I think immediately below those "use" statements would be fine.

Can't we at least make this @t() ?

If we're willing to rename Drupal\Core\Annotation\Translation to Drupal\Core\Annotation\t, then I think so. If we think that's icky, would changing the use statement to use Drupal\Core\Annotation\Translation as t; work?

I didn't really expect to see a plugin ID of "aggregator" here. I expected "AggregatorFetcher" or similar, since Aggregator module defines multiple plugin types.

The plugin ID is currently 'aggregator' within aggregator_aggregator_fetch_info(), because prior to the initial plugin patch (and in D7), aggregator.module was limited to one fetcher plugin per module, and automatically assigned the module name as the value of the corresponding $conf variable. This is not the pattern used by other plugin systems like image_image_effect_info() which allow multiple plugin ids (e.g., "image_resize", "image_crop") per module. At some point, we can rename "aggregator" to "aggregator_default" or something else, but that will require an update function to update existing values in the aggregator.settings config object.

Up to now, we've followed a convention of plugin ids being lowercase (and suitable as machine names). Maybe allowing camel case makes sense? In which case, perhaps allowing 'plugin_id' to be omitted from annotations and defaulting to the class name would be cool?

t() is a completely non-self-documenting function. Let's get over ourselves and just use the class name Translation. It's self-evident, while t() is a Drupal-ism.

Re #9:

If we use annotations for plugins, we probably still have non-plugin _info hooks. Also, some of these things currently lives in .info files.

Here's my thoughts on how these things fit together:

- Modules and themes are not plugins in the way we're using the term, because we're choosing to use the term "plugins" to refer to more granular things, whereas modules and themes are bigger things. However, modules and themes also share the same characteristic that plugins have which is that there's a UI that lists all of them (including disabled ones) without loading their code into memory. For modules and themes, we use .info files for the metadata needed by the listing UI.

- Some of our _info() hooks get data about things that are purely data. There's no corresponding PHP code that we're trying to avoid loading until the thing is enabled/selected. The examples I found of these include hook_library_info(), hook_language_types_info(), and hook_hook_info(). I see these as appropriate to remain as info hooks until someone suggests an alternative. While the plugin system allows for purely-definition, no-behavior "plugins", I don't really see the point, and think it might be easier conceptually to reserve the concept of a "plugin" to something that has both metadata and PHP code.

- We have a couple dozen info hooks that logically are plugins (the info hook returns metadata about a thingie, and there's also corresponding PHP functions that implement behavior specific to that thingie). Whether or not we convert all of them to the plugin system in D8 is an open question. I know people have expressed interest in converting some of them, like image effects, text filters, blocks, fields/formatters/widgets, and actions, but whether all of these and the others get done, who knows. To Dries's point in #9 about distractions, I don't think we should try to push for every plugin-like system to get converted to actual plugins, because there's higher priority things to work on. But, to the extent people want to scratch that itch, it's there for the scratching. Anyway, for anything that does get converted to plugins, I think it would be good to standardize on using PSR-0 classes for the implementation and annotations for the metadata (definitions) for the reasons covered in this issue: keeps everything about a plugin in one file, efficient use of memory, follows conventions used by other major PHP frameworks.

- With the above, we'll end up with 3-4 ways of adding code thingies to Drupal. .info files for modules and themes. _info() hooks for pure data. Classes with annotations for plugins. And _info() hooks and procedural functions for legacy plugin-like things. The first 3 are different techniques used for 3 substantially different concepts, so I think that should be fairly easy to keep clear. The last one is sad, but maybe we can add some "@todo D9:" comments for them to make clear to people that it's just legacy stuff.

We had a pretty long discussion in IRC yesterday. Copypasting that:
(roughly polished summary -- also, unfortunately, I lost the beginning of the backscroll)

OK, to sum up: my current thinking is along the lines of this:

1) id + label + description sound fine to me, since required for discovery + UI.
2) I'd also be fine with additional, plugin-specific annotations à la @ViewUsesHookBlock.
3) Migrating Arrays Of Doom™ to annotations, I feel extremely uncomfortable with.
Either
4) getInfo() methods [where needed],
or
5) supply meta-data in common meta-data ways; e.g., MyPlugin.yml

It looks bogus that we want to express access in annotations. The syntax of Annotations is concerning to begin with - it seems to be following JSON to large extents, including its strict serialization rules. I mean, why don't we port everything from PHP to Annotations-fake-JSON? We don't want to learn pseudo-code. It's a custom macro language, which re-invents PHP. The moment you put functions into annotations, you do invent a new programming language.

It's a way of attaching metadata in php because as a language php isn't flexible enough to support that.

Only parameters are metadata. Everything else really does not belong into metadata, and thus not into annotations. The only reason I can accept @Translation is that we need a way to _mark_ translatable strings. At the same time, I think it is wrong that the current code immediately translates those strings.

We lose flexibility by using an annotation instead of a method. That has to be well justified, because its declarative instead of dynamic; i.e., you can have as many conditions, PHP code, classes, variables, dependencies, and whatnot in your info hook.

I also have troubles to see how those default settings play together with CMI. (They rather hint at a default config file for each plugin type; i.e., replacing those default settings in annotations with a default config file. If they are essentially the default configuration for the plugin type? Where's the difference to default module config?)

The examples also contain constants. But constants are variables. Annotations should not contain variables.

Custom annotations as values for annotations should be a code smell and we should justify them.

What if a plugin requires a complex access condition that cannot be delivered by @Access? cufa() should be able to support anything, except calls that include $this.

But no matter how you slice that, you're adding support for a call to some other code, which means you're now no longer able to have the DX you want, because as a developer I now have to look elsewhere to find out what my annotation actually returns -- which is pretty much exactly why that's NOT declarative.

Anything past giving a value that would get plugged into a hardcoded function like user_access() is not really appropriate.

I really like the idea of annotations, and I was and still am fine with the example that is contained in the actual patch. But I really dislike the amount of endless flexibility and thus possible abuse, which allows coders to inject settings and whatnot non-metadata into annotations. I wish we could enforce limitations.

AFAICS, the PluginAnnotation class would be able to enforce limits.

Overall, I think this is about some coding fundamentals. That is, because annotations are really not bound to the plugin system in any way. I understand the counter-perspective of plugin developers in that these things might be nice, but honestly, I also see kind of a tunnel-vision in that perspective. First and foremost, I do not think we want to invent a new programming language. Declaring metadata is fine. But anything beyond that is highly questionable and needs to be justified with very good technical arguments. There's no way to use them (yet) outside of plugins. But the focus really is on "(yet)". There's absolutely no reason why we couldn't use them more and in other areas. However, for that we need a fundamental understanding of what is metadata and thus what belongs into annotations.

I totally understand that this sorta blocks the blocks/layouts effort. Therefore, we should consider to look into compromises of e.g. enforcing sane limits through the PluginAnnotation class... e.g., looking at https://gist.github.com/b84839720e84786313b2, it could explicitly support the keys plugin_id, label, description, and module, and deny/ignore anything else.

What also bugs me is that there is no declared API for the stuff that is defined in a particular @Plugin. Where do developers go look after possible parameters? Without any kind of limitation, this smells like one big container of data garbage that doesn't follow any kind of schema. That is a downside of using the @Plugin method; we're enforcing a data structure that gets passed in.

Anything in an annotation should be to provide data to something else, OTHER than your class. If we're just using it as a shorthand to provide data to its own method, that gets architecturally messy.

Something like this looks acceptable: https://gist.github.com/b84839720e84786313b2

I'd rename plugin_id to id, subject to label, allow for description, can accept module, but would then reach the point at which I'd like the PluginAnnotation class to bail out on any additional garbage.

i.e.: Using annotations for minimal metadata that needs to be there for discovery and UI purposes. For everything else, use regular PHP properties and methods.

1) The 'module' property in the annotation looks wonky. Why does that need to be defined manually, and why isn't it inherited from the namespace/extension supplying the plugin? -- "module" can't really be introspected, and it might only be needed for legacy purposes; probably can be a regular method/property of the plugin class, too.

2) technically minor, but documentation-wise important: I can only guess that the fake-JSON syntax of those Annotations does not allow for "comments". (gosh, having to wrap that in quotes is a little insane) -- I think that's an important detail. It's unimportant whether it is right or bad to have a comment in an annotation declaration. What matters is whether it is possible or not.

I think what I'm saying is that I understand the desire for flexibility, but every single additional property really needs to be justified. Otherwise, the counter-arguments of a lacking API/schema standard/documentation, memory consumption, badass DX, etc will outweigh the flexibility argument.

As a plugin implementation developer, how do I figure out which schema I need to implement for a particular plugin type, and given that there can possibly be a gazillion properties, where do I find their documentation, so I can actually define my plugin?

So, we'd expect that sort of documentation to exist on the plugin's driving abstract class, or interface (depending on the plugin type developer's preference). We could also see documenting it on the Plugin Manager class if we think that's a good location for such things. A standard for it should certainly be defined.

Any of those places probably make perfectly good sense. Plugin Manager class is the only class that you will consistently need, but by the same token, core could totally provide a default one of those that 90% of all plugins could use. And any of those plugin type's could have totally different annotation schemas (as you say).

In Views, we are currently overusing the plugin metadata. In an IRC discussion, we agreed that:

1) Metadata should be reserved for data that is used outside of the class instantiation.
2) Metadata that is used only after object instantiation should be moved to methods and/or protected variables that are accessed by said methods.

Amendment to this: Metadata is used in other places for data that is injected externally. (In CTools I often use it to control derivative behavior, because derivatives have only the metadata as a place to put their information. For example, the content type that renders a single field instance contains metadata for the entity type, field machine name, and associated data. This data also wouldn't appear in an annotation, so is meaningless to this discussion, but I'm completionist and felt compelled to mention.

#1698108: Update Drupal's dependencies went in, so let's also update the patch to use the tokenizer.

I'll do #75 today

That could be optimized a little.

This may be fixed a little.

Edit: note that the Psr0FinderFile is still absolutely necessary because while we hint the class if it extends something then it still might need to be found.

Tried to make a summary

changed tags

Added not

That's some testbot fluke. Here's an even faster version: as we only use class annotations (no methods or somesuch) we do not need to worry about inheritance actually so the finder can be replaced by something truly dumb. Further speedups possible once https://github.com/doctrine/common/pull/173 gets merged up as that will allow to tokenize only the header down to the actual class declaration and nothing else.

Gah! I have fixed that but apparently didn't run git add -u.

Prettier code. And before someone objects at not having enough tests, all the aggregator tests fail as they failed above if the code is wrong.

Couple of doc issues. Not everything covered.

+++ b/core/lib/Drupal/Component/Reflection/MockFileFinder.php
@@ -0,0 +1,47 @@
+   * Implements \Doctrine\Common\Reflection\ClassFinderInterface::__construct().

Don't think we ever add the first backwards slash

+++ b/core/lib/Drupal/Core/Annotation/AnnotationInterface.php
@@ -0,0 +1,15 @@
+  /**

Add a newline

+++ b/core/lib/Drupal/Core/Annotation/Plugin.php
@@ -0,0 +1,39 @@
+  protected $definition;

Can use a docblock

+++ b/core/lib/Drupal/Core/Annotation/Plugin.php
@@ -0,0 +1,39 @@
+   * Build up the plugin definition and invoke the get() method for any classed

We have standards for documenting constructors

I did a documentation pass. I added several @todos, because I wasn't sure and didn't want to make anything up.

Also, we're now stuck between a rock and a hard place for method docblocks. According to http://drupal.org/node/1353118 we should use the full PSR-0 path in documentation, but it's also supposed to stay under 80 characters.

Wrong patch

Now we cache the discovery. And I nuked the DirectoryIterator class in favor of the php manual recommended pathinfo($fileinfo->getFilename(), PATHINFO_EXTENSION).

Sigh.

OK, OK. It needs the cache table created. Why can't we just use mongo :P ?

+++ b/core/lib/Drupal/Core/Annotation/AnnotationInterface.phpundefined
@@ -0,0 +1,20 @@
+/**
+ * Defines a common interface for @todo.
+ */
+interface AnnotationInterface {

+++ b/core/lib/Drupal/Core/Annotation/Plugin.phpundefined
@@ -0,0 +1,43 @@
+class Plugin implements AnnotationInterface {
+
+  /**
+   * @todo.
+   */
+  protected $definition;

+++ b/core/lib/Drupal/Core/Annotation/Translation.phpundefined
@@ -0,0 +1,43 @@
+class Translation implements AnnotationInterface {
+
+  /**
+   * @todo.
+   */
+  protected $translation;

+++ b/core/lib/Drupal/Core/Plugin/Discovery/AnnotatedClassDiscovery.phpundefined
@@ -0,0 +1,81 @@
+class AnnotatedClassDiscovery implements DiscoveryInterface {
+
+  /**
+   * @todo.
+   */
+  function __construct($owner, $type) {

These are the remaining doc @todos.

+++ b/core/lib/Drupal/Core/Plugin/Discovery/AnnotatedClassDiscovery.phpundefined
@@ -0,0 +1,81 @@
+            // @TODO: Once core requires 5.3.6 use $fileiinfo->getExtension()
+            // instead.

I would change this comment to // @todo Once core requires PHP 5.3.6, use DirectoryIterator::getExtension().

+++ b/core/lib/Drupal/Core/Plugin/Discovery/AnnotatedClassDiscovery.phpundefined
@@ -0,0 +1,81 @@
+              $class = str_replace(DIRECTORY_SEPARATOR, '\\', "$prefix/". $fileinfo->getBasename('.php'));

Missing a space between " and .

Marking needs review for the bot.

This patch really tends to mushroom out of hand. Added an upgrade -- in fix bootstrap because I suspect sooner than later we will need it for bootstrap.

SearchCommentTest Drupal\search\Tests\SearchCommentTest->testSearchResultsCommentAccess()

Random failures again... Argh...

#97: 1683644-96.patch queued for re-testing.

Note: Comment Search tests 193 passes, 0 fails, 0 exceptions, and 66 debug messages. I tested manually.

Yay, it passed! Back to needs work for #95.

Fixed #95 (although the kept the @todo for getExtension as it is just fixed the spelling).

We need to codify the results of #73-74.

To recap:

1. Metadata should be reserved for data that is used outside of the class instantiation.
2. Metadata that is used only after object instantiation should be moved to methods and/or protected variables that are accessed by said methods.

However, the patch passes, and I've tested it with the Views+Annotation sandbox, and it works 100%.

Therefore, marking RTBC. I think this could go in as is with targeted follow-ups.

We discussed the rename of "plugin_id" to "id" in irc. And (at least in views) the key is used in other php code. So if you rewrite it to id it's harder to link this to plugins. (but we do understand that it looks duplicated here)

Just re iterating what timplunkett said in #104, This is working well in our sandbox in it's current state (all metadata in annotations). This will be a views issue to refactor this metadata into the classes, so followups there perfect.

Ugly interdiff but it should point out I only changed some docs...

re: #105: plugin_id vs. id:

Doesn't the same apply to 'title' and 'description', too? Isn't it just by coincidence that this particular consumer example might have an ambiguous 'id', but not an ambiguous title and description?

If the variable context/self-descriptiveness is really an issue, then I think it would have to be applied to all properties in the annotation, not just 'id'.

Patch looks good in general.

- does this really need a dedicated cache table? Why not use the default cache bin?

- how bad is the performance of this that we need caching for it? Isn't the plugin information mainly used for admin screens viewing lists of plugins? Would be nice to see that profiling so we can answer that question...

- I was a bit put off by the name of 'MockFileFinder', but can only think of 'SpecifiedFileFinder' which is probably worse, so meh.

+   * Builds up the plugin definition and invoke the get() method for any classed
+   * annotations that were used.

Builds up.. and invokes

+ * This is a dumb file finder.

Doesn't need 'This is'.

+   * Parses values passed into this class through the t() function in Drupal and
+   * handle an optional context for the string.

Parses values... and handles.

+ * A discovery mechanism finds annotated plugins in PSR-0 namespaces.

Is this missing 'that', or just "Find annotated...".

Since Dries wasn't sure about this in the first place, I'll give him a chance to look before this goes in - let's say until 17th August so it's in before DrupalCon if he doesn't get back to it.

I thought a bit about this:

1) The doc issues are trivial
2) Talked with catch and there will be one cache entry for each pluginType so it probly can be added to the default 'cache' table
3) About caching, with this architecture it rly depends on the plugin type. Each plugin type can decide if it wants to use a caching mechanism or not. So whatever we decide here Views and other Modules can still decide on their own if they want this to be cached.

So for this patch we only need to decide if we want to cache the aggregator feeds plugins data.

Ok new patch:

1) no more caching as it not needed at the moment as the annotation process for aggregator only happens the ui
2) changed plugin_id to id as sun has a valid point in #108
3) fixed the docs

(hopefully did this correct)

+++ b/core/lib/Drupal/Component/Reflection/MockFileFinder.php
@@ -0,0 +1,49 @@
+/**
+ * Definition of Drupal\Core\Plugin\Discovery\DumbFindFile.

1) Missing @file.

2) The class is actually MockFileFinder, not DumbFindFile -- and the namespace seems outdated, too.

+++ b/core/lib/Drupal/Core/Annotation/Translation.php
@@ -0,0 +1,47 @@
+    $this->translation = t($string, array(), $options);

Directly translating translatable strings, instead of marking them as translatable still looks wrong to me.

First of all, it makes the declaration no longer really declarative.

Second, doing so has nasty side-effects and consequences — such as having to make the CacheDecorator aware of the "current" language and making it use a language-specific cache ID, plus of course appropriate 'language' => array($current_langcode) cache tags.

(The same applies to any other possible additional annotations, such as @Access that was mentioned earlier; leading to much more complex cache tags.)

Can we remove the t()?

+++ b/core/modules/aggregator/lib/Drupal/aggregator/Plugin/aggregator/fetcher/DefaultFetcher.php
@@ -8,12 +8,22 @@
 /**
  * Defines a default fetcher implementation.
  *
  * Uses drupal_http_request() to download the feed.
  */
+
+/**
+ * @Plugin(
+ *   id = "aggregator",
+ *   title = @Translation("Default fetcher"),
+ *   description = @Translation("Downloads data from a URL using Drupal's HTTP request handler.")
+ * )
+ */
 class DefaultFetcher implements FetcherInterface {

Based to the discussion we had, the separate/split phpDoc should no longer be necessary and can be merged into one now.

I filed #1722394: [policy] Determine standard for annotations and plugin documentation. Please update/correct that issue's summary as appropriate.

xpost

I'll work on addressing some of #112 and add a few additional cleanups.

I'm tempted to commit this patch as is, so we can make progress elsewhere. As predicted, this patch became a bit of a distraction that may cause unwanted delays. I'll wait a couple more hours for xjm's next version.

Attached:

• Combines the regular docblock with the annotation as suggested above. Note that we can and should follow up on that in #1722394: [policy] Determine standard for annotations and plugin documentation; it should not block the initial commit. If it doesn't work in this patch, I'll revert that change.
• Cleans up some minor documentation and code style issues.
• Adds three @todos for missing documentation. Minor documentation issues should be corrected in followup issues, but the documentation gate specifies that the new classes should have documentation before they go in.

Not addressed:

Directly translating translatable strings, instead of marking them as translatable still looks wrong to me.

First of all, it makes the declaration no longer really declarative.

Second, doing so has nasty side-effects and consequences — such as having to make the CacheDecorator aware of the "current" language and making it use a language-specific cache ID, plus of course appropriate 'language' => array($current_langcode) cache tags.

(The same applies to any other possible additional annotations, such as @Access that was mentioned earlier; leading to much more complex cache tags.)

Can we remove the t()?

Removing the t() seemed to directly contradict the docblock for the method, which states:

Parses values passed into this class through the t() function in Drupal and handles an optional context for the string.

So this didn't really make sense to me, and I didn't touch it. Is it possible to file a followup issue for this point instead?

NR for the bot to confirm that the combined docblocks do in fact work.

+++ b/core/lib/Drupal/Core/Annotation/Plugin.phpundefined
@@ -0,0 +1,53 @@
+/**
+ * Defines a Plugin object.
+ *
+ * @todo
+ *   Add some documentation about what plugins are...
+ *
+ * @Annotation
+ */
+class Plugin implements AnnotationInterface {

+++ b/core/lib/Drupal/Core/Annotation/Translation.phpundefined
@@ -0,0 +1,53 @@
+/**
+ * Defines a translation object.
+ *
+ * @todo
+ *   Document this class.
+ *
+ * @Annotation
+ */
+class Translation implements AnnotationInterface {

+++ b/core/lib/Drupal/Core/Plugin/Discovery/AnnotatedClassDiscovery.phpundefined
@@ -0,0 +1,84 @@
+  /**
+   * Implements Drupal\Component\Plugin\Discovery\DiscoveryInterface::getDefinitions().
+   *
+   * @todo
+   *   Inline documentation and whitespace should be added to this method to
+   *   make it more legible.
+   */
+public function getDefinitions() {

...And here are the three spots I think we need to add additional documentation.

1. Following up on @sun's comment, I wonder why we even need this @Translation stuff at all? (We don't have anything like that for .info files, yet somehow we have translatable strings in .info files which get translated on display.) I think we should consider removing that.
2. Following up on @effulgentsia's comment in #70, why do we need the "id" key at all? As he said, we should be able to figure it out from the location in the filesystem, right? This would also address my earlier comment that we shouldn't be letting this code fail silently if it's performing important functionality. (The code as written will still fail silently in many situations, e.g. if you mistype "@Plugin" as "Plugin" your plugin will disappear with no warning whatsoever and no way to directly debug it.) Whereas if it were only used to display human-readable names of plugins, rather than detect their actual presence, this concern becomes less significant.

In short, I'm wondering why it needs to be any more complicated than this:

/**
 * @Plugin(
 *   title = "Default fetcher",
 *   description = "Downloads data from a URL using Drupal's HTTP request handler."
 * )
 */

That said, I think this patch is a huge improvement over the previous one, and I could definitely get behind the idea of using annotations in this limited way. I'm skeptical about the memory improvements shown in @effulgentsia's test in #50 (unless someone can point me to a page in the Views UI that would reasonably want to display a list of all Views plugins of all types at once, rather than just a single type), so I think the actual memory savings here in a realistic use case are probably going to be more like a few MB than 18 MB. On the other hand, that's still significant in some cases.

In the case of block selection, you will need to display a list of all blocks, or at least a subset that can only be distilled down from the list of all blocks. Over time, there are likely to be more block plugins in the system than all views plugins combined.

That is true. On the other hand, in Drupal 7 most of the code for those blocks lives in .module files, so we currently load it into memory on every page request, not just on admin pages :)

However, I can see that going forward in Drupal 8, we might be able to save a fair amount of memory on the block administration page with this approach (compared to going forward in Drupal 8 with other approaches).

re: #122: @Translation + t()

The only reason why we're currently translating translatable strings in many info hooks is that we do not have a generic way to identify translatable strings for potx yet. One of the few hooks for which "native" support was hacked into potx is hook_menu() (including an even larger hack to even support hook_menu_alter()), and that's why we don't use t() in there. Why? Because it wouldn't make sense to maintain the always identical router info separately for each language.

So the stated reasons for why this would be a good idea are plain bugs in my book. The declarations/definitions of the plugins are not different per language. Their definitions are exactly the same. We essentially have multiple cache items that have to be properly maintained instead of one. The only time the translatable strings actually matter is when 1) you have a multilingual site, 2) you're administering something that uses plugins, and most importantly, 3) the current interface language is not English.

For further details see #1542144: Mark strings as localizable/translatable (new t()-alike string function that isn't t(), only for potx)

I agree with #126. Many info hooks are currently expected to call t(). Therefore, adding an @Translation annotation is a reasonable thing to do in this issue. A follow up that removes t() from info hooks and removes the @Translation annotation sounds lovely though.

Unassigning chx, because that was just a xpost in #116. Not reassigning to xjm, because I'm interpreting #117/#118 to mean she's not planning to implement those remaining docs issues.

For #119.1, is #1542144: Mark strings as localizable/translatable (new t()-alike string function that isn't t(), only for potx) a sufficient follow up?
For #119.2, I opened #1722860: Make @id optional (provide a default) in AnnotatedClassDiscovery.

I think that only leaves finishing the docs requested in #118. Am I right, or is this hung up on anything else?

Removing tags that no longer apply.

I went the extra mile, and:

1) applied the latest patch,
2) re-implanted the CacheDecorator from #107,
3) installed Language and Locale,
4) enabled an additional language,
5) translated the "Default fetcher" (whatever that means :P) into German,
6) hacked aggregator.admin.inc to also output the aggregator fetcher configuration widget in case there is only one,
7) and here's what I found:

1. CacheDecorator does not care for @Translation at all, which means that the identical plugin definitions are reloaded from cache, regardless of the current language. In short, the cache was primed in German, and I also saw the German plugin title when switching back to English.
2. CacheDecorator does not specify cache tags for the entries it creates.
3. CacheDecorator::__construct() defaults to a bogus $cache_bin = 'default', which does not exist and has to be changed to $cache_bin = 'cache' to make it cache anything in the first place.

Screenie:

So in essence, the plugin caching is not prepared at all for @Translation (and any other annotation).

I'm happy to move forward here, as long as this is properly tracked as a critical bug.

Done so: #1722882: Plugin CacheDecorator caches globally, ignores request context, and does not specify tags for cache items

Here's an interdiff against #117 for those following along at home :)

This is sort of like saying that they won't be able to effectively debug their code if they typo the hook they want to implement.

Whenever I have a hook that doesn't seem like it's doing anything, the first thing I do is put dsm('here') as the first line of the function to see if it's being called (or dsm(debug_backtrace()) if it seems like it's getting called but not when I expect it to be). Can't do that here. I think #1722860: Make @id optional (provide a default) in AnnotatedClassDiscovery is a sufficient followup for that though.

For #119.1, is #1542144: Mark strings as localizable/translatable (new t()-alike string function that isn't t(), only for potx) a sufficient follow up?

The @Translation thing seems like it might be more fundamental. I was actually pointing out something simpler than the issues mentioned above; I don't think we necessarily have to solve this for all info hooks, just that if you compare to .info files (which annotations seem most closely related to) we have no equivalent of @Translation there; instead, we just assume that we know 'name' and 'description' are the only keys that need translation and work with those.

The default getDefinitions() method here could certainly call t() on a similar hardcoded list (putting aside @sun's concerns which would apply to that as much as to @Translation). I think that would be a simpler thing to start with, personally, and extend it later only if necessary.... I mean, .info files have been around for a long time and I'm not aware of many (any?) contrib modules that have needed or wanted to put other human-readable strings in them besides the two above. I'd therefore suggest being wary of complicating what may be the 99% use case here. If we're wrong, it's not like it would be hard for someone to extend the class and do it differently.

A minor thing, but skimming through the patch I could not figure out why the "use...CacheDecorator" line needed to be added here:

--- a/core/modules/aggregator/lib/Drupal/aggregator/Plugin/FetcherManager.php
+++ b/core/modules/aggregator/lib/Drupal/aggregator/Plugin/FetcherManager.php
@@ -8,8 +8,9 @@
 namespace Drupal\aggregator\Plugin;
 
 use Drupal\Component\Plugin\PluginManagerBase;
-use Drupal\Core\Plugin\Discovery\HookDiscovery;
 use Drupal\Component\Plugin\Factory\DefaultFactory;
+use Drupal\Core\Plugin\Discovery\AnnotatedClassDiscovery;
+use Drupal\Core\Plugin\Discovery\CacheDecorator;
 
 /**
  * Manages aggregator fetcher plugins.
@@ -17,7 +18,7 @@ use Drupal\Component\Plugin\Factory\DefaultFactory;
 class FetcherManager extends PluginManagerBase {
 
   public function __construct() {
-    $this->discovery = new HookDiscovery('aggregator_fetch_info');
+    $this->discovery = new AnnotatedClassDiscovery('aggregator', 'fetcher');
     $this->factory = new DefaultFactory($this->discovery);
   }
 }

Either that's a mistake, or I'm just missing something about the patch.

Removed
use Drupal\Core\Plugin\Discovery\CacheDecorator;
for now, so let us please don't talk about caching here anymore.

Let's commit this and move forward.

@Translation is not perfect, but we *do* want a way to mark strings as being "translatable". Not having that in several places (hook_menu(), info files, etc.) lead to hardcoded special cases in potx, which is really not perfect. This does *not* mean that we should translate those strings right away. We could implement a delayed translation mechanism so that we cache only once, but translate *only the translatable strings* on demand.

All of that can be dealt with in follow-up issues. Please, let's stop derailing this any further.

Ok did some manual testing on my windows machine and with the views annotation. I have 2 problems.
I'm not sure this is not a views problem but I'll say it anyway.

1) On my windows machine I had to change
$class = str_replace(DIRECTORY_SEPARATOR, '\\', "$prefix/" . $fileinfo->getBasename('.php'));
to
$class = str_replace(DIRECTORY_SEPARATOR, '\\', "$prefix\\" . $fileinfo->getBasename('.php'));

2) With no caching my apache server cpu goes totally nuts on http://localhost/drupal8/admin/structure/views and related pages.
My pretty unoffical benchmark technique (aka look-at-your-watch-and-count-seconds method) messures 15+ seconds on my non optimized xamp setup.

[EDIT]
Ow its a views problem:
Drupal\Core\Plugin\Discovery\AnnotatedClassDiscovery->getDefinitions()
gets called 104 times :)
[EDIT2]
Hacked in the cache decorator in views and the performance issues dissapeared.

Aggregator plugin fetching went smoothly.

Fixed #141 it's a valid complaint. THanks for the testing.

Rerolling with some minor cleanups.

Regarding #141, do we have an issue filed for that in Views, and if so, can we link it here?

Edit: Let's also please file that followup issue for translations and link it here as well. Edit 2: Oh, it's #1542144: Mark strings as localizable/translatable (new t()-alike string function that isn't t(), only for potx)?

Mostly docs cleanups.

Followup issues:

• Translation detection issue: #1542144: Mark strings as localizable/translatable (new t()-alike string function that isn't t(), only for potx)
• ID key: #1722860: Make @id optional (provide a default) in AnnotatedClassDiscovery
• Cache Decorator issue: #1722882: Plugin CacheDecorator caches globally, ignores request context, and does not specify tags for cache items
• Standards / conflict with @endlink: #1722394: [policy] Determine standard for annotations and plugin documentation
• Views issue for #141: #1724048: Use of annotations for plugin discovery creates severe performance issues in views

I'll add this information to the issue summary.

minor fixes

Summary updated. Please add any additional followup issues as appropriate. Also those tags seem to keep reappearing. :)

Thanks for the additional docs and clean-ups. Looks good to me. Thanks all!

Updated issue summary.

Updated issue summary.

I think #1542144: Mark strings as localizable/translatable (new t()-alike string function that isn't t(), only for potx) is tangentially related to what I'm asking but not the same thing. And it also may be a very hard issue to solve.

This issue, meanwhile, isn't even about translation, but it's adding an entirely new translation method, and I'm asking whether that's really a good idea to do here. Especially given that it's used in a limited way (and will not work other places in the codebase, including .info files despite the fact that they're not PHP code either, and are otherwise very similar to how annotations will be used). We can't keep adding new, not-quite-complete requirements forever and expect developers to follow them.

As a possible compromise maybe we could look at getting t() itself working here... although the code required for that behind the scenes would be a little ugly. I'm out of ideas otherwise and have to head out for the rest of the day so can't look into that more now.

Reviewed one more time and committed to 8.x. Thanks for the great collaboration on this issue; and for spinning off several follow-up issues. Onwards!

Also, if there's a separate issue with the translations, then let's file that and link it here too.

Added http://drupal.org/node/1704454/revisions/view/2246978/2293898

New people are not going to get this... it's a PHP comment - with a funny new syntax - that actually does something? ...but it only works some of the time. Other times you still need to use an info hook? Ohboy...

New people are not going to get this... it's a PHP comment - with a funny new syntax - that actually does something?

Class annotations that specify information that affect functionality are not a Drupalism. Java developers, PHP developers who've used Zend, Symfony, etc. will already be familiar with this. But, yes, it will be new to people for whom Drupal is the first framework they're encountering that uses this.

but it only works some of the time. Other times you still need to use an info hook?

See #72. I agree there's still work to do to remove unnecessary inconsistencies and explain the necessary ones.

I can't find a change record?

http://drupal.org/list-changes/drupal?keywords_description=annotation&to...

GRR, serious WTF. Anyone know how to document these plugins? If I provide a base class that other modules extend, am I accidentally going to expose my (abstract) base class as a plugin while trying to document it? So far I haven't found a single place where the structure of the @Plugin() definitions are documented. Some put things like settings that are saved, others use a list of libraries that should be added via drupal_add_library(), lots of them provide titles and descriptions (but not all). Eventually these definitions are converted into arrays, mimicking what we previously had with hook_*_info(). However until hooks, there is NO place where any of these data structures are documented. Now that we have 300+ plugins in core, we're suffering a severe documentation shortage.

There's no change notification because this issue doesn't change anything, just adds a new plugin discovery method. The individual changes are each documented. For entity annotation info, see http://drupal.org/node/1827470 ; for blocks, see http://drupal.org/node/1880620 ; etc.

Also tagging a closed issue doesn't help because no one will see it. ;) I just happened to look because @quicksketch linked this issue

To follow up on my own comment, so far the best example of documenting these is for Entities, documented in the EntityManager class. Though I'm suggesting we move that to the Entity base class, where I think it could be more easily located. See #1881794: Link the EntityManager from the Entity base class (or EntityInterface?).

Oh, another thing that might help: Use "plugin" as your keyword for searching the change notices:
http://drupal.org/list-changes/drupal?keywords_description=plugin&to_bra...

#160: I thought this was the canonical issue for plugin annotations. What I am missing, is documentation for the syntax of plugin annotations. It's obvious for strings, booleans and associative arrays, but it's not clear for unordered (or are they numerically indexed?) arrays. Apparently the syntax is {"foo", "bar"}, but this is not documented *anywhere*. That's what I'd like to see a change record for.

Essentially, like http://drupal.org/node/1704454 documents the new plugin system that was introduced at #1497366: Introduce Plugin System to Core, I'd like to see a change record for annotations specifically. Alternatively, http://drupal.org/node/1704454 can be amended to include the full docs on the plugin annotation syntax.

This is a first version of a documentation, please help to improve it:

http://drupal.org/node/1882526

@dawehner: stellar! Thanks :) That'll help in easing the learning curve for hundreds if not thousands of Drupal developers!

@quicksketch

So what views does is a abstract PluginBase.php and a Standard.php, so you can document on the pluginBase but also have a working plugin in the other file, which does nothing.

So what views does is a abstract PluginBase.php and a Standard.php, so you can document on the pluginBase but also have a working plugin in the other file, which does nothing.

Thanks @dawehner. As far as I can tell, there's no documentation for how the annotation is structured in any of the Views base classes I've examined (PluginBase, HandlerBase, etc.). I was looking for something similar to xjm's documentation of the annotation plugin structure for entities, which is in the EntityManger class. In Views' base classes, the *methods* are documented, but what the keys of the plugin definition (as done in an annotation), I haven't been able to find anywhere. I couldn't find the Standard.php file you were describing with a sample plugin anywhere either, which may have been a valid substitute for the kind of documentation I was looking for.

@quicksketch, sounds like it would be good to add a sub-issue to #1856544: [META] Views documentation improvements. Significantly expanding the base plugin docs is in the scope of that issue, but it would be good to give suggestions for Views there.

Thanks @dawehner. As far as I can tell, there's no documentation for how the annotation is structured in any of the Views base classes I've examined (PluginBase, HandlerBase, etc.). I was looking for something similar to xjm's documentation of the annotation plugin structure for entities, which is in the EntityManger class. In Views' base classes, the *methods* are documented, but what the keys of the plugin definition (as done in an annotation), I haven't been able to find anywhere. I couldn't find the Standard.php file you were describing with a sample plugin anywhere either, which may have been a valid substitute for the kind of documentation I was looking for.

Yeah I was just talking about a theoretical approach, though there is an issue for that as well, sorry: #1882558: [META] Document all views plugins types

Let's create follow-up issues (using the plugin system component) for anything that needs clarification or further discussion.

AFAIK, there's a dedicated handbook page/section for the new plugin system. However, I also agree that the in-code DX for plugin-type-specific docs could be improved (and relocated to a standard location for all plugin types — ideally, the plugin's interface).

Also, if there's a separate issue with the translations, then let's file that and link it here too.

I decided to wait 8 months and see if it came up on its own. Or something like that :)

In any case, it sort of did... I believe #1966246: [meta] Introduce specific annotations for each plugin type is now the relevant followup for that issue, as well as some of the others which were brought up in the comments since then.

.