Problem/Motivation

In drupal we have several ways to collect metadata. We mostly use hooks. When using hooks the metadata and the actual code that uses the data are seperated. To improve dx we would like to capture the metadata inside the code that needs it.

In the current testing system we use a getInfo() function inside each class to collect the data. The problem with this approach is that we need to put every test file into memory. For tests this is/was acceptable because developer environments where tests are typically run are much less memory constrained than live sites.

For plugins we also need such a discovery mechanism. And yes we don't want to use hooks :)
But we can't use the getInfo() method because it would require loading every class in memory and so affect our memory consumption too much. Also it would make documentation much harder as it would require parsing PHP code to get it out.

Proposed resolution

Use the doctrine annotation mechanism to fetch the data. In Doctrine 2.2 it had the same problems as the getInfo() approach, it had to load every single file into memory. It's likely that Drupal will contain lots of plugins so memory usage would be sky high .

Chx made a patch for doctrine 2.3 that uses the php tokenizer. This approach consumes more cpu but it's memory usage isn't related to the number of plugins. Instead it has a peak memory related to the filesize. As Psr0 contains lots of small files with only a single class in it this shouldn't be a problem. Chx thinks this should give an average memory peak of 700kb for each file.

Restrictions
With a annotation mechanism in place, some might put too much inside it. Thats why we should define properly what metadata can be defined with annotations.

A possible agreement: (merlinofchaos after a views irc discussion)
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.

Remaining tasks

See also

API changes

Annotations are available as a mechanism for plugin discovery (replacing info hooks).

Original report by EclipseGc

chx has done some very excellent work getting annotations+directory based discovery working. I wanted to expose this to the community at large, however this patch includes a bunch of symfony updates that were delivered through compiler. This issue is affecting me directly at the moment #1683046: Add the Doctrine Common PHP library. Once that's committed this patch will get much saner.

I've provided a full patch and an interdiff against the plugins-next branch of http://drupal.org/sandbox/eclipsegc/1441840

The 8.x branch of http://drupal.org/sandbox/eclipsegc/1663586 should work with this patch now.

Eclipse

