Change Aggregator block options from number to boolean

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?

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.

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.

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

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

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

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 ?

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

then just count me as a bug reporter

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.

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

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

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.

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

Doing a reroll first.

Missed a small thing... Attaching the fix.

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