Just wanted to get a confirmation on this, but I can't seem see the pagination on the aggregator feeds here - http://drupal7.dev.openconcept.ca/aggregator

There should be enough feeds there to trigger it, but this would be a critical error if others are seeing it.

Files: 
CommentFileSizeAuthor
#38 aggregator-paging-743234-38.patch5.52 KBxjm
PASSED: [[SimpleTest]]: [MySQL] 33,972 pass(es).
[ View ]
#37 aggregator-paging-743234-37.patch5.48 KBwebchick
PASSED: [[SimpleTest]]: [MySQL] 33,444 pass(es).
[ View ]
#36 aggregator-paging-743234-36-D8.patch5.48 KBtwistor
#31 743234-aggregator-paging-test-only.patch1.94 KBmr.baileys
FAILED: [[SimpleTest]]: [MySQL] 32,987 pass(es), 1 fail(s), and 0 exception(es).
[ View ]
#31 743234-aggregator-paging-with-test.patch5.14 KBmr.baileys
PASSED: [[SimpleTest]]: [MySQL] 33,005 pass(es).
[ View ]
#29 743234-aggregator-paging-with-test.patch5.08 KBmr.baileys
PASSED: [[SimpleTest]]: [MySQL] 33,015 pass(es).
[ View ]
#27 743244-aggregator-paging-4.patch3.22 KBtwistor
PASSED: [[SimpleTest]]: [MySQL] 33,671 pass(es).
[ View ]
#16 743244-aggregator-paging-3.patch3.21 KBmr.baileys
PASSED: [[SimpleTest]]: [MySQL] 29,932 pass(es).
[ View ]
#12 743244-aggregator-paging-2.patch3.19 KBmr.baileys
PASSED: [[SimpleTest]]: [MySQL] 31,538 pass(es).
[ View ]
#10 aggregator-paging.patch3.26 KBmr.baileys
PASSED: [[SimpleTest]]: [MySQL] 31,509 pass(es).
[ View ]

Comments

I can't access that page :)

woops.. change the permissions.. You should be able to see it now.

I can see 20 feeds. No pagination.

Yup. and for clarification this link is comprised of three sources two of which have over 20 feeds. Twitter is probably the most obvious as I'm fairly vocal:
http://drupal7.dev.openconcept.ca/aggregator/sources/3
http://twitter.com/mgifford

But on the admin side there are 40, 25 & 3 feeds respectively, so the generic aggregator should either be displaying 68 feeds with no pagination or 20 pages with pagination.

Status:Active» Closed (works as designed)

This currently is the "by design" behavior for the aggregator. The "/aggregator" path is implemented by aggregator_page_last(), which is documented as

displays the most recent items gathered from any feed.

The number of most recent items to show is currently hardcoded as 20 in aggregator_feed_items_load(), called by aggregator_page_last().

Since this is marked as a bug, I'm setting this to "by design". However, feel free to convert this into a feature request to add pagination and/or make the limit configurable if you feel the design is flawed.

Status:Closed (works as designed)» Needs work

Interesting.. Well, it's a change in behavior from Drupal 5 & 6 as I recall. Certainly if you've got a lot of lists having basic pagination is pretty critical.

Drupal 5:
http://openconcept.ca/aggregator/categories/2

Drupal 6:
http://clf.openconcept.ca/aggregator

The definition you provided hasn't changed just the behavior. If the definition had changed then I'd agree that there was a change of design & that this should be a feature request and not a bug. If you'd like to have the definition for aggregator_page_last() read:

"displays only the last 20 recent items gathered from any feed."

I think you'd have some resistance.

Agreed, if previous version had pagers than this is probably not by design...

Title:Pagination not showing up/aggregator does not use pagination
Version:7.x-dev» 8.x-dev

Even though D7 is out I wonder if we might be able to get it in a maintenance release.

I think this must have been added as a shortcut when upgrading this module to D7 from D6. I think it boils down to this function:
aggregator_feed_items_load().

Version 6 used:
$result = pager_query($sql, 20);

which needs to be replaced like:
http://blog.worldempire.ch/story/howto-convert-module-dbtng

Version:8.x-dev» 7.x-dev
Assigned:Unassigned» mr.baileys
Priority:Normal» Major
Status:Needs work» Needs review
StatusFileSize
new3.26 KB
PASSED: [[SimpleTest]]: [MySQL] 31,509 pass(es).
[ View ]

Let's keep this at D7 for the time being since the D8 branch hasn't been created yet and this is a bug, not a feature request. I think mgifford is correct in that this is a fairly serious shortcoming (for example, if drupal.org were to be upgraded to D7 now, Planet Drupal would only show the 20 most recent stories, without pagination)

Status:Needs review» Needs work