Files: 
CommentFileSizeAuthor
#144 annotations-1683644-143.patch11.43 KBxjm
PASSED: [[SimpleTest]]: [MySQL] 39,984 pass(es).
[ View ]
#144 interdiff.txt5.03 KBxjm
#142 1683644-138.patch12.57 KBchx
PASSED: [[SimpleTest]]: [MySQL] 39,980 pass(es).
[ View ]
#139 1683644-138.patch12.56 KBaspilicious
PASSED: [[SimpleTest]]: [MySQL] 39,972 pass(es).
[ View ]
#135 interdiff.txt4.04 KBtim.plunkett
#134 annotations.patch11.46 KBEclipseGc
PASSED: [[SimpleTest]]: [MySQL] 39,984 pass(es).
[ View ]
#132 annotations.patch12.41 KBEclipseGc
PASSED: [[SimpleTest]]: [MySQL] 0 pass(es).
[ View ]
#131 plugin-cachedecorator-language.png47.38 KBsun
#117 annotations-1683644-117.patch10.54 KBxjm
PASSED: [[SimpleTest]]: [MySQL] 39,984 pass(es).
[ View ]
#117 interdiff.txt4.45 KBxjm
#111 1683644-111.patch11.36 KBaspilicious
PASSED: [[SimpleTest]]: [MySQL] 39,988 pass(es).
[ View ]
#107 1683644-106.patch14.95 KBaspilicious
PASSED: [[SimpleTest]]: [MySQL] 39,982 pass(es).
[ View ]
#107 annotations-interdif.txt808 bytesaspilicious
#103 1683644-103.patch13.45 KBchx
PASSED: [[SimpleTest]]: [MySQL] 39,976 pass(es).
[ View ]
#103 interdiff.txt2.57 KBchx
#97 1683644-96.patch13.32 KBchx
PASSED: [[SimpleTest]]: [MySQL] 39,976 pass(es).
[ View ]
#97 interdiff.txt2.02 KBchx
#94 1683644-94.patch11.23 KBchx
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: PHP Fatal error encountered during run_tests.sh. See review log for details..
[ View ]
#94 interdiff.txt695 byteschx
#92 1683644-92.patch10.47 KBchx
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: PHP Fatal error encountered during run_tests.sh. See review log for details..
[ View ]
#92 interdiff.txt718 byteschx
#90 1683644-89.patch10.49 KBchx
FAILED: [[SimpleTest]]: [MySQL] 39,904 pass(es), 1 fail(s), and 0 exception(s).
[ View ]
#90 interdiff.txt3.41 KBchx
#89 annotations-1683644-88.patch10.56 KBtim.plunkett
PASSED: [[SimpleTest]]: [MySQL] 39,896 pass(es).
[ View ]
#88 drupal-1175764-20.patch9.61 KBtim.plunkett
PASSED: [[SimpleTest]]: [MySQL] 39,900 pass(es).
[ View ]
#88 interdiff.txt4.73 KBtim.plunkett
#86 1683644-85.patch10.31 KBchx
PASSED: [[SimpleTest]]: [MySQL] 39,901 pass(es).
[ View ]
#86 interdiff.txt4.21 KBchx
#85 1683644-84.patch9.81 KBchx
PASSED: [[SimpleTest]]: [MySQL] 39,904 pass(es).
[ View ]
#85 interdiff.txt979 byteschx
#83 1683644-82.patch9.82 KBchx
FAILED: [[SimpleTest]]: [MySQL] 39,923 pass(es), 0 fail(s), and 27 exception(s).
[ View ]
#83 interdiff.txt2.22 KBchx
#81 1683644-80.patch9.85 KBchx
FAILED: [[SimpleTest]]: [MySQL] 39,903 pass(es), 3 fail(s), and 0 exception(s).
[ View ]
#81 interdiff.txt1.72 KBchx
#79 1683644-79.patch9.86 KBchx
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: PHP Fatal error encountered during run_tests.sh. See review log for details..
[ View ]
#79 interdiff.txt2.18 KBchx
#78 1683644-78.patch8.82 KBchx
FAILED: [[SimpleTest]]: [MySQL] 39,820 pass(es), 36 fail(s), and 46 exception(s).
[ View ]
#78 interdiff.txt1.63 KBchx
#64 drupal-1683644-64.patch8.63 KBtim.plunkett
PASSED: [[SimpleTest]]: [MySQL] 39,764 pass(es).
[ View ]
#63 drupal-1683644-63.patch8.41 KBtim.plunkett
PASSED: [[SimpleTest]]: [MySQL] 39,760 pass(es).
[ View ]
#50 test.txt2.65 KBeffulgentsia
#35 annotations.patch8.38 KBEclipseGc
PASSED: [[SimpleTest]]: [MySQL] 39,758 pass(es).
[ View ]
#33 annotations.patch7.65 KBEclipseGc
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: PHP Fatal error encountered during run_tests.sh. See review log for details..
[ View ]
#31 annotations.patch7.61 KBEclipseGc
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: PHP Fatal error encountered during run_tests.sh. See review log for details..
[ View ]
#29 annotations.patch7.49 KBEclipseGc
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: PHP Fatal error encountered during run_tests.sh. See review log for details..
[ View ]
#28 1683644.patch7.63 KBdawehner
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: PHP Fatal error encountered during run_tests.sh. See review log for details..
[ View ]
#24 annotations.patch7.62 KBEclipseGc
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: PHP Fatal error encountered during run_tests.sh. See review log for details..
[ View ]
#5 annotations.patch4.74 KBEclipseGc
PASSED: [[SimpleTest]]: [MySQL] 37,290 pass(es).
[ View ]
#2 annotations.patch886.42 KBEclipseGc
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch annotations_0.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
interdiff.patch813.73 KBEclipseGc
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch interdiff_12.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
annotations.patch887.32 KBEclipseGc
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch annotations.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Comments

