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.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

aspilicious’s picture

I can't access that page :)

mgifford’s picture

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

aspilicious’s picture

I can see 20 feeds. No pagination.

mgifford’s picture

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.

mr.baileys’s picture

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.

mgifford’s picture

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.

mr.baileys’s picture

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

Jody Lynn’s picture

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

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

mr.baileys’s picture

Version: 8.x-dev » 7.x-dev
Assigned: Unassigned » mr.baileys
Priority: Normal » Major
Status: Needs work » Needs review
FileSize
3.26 KB

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)

Berdir’s picture

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:

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

mr.baileys’s picture

Status: Needs work » Needs review
FileSize
3.19 KB

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

Berdir’s picture

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

mr.baileys’s picture

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.

mgifford’s picture

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

mr.baileys’s picture

Status: Needs work » Needs review
FileSize
3.21 KB

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

mgifford’s picture

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

Berdir’s picture

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.

mgifford’s picture

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.

Delawaredbutters’s picture

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?

mgifford’s picture

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.

Delawaredbutters’s picture

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.

mgifford’s picture

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

This should be in D7, but...

Everett Zufelt’s picture

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

sun’s picture

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.

mgifford’s picture

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

twistor’s picture

Status: Needs review » Reviewed & tested by the community
FileSize
3.22 KB

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.

sun’s picture

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.
mr.baileys’s picture

Status: Needs work » Needs review
FileSize
5.08 KB

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.

sun’s picture

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.

mr.baileys’s picture

Status: Needs work » Needs review
FileSize
5.14 KB
1.94 KB

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

mgifford’s picture

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

What am I missing?

twistor’s picture

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.

chx’s picture

Issue tags: -Needs tests

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

webchick’s picture

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

twistor’s picture

Status: Needs work » Needs review
Issue tags: -Needs backport to D7
FileSize
5.48 KB

+Needs forward port to D8.

webchick’s picture

Heh, thanks.

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

xjm’s picture

Status: Needs review » Reviewed & tested by the community
FileSize
5.52 KB

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

catch’s picture

Status: Reviewed & tested by the community » Fixed

Looks good. Committed/pushed to 8.x.

andypost’s picture

Status: Fixed » Closed (fixed)

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