Updated: Comment #82

Problem/Motivation

Aggregator categories are one of the last core sub-systems that do not use the entity API.
Instead it relies on its own API. While we tried making its storage swappable with a seperate service, categories are still un-translatable and we are still re-inventing the wheel.

Proposed resolution

Categories should be Terms instead.
Get rid of the custom code and the raw-sql queries. Use the entity API instead.
Get rid of the custom tables that handle the relations between categories and feeds/feed items and instead use Field API ; Attach an entity reference field on feeds and feed items entities instead.
We will also need an aggregator specific vocabulary to put existing categories from sites upgrading from D7.

Remaining tasks

A maintainer is needed to approve this API change for D8
Reroll patch in #72 and see wherre we stand with tests after #2004316: Exception on field_purge_batch() after deleting a field was fixed.
Fix any potential newly introduced failures.
Review and RTBC

User interface changes

All the category crud forms will use the term forms instead provided by the taxonomy module, which means taxonomy will be a dependency

Aggregator will get its own field-ui for managing feed entities under the current administration page

API changes

aggregator_category, aggregator_category_feed and aggregator_category_item tables are being removed in favor of taxonomy tables and tables created by Field API
aggregator_category_selector is removed from aggregator's config and moves to the field instance settings instead
aggregator_save_category is removed in favor of just $category->save()
aggregator_category_load is removed in favor of entity_load

Original report by qgil

The Aggregator module allows the creation of categories to file the news feed. The creation of categories is done manually, though.

What about the possibility to assign an already created vocabulary to define the aggrgator categories?

It would be quite useful. For example, if my site has a vocabulary "topic" with entries such as film, radio, tv, press with which I organize articles and the forum, I could use this same vocabulary to organize news feeds.

Files: 
CommentFileSizeAuthor
#72 drupal-aggregator_taxonomy-15266-72.patch104.29 KBParisLiakos
FAILED: [[SimpleTest]]: [MySQL] 55,846 pass(es), 35 fail(s), and 1 exception(s).
[ View ]
#68 drupal-aggregator_taxonomy-15266-68.patch106.08 KBParisLiakos
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch drupal-aggregator_taxonomy-15266-68.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#70 drupal-aggregator_taxonomy-15266-70.patch105.19 KBParisLiakos
FAILED: [[SimpleTest]]: [MySQL] 55,731 pass(es), 42 fail(s), and 7 exception(s).
[ View ]
#62 drupal-aggregator_taxonomy-15266-62.patch102.33 KBtwistor
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch drupal-aggregator_taxonomy-15266-62.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#62 interdiff.txt3.74 KBtwistor
#57 drupal-aggregator_taxonomy-15266-57.patch102.27 KBtwistor
PASSED: [[SimpleTest]]: [MySQL] 54,031 pass(es).
[ View ]
#57 interdiff.txt5.4 KBtwistor
#55 drupal-aggregator_taxonomy-15266-55.patch98.49 KBParisLiakos
FAILED: [[SimpleTest]]: [MySQL] 54,033 pass(es), 25 fail(s), and 0 exception(s).
[ View ]
#55 interdiff.txt3.57 KBParisLiakos
#53 drupal-aggregator_taxonomy-15266-53.patch98.57 KBParisLiakos
PASSED: [[SimpleTest]]: [MySQL] 54,012 pass(es).
[ View ]
#53 interdiff.txt1.63 KBParisLiakos
#52 drupal-aggregator_taxonomy-15266-52.patch98.66 KBParisLiakos
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion.
[ View ]
#52 interdiff.txt4.24 KBParisLiakos
#49 drupal-aggregator_taxonomy-15266-49.patch98.83 KBParisLiakos
PASSED: [[SimpleTest]]: [MySQL] 54,103 pass(es).
[ View ]
#49 interdiff.txt786 bytesParisLiakos
#44 interdiff.txt6.17 KBParisLiakos
#43 drupal-aggregator_taxonomy-15266-43.patch98.82 KBParisLiakos
FAILED: [[SimpleTest]]: [MySQL] 53,911 pass(es), 1 fail(s), and 0 exception(s).
[ View ]
#36 drupal-aggregator_taxonomy-15266-36.patch97.93 KBParisLiakos
PASSED: [[SimpleTest]]: [MySQL] 54,084 pass(es).
[ View ]
#34 drupal-aggregator_taxonomy-15266-34.patch96.65 KBParisLiakos
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion.
[ View ]
#32 drupal-aggregator_taxonomy-15266-32.patch95.43 KBParisLiakos
FAILED: [[SimpleTest]]: [MySQL] 54,018 pass(es), 16 fail(s), and 2 exception(s).
[ View ]
#29 drupal-aggregator_taxonomy-15266-28.patch88.41 KBParisLiakos
FAILED: [[SimpleTest]]: [MySQL] 52,931 pass(es), 284 fail(s), and 106 exception(s).
[ View ]
#25 drupal-aggregator_taxonomy-15266-25.patch110.19 KBParisLiakos
FAILED: [[SimpleTest]]: [MySQL] 53,967 pass(es), 16 fail(s), and 2 exception(s).
[ View ]
#20 drupal-aggregator_taxonomy-15266-20.patch90.23 KBParisLiakos
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch drupal-aggregator_taxonomy-15266-20.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Comments