EclipseGc’s picture

A working branch for this can be found here:
git clone --recursive --branch 8.x-annotations http://git.drupal.org/sandbox/eclipsegc/1441840.git drupal

Eclipse

EclipseGc’s picture

StatusFileSize
new886.42 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch annotations_0.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

did the patch backwards.

Damien Tournoud’s picture

<?php
+    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).

<?php
    
// 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?

<?php
+    $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.

chx’s picture

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.

EclipseGc’s picture

StatusFileSize
new4.74 KB
PASSED: [[SimpleTest]]: [MySQL] 37,290 pass(es).
[ View ]

Haven't taken any of the comments into consideration yet, but the patch is MUCH smaller and more manageable now.

Eclipse

RobLoach’s picture

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.

EclipseGc’s picture

As an example http://drupal.org/sandbox/eclipsegc/1663586 8.x branch is working against this patch currently.

EclipseGc’s picture

Status:Active» Needs review

I'm going to move this to NR for testing purposes and to keep this moving forward. Hoping to maybe get this in core before Midwest Drupal Summit. Seems simple enough I think it might be possible.

Eclipse

Dries’s picture

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.

catch’s picture

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.

EclipseGc’s picture

@Dries,

I can appreciate that stance, I think my counter to that is pretty simple.

  1. No we don't NEED annotation, but I know that merlin (for one) would really really like to have a cohesive solution like this for having all information about a given plugin in one spot. I'll ask him to comment here as well.
  2. This patch is only 4 files, the discovery and the 3 annotation classes I've identified thus far (full disclosure I've only thought of 1 more annotation class I want, but it's related to the context/data sources discussion and is a ways off in any event).
  3. I'm not holding up development of the blocks stuff on this. I got it to this point in a relatively short period of time (largely due to chx's leg work on it while we were still discussing plugins) and I'm developing against it currently. If we ultimately decide NOT to include this, I'll be disappointed, but I'm also willing to gamble the blocks patch against it knowing I might have to swap out discovery methods before this goes into core.

In short, I'd like this in ASAP, but if that doesn't happen, it's not slowing me down, so we can take our time with this discussion if that makes you more comfortable. With regard to info files, I can sort of see what you're getting at here, but at the same time, I think moving modules in general to a OO paradigm is probably a huge undertaking. I'm sort of viewing plugins as being a place where we can experiment some with these different ideas (due to the pluggable discovery) and figure out what works best. Annotations seem a really clear winner, but likewise, there's nothing to prevent us from building a discovery specific to something like info files. I'm just rambling at this point, I think the point I was trying to make was that this is the last clear discovery solution we have defined, and that the only people who will need to worry about it are people getting familiar with the plugin system which is already a pretty big change in thinking, so having something else that continues to be a bit different (and yet hopefully really interesting, and useful) doesn't feel like that big of a disconnect. If you disagree with that, I totally understand, and like I said, I'm not holding up development on this patch, it just felt small enough, and non-invasive enough to perhaps get a fast commit against.

catch’s picture

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

EclipseGc’s picture

He went on to mention how some of what's in annotations lives in .info files currently. That's all I was trying to address.

chx’s picture

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?

EclipseGc’s picture

Also, I was chatting with some Doctrine people last night and they told me Zend Framework has actually adopted this at this point as well, so that's both of the big frameworks we have really considered using exactly this approach.

neclimdul’s picture

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.

chx’s picture

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.

David_Rothstein’s picture

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.

neclimdul’s picture

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?

chx’s picture

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.

chx’s picture

Status:Needs review» Reviewed & tested by the community

This is good to go as a first step.

sun’s picture

