Convert aggregator processors and parsers to plugins

Only skimmed over it but looks nice.

+++ /dev/nullundefined
@@ -1,192 +0,0 @@
-/**
- * @file
- * Documentation for aggregator API.

Why are we removing the api documentation?

Nice patch, didn't read everything but awesome clean-up, just a few notes...

+++ b/core/modules/aggregator/aggregator.moduleundefined
@@ -313,18 +313,22 @@ function aggregator_cron() {
-  db_update('aggregator_feed')
-    ->fields(array('queued' => 0))
+  $result = entity_query('aggregator_feed')
     ->condition('queued', REQUEST_TIME - (3600 * 6), '<')

Off topic, but it's amazing that you can just switch a db_select() (well, in this case it's db_update but the result is the same) with entity_query() and. it. just. works. Even if it's a simple one as in this case.

+++ b/core/modules/aggregator/aggregator.moduleundefined
@@ -421,19 +423,7 @@ function aggregator_remove(Feed $feed) {
 function _aggregator_get_variables() {
   $config = config('aggregator.settings');
-  $fetcher = $config->get('fetcher');
-
-  $parser = $config->get('parser');
-  if ($parser == 'aggregator') {
-    include_once DRUPAL_ROOT . '/' . drupal_get_path('module', 'aggregator') . '/aggregator.parser.inc';
-  }
-
-  $processors = $config->get('processors');
-  if (in_array('aggregator', $processors)) {
-    include_once DRUPAL_ROOT . '/' . drupal_get_path('module', 'aggregator') . '/aggregator.processor.inc';
-  }
-
-  return array($fetcher, $parser, $processors);
+  return array($config->get('fetcher'), $config->get('parser'), $config->get('processors'));

Wondering if we want to kill this function and access the config directly where it's used? No need for this hack anymore...

+++ b/core/modules/aggregator/aggregator.moduleundefined
@@ -564,55 +562,6 @@ function aggregator_filter_xss($value) {
-function aggregator_sanitize_configuration() {

Is this still checked somewhere or do we no longer care? It could still happen that a module that provides a processor is disabled... but we don't do this for other plugins either I guess. And if the plugin no longer exits then I guess we can fall back to the default in the manager? (haven't actually checked if we do that)

+++ b/core/modules/aggregator/tests/lib/Drupal/aggregator_test/Plugin/aggregator/parser/TestParser.phpundefined
@@ -0,0 +1,33 @@
+   * Implements \Drupal\aggregator\Plugin\ParserInterface::parse().
+   * @todo Actually test this.

Nitpick, can we have a space between the Implements and @todo lines? ;)

Removing it sounds fine to me, as long as we don't crash if something is missing. Views is also simply displaying a "plugin missing" message, no need to do something different here I think.

#17: drupal-aggregator_plugins-1930274-17.patch queued for re-testing.

Nit-pick mode...

+++ b/core/modules/aggregator/aggregator.moduleundefined
@@ -398,42 +403,17 @@ function aggregator_save_category($edit) {
+  // Call ProcessorInterface::remove() on all processors.

This should probably use the fully qualified namespace so that api.drupal.org (and a manual reader) can resolve it. Starting with a \

+++ b/core/modules/aggregator/aggregator.moduleundefined
@@ -446,15 +426,29 @@ function aggregator_refresh(Feed $feed) {
+    // @todo Maybe add a watchdog entry?

Yep, I'd suggest to add a watchdog_exception('aggregator', $e) here. Is almost easier than adding a @todo, so we can IMHO just do it here.

+++ b/core/modules/aggregator/aggregator.moduleundefined
@@ -464,43 +458,49 @@ function aggregator_refresh(Feed $feed) {
+        if (@count($feed->items)) {

The @count was already ther but that looks a bit ugly. Why do we need it? Can't we just add a !empty($feed->items) or something instead? Doesn't need to be in this issue.

+++ b/core/modules/aggregator/lib/Drupal/aggregator/Plugin/AggregatorPluginManager.phpundefined
@@ -2,7 +2,7 @@
- * Definition of Drupal\aggregator\Plugin\FetcherManager.
+ * Definition of Drupal\aggregator\Plugin\AggregatorPluginManager.

When changing this anyway, let's change it to "Contains \Drupal\..."

+++ b/core/modules/aggregator/lib/Drupal/aggregator/Plugin/ParserInterface.phpundefined
@@ -0,0 +1,53 @@
+   * @return
+   *   TRUE if parsing was successful, FALSE otherwise.

Should be @return bool then.

+++ b/core/modules/aggregator/lib/Drupal/aggregator/Plugin/ProcessorInterface.phpundefined
@@ -0,0 +1,86 @@
+   *   contains an array of feed items downloaded and parsed at the parsing stage.
+   *   See Drupal\aggregator\Plugin\FetcherInterface::parse()
+   *   for the basic format of a single item in the $feed->items array.

"stage." is a tiny bit over 80 characters. On the other side, there is some more space behind ".. parse()". Generally, docblocks should get as close to 80 characters as possible without getting above. Also, the parse() reference should have a leading \

+++ b/core/modules/aggregator/lib/Drupal/aggregator/Plugin/aggregator/fetcher/DefaultFetcher.phpundefined
@@ -16,7 +16,7 @@
- * Uses drupal_http_request() to download the feed.
+ * Uses http_default_client class to download the feed.

Should probably say service and not class.

+++ b/core/modules/aggregator/lib/Drupal/aggregator/Plugin/aggregator/parser/DefaultParser.phpundefined
@@ -0,0 +1,351 @@
+   * The extracted channel info.
+   * @var array
+   */
+  protected $channel = array();

