Aggregator exposes a block per feed. Sites with 1000 feeds will have 1000 aggregator blocks on admin/build/block - this does not scale.

I for one would be all for removing this feature altogether.

I'm guessing there will be some objection to removing. At least we need to decouple block generation from feeds present in system. Ideas for UI?

Files: 
CommentFileSizeAuthor
#21 aggregator-block-options-398556-21.patch2.97 KBhussainweb
PASSED: [[SimpleTest]]: [MySQL] 57,891 pass(es).
[ View ]
#20 aggregator-block-options-398556-20.patch2.99 KBhussainweb
FAILED: [[SimpleTest]]: [MySQL] 57,827 pass(es), 3 fail(s), and 2 exception(s).
[ View ]
#14 398556-11.patch2.18 KBJody Lynn
PASSED: [[SimpleTest]]: [MySQL] 53,022 pass(es).
[ View ]
#11 aggregator-398556_11.patch4.64 KBdrzraf
PASSED: [[SimpleTest]]: [MySQL] 40,200 pass(es).
[ View ]
#8 aggregator-398556_8.patch4.42 KBdrzraf
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch aggregator-398556_8.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#6 aggregator-398556.patch4.9 KBsheise
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch aggregator-398556_0.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#4 aggregator-398556.patch4.72 KBsheise
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch aggregator-398556.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Comments

geodaniel’s picture

Setting the 'News items in block' option to 0 stops it from exposing the block already... that could just be set to default to 0 instead of 5. Perhaps better would be to add a 'Create block of feed items' checkbox and, if checked, expose the select box to choose how many items get shown?

alex_b’s picture

#1:

It's kind of weird in the first place that the presentation in a block is part of the feed settings. I'd rather remove the block setting entirely from the feed form and replace it by a new tab on admin/build/block/add

http://skitch.com/alexbarth/b8amh/blocks-localhost ?

Jody Lynn’s picture

Title:One block per feed does not scale» Aggregator block options
Version:7.x-dev» 8.x-dev
Category:bug» feature

The same is true for menu module and its block creation.

I think the 'Create block of feed items' checkbox is right on, and a similar checkbox could be added to menus as well.

The 'news items in block' field and its lame outdated and complicated description: "Drupal can make a block with the most recent news items of this feed. You can configure blocks to be displayed in the sidebar of your page. This setting lets you configure the number of news items to show in this feed's block. If you choose '0' this feed's block will be disabled." should be reimagined as a block setting, and there should be a link from aggregator feed editing to this block configuration page when a block has been created.

sheise’s picture

StatusFileSize
new4.72 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch aggregator-398556.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Here's an attempted patch.

This patch:
- Replaces the "Number of items to display in the feed’s block" setting on the feed with a checkbox for creating a block.
- Hijacks the aggregator_feed "block" db column for this purpose and also adds a new "items" column.
- Add "Number of items" setting on the block settings page.

This all seems to work ok, except if you have placed a block somewhere and uncheck the setting to create block, errors will then appear on the page. This is the first work I've done on D7 so I wonder if it's something with the new db layer that I am unfamiliar with.

Jody Lynn’s picture