Title:Start looking at plugin annotations as discovery» Annotations for plugin discovery
Component:other» base system
Status:Reviewed & tested by the community» Needs work
Issue tags:+API addition

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.

Dries’s picture

Title:Annotations for plugin discovery» Start looking at plugin annotations as discovery
Component:base system» other
Issue tags:-API addition

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?

EclipseGc’s picture

Status:Needs work» Needs review
StatusFileSize
new7.62 KB
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: PHP Fatal error encountered during run_tests.sh. See review log for details..
[ View ]

An updated based upon Dries' comments, includes switching aggregator fetchers to annotations.

Eclipse

Status:Needs review» Needs work
Issue tags:-Blocks-Layouts

The last submitted patch, annotations.patch, failed testing.

EclipseGc’s picture

Status:Needs work» Needs review

#24: annotations.patch queued for re-testing.

Status:Needs review» Needs work
Issue tags:+Blocks-Layouts

The last submitted patch, annotations.patch, failed testing.

dawehner’s picture

Status:Needs work» Needs review
StatusFileSize
new7.63 KB
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: PHP Fatal error encountered during run_tests.sh. See review log for details..
[ View ]
+++ 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 ...

EclipseGc’s picture

StatusFileSize
new7.49 KB
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: PHP Fatal error encountered during run_tests.sh. See review log for details..
[ View ]

My bad, here's a few more fixes.

Status:Needs review» Needs work

The last submitted patch, annotations.patch, failed testing.

EclipseGc’s picture

Status:Needs work» Needs review
StatusFileSize
new7.61 KB
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: PHP Fatal error encountered during run_tests.sh. See review log for details..
[ View ]

Trying this again.

Status:Needs review» Needs work

The last submitted patch, annotations.patch, failed testing.

EclipseGc’s picture

Status:Needs work» Needs review
StatusFileSize
new7.65 KB
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: PHP Fatal error encountered during run_tests.sh. See review log for details..
[ View ]

Trying out a hunch tim.plunkett has with regard to the directory iterator in a foreach loop.

Status:Needs review» Needs work

The last submitted patch, annotations.patch, failed testing.

EclipseGc’s picture

StatusFileSize
new8.38 KB
PASSED: [[SimpleTest]]: [MySQL] 39,758 pass(es).
[ View ]

OK, tim.plunkett determined that this was a php version issue. We need to be running php 5.3.6+ in order to take advantage of DirectoryIterator::getExtension(). In order to facilitate this for the moment I wrote a ridiculously simple class that extends DirectoryIterator and provides a custom getExtension() method. This can be removed once the requirements for D8 are upgraded.

Eclipse

chx’s picture

Status:Needs work» Needs review
EclipseGc’s picture

YAY! We have passing test! (Thanks chx for updating the status)

David_Rothstein’s picture

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?

merlinofchaos’s picture

BTW this is my simple argument in favor of annotations:

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

EclipseGc’s picture

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?

This seems unlikely. A typo in the annotation is likely to either throw an exception, or result in any error you can imagine a typo causing in an info hook. These are basically the two options by and large.

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.

I'll admit to not knowing much about this topic, chances are the Doctrine folks have some clear documentation on how this should be handled, and/or whether this is even a problem.

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?

I think we need to define "dynamic text" a bit. If we're discussing something that might be similar to a foreach statement in an info hook, then a derivative class (which is the appropriate way to handle this) takes care of that just fine. If we're discussing translatable text, then the @Translation() class for the purposes of annotations is utilized. Ultimately we should be able to make various annotation classes that wrap the sort of logic problems we might have and give us options to solve them. Logic itself may not be doable within the annotation, but Doctrine's class based approach to annotation parsing means we can do an awful lot of what we need.

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?

chx's code is actually in Doctrine 2.3, we are running 2.2, so none of his work has actually be implemented for our benefit yet. We need to upgrade to Doctrine 2.3 first (and we need that to be able to use constants in the annotation anyway).

Eclipse

