Updated: Comment #0

Problem/Motivation

Aggregator module is doing too much.
Especially the category handling is responsible for loads of custom code, which frankly we don't need in core.

The CRUD logic is everywhere, from entity-storage controllers, forms to legacy procedural functions. Of course nothing is alterable there are no hooks.

We tried to make things at least swappable by introducing the CategoryStorage, but eventually figured out (#2068393: Mark aggregator_save_category() as deprecated) that it leads back to #15266: Replace aggregator category system with taxonomy which is not likely to happen. Even if it does though, i can't guarantee that things won't start break badly once someone deletes the category field..
One could say we can mark this field as locked, but then this issue loses much of its point.
We do not hardcode fields elsewhere in core, why do it now?

Instead of getting stuck for one more release with code that is totally out of sync of core APIs, remove it from core, let it advance to contrib, by correctly using fields and views, because lets face it not much people care about it in core.

Proposed resolution

Completely remove the category handling code and transfer it to contrib (aggregator_category). I am willing to maintain the code with any upgrade path needed, and in the long run turn everything to fields/views.

Remaining tasks

Write patch, review, commit.

User interface changes

No more categories in UI.

API changes

All category related functions are removed.

Files: 
CommentFileSizeAuthor
#25 drupal-aggregator_categories-2127725-25.patch105.65 KBParisLiakos
PASSED: [[SimpleTest]]: [MySQL] 59,132 pass(es).
[ View ]
#16 drupal-aggregator_categories-2127725-15.patch105.58 KBParisLiakos
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch drupal-aggregator_categories-2127725-15.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#8 drupal-aggregator_categories-2127725-8.patch105.41 KBParisLiakos
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch drupal-aggregator_categories-2127725-8.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#1 drupal-aggregator_categories-2127725-1.patch105.21 KBParisLiakos
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch drupal-aggregator_categories-2127725-1.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Comments

ParisLiakos’s picture

Status:Active» Needs review
StatusFileSize
new105.21 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch drupal-aggregator_categories-2127725-1.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

43 files changed, 20 insertions(+), 2309 deletions(-)

kim.pepper’s picture

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.

ParisLiakos’s picture

Issue summary:View changes
ParisLiakos’s picture

Issue summary:View changes
jibran’s picture

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

dawehner’s picture

Status:Needs review» Needs work

The last submitted patch, 1: drupal-aggregator_categories-2127725-1.patch, failed testing.

ParisLiakos’s picture

Status:Needs work» Needs review
StatusFileSize
new105.41 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch drupal-aggregator_categories-2127725-8.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

@jibran: :P
I will create the project once this is committed cause i want to split into the new repo as much as of the latest git history i can;)

Here is a reroll

Status:Needs review» Needs work

The last submitted patch, 8: drupal-aggregator_categories-2127725-8.patch, failed testing.

ParisLiakos’s picture

Status:Needs work» Needs review

Status:Needs review» Needs work

The last submitted patch, 8: drupal-aggregator_categories-2127725-8.patch, failed testing.

ParisLiakos’s picture

Status:Needs work» Needs review
sun’s picture

Status:Needs review» Needs work

The last submitted patch, 8: drupal-aggregator_categories-2127725-8.patch, failed testing.

kim.pepper’s picture

Issue tags:+Needs reroll
ParisLiakos’s picture

Status:Needs work» Needs review
StatusFileSize
new105.58 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch drupal-aggregator_categories-2127725-15.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

reroll

ParisLiakos’s picture

Issue tags:-Needs reroll
catch’s picture

Assigned:ParisLiakos» Dries

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.

sun’s picture

Status:Needs review» Reviewed & tested by the community

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.

Gábor Hojtsy’s picture

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

Dries’s picture

Assigned:Dries» Unassigned

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

Dries’s picture

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

Dries’s picture

Status:Reviewed & tested by the community» Needs work

The last submitted patch, 16: drupal-aggregator_categories-2127725-15.patch, failed testing.

ParisLiakos’s picture

Status:Needs work» Needs review
StatusFileSize
new105.65 KB
PASSED: [[SimpleTest]]: [MySQL] 59,132 pass(es).
[ View ]

yay! good news:)

merged 8.x, no conflicts
here is a reroll

ParisLiakos’s picture

Status:Needs review» Reviewed & tested by the community

back to rtbc

webchick’s picture

Title:Remove category handling from aggregator» Change notice: Remove category handling from aggregator
Priority:Normal» Major
Status:Reviewed & tested by the community» Active
Issue tags:+Needs change record

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

We need a change notice for this.

kim.pepper’s picture

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

ParisLiakos’s picture

yeah something like this..i can add the link to the new module/sandbox page once it is ready

kim.pepper’s picture

Status:Active» Fixed
Issue tags:-Needs change record

Change notice added

kim.pepper’s picture

kim.pepper’s picture

Title:Change notice: Remove category handling from aggregator» Remove category handling from aggregator
penyaskito’s picture

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?

ParisLiakos’s picture

Gábor Hojtsy’s picture

Issue tags:+D8MI, +language-content

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

Status:Fixed» Closed (fixed)

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