+++ modules/aggregator/aggregator.pages.inc
@@ -90,30 +90,58 @@ function aggregator_page_category_form($form, $form_state, $category) {
+ *   The type of filter for the items. Possible values are: ¶
+ *   - 'sum': Filter the feed items to generate a summary of most recent ¶
+ *     stories.
+ *   - 'source': Filter the feed items, limiting the result to items from a ¶

Trailing spaces.

+++ modules/aggregator/aggregator.pages.inc
@@ -90,30 +90,58 @@ function aggregator_page_category_form($form, $form_state, $category) {
+      $query->fields('i')
+        ->condition('i.fid', $data->fid);

->fields('i') should be on a separate line.

+++ modules/aggregator/aggregator.pages.inc
@@ -90,30 +90,58 @@ function aggregator_page_category_form($form, $form_state, $category) {
+  $query = $query->extend('PagerDefault')->limit(20);
+  $query->orderBy('i.timestamp', 'DESC')->orderBy('i.iid', 'DESC');
+  $result = $query->execute()->fetchAll();

This can be written like this:

<?php
$result
= $query
 
->extend('PagerDefault')
  ->
limit(20)
  ->
orderBy('i.timestamp', 'DESC')
  ->
orderBy('i.iid', 'DESC')
  ->
execute();
?>

->fetchAll() is not necessary, because "foreach($result as $item)" will to that implicitly. Actually, it will do better than that, using the Iterator to fetch each row separatly instead of storing it in a large array.

Powered by Dreditor.

Status:Needs work» Needs review
StatusFileSize
new3.19 KB
PASSED: [[SimpleTest]]: [MySQL] 31,538 pass(es).
[ View ]

Thanks for the review, re-rolled with the changes you suggested.

+++ modules/aggregator/aggregator.pages.inc
@@ -90,30 +90,61 @@ function aggregator_page_category_form($form, $form_state, $category) {
+      $query->leftJoin('aggregator_item', 'i', 'c.iid = i.iid')
+        ->leftJoin('aggregator_feed', 'f', 'i.fid = f.fid')

Unfortunately, leftJoin() calls can not be chained like this because it doesn't return the query object.

If this doesn't fail, then it means that we don't have tests for this feature :)

Note there are several issues about adding better test coverage to aggregator.module, e.g. #276493: Tests needed: aggregator.module.

Powered by Dreditor.

Status:Needs review» Needs work

Yes, test-coverage for aggregator seems to be lagging behind the rest of core. I'll fix the chaining and reroll with a test-case for this particular issue, hopefully later today.

The second patch fails for me /aggregator/categories/2
Fatal error: Call to a member function leftJoin() on a non-object in /DRUPAL/modules/aggregator/aggregator.pages.inc on line 132

But seems to work fine aggregator/sources/13?page=3

Status:Needs work» Needs review
StatusFileSize
new3.21 KB
PASSED: [[SimpleTest]]: [MySQL] 29,932 pass(es).
[ View ]

New patch addressing the issues raised in #13 and #15...

Looks good to me. I applied it here http://wa2012.ca/aggregator

I'd like to have the MySQL reviewed before it gets marked RTBC.

http://wa2012.ca/aggregator/categories/2

In my public sample here I didn't have one that needed pagination http://wa2012.ca/aggregator/sources/

However, in another instance (not yet public) I was able to verify that this worked too.

I'd also like to add in a header element while we are doing this to the pagination for consistency & accessibility.

As with the blog pagination, this will help screen readers with navigation:

<h2 class="element-invisible">Pages</h2>

which can be simply added here:
http://api.drupal.org/api/drupal/modules--aggregator--aggregator-wrapper...

Patch looks good visually.

@mgifford: As Drupal 7 is stable now, strings and markup is AFAIK frozen. Maybe exceptions will be made (give that for example 'Pages' is already used somewhere and is therefore translated) but we should try to keep things separated and as non-invasive as possible. So I'd suggest to create a separate issue for that.

It's the same output string as used elsewhere. It's also ironically the reason that I first stumbled across this issue. I was looking for places we'd need pagination to insert a header.

I think that if we're opening up this issue for inclusion in a maintenance release we best do it right and consistent with the other pagination.

If the mysql is good to go I'll rework a patch and see if we can get them both included at once. If the maintainers want to consider it differently then it's easy to build a different issue and have it considered there.

Update: found this for others that are as confused as me

http://drupal.org/node/534548

my issue is resolved.

How can this patch be applied. AKA how can I fix this issue on my site? I have read http://drupal.org/patch/apply and am only more confused?

Doesn't the patch just replace code in the aggregator module? Can I just copy the code in the patch and paste it over code in the module?

I must sound like a total noob but really, how can I fix this issue on my site?

You can copy/paste the code in but you'd have to edit it a bunch.

If you don't have command line access and are using an editor and ftp or something then ya, you're probably not going to be able to apply a patch.

Thanks for the reply, it did seem complected, once I realized how to read the key, it was quite simple to just copy the code in and delete what was taken out. Just go one step at a time.

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

This should be in D7, but...

#16: 743244-aggregator-paging-3.patch queued for re-testing.

Priority:Major» Normal
Issue tags:+needs backport to D7

Looks like a stupid oversight, but there are countless of ways to remedy this issue if you face it. Hence, a regular bug like any other.

#16: 743244-aggregator-paging-3.patch queued for re-testing.

Status:Needs review» Reviewed & tested by the community
StatusFileSize
new3.22 KB
PASSED: [[SimpleTest]]: [MySQL] 33,671 pass(es).
[ View ]

All I did was re-roll it to be easily applied without -p0. Marking it as RTBC. I've reviewed the queries as per #17. Let me know if it's not kosher.

Status:Reviewed & tested by the community» Needs work
Issue tags:+Needs tests

1) Is a themed pager already part of the output elsewhere in the code? The patch doesn't add one, just adjusts the query.

2) Ideally, this should be covered by basic tests. Nothing überfancy, just confirm there's a pager.

3) In the phpDoc, the property/key names in the list shouldn't be in ' quotes, just

