text:
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.
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
- The proposed restrictions above need to be documented somewhere.
- The plugin documentation should be updated to describe the use of annotations.
- There are a number of followup issues:
- #1542144: Mark strings as localizable/translatable (new t()-alike string function that isn't t(), only for potx)
- #1722860: Make @id optional (provide a default) in AnnotatedClassDiscovery
- #1722882: Plugin CacheDecorator caches globally, ignores request context, and does not specify tags for cache items
- #1722394: [policy] Determine standard for annotations and plugin documentation
- #1724048: Use of annotations for plugin discovery creates severe performance issues in views
- A change notification should be added.
See also
- #1675260: Implement PHP reading/writing secured against "leaky" script
- #211182: Updates run in unpredictable order
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.
PASSED: [[SimpleTest]]: [MySQL] 39,980 pass(es). View
PASSED: [[SimpleTest]]: [MySQL] 39,972 pass(es). View
Comment #3Damien Tournoud CreditAttribution: Damien Tournoud commented
+ AnnotationRegistry::registerAutoloadNamespace('Drupal\Core\Annotation', array(DRUPAL_ROOT . '/core/lib'));
You can probably stick a
AnnotationRegistry::registerLoader(array($loader, 'loadClass')); in
drupal_classloader() (that's what Symfony does).
// Register PEAR-style vendor namespaces.
// All Twig-borrowed code lives in /core/vendor/twig.
'Twig' => DRUPAL_ROOT . '/core/vendor/twig/twig/lib',
+ // All Symfony-borrowed code lives in /core/vendor/Symfony.
+ 'Symfony' => DRUPAL_ROOT . '/core/vendor',
+ 'Doctrine' => DRUPAL_ROOT . '/core/vendor',
Why is that necessary?
+ $namespaces = drupal_classloader()->getNamespaces();
+ foreach ($namespaces as $ns => $namespace_dirs) {
+ $ns = str_replace('\\', DIRECTORY_SEPARATOR, $ns);
+ foreach ($namespace_dirs as $dir) {
+ $prefix = implode(DIRECTORY_SEPARATOR, array(
+ $ns,
+ 'Plugins',
+ $this->owner,
+ $this->type
+ ));
+ $dir .= DIRECTORY_SEPARATOR . $prefix;
+ if (file_exists($dir)) {
+ foreach (new DirectoryIterator($dir) as $fileinfo) {
+ if ($fileinfo->getExtension() == 'php') {
+ $reflectionClass = new ReflectionClass(str_replace(DIRECTORY_SEPARATOR, '\\', "$prefix/". $fileinfo->getBasename('.php')));
+ if ($annotation = $reader->getClassAnnotation($reflectionClass, 'Drupal\Core\Annotation\Plugin')) {
+ $definition = $annotation->getDefinition();
+ $definition['class'] = $reflectionClass->name;
+ $definitions[$definition['plugin_id']] = $definition;
+ }
+ }
+ }
+ }
+ }
+ }
This hurts a little :) It requires us to do assumptions that are not in the PSR-0 specification. This kind of introspection should really be provided by the autoloader itself. The composer ClassLoader for example doesn't even have a
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.
Comment #4chx CreditAttribution: chx commented
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.
Comment #6RobLoach CreditAttribution: RobLoach commented
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.
Comment #9Dries CreditAttribution: Dries commented
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.
Comment #10catch CreditAttribution: catch commented
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.
Comment #12catch CreditAttribution: catch commented
Just a note Dries said _info() hooks, not .info files.
Comment #14chx CreditAttribution: chx commented
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?
Comment #16neclimdul CreditAttribution: neclimdul commented
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.
Comment #17chx CreditAttribution: chx commented
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.
Comment #18David_Rothstein CreditAttribution: David_Rothstein commented
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.
Comment #19neclimdul CreditAttribution: neclimdul commented
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?
Comment #20chx CreditAttribution: chx commented
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.
Comment #21chx CreditAttribution: chx commented
|Needs review
|» Reviewed & tested by the community
This is good to go as a first step.
Comment #22sun CreditAttribution: sun commented
|Start looking at plugin annotations as discovery
|» Annotations for plugin discovery
|» base system
|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.
Comment #23Dries CreditAttribution: Dries commented
|Annotations for plugin discovery
|» Start looking at plugin annotations as discovery
|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?
Comment #28dawehner CreditAttribution: dawehner commented
|Needs work
|» Needs review
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 ...
Comment #36chx CreditAttribution: chx commented
|Needs work
|» Needs review
Comment #38David_Rothstein CreditAttribution: David_Rothstein commented
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:
- 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?
- 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.
- 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?
Comment #39merlinofchaos CreditAttribution: merlinofchaos commented
BTW this is my simple argument in favor of annotations:
Comment #41tim.plunkett CreditAttribution: tim.plunkett commented
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 :)
Comment #42tim.plunkett CreditAttribution: tim.plunkett commented
|Needs review
|» Reviewed & tested by the community
I meant to mark this RTBC.
Comment #43David_Rothstein CreditAttribution: David_Rothstein commented
|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.
Comment #44tim.plunkett CreditAttribution: tim.plunkett commented
In addition to the technical arguments, I can attest that there is a huge benefit as far as DX goes.
Comment #45chx CreditAttribution: chx commented
|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.
Comment #46David_Rothstein CreditAttribution: David_Rothstein commented
|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?
Comment #47aspilicious CreditAttribution: aspilicious commented
So you're proposing some kind of "getInfo" discovery?
Comment #48catch CreditAttribution: catch commented
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.
Comment #49chx CreditAttribution: chx commented
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.
Comment #50effulgentsia CreditAttribution: effulgentsia commented
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?]
Comment #51aspilicious CreditAttribution: aspilicious commented
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...
Comment #52tim.plunkett CreditAttribution: tim.plunkett commented
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.
Comment #53effulgentsia CreditAttribution: effulgentsia commented
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.
Comment #54chx CreditAttribution: chx commented
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?
Comment #55sun CreditAttribution: sun commented
|Start looking at plugin annotations as discovery
|» Use Annotations for plugin discovery
|Issue tags:
|+Needs architectural review, +API addition
Trying once more...
Comment #57effulgentsia CreditAttribution: effulgentsia commented
+++ 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?
Comment #59chx CreditAttribution: chx commented
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.
Comment #62Dries CreditAttribution: Dries commented
|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
Comment #63tim.plunkett CreditAttribution: tim.plunkett commented
|Needs work
|» Needs review
PASSED: [[SimpleTest]]: [MySQL] 39,760 pass(es). View
Rerolled for #62.
Comment #64tim.plunkett CreditAttribution: tim.plunkett commented
That had trailing whitespace and some other oddities.
Comment #65sun CreditAttribution: sun commented
+++ 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
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" ?
Comment #66tim.plunkett CreditAttribution: tim.plunkett commented
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.
Comment #67sun CreditAttribution: sun commented
@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)
Comment #68sun CreditAttribution: sun commented
+++ 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].)
Comment #69effulgentsia CreditAttribution: effulgentsia commented
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?
Comment #70effulgentsia CreditAttribution: effulgentsia commented
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?
Comment #71Crell CreditAttribution: Crell commented
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.
Comment #72effulgentsia CreditAttribution: effulgentsia commented
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.
Comment #73sun CreditAttribution: sun commented
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.
4) getInfo() methods [where needed],
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).
Comment #74merlinofchaos CreditAttribution: merlinofchaos commented
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.
Comment #75effulgentsia CreditAttribution: effulgentsia commented
|Needs review
|» Needs work
#1698108: Update Drupal's dependencies went in, so let's also update the patch to use the tokenizer.
Comment #77chx CreditAttribution: chx commented
I'll do #75 today
Comment #78chx CreditAttribution: chx commented
|Needs work
|» Needs review
FAILED: [[SimpleTest]]: [MySQL] 39,820 pass(es), 36 fail(s), and 46 exception(s). View
Comment #79chx CreditAttribution: chx commented
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.
Comment #81chx CreditAttribution: chx commented
|Needs work
|» Needs review
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.
Comment #82.0aspilicious CreditAttribution: aspilicious commented
Tried to make a summary
Comment #82.1aspilicious CreditAttribution: aspilicious commented
changed tags
Comment #82.2aspilicious CreditAttribution: aspilicious commented
Added not
Comment #83chx CreditAttribution: chx commented
|Needs work
|» Needs review
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.
Comment #85chx CreditAttribution: chx commented
|Needs work
|» Needs review
|979 bytes
PASSED: [[SimpleTest]]: [MySQL] 39,904 pass(es). View
Gah! I have fixed that but apparently didn't run git add -u.
Comment #86chx CreditAttribution: chx commented
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.
Comment #87aspilicious CreditAttribution: aspilicious commented
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
Comment #88tim.plunkett CreditAttribution: tim.plunkett commented
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.
Comment #89tim.plunkett CreditAttribution: tim.plunkett commented
Wrong patch
Comment #90chx CreditAttribution: chx commented
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).
Comment #92chx CreditAttribution: chx commented
|Needs work
|» Needs review
|718 bytes
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: PHP Fatal error encountered during run_tests.sh. See review log for details.. View
Comment #94chx CreditAttribution: chx commented
|695 bytes
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 ?
Comment #95tim.plunkett CreditAttribution: tim.plunkett commented
|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.
Comment #97chx CreditAttribution: chx commented
|Needs work
|» Needs review
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.
Comment #99aspilicious CreditAttribution: aspilicious commented
SearchCommentTest Drupal\search\Tests\SearchCommentTest->testSearchResultsCommentAccess()
Random failures again... Argh...
Comment #100tim.plunkett CreditAttribution: tim.plunkett commented
|Needs work
|» Needs review
Comment #101chx CreditAttribution: chx commented
Note: Comment Search tests 193 passes, 0 fails, 0 exceptions, and 66 debug messages. I tested manually.
Comment #102tim.plunkett CreditAttribution: tim.plunkett commented
|Needs review
|» Needs work
Yay, it passed! Back to needs work for #95.
Comment #103chx CreditAttribution: chx commented
|Needs work
|» Needs review
PASSED: [[SimpleTest]]: [MySQL] 39,976 pass(es). View
Fixed #95 (although the kept the @todo for getExtension as it is just fixed the spelling).
Comment #104tim.plunkett CreditAttribution: tim.plunkett commented
|Needs review
|» Reviewed & tested by the community
We need to codify the results of #73-74.
To recap:
- Metadata should be reserved for data that is used outside of the class instantiation.
- 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.
Comment #105aspilicious CreditAttribution: aspilicious commented
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)
Comment #106damiankloip CreditAttribution: damiankloip commented
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.
Comment #107aspilicious CreditAttribution: aspilicious commented
|808 bytes
PASSED: [[SimpleTest]]: [MySQL] 39,982 pass(es). View
Ugly interdiff but it should point out I only changed some docs...
Comment #108sun CreditAttribution: sun commented
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'.
Comment #109catch CreditAttribution: catch commented
|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.
Comment #110aspilicious CreditAttribution: aspilicious commented
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.
Comment #111aspilicious CreditAttribution: aspilicious commented
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)
Comment #112sun CreditAttribution: sun commented
|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.
Comment #113xjm CreditAttribution: xjm commented
|Needs work
|» Needs review
I filed #1722394: [policy] Determine standard for annotations and plugin documentation. Please update/correct that issue's summary as appropriate.
Comment #114xjm CreditAttribution: xjm commented
|Needs review
|» Needs work
Comment #115xjm CreditAttribution: xjm commented
Comment #116Dries CreditAttribution: Dries commented
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.
Comment #117xjm CreditAttribution: xjm commented
|Needs work
|» Needs review
- 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
@todosfor 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.
Comment #118xjm CreditAttribution: xjm commented
|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.
Comment #119David_Rothstein CreditAttribution: David_Rothstein commented
- 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.
- Following up on @effulgentsia's comment in #70, why do we need the "id" key at all? As he said, we should be able to figure it out from the location in the filesystem, right? This would also address my earlier comment that we shouldn't be letting this code fail silently if it's performing important functionality. (The code as written will still fail silently in many situations, e.g. if you mistype "@Plugin" as "Plugin" your plugin will disappear with no warning whatsoever and no way to directly debug it.) Whereas if it were only used to display human-readable names of plugins, rather than detect their actual presence, this concern becomes less significant.
In short, I'm wondering why it needs to be any more complicated than this:
* @Plugin(
* title = "Default fetcher",
* description = "Downloads data from a URL using Drupal's HTTP request handler."
* )
That said, I think this patch is a huge improvement over the previous one, and I could definitely get behind the idea of using annotations in this limited way. I'm skeptical about the memory improvements shown in @effulgentsia's test in #50 (unless someone can point me to a page in the Views UI that would reasonably want to display a list of all Views plugins of all types at once, rather than just a single type), so I think the actual memory savings here in a realistic use case are probably going to be more like a few MB than 18 MB. On the other hand, that's still significant in some cases.
Comment #120merlinofchaos CreditAttribution: merlinofchaos commented
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.
Comment #121David_Rothstein CreditAttribution: David_Rothstein commented
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).
Comment #125sun CreditAttribution: sun commented
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)
Comment #127effulgentsia CreditAttribution: effulgentsia commented
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.
Comment #129effulgentsia CreditAttribution: effulgentsia commented
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?
Comment #130effulgentsia CreditAttribution: effulgentsia commented
Removing tags that no longer apply.
Comment #131sun CreditAttribution: sun commented
|» chx
|Issue tags:
|+Needs architectural review
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:
- 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.
- CacheDecorator does not specify cache tags for the entries it creates.
- 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.
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
Comment #135tim.plunkett CreditAttribution: tim.plunkett commented
Here's an interdiff against #117 for those following along at home :)
Comment #136David_Rothstein CreditAttribution: David_Rothstein commented
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.
Comment #137David_Rothstein CreditAttribution: David_Rothstein commented
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.
Comment #139aspilicious CreditAttribution: aspilicious commented
use Drupal\Core\Plugin\Discovery\CacheDecorator;
for now, so let us please don't talk about caching here anymore.
Comment #140Damien Tournoud CreditAttribution: Damien Tournoud commented
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.
Comment #141aspilicious CreditAttribution: aspilicious commented
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'));
$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.
Ow its a views problem:
gets called 104 times :)
Hacked in the cache decorator in views and the performance issues dissapeared.
Aggregator plugin fetching went smoothly.
Comment #142chx CreditAttribution: chx commented
|Needs review
|» Reviewed & tested by the community
PASSED: [[SimpleTest]]: [MySQL] 39,980 pass(es). View
Fixed #141 it's a valid complaint. THanks for the testing.
Comment #143xjm CreditAttribution: xjm commented
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)?
Comment #144xjm CreditAttribution: xjm commented
|» Unassigned
Mostly docs cleanups.
Followup issues:
- Translation detection issue: #1542144: Mark strings as localizable/translatable (new t()-alike string function that isn't t(), only for potx)
- ID key: #1722860: Make @id optional (provide a default) in AnnotatedClassDiscovery
- Cache Decorator issue: #1722882: Plugin CacheDecorator caches globally, ignores request context, and does not specify tags for cache items
- Standards / conflict with
@endlink: #1722394: [policy] Determine standard for annotations and plugin documentation
- Views issue for #141: #1724048: Use of annotations for plugin discovery creates severe performance issues in views
I'll add this information to the issue summary.
Comment #144.0xjm CreditAttribution: xjm commented
minor fixes
Comment #145xjm CreditAttribution: xjm commented
Summary updated. Please add any additional followup issues as appropriate. Also those tags seem to keep reappearing. :)
Comment #146sun CreditAttribution: sun commented
Thanks for the additional docs and clean-ups. Looks good to me. Thanks all!
Comment #146.0sun CreditAttribution: sun commented
Updated issue summary.
Comment #146.1xjm CreditAttribution: xjm commented
Updated issue summary.
Comment #147David_Rothstein CreditAttribution: David_Rothstein commented
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.
Comment #148Dries CreditAttribution: Dries commented
|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!
Comment #150xjm CreditAttribution: xjm commented
|Use Annotations for plugin discovery
|» Change notification for: Use Annotations for plugin discovery
|» Critical
|» Active
|Issue tags:
|+Needs change record
Comment #151xjm CreditAttribution: xjm commented
Also, if there's a separate issue with the translations, then let's file that and link it here too.
Comment #152sun CreditAttribution: sun commented
Comment #153chx CreditAttribution: chx commented
|» Fixed
Comment #154Tor Arne Thune CreditAttribution: Tor Arne Thune commented
|Change notification for: Use Annotations for plugin discovery
|» Use Annotations for plugin discovery
|» Normal
|Issue tags:
|-Needs change record
Comment #156jenlampton CreditAttribution: jenlampton commented
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...
Comment #157effulgentsia CreditAttribution: effulgentsia commented
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.
Comment #158Wim Leers CreditAttribution: Wim Leers commented
I can't find a change record?
Comment #159quicksketch CreditAttribution: quicksketch commented
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.
Comment #160xjm CreditAttribution: xjm commented
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
Comment #161quicksketch CreditAttribution: quicksketch commented
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?).
Comment #162xjm CreditAttribution: xjm commented
Oh, another thing that might help: Use "plugin" as your keyword for searching the change notices:
Comment #163Wim Leers CreditAttribution: Wim Leers commented
|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.
Comment #164dawehner CreditAttribution: dawehner commented
This is a first version of a documentation, please help to improve it:
Comment #165Wim Leers CreditAttribution: Wim Leers commented
@dawehner: stellar! Thanks :) That'll help in easing the learning curve for hundreds if not thousands of Drupal developers!
Comment #166dawehner CreditAttribution: dawehner commented
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.
Comment #167quicksketch CreditAttribution: quicksketch commented
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.
Comment #168xjm CreditAttribution: xjm commented
@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.
Comment #169dawehner CreditAttribution: dawehner commented
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
Comment #170sun CreditAttribution: sun commented
|» 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).
Comment #171sun CreditAttribution: sun commented
|» plugin system
Comment #172David_Rothstein CreditAttribution: David_Rothstein commented
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.
Comment #172.0David_Rothstein CreditAttribution: David_Rothstein commented