tim.plunkett’s picture

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

tim.plunkett’s picture

Status:Needs review» Reviewed & tested by the community

I meant to mark this RTBC.

David_Rothstein’s picture

Status:Reviewed & tested by the community» Needs review

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.

tim.plunkett’s picture

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

chx’s picture

Status:Needs review» Reviewed & tested by the community

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.

David_Rothstein’s picture

Status:Reviewed & tested by the community» Needs review

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?

aspilicious’s picture

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

catch’s picture

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.

chx’s picture

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.

effulgentsia’s picture

Issue tags:+VDC
StatusFileSize
new2.65 KB

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

aspilicious’s picture

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

tim.plunkett’s picture

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.

effulgentsia’s picture

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.

chx’s picture

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?

sun’s picture

Title:Start looking at plugin annotations as discovery» Use Annotations for plugin discovery
Issue tags:+Needs architectural review, +API addition

Trying once more...

EclipseGc’s picture

Status:Needs review» Reviewed & tested by the community

Had a discussion with David Rothstein with regard to his objections and specifically focussed on APC/opcode caches in general. I popped into the #doctrine channel and asked and stof told me APC should work, but that eAccelerator is broken. I don't think this negatively affects and, and David said he wouldn't object too loudly if I re-rtbc'd this issue. If anyone still feels there are additional concerns here, then don't hesitate to move this away from RTBC.

Eclipse

effulgentsia’s picture

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

EclipseGc’s picture

Those all sound like excellent question, none of which I honestly know the answer to. :-S Let me see what I can learn.

chx’s picture

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.

EclipseGc’s picture

#35: annotations.patch queued for re-testing.

EclipseGc’s picture

This is still passing even after the composer stuff has been committed, so I'm going to leave it RTBC

Dries’s picture

Status:Reviewed & tested by the community» Needs work

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

tim.plunkett’s picture

Status:Needs work» Needs review
StatusFileSize
new8.41 KB
PASSED: [[SimpleTest]]: [MySQL] 39,760 pass(es).
[ View ]

Rerolled for #62.

tim.plunkett’s picture

StatusFileSize
new8.63 KB
PASSED: [[SimpleTest]]: [MySQL] 39,764 pass(es).
[ View ]

That had trailing whitespace and some other oddities.

sun’s picture

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

tim.plunkett’s picture

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.

sun’s picture

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

sun’s picture

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

effulgentsia’s picture

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?

effulgentsia’s picture

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?

Crell’s picture

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.

effulgentsia’s picture

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.

sun’s picture

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

merlinofchaos’s picture

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.

effulgentsia’s picture

Status:Needs review» Needs work

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

EclipseGc’s picture

That's a REALLY great point. I have a patch I've been sitting on (too much OTHER work) I'll try to work this is as well.

chx’s picture

Assigned:Unassigned» chx

I'll do #75 today

chx’s picture

Status:Needs work» Needs review
StatusFileSize
new1.63 KB
new8.82 KB
FAILED: [[SimpleTest]]: [MySQL] 39,820 pass(es), 36 fail(s), and 46 exception(s).
[ View ]
chx’s picture

StatusFileSize
new2.18 KB
new9.86 KB
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: PHP Fatal error encountered during run_tests.sh. See review log for details..
[ View ]

That could be optimized a little.

Status:Needs review» Needs work

The last submitted patch, 1683644-79.patch, failed testing.

chx’s picture

Status:Needs work» Needs review
StatusFileSize
new1.72 KB
new9.85 KB
FAILED: [[SimpleTest]]: [MySQL] 39,903 pass(es), 3 fail(s), and 0 exception(s).
[ View ]

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.

Status:Needs review» Needs work

The last submitted patch, 1683644-80.patch, failed testing.

aspilicious’s picture

Issue summary:View changes

Tried to make a summary

aspilicious’s picture

Issue summary:View changes

changed tags

aspilicious’s picture