*   - foo: The number of Foo.

Status:Needs work» Needs review
StatusFileSize
new5.08 KB
PASSED: [[SimpleTest]]: [MySQL] 33,015 pass(es).
[ View ]

Thanks for the feedback sun,

1) Is a themed pager already part of the output elsewhere in the code? The patch doesn't add one, just adjusts the query.

Yes, the missing pager is a regression from D6, which used pager_query() to fetch the feed items. The pager_query() was dropped when converting to DB:TNG, and while template_preprocess_aggregator_wrapper() still has a call to theme('pager'), the rendered pager is now empty since the global pager variables are not initialized. The initialization of those global pager vars is done by pager_default_initialize(), which is called by extending the query with PagerDefault (in D6 it was called by pager_query()), hence after applying this patch the pagers automatically show up.

I have fixed the incorrectly quoted list items in the Doxygen-block and added a basic test to verify the presence of the pager.

Status:Needs review» Needs work

+++ b/modules/aggregator/aggregator.test
@@ -807,6 +813,27 @@ class AggregatorRenderingTestCase extends AggregatorTestCase {
+   * Create a feed and check that feed's page.

pageR ?

+++ b/modules/aggregator/aggregator.test
@@ -807,6 +813,27 @@ class AggregatorRenderingTestCase extends AggregatorTestCase {
+    $this->assertTrue($this->xpath("//ul[@class='pager']"), t('Individual source page contains a pager.'));

The class attribute value needs to use a token placeholder :class + , array(':class' => 'pager')

xpath() returns a DOM element set, usually denoted as $elements. The assertTrue() has to to check !empty($elements[0])

After those adjustments, this should be RTBC. Please upload two patches, one only containing the test changes, to confirm that this test catches the bug.

3 days to next Drupal core point release.

Status:Needs work» Needs review
StatusFileSize
new5.14 KB
PASSED: [[SimpleTest]]: [MySQL] 33,005 pass(es).
[ View ]
new1.94 KB
FAILED: [[SimpleTest]]: [MySQL] 32,987 pass(es), 1 fail(s), and 0 exception(es).
[ View ]

Create a feed and check that feed's page.

I left this at "page" instead of "pager" since I figure the test case can be extended with other tests specific to a feed's display page later on, even through right now it only tests the pager-aspect.

Uploaded a new patch which uses the placeholder and !empty($elements)-construct with the xpath(), and a test-only patch as per request...

I didn't find a copy of this in my D8 install - modules/aggregator/aggregator.pages.inc

What am I missing?

Status:Needs review» Reviewed & tested by the community

@mgifford, this hasn't been committed yet.

However, it looks like all of the issues in #30 have been addressed.

Issue tags:-Needs tests

I have the file as well and the patch does look good.

Status:Reviewed & tested by the community» Needs work

Shoot. :( I committed/pushed to 7.x but the patch doesn't apply to 8.x. I can haz quick re-roll for D8?

(Note to self: Apply to both, then push.)

Status:Needs work» Needs review
Issue tags:-needs backport to D7
StatusFileSize
new5.48 KB

+Needs forward port to D8.

StatusFileSize
new5.48 KB
PASSED: [[SimpleTest]]: [MySQL] 33,444 pass(es).
[ View ]

Heh, thanks.

Re-uploaded without -D8 extension so bot tests it.

Status:Needs review» Reviewed & tested by the community
StatusFileSize
new5.52 KB
PASSED: [[SimpleTest]]: [MySQL] 33,972 pass(es).
[ View ]

Rerolled for core/. Assuming this is RTBC since it already went into D7. :)

Status:Reviewed & tested by the community» Fixed

Looks good. Committed/pushed to 8.x.

Status:Fixed» Closed (fixed)

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