text:
A pet owner is "distraught" after her four-year-old dog Ted was stolen outside a supermarket on Saturday.
Philomena Hynes of North Finchley tied Ted, her Cavalier King Charles Spaniel, to a railing outside Tesco Express in Friern Barnet Lane, New Southgate, while she bought groceries at 4.25pm.
Mrs Hynes said: "I was only in the shop for about three minutes. When I got to the counter I turned around to say to Ted, don't worry mummy's coming, but he wasn't there.
"The lead was still tied to the railing but there was no Ted. I was completely distraught."
Mrs Haynes searched the area but found no sign of her dog, which she has had since he was just nine weeks old.
She said: "It was terrible to have to go home without him. He's my life. My little Ted is my companion, he goes everywhere with me."
Ted is a sandy colour and is described as a " very friendly"and well known in the local area. He wears a black leather collar with small diamante studs.
Mrs Haynes, who volunteers in the North London Hospice store in Finchley High Road, has put up more than 24 posters offering a £500 reward for Ted's safe return.
She said: "I just want him back. I'm devastated and so upset."
Anyone with information is asked to contact police.

subset: commoncrawl


text:
SNMPv3 setup
The first thing we have to do to use SNMPv3 is to set it up. Now, if you're really good with snmpd.conf syntax... well, you don't need this article. However, rather than hand-editing the conf file, we're going to take advantage of a handy utility called snmpconf. (Warning for the Terminal-phobic: SNMP requires a lot of command-line work. There's no way to really avoid it. If you're an administrator who still fears Terminal and the command line, you're going to need to move past that.) snmpconf is a Perl script designed to make setting up snmp easier—something which it's pretty good at.
One nice thing, is that if you use snmpconf with the -i switch, (
snmpconf -i), it will even put the various configuration files you create and modify in their correct locations for you. Handy, no? Of course, since you're modifying system-level configurations, you have to run snmpconf via sudo to have the -i switch work correctly, so you have:
sudo /usr/bin/snmpconf -i
When you run this, if there are any existing configuration files, (and there most likely will be), you'll be asked if you want snmpconf to read in the data from one, some, or none of those files. I prefer to start clean, so I usually pick none. (If you already have a functional SNMP setup and are adding SNMPv3 to it, pick the more appropriate option for your setup.) Enter
none and hit return. Next, you're going to be asked what files you want to modify: snmpd.conf, snmptrapd.conf, or snmp.conf. Since we want to set up how the system responds to SNMPv3 requests from other systems, we want to modify the configuration of the SNMP daemon, snmpd. Therefore, we're going to select option 1—snmpd.conf:
The next screen will give you all the options available for snmpd. Since we're trying to keep this at least somewhat small, and the other information is available elsewhere, we're going to ignore all the options that don't involve setting up SNMPv3. So, we're going to choose option 1, Access Control Setup, because that's where we start configuring our SNMPv3 users:
Once we've done this, we see we have two options for SNMPv3 setup; a read-write user and a read-only user. Since you can use SNMP to set values, not just get them, it is important to use a good password for the read-write user. As an example, a lot of networks use this setting ability, via the snmpset command, to disable ports on a switch if they detect a computer that's been corrupted by malware, and is now trying to do inappropriate things on the network, like setting up IRC servers. So you have to give careful consideration to what the read-write user's credentials will be. One thing you can do, and it's not a bad idea, is to have different user logins for read-write and read-only. Monitoring software that won't need to change settings only needs read-only access.
With all that in mind, let's set up our read-write user, option 1:
We get an example of the arguments we'll eventually use here, and then we're asked for the user name. Only enter the user name here. I have learned, painfully, over the years that getting clever with snmpconf leads to madness and bad config files. Also note that snmpconf is telling you what user you're configuring, namely rwuser:
For our example, we're calling this user "r-wuser," just to make what we're doing more clear. Next, you're asked the minimum security level required for that user with three options: noauth, auth, and priv, with the default being auth. The difference between the three is what kind of security is used for authentication. noauth is what you get with SNMPv1 and v2-no encryption; plus, as long as what you enter matches the password/community string, it's good. auth uses either HMAC MDF or SHA to authenticate, so it's quite a bit more secure than string matching. priv takes the mechanism used by auth, and encrypts it with DES for even more security. My preference here is to use priv. If you're going to use noauth, there's not much of a point in using SNMPv3. Enter the desired security level and hit return:
Next, you're asked what OID you want to restrict this user to. As a quick background, an OID is the container for the value you're requesting. So if you want to know uptime, you'd request the value in the 188.8.131.52.18.104.22.168.0 OID. In some cases, you may want to really lock down access to a device or devices for SNMP read-write access to a specific OID. If so, you'd enter it here. I have yet to have a need for this, so I just hit return, which grants access to all OIDs. That's the last step in configuring the SNMPv3 read-write user, and you return back to the Access Control Setup screen:
Setting up the read-only user is identical to setting up the read-write user, so we won't cover that in detail. Obviously, you'll want to use a different user name for read-only than for read-write. Once you're done, enter
finished, hit return, and you move back to the snmpd config main screen:
finished again, and you're back to the snmpconf main screen:
If you enter
quit here, snmpconf will write your selections to the snmpd.conf file, and move it to the correct locations. If you want to change anything before that happens, this is your last chance to do so without repeating every step. If you're happy, then enter
quit and hit return. snmpconf will tell you what happened, and where it put the files.