Version:4.5.0» x.y.z

Anything done about this?

Version:x.y.z» 7.x-dev

Any news about this?

Moving to cvs.

Title:Assign vocabulary for Aggregator categoriesReplace aggregator category system with taxonomy

#293318: Convert Aggregator feeds into entities will open the opportunity to replace aggregator categorization with taxonomy in a very simple way: Aggregator items could be listed by their feed's taxonomies. The only setting necessary would be to assign which vocabulary to use for generating listings of aggregator items.

This approach will support categorization on a feed level, but not on a feed item level. I personally think this limitation would be fine.

Objections?

The main question I have is what to do about the existing aggregator URLs - would we create new vocabularies for the existing categories and use hook_term_path() maybe?

Version:7.x-dev» 8.x-dev

I think #293318: Convert Aggregator feeds into entities will make this easier...attach a taxonomy reference field on aggregator feeds?

Dries (and I) have indicated support for this at #293318: Convert Aggregator feeds into entities. Hope someone takes this on.

Category:feature» task

i believe this falls into tasks now

Yes, agreed that this should be considerably easier to do now.

Not sure if this should be postponed on the conversion of taxonomy term reference fields to ER. Not sure, maybe ask Amitaibu/amateescu if they see any problems.

I think one question here is how we take over the categories from the feed to the feed item as they are created.

And if the category should be a hardcoded field or a dynamic one that is just defined by a profile or something. We could no longer provide default items per category blocks and pages but you'd have to create them with views. But that's where we want to go anyway (although maybe with a views wizard or something).

