Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
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.
Comment | File | Size | Author |
---|---|---|---|
#38 | aggregator-paging-743234-38.patch | 5.52 KB | xjm |
#37 | aggregator-paging-743234-37.patch | 5.48 KB | webchick |
#36 | aggregator-paging-743234-36-D8.patch | 5.48 KB | twistor |
#31 | 743234-aggregator-paging-test-only.patch | 1.94 KB | mr.baileys |
#31 | 743234-aggregator-paging-with-test.patch | 5.14 KB | mr.baileys |
Comments
Comment #1
aspilicious CreditAttribution: aspilicious commentedI can't access that page :)
Comment #2
mgiffordwoops.. change the permissions.. You should be able to see it now.
Comment #3
aspilicious CreditAttribution: aspilicious commentedI can see 20 feeds. No pagination.
Comment #4
mgiffordYup. 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.
Comment #5
mr.baileysThis currently is the "by design" behavior for the aggregator. The "/aggregator" path is implemented by aggregator_page_last(), which is documented as
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.
Comment #6
mgiffordInteresting.. 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.
Comment #7
mr.baileysAgreed, if previous version had pagers than this is probably not by design...
Comment #8
Jody LynnComment #9
mgiffordEven 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
Comment #10
mr.baileysLet'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)
Comment #11
BerdirTrailing spaces.
->fields('i') should be on a separate line.
This can be written like this:
->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.
Comment #12
mr.baileysThanks for the review, re-rolled with the changes you suggested.
Comment #13
BerdirUnfortunately, 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.
Comment #14
mr.baileysYes, 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.
Comment #15
mgiffordThe 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
Comment #16
mr.baileysNew patch addressing the issues raised in #13 and #15...
Comment #17
mgiffordLooks 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:
which can be simply added here:
http://api.drupal.org/api/drupal/modules--aggregator--aggregator-wrapper...
Comment #18
BerdirPatch 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.
Comment #19
mgiffordIt'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.
Comment #20
Delawaredbutters CreditAttribution: Delawaredbutters commentedUpdate: 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?
Comment #21
mgiffordYou 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.
Comment #22
Delawaredbutters CreditAttribution: Delawaredbutters commentedThanks 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.
Comment #23
mgiffordThis should be in D7, but...
Comment #24
Everett Zufelt CreditAttribution: Everett Zufelt commented#16: 743244-aggregator-paging-3.patch queued for re-testing.
Comment #25
sunLooks 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.
Comment #26
mgifford#16: 743244-aggregator-paging-3.patch queued for re-testing.
Comment #27
twistor CreditAttribution: twistor commentedAll 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.
Comment #28
sun1) 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
Comment #29
mr.baileysThanks for the feedback sun,
Yes, the missing pager is a regression from D6, which used
pager_query()
to fetch the feed items. Thepager_query()
was dropped when converting to DB:TNG, and whiletemplate_preprocess_aggregator_wrapper()
still has a call totheme('pager')
, the rendered pager is now empty since the global pager variables are not initialized. The initialization of those global pager vars is done bypager_default_initialize()
, which is called by extending the query with PagerDefault (in D6 it was called bypager_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.
Comment #30
sunpageR ?
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.
Comment #31
mr.baileysI 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...
Comment #32
mgiffordI didn't find a copy of this in my D8 install - modules/aggregator/aggregator.pages.inc
What am I missing?
Comment #33
twistor CreditAttribution: twistor commented@mgifford, this hasn't been committed yet.
However, it looks like all of the issues in #30 have been addressed.
Comment #34
chx CreditAttribution: chx commentedI have the file as well and the patch does look good.
Comment #35
webchickShoot. :( 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.)
Comment #36
twistor CreditAttribution: twistor commented+Needs forward port to D8.
Comment #37
webchickHeh, thanks.
Re-uploaded without -D8 extension so bot tests it.
Comment #38
xjmRerolled for core/. Assuming this is RTBC since it already went into D7. :)
Comment #39
catchLooks good. Committed/pushed to 8.x.
Comment #40
andypostfiled #1337018: Convert /aggregator to a view