text:
Simple Squash and Sausage Recipe
"Dinner doesn't get any easier than this stovetop meal for two that I created one night," relates Lela Rauch from Oakland, Michigan. "My husband insisted that I remember the colorful combination so that I could make it again."
- 1/2 pound smoked sausage, cut into 1/4-inch slices
- 1 tablespoon olive oil
- 1 medium zucchini, cut into 1/4-inch slices
- 1 medium yellow summer squash, cut into 1/4-inch slices
- 2 tablespoons apricot preserves
- 1. In a large skillet, saute sausage in oil until lightly browned; remove and keep warm. In the same skillet, saute zucchini and yellow squash until crisp tender. Return sausage to the pan. Stir in preserves. Cook and stir until heated through. Yield: 2 servings.
Nutritional Facts
1 each: 495 calories, 38g fat (14g saturated fat), 76mg cholesterol, 1295mg sodium, 23g carbohydrate (18g sugars, 3g fiber), 18g protein.
Reviews for Simple Squash and Sausage
© 2017 RDA Enthusiast Brands, LLC


text:
Court Ruling Keeps Medical Marijuana Patients' Records Confidential
California Supreme Court Refuses to Review Appellate Decision
Medical Marijuana patients and their doctors scored another victory this week when the California Supreme Court refused to review an appellate decision blocking the California Medical Board from searching the records of Dr. David Bearman.
The decision protects doctors and patients in possession of medical marijuana (as codified in Proposition 215) from violation of their privacy rights by law enforcement and the California Medical Board
"This is a wonderful victory which sustains the sanctity of the doctor-patient relationship, privacy of patient records and provides the court common sense affirmation that a letter of approval is no more an invitation to go fishing in a patient's records than is showing a law enforcement person your prescription," wrote Dr. Bearman in comments to supporters. "It is another building block in the legal wall of support for Proposition 215."
In April 2001, a patient who admitted to possessing medical marijuana after being stopped by park rangers at Lake Piru Recreation Area, showed rangers a letter from Dr. David Bearman that attested to his legal right to possess the drug under Proposition 215. The proposition protects seriously and terminally ill patients from criminal penalties for using marijuana medically.
The subpoena alleged that Dr. Bearman had "indiscriminately" recommended the use of medical marijuana to the patient. The Medical Board of California attempted to access the patient's records, but both the patient and doctor refused to turn over the records, citing doctor-patient confidentiality. The court ruled that the medical board "failed to demonstrate sufficient facts to support a finding of good cause to invade the patient's right of privacy" and threw out the case.
"The California Supreme Court refused to sabotage the principle of client patient confidentiality," said Judy Appel, acting director of legal affairs for the Drug Policy Alliance. "The California Medical Board should know better than to bring law enforcement into the doctor's office. This ruling is a victory for sick patients and the physicians who have their best interests at heart."
Dr. Bearman was represented by Seymour Weisberg, Alison Adams of The Chase Law Group, and Joe Allen of Ambrecht and Associates. Dr. Bearman was also supported by the California Medical Association, which filed an amicus brief in the case.
Over the last year, there have been numerous legal victories in both the federal and state courts for California's medical marijuana patients. For example, just this week a federal appeals court ordered the release of Bryan James Eppis, the first medical marijuana patient convicted in federal court after the passage of California's Proposition 215. In another case, People v. Spark, a conservative California appellate court confirmed that California's Compassionate Use Act does not require a patient to be "seriously ill" to use medical marijuana. An appellate panel reversed the conviction of Noel Spark, who was originally sentenced to probation and 6 months in the county jail for having 3 cannabis plants.