Issue summary:View changes

Added not

chx’s picture

Status:Needs work» Needs review
StatusFileSize
new2.22 KB
new9.82 KB
FAILED: [[SimpleTest]]: [MySQL] 39,923 pass(es), 0 fail(s), and 27 exception(s).
[ View ]

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.

Status:Needs review» Needs work

The last submitted patch, 1683644-82.patch, failed testing.

chx’s picture

Status:Needs work» Needs review
StatusFileSize
new979 bytes
new9.81 KB
PASSED: [[SimpleTest]]: [MySQL] 39,904 pass(es).
[ View ]

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

chx’s picture

StatusFileSize
new4.21 KB
new10.31 KB
PASSED: [[SimpleTest]]: [MySQL] 39,901 pass(es).
[ View ]

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.

aspilicious’s picture

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

tim.plunkett’s picture

StatusFileSize
new4.73 KB
new9.61 KB
PASSED: [[SimpleTest]]: [MySQL] 39,900 pass(es).
[ View ]

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.

tim.plunkett’s picture

StatusFileSize
new10.56 KB
PASSED: [[SimpleTest]]: [MySQL] 39,896 pass(es).
[ View ]

Wrong patch

chx’s picture

StatusFileSize
new3.41 KB
new10.49 KB
FAILED: [[SimpleTest]]: [MySQL] 39,904 pass(es), 1 fail(s), and 0 exception(s).
[ View ]

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

Status:Needs review» Needs work

The last submitted patch, 1683644-89.patch, failed testing.

chx’s picture

Status:Needs work» Needs review
StatusFileSize
new718 bytes
new10.47 KB
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: PHP Fatal error encountered during run_tests.sh. See review log for details..
[ View ]

Sigh.

Status:Needs review» Needs work

The last submitted patch, 1683644-92.patch, failed testing.

chx’s picture

StatusFileSize
new695 bytes
new11.23 KB
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: PHP Fatal error encountered during run_tests.sh. See review log for details..
[ View ]

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

tim.plunkett’s picture

Status:Needs work» Needs review
+++ 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.

Status:Needs review» Needs work

The last submitted patch, 1683644-94.patch, failed testing.

chx’s picture

Status:Needs work» Needs review
StatusFileSize
new2.02 KB
new13.32 KB
PASSED: [[SimpleTest]]: [MySQL] 39,976 pass(es).
[ View ]

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.

Status:Needs review» Needs work

The last submitted patch, 1683644-96.patch, failed testing.

aspilicious’s picture

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

Random failures again... Argh...

tim.plunkett’s picture

Status:Needs work» Needs review

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

chx’s picture

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

tim.plunkett’s picture

Status:Needs review» Needs work

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

chx’s picture

Status:Needs work» Needs review
StatusFileSize
new2.57 KB
new13.45 KB
PASSED: [[SimpleTest]]: [MySQL] 39,976 pass(es).
[ View ]

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

tim.plunkett’s picture

Status:Needs review» Reviewed & tested by the community

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.

aspilicious’s picture

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)

damiankloip’s picture

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.

aspilicious’s picture

StatusFileSize
new808 bytes
new14.95 KB
PASSED: [[SimpleTest]]: [MySQL] 39,982 pass(es).
[ View ]

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

sun’s picture

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

catch’s picture

Status:Reviewed & tested by the community» Needs review

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.

aspilicious’s picture

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.

aspilicious’s picture

StatusFileSize
new11.36 KB
PASSED: [[SimpleTest]]: [MySQL] 39,988 pass(es).
[ View ]

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)

sun’s picture

Status:Needs review» Needs work
+++ 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.

xjm’s picture

Status:Needs work» Needs review

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

xjm’s picture

Status:Needs review» Needs work

xpost

xjm’s picture

Assigned:chx» xjm

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

Dries’s picture

Assigned:xjm» chx

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.

xjm’s picture

