Remove category handling from aggregator

I support moving categories to contrib. Having worked on a lot of the conversions for aggregator, I didn't understand the need to have a special way of handling categories.

lets face it not much people care about it in core.

you hurt me :'(.
OTOH I like the idea

let it advance to contrib

Let's create a sandbox and move the deleted code there. If this is green there is nothing to review :D

1: drupal-aggregator_categories-2127725-1.patch queued for re-testing.

8: drupal-aggregator_categories-2127725-8.patch queued for re-testing.

I think it's a good idea to remove this, and it's blocking #597236: Add entity caching to core. If we eventually wanted to add back categories (as taxonomy or however else) that could possibly even happen in a minor release of Drupal 8, but lugging the legacy code around doesn't help anyone.

Assigning to Dries for feedback.

I wanted to state the same as @catch when I hit the re-test button yesterday.

I always assumed that this part of Aggregator module would just be a tiny fragment and was a bit shocked to see how much code there is for handling the feed categories.

I'm not familiar with the entity field issue referenced above, but it definitely makes sense to remove this custom implementation in any case now. Not strictly required, but perhaps with an exception to add back a new way for handling categories later on, if someone comes up with a good and working solution.

Multiple approaches to that would be possible, I guess. The current Aggregator entity architecture of Category → Feed → FeedItem attempts to map a FeedItem (across Feed) to a Category, whereas the FeedItem is implicitly in the related Category (as opposed to an explicit and perhaps even editable field value on the FeedItem).

A taxonomy based approach would open the door for assigning Feeds and FeedItems to more than one Category, whereas the taxonomy term on the FeedItem would have to be a derived/computed value. (interesting use-case in and by itself)

Another approach would be to enable bundles for the Feed entity type, interpreting the bundle as the Category, similar to the new Contact module in D8 — although I guess that approach would not be correct in terms of technical architecture.

In any case, the current swath of code for these custom feed categories seems to have been ported from one major version of Drupal core to the next without much thought about its architecture. It's time to rethink that and re-implement it using modern facilities. As the current code apparently is very convoluted, the best way forward is to simply rip it out first.

This would be also great for D8MI as attaching custom one-off data like categories does not work for translation. If it is a single value base field or a regular configurable field, that works with D8MI, but the aggregator category system as implemented is not multilingual friendly. This would have been one of the minority in D8 which would not get any multilingual support, and now it seems like it is in the way of everybody else as well :D

I'm fine with removing this custom categorization system. It seems like leveraging the existing taxonomy system is the natural evolution for this module.

I tried to apply the patch but it looks like it needs a reroll. Asking for a re-test.

16: drupal-aggregator_categories-2127725-15.patch queued for re-testing.

Okie doke, committed and pushed to 8.x. Thanks!

We need a change notice for this.

How about:

"The Aggregator module no longer supports assigning feed categories to feeds and feed items.

This functionality will now be moved to a contributed project to use taxonomies."

Change notice added

https://drupal.org/node/2152721

public function opmlPage($cid = NULL) {

+++ b/core/modules/aggregator/lib/Drupal/aggregator/Controller/AggregatorController.php
@@ -340,22 +244,13 @@ public function sources() {
   public function opmlPage($cid = NULL) {
-    if ($cid) {
-      $result = $this->database->query('SELECT f.title, f.url FROM {aggregator_feed} f LEFT JOIN {aggregator_category_feed} c on f.fid = c.fid WHERE c.cid = :cid ORDER BY title', array(':cid' => $cid));
-    }
-    else {
-      $result = $this->database->query('SELECT * FROM {aggregator_feed} ORDER BY title');
-    }

$cid arg in opmlPage is unused. Do we need a follow-up for removing it here and in core/modules/aggregator/aggregator.routing.yml? Or is there any reason I didn't see for still having it in the route?

Tagging for D8MI as well since this "covers" a problem area we did not have a solution before and now don't need to :)