Convert Aggregator Fetchers to Plugins

On it.

Just did a skim but it looks awesome. Very straight forward. Thanks!

+++ b/core/modules/aggregator/aggregator.moduleundefined
@@ -620,9 +621,11 @@ function aggregator_refresh($feed) {
+  $success = $fetcher_factory->createInstance($fetcher)->fetch($feed);

It seems like this should probably be wrapped in a try/catch

Completely unrelated because I see they're copy and paste but so I remember to follow up, these seem like minor bugs:

+++ b/core/modules/aggregator/lib/Drupal/aggregator/Plugin/aggregator/fetcher/DefaultFetcher.phpundefined
@@ -0,0 +1,63 @@
+  function fetch($feed) {

This should be by reference even if it is an object.

+++ b/core/modules/aggregator/lib/Drupal/aggregator/Plugin/aggregator/fetcher/DefaultFetcher.phpundefined
@@ -0,0 +1,63 @@
+    return $feed->source_string === FALSE ? FALSE : TRUE;

um... why? This should just be: return !($feed->source_string === FALSE); ?>

Went ahead and made the changes along with the "kitten" because they don't really change anything.

+++ b/core/modules/aggregator/aggregator.admin.inc
+use Drupal\aggregator\Plugin\Type\FetcherType;

+++ b/core/modules/aggregator/aggregator.module
+use Drupal\aggregator\Plugin\Type\FetcherType;

+++ b/core/modules/aggregator/lib/Drupal/aggregator/Plugin/Type/FetcherInterface.php
+ * Definition of Drupal\aggregator\Plugin\Type\FetcherInterface.

+++ b/core/modules/aggregator/lib/Drupal/aggregator/Plugin/Type/FetcherType.php
+ * Definition of Drupal\aggregator\Plugin\Type\FetcherType.

+++ b/core/modules/aggregator/lib/Drupal/aggregator/Plugin/aggregator/fetcher/DefaultFetcher.php
+ * Definition of Drupal\aggregator\Plugin\aggregator\fetcher\DefaultFetcher.

The additional "Type" component in the namespace looks superfluous to me.

+++ b/core/modules/aggregator/aggregator.module
@@ -594,19 +596,18 @@ function aggregator_remove($feed) {
 function _aggregator_get_variables() {
...
   $fetcher = variable_get('aggregator_fetcher', 'aggregator');

@@ -620,9 +621,11 @@ function aggregator_refresh($feed) {
-  // Fetch the feed.
   list($fetcher, $parser, $processors) = _aggregator_get_variables();
-  $success = module_invoke($fetcher, 'aggregator_fetch', $feed);

Do we want to update _aggregator_get_variables() to no longer return $fetcher?

+++ b/core/modules/aggregator/aggregator.module
@@ -789,6 +792,19 @@ function _aggregator_items($count) {
 /**
+ * Implements hook_aggregator_fetch_info().
+ */
+function aggregator_aggregator_fetch_info() {
+  return array(
+    'aggregator' => array(
+      'class' => 'Drupal\aggregator\Plugin\aggregator\fetcher\DefaultFetcher',
+      'title' => t('Default fetcher'),
+      'description' => t('Downloads data from a URL using Drupal\'s HTTP request handler.'),
+    ),
+  );
+}

I thought we'd get rid of info hooks...?

+++ b/core/modules/aggregator/lib/Drupal/aggregator/Plugin/Type/FetcherInterface.php
@@ -0,0 +1,33 @@
+ * A fetcher downloads feed data to a Drupal site. The fetcher is called at the
+ * first of the three aggregation stages: first, data is downloaded by the
+ * active fetcher; second, it is converted to a common format by the active
+ * parser; and finally, it is passed to all active processors, which manipulate
+ * or store the data.

Everything after the first sentence belongs into some high-level architectural module/group documentation (in code), which we probably need to (re-)design/standardize for the new plugin system first.

Totally a separate issue, just mentioning.

+++ b/core/modules/aggregator/lib/Drupal/aggregator/Plugin/Type/FetcherInterface.php
@@ -0,0 +1,33 @@
+   * @param $feed
+   *   A feed object representing the resource to be downloaded. $feed->url
+   *   contains the link to the feed. Download the data at the URL and expose it
+   *   to other modules by attaching it to $feed->source_string.

This seems to be a Configurable Thingie™. I can't wait to convert that. ;) (@see #1668820: Concept, base class, and interface for Configurables (e.g., node types, image styles, vocabularies, views, etc))

+++ b/core/modules/aggregator/lib/Drupal/aggregator/Plugin/aggregator/fetcher/DefaultFetcher.php
@@ -0,0 +1,63 @@
+namespace Drupal\aggregator\Plugin\aggregator\fetcher;

The second "aggregator" is definitely wrong in this namespace.

That makes sense, thanks :)

1) I'm confident the identical questions will come up in the main issue, so "be warned" ;) -- perhaps there's a way to address that upfront; e.g., via (temporary or not) in-code docs.

2) Still not sure about the extra "Type" part in the namespace. I wasn't thinking about the extra directory, only about the duplication of Type within the class name. In order to produce a potential name clash, something Type-alike would have to be located within the parent \Plugin space. I can't think of anything, do you?

I actually think it is good clean-up.

However, there are a couple of unexpected things for me:

+++ b/core/modules/aggregator/aggregator.admin.incundefined
@@ -442,16 +443,11 @@ function aggregator_admin_form($form, $form_state) {
+  $fetcher_lister = new FetcherType();
+  $fetchers = array();
+  foreach ($fetcher_lister->getDefinitions() as $id => $definition) {
+    $label = $definition['title'] . ' <span class="description">' . $definition['description'] . '</span>';

It is a bit odd that an instance of a 'FetcherType' returns an object that returns a list. I wonder if 'FetcherType' is best renamed to 'FetcherFactory' or something?

+++ b/core/modules/aggregator/aggregator.api.phpundefined
@@ -11,58 +11,29 @@
+    'aggregator' => array(
+      'class' => 'Drupal\aggregator\Plugin\aggregator\fetcher\DefaultFetcher',
+      'title' => t('Default fetcher'),

It would be nice if we could get rid of the 'aggregator' in the middle of the path. Not sure that makes sense though.

+++ b/core/modules/aggregator/aggregator.api.phpundefined
@@ -11,58 +11,29 @@
+      'title' => t('Default fetcher'),
+      'description' => t('Downloads data from a URL using Drupal\'s HTTP request handler.'),

I expected the title and the description to be provided by DefaultFetcher() through two getters; getTitle() and getDescription().

+++ b/core/modules/aggregator/aggregator.moduleundefined
@@ -620,9 +621,11 @@ function aggregator_refresh($feed) {
+  // Fetch the feed.
+  $fetcher_factory = new FetcherType();

Again, I find it weird that FetcherType creates a factory. Better to rename FetcherType to FetcherFactory.

+++ b/core/modules/aggregator/aggregator.moduleundefined
@@ -789,6 +792,19 @@ function _aggregator_items($count) {
+  return array(
+    'aggregator' => array(
+      'class' => 'Drupal\aggregator\Plugin\aggregator\fetcher\DefaultFetcher',
+      'title' => t('Default fetcher'),
+      'description' => t('Downloads data from a URL using Drupal\'s HTTP request handler.'),
+    ),

This duplication would be avoided if we had getTitle() and getDescription() as part of DefaultFetcher.

I echo Eclipse: It's *very* important that title and description and any other properties needed for "generate a list of these" do not require instantiating the class; when you have potentially hundreds of plugins, that's a lot of memory we don't want to use just to generate a list of titles and descriptions of all of them.

Also, what duplication are you talking about? The duplication you see is from a documentation function? Perhaps the documentation hook should document 'Example fetcher' instead of 'Default fetcher'?

While I do have issues with 'PluginType' as a name, it's more than just a Factory because more complicated plugin systems will do more than just getDefinition and Instance off of it. I posited Controller but it was pointed out to me that it's not an MVC Controller so that name might not be suitable. Not sure what that leaves us. I'd go with Overlord but I don't think anybody but me would laugh.

Possible words that I can think of off the top of my head: Manager, Handler, Arbiter.

Manager isn't too bad. Handler would make me sad because Views has a bunch of handlers but we're already going to have a bit of a terminology problem here anyway.

It would be nice if we could get rid of the 'aggregator' in the middle of the path. Not sure that makes sense though.

Yeah, I actually went into a bit of detail on this in my response to sun. The aggregator in the middle is in anticipation of getting annotations committed, and is actually very useful outside of the defining module because it tells us what module defined the plugin type that handles plugins in this directory. But ultimately, that directory is a supporting directory for the annotation based plugins as we move that direction.

It'd probably make more sense if the Plugin namespace are CamelCased
Plugins\Aggregator\Fetcher

I expected the title and the description to be provided by DefaultFetcher() through two getters; getTitle() and getDescription().

This duplication would be avoided if we had getTitle() and getDescription() as part of DefaultFetcher.

Right that could maybe be handled that way but there's a logic to the way we handle metadata like this. I'll take a bit different approach to explaining it. Plugins generally revolve around this metadata because that's the metadata we use to build UI's and sometimes functionality. For example, a common plugin type EclipseGC will be making is a layout and the plugin would look something like this:

twocol.yaml

title: Two Column
description: A simple two column layout with equal widths.
css: twocol.css
js: twocol.js
template: twocol.tpl.php

In this example you'll notice there's no class to pass around or pull metadata off of. There will be a default class that all plugins use that gets passed this metadata and interacts with the layout rendering code. And when the UI is running only the metadata is of any concern to us.

So what's to take from this is the need for the DiscoveryInterface to separate the implementation/storage of the metadata from the code requesting it. So then we have options for implementations like the yaml file I made, an info hook, the really cool annotation reader chx wrote that doesn't load code into memory, a static or set of static methods on the plugin... really the sky's the limit but for the common case I'm going to be suggesting chx's annotation reader we're working on getting ready as a follow up.

Hope that clarifies the info hook.

As far as the type discussion goes, this is the first time its really come up. It has seemed to be pretty clear since its holding the logic for the type. I think this is something we can discuss better in the reviews eclipse is working on setting up though.

It is a bit odd that an instance of a 'FetcherType' returns an object that returns a list. I wonder if 'FetcherType' is best renamed to 'FetcherFactory' or something?

Again, I find it weird that FetcherType creates a factory. Better to rename FetcherType to FetcherFactory.

While I do have issues with 'PluginType' as a name, it's more than just a Factory because more complicated plugin systems will do more than just getDefinition and Instance off of it.

Yep, this keeps on surfacing as something Dries and others find unintuitive. As I said in #1497366-150: Introduce Plugin System to Core:

how we can make the whole concept of a plugin type (or container/lister/whoozywhatsit) clear. Essentially, we have a registry and a factory rolled up into one object, and even someone as smart as Dries is getting tripped up on that.

Even for simple types, a plugin type is more than just a factory, because getDefinitions() is not a common feature of factories, at least as I understand what factories typically are in other systems. And as merlinofchaos points out, some plugin types will contain additional functionality appropriate to the particular features of that system. The approach I took in #2 was to name the class FetcherType (since it's neither just a lister, nor just a factory, and "Type" is the only word we've come up with so far to capture the combination of things this class does), but to assign the object to the local variables $fetcher_lister and $fetcher_factory, since in those local contexts, the FetcherType object was only being used for the more limited aspect reflected in those local variable names. But it doesn't look like this successfully resolved the confusion around this.

Still not sure about the extra "Type" part in the namespace.

I think it would be good to come up with a convention for how to organize 3 kinds of things that modules may want to implement related to plugins:
- plugins
- plugin types
- plugin system components (e.g., alternate factories, alternate discovery methodologies)

What #2 did was:
- put plugins in Drupal\[module implementing the plugin]\Plugin\[module implementing the plugin type this plugin is for]\[name of the plugin type this plugin is for]\[name of the plugin]
- put plugin types in Drupal\[module implementing the plugin type]\Plugin\Type\[name of the plugin type]
- no plugin system components needed for aggregator, but if it were, I would have put it into Drupal\[module implementing the plugin system component]\Plugin\[Discovery|Factory|etc.]\[some descriptive class name]

Perhaps a better convention would be 3 top-level (within Drupal\$module) namespaces: Plugin, PluginType, PluginComponent? For example:
- Drupal\aggregator\PluginType\FetcherType
- Drupal\aggregator\Plugin\aggregator\fetcher\DefaultFetcher
- If there were a component: Drupal\aggregator\PluginComponent\Discovery\SomeCustomDiscovery

Any other ideas?

It'd probably make more sense if the Plugin namespace are CamelCased
Plugins\Aggregator\Fetcher

Not sure if this was intentional, but the above suggestion uses Plugins (plural) whereas I stuck with singular, because with the exception of "Tests", we have so far stuck with singular namespace names. I'm open to plural if someone wants to make a compelling case for it. I don't think "because there's often more than one" is a compelling enough case, because many of our namespaces contain more than one class of a similar kind but the namespace is still singular (e.g., Drupal\Core\EventSubscriber).

As far as capitalizing Aggregator, I disagree, because what this part of the namespace refers to is the module "aggregator", and elsewhere, we've agreed to retain lowercase when referring literally to the module name.

As far as capitalizing Fetcher, this is an interesting question, because currently the only string identifier of the "fetcher plugin type" is the class name FetcherType, so this alone would support capitalization. However, CTools has a concept of lowercase identifiers for plugin types, and while that concept doesn't exist yet in the plugins-next branch, it has re-emerged in the plugins-annotations branch, because AnnotatedClassDiscovery needs to be given a module and type, and currently both are lowercase by convention. We can change that, but I think keeping it lowercase would be more consistent if we ever want to use plugin type identifiers as part of a machine name.

I think with the exception of sun's comments that where lost a while back I think this is mostly terminology discussions. I don't want to loose the feedback here but maybe this should have its own issue?

My major concerns have been addressed by clarifications actually.

The remaining concerns are on:

- namespace names (e.g., /Plugin/Type/FetcherType vs. /Plugin/FetcherType)

- *Type plugin classes being both the type definition, definitions/variant/derivative lister, and factory.

Frankly, I believe that both of them will be much more easy to resolve, once we've converted some more stuff throughout core, see it live, and see the big picture. There's nothing easier than to move/rename files ;)

Per #15 and discussion with Dries, neclimdul, and EclipseGc, we renamed Type to Manager within class names. I also removed the Type subnamespace from the aggregator use case and tests.

http://drupalcode.org/sandbox/eclipsegc/1441840.git/commitdiff/dde4875

Suns concerns are addressed and Dries concerns I think are addressed. Seems like we need a follow up core issue tagged revisit before release on the namespace issue and I think we can move this back to fixed.

Thanks for cleaning up the names effulgentsia.

Seems like we need a follow up core issue tagged revisit before release on the namespace issue

I don't think we even need this quite yet. Once an initial patch lands and people start converting systems over, the namespace structure topic will naturally come up, be discussed, and if necessary, a dedicated issue for it will get made.

just opened: #1930274: Convert aggregator processors and parsers to plugins