Status:Active» Needs work
+++ aggregator.admin.inc 7 Jan 2011 17:57:34 -0000
@@ -84,11 +84,10 @@ function aggregator_form_feed($form, &$f
-  $form['block'] = array('#type' => 'select',

Existing issue, but there should be a new line after each array( in this function

+++ aggregator.admin.inc 7 Jan 2011 17:57:34 -0000
@@ -84,11 +84,10 @@ function aggregator_form_feed($form, &$f
+    '#default_value' => isset($feed->block) ? $feed->block : '',

Have it default to 1 if $feed->block is not set to match current behavior.

+++ aggregator.install 7 Jan 2011 17:57:34 -0000
@@ -50,7 +50,14 @@ function aggregator_schema() {
+      'items' => array(
+        'type' => 'int',
+        'not null' => TRUE,
+        'default' => 0,
+        'size' => 'tiny',
+        'description' => 'Number of items to display in the feed’s block.',

Use a new variable to store the new block configuration settings to be consistent with other modules' block configs

+++ aggregator.module 7 Jan 2011 17:57:34 -0000
@@ -371,6 +371,17 @@ function aggregator_block_configure($del
+      '#default_value' => $value,

default value should be 5 if it is not set

+++ aggregator.module 7 Jan 2011 17:57:34 -0000
@@ -371,6 +371,17 @@ function aggregator_block_configure($del
+      '#options' => drupal_map_assoc(array(1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16, 17, 18, 19, 20)),
+    );

replace this array with range(1, 20)

+++ aggregator.module 7 Jan 2011 17:57:34 -0000
@@ -371,6 +371,17 @@ function aggregator_block_configure($del
+  }  ¶

extra whitespace

You also need to alter aggregator_feed_save to delete the block if the option is unchecked.

Powered by Dreditor.

sheise’s picture

StatusFileSize
new4.9 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch aggregator-398556_0.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Ok, here's a second patch with the changes suggested by Jody Lynn.

Jody Lynn’s picture

+++ aggregator.admin.inc 7 Jan 2011 22:37:06 -0000
@@ -84,11 +84,11 @@ function aggregator_form_feed($form, &$f
+  ¶
+  $form['block'] = array('#type' => 'checkbox',
+    '#title' => t('Create block of feed items'),
+    '#default_value' => isset($feed->block) ? $feed->block : '1',
+    '#description' => t("Drupal can make a block with the most recent news items of this feed. You can <a href="@block-admin">configure blocks</a> to be displayed in the sidebar of your page.", array('@block-admin' => url('admin/structure/block'))),
   );

Remove the extra line break.

Needs a line break between array( and '#type' (and likewise for the other form elements in this function)

Don't put number 1 in quotes.

+++ aggregator.install 7 Jan 2011 22:37:06 -0000
@@ -50,7 +50,7 @@ function aggregator_schema() {
+        'description' => 'Whether a block has been created for this feed.',

'Whether a block has been created' implies a history exists between this data record and block creation. Maybe something like 'Boolean indicating if there should be a corresponding block for this feed.'

+++ aggregator.module 7 Jan 2011 22:37:06 -0000
@@ -371,6 +371,15 @@ function aggregator_block_configure($del
+  if ($type == 'feed') {
+    $form['aggregator_block_items'] = array(
+      '#type' => 'select',
+      '#title' => t('Number of news items in block'),
+      '#default_value' => variable_get('aggregator_block_items' . $id , 5),
+      '#options' => drupal_map_assoc(range(1,20)),
+    );
+    return $form;
+  }  ¶

Add an underscore in the variable name, aggregator_block_items_

Need a space: range(1, 20)

Watch the trailing whitespace (look at it in dreditor)

+++ aggregator.module 7 Jan 2011 22:37:06 -0000
@@ -397,9 +409,9 @@ function aggregator_block_view($delta =
+          $result = db_query_range("SELECT * FROM {aggregator_item} WHERE fid = :fid ORDER BY timestamp DESC, iid DESC", 0, variable_get('aggregator_block_items' . $id), array(':fid' => $id));

variable_get always needs to have its default value as the second parameter (5 in this case)

+++ aggregator.module 7 Jan 2011 22:37:06 -0000
@@ -497,6 +509,13 @@ function aggregator_save_feed($edit) {
+    if ($edit['block'] == '0') {
+      // Make sure there is no active block for this feed.
+      db_delete('block')
+        ->condition('module', 'aggregator')
+        ->condition('delta', 'feed-' . $edit['fid'])
+        ->execute();
+    }  ¶

if (!$edit['block']) is more compact

watch the trailing whitespace

It looks like there are actually two different ways to make blocks: aggregatory categories and aggregator feeds, so this needs to be extended to work with both ways.

Also, for the new variables, they need to be deleted when a feed/category is deleted, and on hook_uninstall.

Powered by Dreditor.

drzraf’s picture

StatusFileSize
new4.42 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch aggregator-398556_8.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

rerolled (against D7 but still patch with relative paths) to address issues from #7 except :

new variables, they need to be deleted
=> added variable_del() when the block is disabled
That means that this settings is not stored across activation/deactivation of a block
But we can't know which variables id are left at module deactivation time

aggregatory categories and aggregator feeds, so this needs to be extended to work with both ways.
=> is this really needed, ie dozens of categories ?

ParisLiakos’s picture

Status:Needs work» Needs review

Status:Needs review» Needs work

The last submitted patch, aggregator-398556_8.patch, failed testing.

drzraf’s picture

Version:8.x-dev» 7.x-dev
Status:Needs work» Needs review
StatusFileSize
new4.64 KB
PASSED: [[SimpleTest]]: [MySQL] 40,200 pass(es).
[ View ]
ParisLiakos’s picture

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

The change should go in d8 first..it is too late to add features to d7

drzraf’s picture

then just count me as a bug reporter

Jody Lynn’s picture

Category:feature» bug
StatusFileSize
new2.18 KB
PASSED: [[SimpleTest]]: [MySQL] 53,022 pass(es).
[ View ]

It looks like when the new block system went in, the ability to configure the number of items shown per block already went in.

However, the old aggregator settings to configure the number of items per block were not removed although they no longer work, so this is now a bug.

ParisLiakos’s picture

Status:Needs review» Needs work

I guess we dont need an upgrade path here (this should happen here? #1871700: Provide an upgrade path to the new Block architecture from Drupal 7), but we should update entity storages/forms as well

Btw, see also #1888702: Use configuration selection instead of derivatives for some blocks

xjm’s picture

Note that we'll also probably be converting these blocks to views.

xjm’s picture

hussainweb’s picture

Title:Aggregator block options» Change Aggregator block options from number to boolean
Assigned:Unassigned» hussainweb

I see that this issue will help in cleaning up a test in AggregatorRenderingTest. The test passes, but does not catch an error.

I observed it when working on #2047593: Inject dependencies into EntityDerivative derivative class. Like I said, the test had passed but I saw an error when checking the verbose output. I then wondered why was the block count repeated in two places, and then I found this issue.

I will work on this patch tonight and add/edit relevant tests.

hussainweb’s picture

Priority:Normal» Major

I think this should count as major due to its nature and various relations to other issues in queue.

hussainweb’s picture

Status:Needs work» Needs review
StatusFileSize
new2.99 KB
FAILED: [[SimpleTest]]: [MySQL] 57,827 pass(es), 3 fail(s), and 2 exception(s).
[ View ]

Doing a reroll first.

hussainweb’s picture

StatusFileSize
new2.97 KB
PASSED: [[SimpleTest]]: [MySQL] 57,891 pass(es).
[ View ]

Missed a small thing... Attaching the fix.

Berdir’s picture

Status:Needs review» Closed (duplicate)

#1888702: Use configuration selection instead of derivatives for some blocks is RTBC and solves this problem by only offering a single aggregator block which you can then configure to use a given feed or category (one block for each) and removes that option from feeds completely.

Re-introducing that as a checkbox would IMHO be a feature, feel free to re-open if you want that, closing for now.