Needs a space above @var I think. Same for those below.

+++ b/core/modules/aggregator/lib/Drupal/aggregator/Plugin/aggregator/processor/DefaultProcessor.phpundefined
@@ -0,0 +1,209 @@
+    // @todo This should probably be removed.
+    drupal_set_message(t('The news items from %site have been removed.', array('%site' => $feed->label())));

Removed or moved to wherever it's called, but not something to fix for this issue I guess.

+++ b/core/modules/aggregator/lib/Drupal/aggregator/Plugin/aggregator/processor/DefaultProcessor.phpundefined
@@ -0,0 +1,209 @@
+   * Implements \Drupal\aggregator\Plugin\ProcessorInterface::postProcess().
+   * Expires items from a feed depending on expiration settings.

Another space here.

+++ b/core/modules/aggregator/lib/Drupal/aggregator/Plugin/aggregator/processor/DefaultProcessor.phpundefined
@@ -0,0 +1,209 @@
+   * @param $count
+   *   Items count.
+   *
+   * @return
+   *   A string that is plural-formatted as "@count items".
+   */
+  protected function formatItems($count) {

Most of it is moved code, but while we're moving it, probably doesn't hurt to add some types to @param and @return where it's missing.

+++ b/core/modules/aggregator/lib/Drupal/aggregator/Tests/AggregatorConfigurationTest.phpundefined
@@ -36,5 +46,22 @@ function testSettingsPage() {
+    // @todo Re-enable when http://drupal.org/node/1780396 is fixed
+    //$this->assertResponse(200);

What does this have to do with that issue? The namespace/plugin issues should be fixed...

+++ b/core/modules/aggregator/lib/Drupal/aggregator/Tests/FeedFetcherPluginTest.phpundefined
@@ -0,0 +1,54 @@
+
+  /**
+   * Overrides \Drupal\simpletest\WebTestBase::setUp().
+   */
+  public function setUp() {

setUp() currently doesn't need a docblock.

+++ b/core/modules/aggregator/aggregator.admin.incundefined
@@ -341,70 +341,50 @@ function aggregator_admin_form($form, $form_state) {
+  // Store definitions and managers so we can access them later.
+  $form['#definitions'] = $definitions;

Aren't we supposed to be putting things in $form_state instead?

+++ b/core/modules/aggregator/aggregator.moduleundefined
@@ -304,7 +305,7 @@ function aggregator_permission() {
 function aggregator_cron() {
- $result = db_query('SELECT fid FROM {aggregator_feed} WHERE queued = 0 AND checked + refresh < :time AND refresh <> :never', array(
+  $result = db_query('SELECT fid FROM {aggregator_feed} WHERE queued = 0 AND checked + refresh < :time AND refresh <> :never', array(
     ':time' => REQUEST_TIME,
     ':never' => AGGREGATOR_CLEAR_NEVER

entity_query()?

Review was interrupted, more to come.

Looks great to me. Some final nitpicks and then I'll RTBC, looks like nobody else in interested in reviewing this :(

Maybe we can try to get some of the Feeds guys (looks like there's not much activity, but maybe @twistor?) interested in our aggregator.module improvements, so that they can actually rely on parts of it instead of doing their own thing in 8.x?

+++ b/core/modules/aggregator/aggregator.moduleundefined
@@ -464,43 +458,48 @@ function aggregator_refresh(Feed $feed) {
+  // Processing is done call postProcess on enabled processors.
+  foreach ($processor_instances as $instance) {

Processing is done*,* call ...

+++ b/core/modules/aggregator/lib/Drupal/aggregator/Plugin/aggregator/fetcher/DefaultFetcher.phpundefined
@@ -16,7 +16,7 @@
- * Uses drupal_http_request() to download the feed.
+ * Uses http_default_client service to download the feed.

Uses *the* http_default_client service, I think

+++ b/core/modules/aggregator/lib/Drupal/aggregator/Plugin/aggregator/parser/DefaultParser.phpundefined
@@ -0,0 +1,357 @@
+   * @return
+   *   FALSE on error, TRUE otherwise.

@return bool.

+++ b/core/modules/aggregator/lib/Drupal/aggregator/Plugin/aggregator/parser/DefaultParser.phpundefined
@@ -0,0 +1,357 @@
+   * Performs an action when an opening tag is encountered.
...
+   * Performs an action when a closing tag is encountered.
...
+   * Performs an action when data is encountered.

Can we add some @param's to those. And maybe state that those are callbacks for the XML parser.

Just want to make sure that the patch isn't going to come back from RTBC because of something like this :)

+++ b/core/modules/aggregator/lib/Drupal/aggregator/Plugin/aggregator/parser/DefaultParser.phpundefined
@@ -0,0 +1,357 @@
+   * @param $date_str
+   *   A string with a potentially W3C DTF date.
+   *
+   * @return

string for @param and int|false for @return I think.

+++ b/core/modules/aggregator/tests/lib/Drupal/aggregator_test/Plugin/aggregator/fetcher/TestFetcher.phpundefined
@@ -0,0 +1,39 @@
+    if ($feed->label() == 'Test feed') {

Can we use a different string for this? Maybe "Do not fetch" or "Return FALSE" or something other that's more self-speaking :)

+++ b/core/modules/aggregator/lib/Drupal/aggregator/Plugin/aggregator/parser/DefaultParser.phpundefined
@@ -321,10 +342,10 @@ function elementData($parser, $data) {
-   * @return
+   * @return int|FALSE

super-minor thing, false should be lowercase, see http://drupal.org/node/1354#types.

This can be fixed while commiting or when this will need a re-roll.

This looks great to me and is a very nice example of plugin conversions. Cleaner, interface-based and documented API's, unification with other systems, has better tests and improved inline documentation. Nothing left to complain about :)

#30: drupal-aggregator_plugins-1930274-30.patch queued for re-testing.

Yeah sure testbot, as if *that* change could have broken anything.

I am not changing status, but think drupal_container() notes should be addressed before this goes in. The rest are nitpicks from reading through the patch for first time.

+++ b/core/modules/aggregator/aggregator.admin.incundefined
@@ -341,70 +341,50 @@ function aggregator_admin_form($form, $form_state) {
+    $definitions[$type] = isset($definitions[$type]) ? $definitions[$type] : array();
+    $managers[$type] = drupal_container()->get("plugin.manager.aggregator.$type");
+    foreach ($managers[$type]->getDefinitions() as $id => $definition) {

I am pretty sure drupal_container() has been deprecated in favor of Drupal::service(). Can this be implemented here?

+++ b/core/modules/aggregator/aggregator.moduleundefined
@@ -398,42 +403,18 @@ function aggregator_save_category($edit) {
+  // Call \Drupal\aggregator\Plugin\ProcessorInterface::remove()
+  // on all processors.
+  $manager = drupal_container()->get('plugin.manager.aggregator.processor');
+  foreach ($manager->getDefinitions() as $id => $definition) {

This comment can be wrapped better. Ibid re: drupal_container().

+++ b/core/modules/aggregator/aggregator.moduleundefined
@@ -446,15 +427,28 @@ function aggregator_refresh(Feed $feed) {
+  // Retrieve processor manager now.
+  $processor_manager = drupal_container()->get('plugin.manager.aggregator.processor');
+  // Store instances in an array so we dont have to instantiate new objects.

Ibid re: drupal_container()

+++ b/core/modules/aggregator/aggregator.moduleundefined
@@ -464,43 +458,48 @@ function aggregator_refresh(Feed $feed) {
+        // If there are items on the feed, let all enabled processors do their work on it.
+        if (!empty($feed->items)) {
+          foreach ($processor_instances as $instance) {

I am pretty sure code comments are supposed to wrap at 80 characters as well.

+++ b/core/modules/aggregator/lib/Drupal/aggregator/FeedStorageController.phpundefined
@@ -51,9 +51,10 @@ protected function preDelete($entities) {
+      // Notify processors to remove stored items.
+      $manager = drupal_container()->get('plugin.manager.aggregator.processor');
+      foreach ($manager->getDefinitions() as $id => $definition) {

drupal_container() again.

+++ b/core/modules/aggregator/lib/Drupal/aggregator/Plugin/AggregatorPluginManager.phpundefined
@@ -13,19 +13,21 @@
+  }
+}

My understanding is that coding standards call for blank line before closing brace of a class. Same with other classes below (some are already correct).

+++ b/core/modules/aggregator/lib/Drupal/aggregator/Plugin/ProcessorInterface.phpundefined
@@ -0,0 +1,86 @@
+  /**
+   * Process feed data.

Processes? (third person verb tense)

+++ b/core/modules/aggregator/lib/Drupal/aggregator/Plugin/ProcessorInterface.phpundefined
@@ -0,0 +1,86 @@
+  /**
+   * Refresh feed information.

Refreshes?

+++ b/core/modules/aggregator/lib/Drupal/aggregator/Plugin/ProcessorInterface.phpundefined
@@ -0,0 +1,86 @@
+   * Called after the processing of the feed is completed
+   * by all selected processors.

Can be better wrapped for 80 characters.

+++ b/core/modules/aggregator/lib/Drupal/aggregator/Plugin/ProcessorInterface.phpundefined
@@ -0,0 +1,86 @@
+  /**
+   * Remove stored feed data.

Removes?

+++ b/core/modules/aggregator/lib/Drupal/aggregator/Tests/AggregatorConfigurationTest.phpundefined
@@ -23,12 +23,22 @@ public static function getInfo() {
+    $this->drupalGet('admin/config/services/aggregator/settings');
+    // Make sure that test plugins are present.
+    $this->assertText(t('Test fetcher'));
+    $this->assertText(t('Test parser'));
+    $this->assertText(t('Test processor'));

Are we ever testing this settings page in a language other than English? If not, t() calls can be removed to speed up tests.

+++ b/core/modules/aggregator/lib/Drupal/aggregator/Tests/AggregatorTestBase.phpundefined
@@ -149,9 +149,9 @@ function getDefaultFeedItemCount() {
    * @param $expected_count
-   *   Expected number of feed items.
+   *   Expected number of feed items. If omitted no check will happen.

This needs a type hint. 'int|null' correct? Also description should start with '(optional)'. Finally, I might suggest 'If omitted or set to NULL,'. (note comma)

#36 +1RTBC

#1925048: Convert aggregator's system_config_form() to SystemConfigFormBase is postponed on this.

#36: drupal-aggregator_plugins-1930274-36.patch queued for re-testing.

wow
looking at #36 and it looks like Feeds contrib into core )))
Code looks good for me +1 RTBC

marked #1827164: Integrate Feeds into core #1136482: [policy] Deprecate aggregator.module in D9 core and remove it in D10 as duplicates
and bumping this one as major due to covering a lot of possible issues with Feeds and possible help from Feeds contrib team

Nice patch, +1 to commit. Also there's some small suggestions
diffstat 23 files changed, 1201 insertions(+), 893 deletions(-)

+++ b/core/modules/aggregator/aggregator.admin.incundefined
@@ -341,70 +341,50 @@ function aggregator_admin_form($form, $form_state) {
+    foreach ($managers[$type]->getDefinitions() as $id => $definition) {
+      $label = $definition['title'] . ' <span class="description">' . $definition['description'] . '</span>';
+      $definitions[$type][$id] = $label;

This looks ugly, please change to format_string()
actually admin_form() needs follow-up for usability

+++ b/core/modules/aggregator/aggregator.moduleundefined
@@ -304,7 +305,7 @@ function aggregator_permission() {
 function aggregator_cron() {
- $result = db_query('SELECT fid FROM {aggregator_feed} WHERE queued = 0 AND checked + refresh < :time AND refresh <> :never', array(
+  $result = db_query('SELECT fid FROM {aggregator_feed} WHERE queued = 0 AND checked + refresh < :time AND refresh <> :never', array(
     ':time' => REQUEST_TIME,
     ':never' => AGGREGATOR_CLEAR_NEVER

+++ b/core/modules/aggregator/lib/Drupal/aggregator/Plugin/aggregator/processor/DefaultProcessor.phpundefined
@@ -0,0 +1,210 @@
+      if ($entry = entity_load_multiple_by_properties('aggregator_item', $values)) {
...
+    $iids = Database::getConnection()->query('SELECT iid FROM {aggregator_item} WHERE fid = :fid', array(':fid' => $feed->id()))->fetchCol();
...
+      $iids = Database::getConnection()->query('SELECT iid FROM {aggregator_item} WHERE fid = :fid AND timestamp < :timestamp', array(
+        ':fid' => $feed->id(),

+++ b/core/modules/aggregator/lib/Drupal/aggregator/Tests/FeedProcessorPluginTest.phpundefined
@@ -0,0 +1,70 @@
+    $entities = entity_load_multiple_by_properties('aggregator_feed', array('description' => $feed->description->value));

needs follow-up to convert to entity_query() to use entity storage controller

Suppose better off with "mysqkism" and have clean entity api for all aggregator

This will throw parse errors on a 304, 403, 404, etc, . I guess it's not actually related to this patch, and more-so to do with the Guzzle conversion. But, we need to handle status codes. Maybe turn off dead feeds. While we're at it, we should handle cache-control and expires headers.

Excuse me for a second, while I take a brain dump.

To elaborate more on my very brief statement in #1827164: Integrate Feeds into core, this doesn't get us too much farther with regard to Feeds depending on Aggregator, or some such thing. I'm not sure how much room is left for architectural improvements, but off the top of my head:

• All processors run for every feed. Could make for an interesting pipeline approach, but breaks the terminology of Feeds. Would it be up to each processor's configuration if they were supposed to run?
• A global parser?
• The interfaces are too limited. For example: Feeds needs fetchers and parsers to be able to provide config forms. We could extend the interfaces, or wrap them, but all we would gain us is re-using the core fetcher and parser. Which brings me to:
• It would be great to have a real RSS/Atom parser in core. Especially the Atom part. I know there's #1268232: XPath-based parser for aggregator, but I've been looking for an excuse to port Universal Feed Parser from Python to PHP. It's pretty much the greatest thing ever. Apologies to Zend_Feed. I still hate you, SimplePie.
• No mapping stage. Might could shoehorn something in there, not sure. But, with mapping comes Importers and an entire new level of complexity. Sidenote: I would love a better name for those. Feed templates? Feed types?
• No batching. This goes along with the interfaces being too simplistic.
• Passing the same Feed object along. Ick. Way too much responsibility on individual plugins. Speaking of which:
• I really want to change processors so that they operate on one item at a time. Then we could have an intelligent batch manager that watches memory usage. À la Drush.
• PubSubHubbub support could be very interesting to add to core.

I'm sure there's more, but that's what stuck in my brain after reviewing the current state of Aggregator. I keep forgetting it exists.

This reminds me of Actions/Triggers vs. Rules.

@twistor: Thanks for joining in :)

The comment caused a bit of confusing, the goal of this issue is not to replace feeds. All it does is convert custom aggregator plugin stuff to the standard plugin architecture. How these plugins work together is AFAIK more or less unchanged. It will be the job of other issues to improve it.

Small improvements can still make it into 8.x (given that we will get below thresholds and we're currently a fair bit away from that) but moving feeds in core obviously not. The best thing might be to create separate issues for your feedback, point out how feeds does and why that's better.

Also, I suggest we stop commenting on this issue because every comment moves it back to the end of the RTBC queue, this has been waiting for quite a while already.

#36: drupal-aggregator_plugins-1930274-36.patch queued for re-testing.

I like everything about this _except_ some of the entity changes in hook_cron(). They seem unrelated to this patch(as far as I can tell) and I wonder if splitting a db_update into "load a bunch of entities and the call save on them" has some serious performance concerns for large collections of feeds.

Rather than mire this issue in "can we get a benchmarking and discuss", can we remove it from this issue and follow up or is it actually connected? If its is actually necessary, lets just move forward and discuss elsewhere if anyone cares.

Agreed. Back to RTBC!

Wow, this is a fantastic patch! It essentially represents almost a complete re-write of aggregator.module, but definitely for the better. Things that used to be pseudo-hooks are now part of an actual interface, and it's a lot clearer now of what's required in order to implement a processer, fetcher, or parser.

I'm sure I could probably find some nits if I looked hard enough, but I think whatever there is we can deal with in follow-ups. Great work!

Committed and pushed to 8.x. Thanks!

rootatwc: Want to be added to MAINTAINERS.txt for aggregator module? :)

Marking for change notice.

Hm, maybe shorten the change notice a bit by removing the code within the functions/methods and instead simply link to the new class on a.d.o as soon as it's there? That and link to the plugin API documentation on d.o. Looks good to me otherwise.

Please add some lines about fetchers

Fetchers were converted and documented in the initial change record: http://drupal.org/node/1704454

Maybe just reference that?

@Berdir yep, please reference both to have the overall picture,

Also this one requires updates in summary based on #47 feedback and follow-ups