Status:Needs work» Needs review
StatusFileSize
new4.45 KB
new10.54 KB
PASSED: [[SimpleTest]]: [MySQL] 39,984 pass(es).
[ View ]

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.

xjm’s picture

Status:Needs review» Needs 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.

David_Rothstein’s picture

  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:

<?php
/**
 * @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.

merlinofchaos’s picture

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.

David_Rothstein’s picture

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

EclipseGc’s picture

Re: Translations

I'm not sure I see the point is processing translations separately every time they need displaying. Some plugin types (blocks for instance) will likely have hundreds of implementations in a typical system, translating all the various titles and descriptions every time they're displayed instead of simply asking for a different language cache of blocks seems like a lot of additional overhead to me, especially when we expect to typically have a cache for any plugin type anyway. Menu trees are currently cached per language. It makes sense to similarly cache the per-language output of plugin definitions.

I'll see what I can do about xjm's todos once the patch passes.

EclipseGc’s picture

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. (...) Whereas if it were only used to display human-readable names of plugins, rather than detect their actual presence, this concern becomes less significant.

The id key is to prevent us from having to know the full PSR-0 name of any class we ever want to implement. Programmatically it might not matter a lot for any system that is actually using discovery, but the second you, as a developer want to start manually calling individual plugins, it becomes a MUCH nicer way to deal with thing. Our only other identifier is the full psr-0 class name, and that's decidedly less friendly.

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.

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.

EclipseGc’s picture

also, the plugin id is the main connector for derivatives. If we remove that, we have to rethink that methodology completely.

sun’s picture

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)

EclipseGc’s picture

@sun

Would you be willing to make that a follow up issue, I don't think it will actually affect the implementation of the plugin types wanting to utilize the annotation system, and this way we have a minimally viable solution in case your end goal is never fully addressed? Since it doesn't fundamentally change how modules interface with the system, that really sounds like an optimization that we'd like to make for drupal in general, not annotations specifically (it's just perhaps easier to address in annotations than elsewhere?). I'd really like to not hold this patch up on this topic if you don't feel it's absolutely necessary to do so before anyone can start using it.

Eclipse

effulgentsia’s picture

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.

EclipseGc’s picture

@effulgentsia

Thanks for cutting through to what I was trying to say. Essentially Annotations can't solve this alone, all of drupal needs to have this problem solved, that's sort of part of the notion of how getDefinitions() is always consistent regardless of the discovery method.

Eclipse

effulgentsia’s picture

Assigned:chx» Unassigned

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?

effulgentsia’s picture

Removing tags that no longer apply.

sun’s picture

Assigned:Unassigned» chx
Issue tags:+Needs architectural review
StatusFileSize
new47.38 KB

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:

plugin-cachedecorator-language.png

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

EclipseGc’s picture

Status:Needs work» Needs review
StatusFileSize
new12.41 KB
PASSED: [[SimpleTest]]: [MySQL] 0 pass(es).
[ View ]

OK, it sounds like we're all on the same page here. I've mentioned in various plugin talks before that the CacheDecorator needs some additional love (and apparently tests) so I'm happy to see that happening, thank you sun.

Added some docs, I think I got them all.

Eclipse

EclipseGc’s picture

err... got some run-tests.sh stuff in there, sorry... fixing

EclipseGc’s picture

StatusFileSize
new11.46 KB
PASSED: [[SimpleTest]]: [MySQL] 39,984 pass(es).
[ View ]

OK, sorry about that

tim.plunkett’s picture

StatusFileSize
new4.04 KB

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

David_Rothstein’s picture

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.

David_Rothstein’s picture

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.

EclipseGc’s picture

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

Bah, fair point, we should ultimately be caching these things, so the annotated class discovery class should be wrapped in a cache decorator class, but as sun mentioned, there are issues there yet. We can take it out, and put it back once we have it fixed, it will ultimately be part of that class, I'm unsure if that matters at the moment. Happy to remove it if the general consensus is that we should.

The @Translation thing seems like it might be more fundamental...

So, I just want to address this again here publicly. There's been a lot of discussion about how we could just [insert something about a small hard coded expectation here] which is really sort of the opposite of what plugins expect. We don't want to limit what you can put there, likewise I don't really want to limit what is translatable either because I have no idea of all the potential use cases, but given the amount this has inherited from ctools, there are likely a lot of situations in which we will need more than any of the proposals I've heard. I would REALLY encourage us to sideline this conversation about hardcoded expectations until D9. I freely admit that I could be wrong in my expectations, but I honestly don't believe that I am, and a cycle of proof (not to mention converting a bunch of core) would do us all good in either proving me wrong or not. Again, we expect there to be a cache wrapped around this stuff, so it should be minimally impactful, however... preemptively limiting what developers CAN do with the system sounds like a REALLY bad idea to me. As a compromise I'd also be willing to revisit this topic after many core plugins types have been converted, but I'm very uncomfortable with limiting annotation based discovery in ways that no other discovery system is likely to be limited. It seems very counter intuitive to do that to me, and ultimately reduced the flexibility and usefulness of the solution.

Eclipse

aspilicious’s picture

StatusFileSize
new12.56 KB
PASSED: [[SimpleTest]]: [MySQL] 39,972 pass(es).
[ View ]

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

Damien Tournoud’s picture

Assigned:chx» Unassigned
Status:Needs review» Reviewed & tested by the community

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.

aspilicious’s picture

Assigned:Unassigned» chx
Status:Reviewed & tested by the community» Needs review

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.

chx’s picture

Status:Needs review» Reviewed & tested by the community
StatusFileSize
new12.57 KB
PASSED: [[SimpleTest]]: [MySQL] 39,980 pass(es).
[ View ]

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

xjm’s picture

Assigned:chx» xjm

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

xjm’s picture

xjm’s picture

Issue summary:View changes

minor fixes

xjm’s picture

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

sun’s picture

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

sun’s picture

Issue summary:View changes

Updated issue summary.

xjm’s picture

Issue summary:View changes

Updated issue summary.

David_Rothstein’s picture

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.

Dries’s picture

Status:Reviewed & tested by the community» Fixed

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!

EclipseGc’s picture

In talking with chx, I made a passing mention that I had run the Translation class past gabor before ever submitting a patch that included it on this issue. This isn't to say that he has approved the entire use case front to back, but he's definitely seen and directed the Translation class to some degree. chx asked me to post that here for posterity sake.

This isn't to say that the translation approach is perfect or anything, just that gabor has not been completely out of the loop on it.

Eclipse

xjm’s picture

Title:Use Annotations for plugin discovery» Change notification for: Use Annotations for plugin discovery
Priority:Normal» Critical
Status:Fixed» Active
Issue tags:+Needs change record
xjm’s picture

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

sun’s picture

Issue tags:+Plugin system
chx’s picture

Tor Arne Thune’s picture

Title:Change notification for: Use Annotations for plugin discovery» Use Annotations for plugin discovery
Priority:Critical» Normal
Issue tags:-Needs change record

Status:Fixed» Closed (fixed)

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

jenlampton’s picture

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

effulgentsia’s picture

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.

Wim Leers’s picture

quicksketch’s picture

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.

xjm’s picture

Issue tags:-Needs change record

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

quicksketch’s picture

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

xjm’s picture

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

Wim Leers’s picture

Status:Closed (fixed)» Active

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

dawehner’s picture

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

http://drupal.org/node/1882526

Wim Leers’s picture

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

dawehner’s picture

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

quicksketch’s picture

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.

xjm’s picture

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

dawehner’s picture

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

sun’s picture

Status:Active» Closed (fixed)

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

sun’s picture

Component:other» plugin system
David_Rothstein’s picture

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.

David_Rothstein’s picture

Issue summary:View changes

.