I think we should create this field in standard profile aggregator.install if taxonomy is enabled. i hate hardcoded fields
This way we give people the possibility to remove them..not everyone uses feed categories.
The category pages would still be available in taxonomy/term/* right? (Edit: No, taxonomy/term/* lists nodes only)
I am not sure about the blocks, but maybe create a view block which lists latest X items. Then it would be as simple as adding a views display in the same view and then a category condition in the filters.

I think one question here is how we take over the categories from the feed to the feed item as they are created.

We already have the feed entity in item creation, so it wont be hard to assign the same category to items.

What i am unsure what to do with: If i look at feed/feed item functionality, i can see that in the Categorize tab of a feed, someone can set different category per feed item alone..

What i am unsure what to do with: If i look at feed/feed item functionality, i can see that in the Categorize tab of a feed, someone can set different category per feed item alone..

Now that an item is an entity, it should be quite easy to add a form controller.

Assigned:Unassigned» ParisLiakos

Status:Active» Postponed

Status:Postponed» Active

As per #13

Issue tags:+D8MI, +language-content

It would be amazing if this could be done. The current category system in aggregator does not support multilingual in any way as far as I know and it is neither a content nor config entity. So it is in the dark lands of non-standard implementation with no common tooling support (not just only multilingual). Tagging for multilingual but no promises whatsoever that this would help anyone pick this up.

hello Gabor, i already started this, i am at about the 40% of the patch..it is gonna be quite big..if anyone wants to give a hand, ping me on irc, and i will create a sandbox

@rootatwc: even if your patch does not work, posting it here would be fabulous so others can take a look (or take it over if you happen to get too frustrated and run far away :).

StatusFileSize
new90.23 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch drupal-aggregator_taxonomy-15266-20.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

well, it is not that it is not working, it just is work in progress..anyway, here is my work so far..i just started fixing tests and polishing stuff
It also contains the entity reference fix from the other issue

+++ b/core/modules/aggregator/aggregator.admin.incundefined
@@ -7,6 +7,82 @@
+  return drupal_container()->get('plugin.manager.entity')->getListController('aggregator_feed_type')->render();

http://drupal.org/node/1929006
\Drupal::service

And it will change again to Drupal::entityManager(), so it has to be changed again anyway => Don't bother.

Status:Postponed» Active

actually this patch is not quite ready for review, it is a waste of time;)
and i think since this #1956434: Can't edit nodes with entity reference fields got committed i can pick this back..i hope i wont find more blockers

another one
#1957888: Exception when a field is empty on programmatic creation
i am not gonna postpone this though, i ll hack field_sql_storage a bit

Status:Active» Needs review
Issue tags:+Needs tests, +Needs upgrade path tests
StatusFileSize
new110.19 KB
FAILED: [[SimpleTest]]: [MySQL] 53,967 pass(es), 16 fail(s), and 2 exception(s).
[ View ]

lets see tests
to make things easier and patch smaller for now, i made taxonomy and er requirements

Status:Needs review» Needs work

The last submitted patch, drupal-aggregator_taxonomy-15266-25.patch, failed testing.

Just a quick question.

+++ b/core/modules/aggregator/aggregator.installundefined
@@ -330,3 +256,166 @@ function aggregator_update_8001() {
+  db_add_field('aggregator_feed', 'type', array(
+    'description' => 'The type of this feed.',
+    'type' => 'varchar',
+    'length' => 32,
+    'not null' => TRUE,
+    'default' => '',
+    'initial' => 'default',

Hm, we do we need bundles for this? Can't we convert it using a single bundle first and then about that functionality in a separate step?

You are probably right, but i have no clue where to expose the UI..and i have to expose it somewhere

+++ b/core/modules/aggregator/aggregator.installundefined
@@ -330,3 +256,166 @@ function aggregator_update_8001() {
+  update_variable_del('aggregator_category_selector');

because of this

May the current admin path, although it ll get crowded a bit

Status:Needs work» Needs review
StatusFileSize
new88.41 KB
FAILED: [[SimpleTest]]: [MySQL] 52,931 pass(es), 284 fail(s), and 106 exception(s).
[ View ]

no bundles, tests will fail again

Interesting, the missing route for 'aggregator_feed_add' makes all the menu entries fail.

Status:Needs review» Needs work

uh, sorry, just remove this entry..i will soon post a new patch, with this fix and upgrade path tests included

Status:Needs work» Needs review
Issue tags:-Needs tests, -Needs upgrade path tests
StatusFileSize
new95.43 KB
FAILED: [[SimpleTest]]: [MySQL] 54,018 pass(es), 16 fail(s), and 2 exception(s).
[ View ]

Upgrade tests added, i am pretty sure there are still some tests that fail

Status:Needs review» Needs work

The last submitted patch, drupal-aggregator_taxonomy-15266-32.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new96.65 KB
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion.
[ View ]

i have no clue why module tests broke -.- and they are soooo slow to try locally:/

Status:Needs review» Needs work

The last submitted patch, drupal-aggregator_taxonomy-15266-34.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new97.93 KB
PASSED: [[SimpleTest]]: [MySQL] 54,084 pass(es).
[ View ]

hopefully this passes

Great work so far!

+++ b/core/modules/aggregator/aggregator.admin.incundefined
@@ -63,30 +63,37 @@ function aggregator_view() {
+    foreach (entity_load_multiple('taxonomy_term', $result) as $category) {
+      $row = array();
+      $items = entity_query('aggregator_item')
+        ->condition('field_aggregator_categories.target_id', $category->id())
+        ->execute();

I'm wondering whether it would be possible to not run N EQs. Can't you run first the one for the terms, and then pass in all term IDs on the aggregator_item one?

WOOT!

#37, I don't think it can be done, maybe with entity_query_aggregate()? I'm not sure. But at least it should have a ->count() call.

Actually, this should do it. The result will have to be re-keyed, but that's all.

<?php
$count_array
= entity_query_aggregate('aggregator_item')
  ->
aggregate('field_aggregator_categories.target_id', 'COUNT')
  ->
groupBy('field_aggregator_categories.target_id')
  ->
execute();
?>

+++ b/core/modules/aggregator/aggregator.admin.incundefined
@@ -144,7 +151,11 @@ function aggregator_form_opml($form, &$form_state) {
+  $options = array();
+  $categories = entity_load_multiple_by_properties('taxonomy_term', array('vid' => 'aggregator_categories'));
+  foreach($categories as $category) {
+    $options[$category->id()] = check_plain($category->label());
+  }
   if ($options) {

Seems like there should be a drupal-wide generic function for this.

+++ b/core/modules/aggregator/aggregator.installundefined
@@ -330,3 +271,171 @@ function aggregator_update_8001() {
+  // Create the entity reference field.
+  $field = array(
+    'field_name' => 'field_aggregator_categories',
+    'type' => 'entity_reference',
+    'cardinality' => -1,
+    'entity_types' => array('aggregator_feed', 'aggregator_item'),
+    'module' => 'entity_reference',
+    'settings' => array(
+      'target_type' => 'taxonomy_term',
+    ),

Could the field be named 'aggregator_categories'?

+++ b/core/modules/aggregator/aggregator.moduleundefined
@@ -279,7 +232,7 @@ function _aggregator_category_title($category) {
function _aggregator_has_categories() {
-  return user_access('access news feeds') && (bool) db_query_range('SELECT 1 FROM {aggregator_category}', 0, 1)->fetchField();
+  return user_access('access news feeds') && (bool) db_query_range("SELECT 1 FROM {taxonomy_term_data} WHERE vid = 'aggregator_categories'", 0, 1)->fetchField();
}

entity_query()?

+++ b/core/modules/aggregator/aggregator.moduleundefined
@@ -338,53 +291,134 @@ function aggregator_queue_info() {
+  if (strpos($root_path, 'aggregator/categories/%') === 0) {
+    $item = menu_get_item('taxonomy/term/' . arg(2) . '/edit');

arg()!

The behavior of the save buttons on aggregator/sources/%/categorize is messed up.
The individual save buttons don't seem to be working. Seems like the ItemFormController() needs a save() method.

woot i just found out entity_query_aggregate:)

Could the field be named 'aggregator_categories'?

Thats what i started doing. but then prefixed it with field_ so i can distinguish it from the aggregator_categories vocabulary

arg()!

woot! kill me now!

The individual save buttons don't seem to be working. Seems like the ItemFormController() needs a save() method.

This stupid form is killing me...the day i replace it with views i am gonna get drunk:)

StatusFileSize
new98.82 KB
FAILED: [[SimpleTest]]: [MySQL] 53,911 pass(es), 1 fail(s), and 0 exception(s).
[ View ]

Seems like there should be a drupal-wide generic function for this.

You mean for a key value of id, category name? why?
Or for a category load? if yes, i think we try to get rid of entity_load* wrappers, see #1757586: Remove MODULE_load*() & Co functions in favor of entity_*() functions

Fix quite some stuff..good news is, if user decides to delete aggregator categories vocabulary, there are no categories menu link and ctegories table in admin

StatusFileSize
new6.17 KB

forgot interdiff

Status:Needs review» Needs work

The last submitted patch, drupal-aggregator_taxonomy-15266-43.patch, failed testing.

Seems like there should be a drupal-wide generic function for this.

I meant mapping a list of entities to $entity->id() => $entity->label()

The failure in #43 is due to getting rid of arg(). I'm not sure what the future is supposed to bring exactly. Maybe, $router_item['map'][2].

Status:Needs work» Needs review

i cannot reproduce locally:/

#43: drupal-aggregator_taxonomy-15266-43.patch queued for re-testing.

StatusFileSize
new786 bytes
new98.83 KB
PASSED: [[SimpleTest]]: [MySQL] 54,103 pass(es).
[ View ]

i lied

Mostly the patch is ready. What's left?

I think all queries should be normalized there's a lot of places which loads vocabulary or term then queries another entity...

Also there's some mark-up nitpicks mostly related to read_more theming

+++ b/core/modules/aggregator/aggregator.moduleundefined
@@ -279,7 +232,13 @@ function _aggregator_category_title($category) {
function _aggregator_has_categories() {
-  return user_access('access news feeds') && (bool) db_query_range('SELECT 1 FROM {aggregator_category}', 0, 1)->fetchField();
+  $categories = array();
+  if ($vocabulary = taxonomy_vocabulary_load('aggregator_categories')) {
+    $categories = entity_query('taxonomy_term')
+      ->condition('vid', $vocabulary->id())
+      ->execute();
+  }
+  return user_access('access news feeds') && !empty($categories);

This check looks too expensive, select 1 from limit 1 is enough

+++ b/core/modules/aggregator/aggregator.pages.incundefined
@@ -469,32 +400,33 @@ function aggregator_page_rss() {
+        if ($summary != $item->description->value) {
+          $summary .= '<p><a href="' . check_url($item->link->value) . '">' . t('read more') . "</a></p>\n";
+++ b/core/modules/aggregator/lib/Drupal/aggregator/Plugin/block/block/AggregatorCategoryBlock.phpundefined
@@ -66,11 +66,14 @@ public function blockSubmit($form, &$form_state) {
+      $read_more = theme('more_link', array('url' => 'aggregator/categories/' . $category->id(), 'title' => t("View this category's recent news.")));

this one require follow-up to remove markup and convert to theme('more_link')

+++ b/core/modules/aggregator/lib/Drupal/aggregator/ItemFormController.phpundefined
@@ -0,0 +1,36 @@
+    // Do not provide a delete action.
+    unset($actions['delete']);

why? maybe better use #access

+++ b/core/modules/aggregator/lib/Drupal/aggregator/Plugin/Derivative/AggregatorCategoryBlock.phpundefined
@@ -42,11 +42,15 @@ public function getDerivativeDefinition($derivative_id, array $base_plugin_defin
+    $categories = entity_query('taxonomy_term')
+      ->condition('vid', 'aggregator_categories')
+      ->execute();
+    if ($categories) {
+      foreach (entity_load_multiple('taxonomy_term', $categories) as $category) {

entity_load_multiple_by_properties against 2 queries

+++ b/core/modules/aggregator/lib/Drupal/aggregator/Plugin/block/block/AggregatorCategoryBlock.phpundefined
@@ -66,11 +66,14 @@ public function blockSubmit($form, &$form_state) {
+    if ($category = taxonomy_term_load($id)) {
+      $result = entity_query('aggregator_item')
+        ->condition('field_aggregator_categories.target_id', $category->id())
+        ->range(0, $this->configuration['block_count'])
+        ->execute();

$id and ->id() is different?

+++ b/core/modules/aggregator/aggregator.moduleundefined
@@ -354,7 +354,7 @@ function aggregator_menu_local_tasks(&$data, $router_item, $root_path) {
+  if (strpos($root_path, 'aggregator/categories/%') === 0 && $term = array_pop($router_item['page_arguments'])) {

So that we don't introduce any extremely confusing bugs later, I think this should be end($router_item['page_arguments']) instead. That way the value isn't removed.

StatusFileSize
new4.24 KB
new98.66 KB
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion.
[ View ]

thanks for the reviews!
timplunkett: you couldnt be more right:)

What's left?

i think my hack below makes this uncommitable

+++ b/core/modules/field_sql_storage/field_sql_storage.moduleundefined
@@ -457,7 +457,8 @@ function field_sql_storage_field_storage_write(EntityInterface $entity, $op, $fi
-      foreach ($items as $delta => $item) {
+      // @todo temp hack till http://drupal.org/node/1957888 is fixed.
+      foreach (array_filter($items) as $delta => $item) {

StatusFileSize
new1.63 KB
new98.57 KB
PASSED: [[SimpleTest]]: [MySQL] 54,012 pass(es).
[ View ]

one more place to fix per andypost's feedback

+++ b/core/modules/aggregator/aggregator.admin.incundefined
@@ -63,31 +64,39 @@ function aggregator_view() {
+    $categories = entity_load_multiple_by_properties('taxonomy_term', array('vid' => 'aggregator_categories'));
+
+    $rows = array();
+    if ($categories) {
+      foreach ($categories as $category) {

entity_load_multiple_by_properties() always returns an array, so the if should be unnecessary.

+++ b/core/modules/aggregator/aggregator.admin.incundefined
@@ -63,31 +64,39 @@ function aggregator_view() {
+        $items = entity_query('aggregator_item')
+          ->condition('field_aggregator_categories.target_id', $category->id())
+          ->execute();

We should use entity_query_aggregate() here to avoid doing N queries. Plus, make it a count() query. See #39.

+++ b/core/modules/aggregator/aggregator.installundefined
@@ -5,6 +5,8 @@
+use Drupal\Component\Uuid\Uuid;
@@ -330,3 +271,171 @@ function aggregator_update_8001() {
+  update_module_enable($modules);
+++ b/core/modules/aggregator/aggregator.moduleundefined
@@ -279,7 +232,7 @@ function _aggregator_category_title($category) {
function _aggregator_has_categories() {
-  return user_access('access news feeds') && (bool) db_query_range('SELECT 1 FROM {aggregator_category}', 0, 1)->fetchField();
+  return user_access('access news feeds') && (bool) db_query_range("SELECT 1 FROM {taxonomy_term_data} WHERE vid = 'aggregator_categories'", 0, 1)->fetchField();
}

:( This could be done with a simpler query than you had, and you don't need to load the vocabulary. But, this is definitely the fastest.

+++ b/core/modules/aggregator/aggregator.pages.incundefined
@@ -412,15 +339,17 @@ function aggregator_page_categories() {
-  foreach ($result as $category) {
+  foreach ($result as $tid) {
+    $category = taxonomy_term_load($tid);
     $summary_items = array();

This should be loaded ahead of time with entity_load_multiple().

+++ b/core/modules/aggregator/aggregator.pages.incundefined
@@ -439,21 +368,23 @@ function aggregator_page_categories() {
   // arg(2) is the passed cid, only select for that category.
   if (arg(2)) {
-    $category = db_query('SELECT cid, title FROM {aggregator_category} WHERE cid = :cid', array(':cid' => arg(2)))->fetchObject();
-    $result = db_query_range('SELECT i.*, f.title AS ftitle, f.link AS flink FROM {aggregator_category_item} c LEFT JOIN {aggregator_item} i ON c.iid = i.iid LEFT JOIN {aggregator_feed} f ON i.fid = f.fid WHERE cid = :cid ORDER BY timestamp DESC, i.iid DESC', 0, $rss_config->get('items.limit'), array(':cid' => $category->cid));
+    $category = taxonomy_term_load(arg(2));
+    $items = aggregator_load_feed_items('category', $category, $rss_config->get('items.limit'));

Can we change this to menu_get_object()?

+++ b/core/modules/aggregator/aggregator.pages.incundefined
@@ -439,21 +368,23 @@ function aggregator_page_categories() {
+  }
+  // Add feed info to each item.
+  foreach ($items as $item) {
+    $item->feed = aggregator_feed_load($item->fid->value);
   }

load_multiple()

Almost any looping on entity_load() or entity_delete() should be a multiple instead.

Assigned:ParisLiakos» Unassigned
StatusFileSize
new3.57 KB
new98.49 KB
FAILED: [[SimpleTest]]: [MySQL] 54,033 pass(es), 25 fail(s), and 0 exception(s).
[ View ]

+++ b/core/modules/aggregator/aggregator.pages.incundefined
@@ -439,21 +368,23 @@ function aggregator_page_categories() {
+  }
+  // Add feed info to each item.
+  foreach ($items as $item) {
+    $item->feed = aggregator_feed_load($item->fid->value);
   }

load_multiple()

So loop over items, get all fids, load multiple and then loop again in items and assign it to them? feels a bit ugly to me:P
here is a patch that fixes the rest (besides aggregation query)

i think i have nothing more to add to this patch, thus unassigning

Assigned:Unassigned» twistor

So loop over items, get all fids, load multiple and then loop again in items and assign it to them? feels a bit ugly to me:P

It's not the prettiest, but it's better to do more loops and less queries.

Trying out the entity_query_aggregate().

StatusFileSize
new5.4 KB
new102.27 KB
PASSED: [[SimpleTest]]: [MySQL] 54,031 pass(es).
[ View ]

Added the entity_query_aggregate() call in aggregator_view() and also changed the duplicate feed checking to use entity_query().

+++ b/core/modules/aggregator/aggregator.admin.incundefined
@@ -63,31 +64,45 @@ function aggregator_view() {
+    $counts = entity_query_aggregate('aggregator_item')

I really like that usage!. Let's better use Drupal::service('entity.query')->getAggregate('aggregator_item')

Status:Needs review» Postponed

@dawehner should we switch the entity_query() calls to Drupal::entityQuery()?

Assigned:twistor» Unassigned
StatusFileSize
new3.74 KB
new102.33 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch drupal-aggregator_taxonomy-15266-62.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Quick find and replace.

Tagging for D8MI.

Status:Postponed» Needs review

As per #59

Status:Needs review» Needs work

The last submitted patch, drupal-aggregator_taxonomy-15266-62.patch, failed testing.

Assigned:Unassigned» ParisLiakos

StatusFileSize
new106.08 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch drupal-aggregator_taxonomy-15266-68.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

was quite hard to catch up...so many changes!
also a found a couple more bugs, one in the taxonomy term entity reference plugin:

+++ b/core/modules/taxonomy/lib/Drupal/taxonomy/Plugin/entity_reference/selection/TermSelection.phpundefined
@@ -67,7 +67,7 @@ public function getReferencableEntities($match = NULL, $match_operator = 'CONTAI
         if ($terms = taxonomy_get_tree($vocabulary->id(), 0)) {
           foreach ($terms as $term) {
-            $options[$vocabulary->id()][$term->id()] = str_repeat('-', $term->depth) . check_plain($term->name);
+            $options[$vocabulary->id()][$term->tid] = str_repeat('-', $term->depth) . check_plain($term->name);
           }
         }

and a typo in the FileldOverview class

+++ b/core/modules/field_ui/lib/Drupal/field_ui/FieldOverview.phpundefined
@@ -132,7 +132,7 @@ public function buildForm(array $form, array &$form_state, $entity_type = NULL,
-          '#title' => $widget_configuration ? $widget_types[$widget_configuration['type']]['label'] : $widget_types['hiden']['label'],
+          '#title' => $widget_configuration ? $widget_types[$widget_configuration['type']]['label'] : $widget_types['hidden']['label'],

The last submitted patch, drupal-aggregator_taxonomy-15266-68.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new105.19 KB
FAILED: [[SimpleTest]]: [MySQL] 55,731 pass(es), 42 fail(s), and 7 exception(s).
[ View ]

#1998992: Typo in FieldOverview.php was really fast:P fixed the typo

Status:Needs review» Needs work

The last submitted patch, drupal-aggregator_taxonomy-15266-70.patch, failed testing.

Assigned:ParisLiakos» Unassigned
Status:Needs work» Needs review
StatusFileSize
new104.29 KB
FAILED: [[SimpleTest]]: [MySQL] 55,846 pass(es), 35 fail(s), and 1 exception(s).
[ View ]

So..In module enable/disable test, forum_uninstall fails.
When calling field_purge_batch() an exception is thrown that an invalid instance was specified...and well since this will take me a while to debug, since i cant reproduce manually yet, and this happens to be the slowest webtest ever..i will try to pick it up in weekend

here is with the rest failures fixed

Status:Needs review» Needs work

The last submitted patch, drupal-aggregator_taxonomy-15266-72.patch, failed testing.

Issue tags:+MENU_LOCAL_ACTION

Blocks removal of MENU_LOCAL_ACTION

This should happen before July 1st it is an API change?

yeah, this is probably not gonna happen until D9

As this has been postponed, I've created a new issue for #2046303: Convert aggregator_form_category to FormInterface and updated the meta issue

Version:8.x-dev» 9.x-dev

the blocker is in, but i guess still 9.x material since its an API change

Version:9.x-dev» 8.x-dev
Status:Postponed» Active
Issue tags:+API change

Can we update the issue summary with api changes and let committer decide weather it is D8 or D9 metrial?

I agree it would be great to let committers decide.

agreed!
updated the issue summary

Issue summary:View changes

Updated issue summary.

Status:Active» Needs work

and the correct status

Issue summary:View changes

Updated issue summary

Issue summary:View changes

Updated issue summary

FYI: The intermediate step of first doing #2127725: Remove category handling from aggregator has quite some support and is RTBC, pending @Dries's approval.

(IMO) The removal does not imply that we cannot or should not continue to explore the proposed solution here for D8. Much rather, it allows us to think about the best possible (re-)implementation from a clean slate, leveraging the best tools and architecture available.