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.
Updated: Comment #0
Problem/Motivation
We are using aggregator_load_feed_items() in OO code, we should be able to just inject the controller and use a method on that. We can retain the procedural wrapper and mark it as deprecated.
Proposed resolution
Add new loadFeedItems() method to the ItemStorageController and interface, mark the procedural function as deprecated
Remaining tasks
Patch
Review
User interface changes
None
API changes
None
Related Issues
#1974394: Convert aggregator_page_category() to a Controller
#1974408: Convert aggregator_page_source() to a Controller
#2040199: Convert aggregator_page_category_form and aggregator_page_source_form to a Controller
Comment | File | Size | Author |
---|---|---|---|
#22 | 2043581-load-feed-items-22.patch | 7 KB | kim.pepper |
#22 | interdiff.txt | 4.43 KB | kim.pepper |
#18 | 2043581-load-feed-items-18.patch | 7.1 KB | kim.pepper |
#18 | interdiff.txt | 968 bytes | kim.pepper |
#9 | 2043581-load-feed-items-9.patch | 6.83 KB | kim.pepper |
Comments
Comment #1
larowlanStraightforward patch.
Comment #2
tim.plunkettIf we're going to bother keeping the wrapper, might as well fix up the @return.
@return \Drupal\aggregator\ItemInterface[]
would be best
Comment #3
larowlanSure
Comment #4
larowlanMissed a few
Comment #5
larowlanand a patch would help
Comment #6
tim.plunkettLooking at this more closely, we have three different codepaths with three different uses for $data.
Why not write three methods with proper typehinting? They can have a protected helper for the shared pager bit at the bottom.
Comment #8
ParisLiakos CreditAttribution: ParisLiakos commentedmisses the return statement;)
#6 would be awesome
Comment #9
kim.pepperThis splits out the methods as per #6 and adds the return statement.
Comment #9.0
kim.pepperAdded related issues
Comment #11
kim.pepper"Failed to write configuration file" Trying again.
Comment #12
kim.pepper#9: 2043581-load-feed-items-9.patch queued for re-testing.
Comment #14
kim.pepperFieldUpgradePathTest fatal. Re-trying.
Comment #15
kim.pepper#9: 2043581-load-feed-items-9.patch queued for re-testing.
Comment #16
tim.plunkettLooks great!
Comment #17
alexpottBeen waiting for this for a while :)
Or loadFeedItemsBySource or loadFeedItemsByCategory... this comment should list the three functions this has been refactored into.
Comment #18
kim.pepperDocs fixes in #17
Comment #19
kim.pepperI assume since it's just doc fixes this can go back to RTBC
Comment #20
xjm#18: 2043581-load-feed-items-18.patch queued for re-testing.
Comment #21
ParisLiakos CreditAttribution: ParisLiakos commentedSorry for checking this that late and i kick it to NW..but methods name dont make much sense to me.
the procedural function name shouldnt not affect the naming.
i would suggest this being
loadAll()
I had to check the docblock to understand what it does and why is different of just load()..
loadByFeed
Source, well, should die sometime. its a "feed" entity not "source"
loadByCategory
No need to have FeedItems in the method name, its the the Feed Item storage controller
Comment #22
kim.pepperThanks for the review.
This fixes all of #21.
Comment #23
ParisLiakos CreditAttribution: ParisLiakos commentedthank you
Comment #24
alexpottLooks great.
Committed 91bd01a and pushed to 8.x. Thanks!
Comment #25
kim.pepperProposed change notice:
Loading Feed Items has moved to the Storage Controller
The aggregator_load_feed_items() function has been split up and moved to 3 separate methods in \Drupal\aggregator\ItemStorageControllerInterface.
In Drupal 7:
In Drupal 8
Comment #26
kim.pepperChange notice added [#2059971]
Comment #26.0
kim.pepperduplicate
Comment #26.1
kim.pepperAdded change notice
Comment #27.0
(not verified) CreditAttribution: commentedremoved change notice