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

#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

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

larowlan’s picture

Status: Active » Needs review
FileSize
4.73 KB

Straightforward patch.

tim.plunkett’s picture

+++ b/core/modules/aggregator/aggregator.pages.incundefined
@@ -103,42 +103,14 @@ function aggregator_page_category_form($form, $form_state, $category) {
  * @return
  *   An array of the feed items.

+++ b/core/modules/aggregator/lib/Drupal/aggregator/ItemStorageControllerInterface.phpundefined
@@ -41,4 +41,28 @@ public function deleteCategories(array $entities);
+   * @return array
+   *   An array of the feed items.

If we're going to bother keeping the wrapper, might as well fix up the @return.

@return \Drupal\aggregator\ItemInterface[]

would be best

larowlan’s picture

larowlan’s picture

larowlan’s picture

and a patch would help

tim.plunkett’s picture

+++ b/core/modules/aggregator/lib/Drupal/aggregator/ItemStorageController.phpundefined
@@ -112,4 +112,42 @@ public function saveCategories(Item $item) {
+      case 'sum':
...
+      case 'source':
...
+      case 'category':

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

Status: Needs review » Needs work

The last submitted patch, aggregator-load-feed-items-2040265.5.patch, failed testing.

ParisLiakos’s picture

+++ b/core/modules/aggregator/aggregator.pages.incundefined
@@ -102,43 +102,17 @@ function aggregator_page_category_form($form, $form_state, $category) {
+  Drupal::entityManager()->getStorageController('aggregator_item')->loadFeedItems($type, $data, $limit);

misses the return statement;)

#6 would be awesome

kim.pepper’s picture

Status: Needs work » Needs review
FileSize
6.55 KB
6.83 KB

This splits out the methods as per #6 and adds the return statement.

kim.pepper’s picture

Issue summary: View changes

Added related issues

Status: Needs review » Needs work

The last submitted patch, 2043581-load-feed-items-9.patch, failed testing.

kim.pepper’s picture

Status: Needs work » Needs review

"Failed to write configuration file" Trying again.

kim.pepper’s picture

#9: 2043581-load-feed-items-9.patch queued for re-testing.

Status: Needs review » Needs work

The last submitted patch, 2043581-load-feed-items-9.patch, failed testing.

kim.pepper’s picture

Status: Needs work » Needs review

FieldUpgradePathTest fatal. Re-trying.

kim.pepper’s picture

#9: 2043581-load-feed-items-9.patch queued for re-testing.

tim.plunkett’s picture

Status: Needs review » Reviewed & tested by the community

Looks great!

alexpott’s picture

Status: Reviewed & tested by the community » Needs work

Been waiting for this for a while :)

+++ b/core/modules/aggregator/aggregator.pages.incundefined
@@ -102,43 +102,27 @@ function aggregator_page_category_form($form, $form_state, $category) {
+ * @deprecated Use \Drupal\aggregator\ItemStorageController::loadFeedItems()
+ *   instead.

Or loadFeedItemsBySource or loadFeedItemsByCategory... this comment should list the three functions this has been refactored into.

kim.pepper’s picture

Status: Needs work » Needs review
FileSize
968 bytes
7.1 KB

Docs fixes in #17

kim.pepper’s picture

Status: Needs review » Reviewed & tested by the community

I assume since it's just doc fixes this can go back to RTBC

xjm’s picture

#18: 2043581-load-feed-items-18.patch queued for re-testing.

ParisLiakos’s picture

Status: Reviewed & tested by the community » Needs work

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

+++ b/core/modules/aggregator/lib/Drupal/aggregator/ItemStorageControllerInterface.phpundefined
@@ -41,4 +41,42 @@ public function deleteCategories(array $entities);
+  public function loadFeedItems($limit = 20);

i would suggest this being loadAll()
I had to check the docblock to understand what it does and why is different of just load()..

+++ b/core/modules/aggregator/lib/Drupal/aggregator/ItemStorageControllerInterface.phpundefined
@@ -41,4 +41,42 @@ public function deleteCategories(array $entities);
+  public function loadFeedItemsBySource($fid, $limit = 20);

loadByFeed
Source, well, should die sometime. its a "feed" entity not "source"

+++ b/core/modules/aggregator/lib/Drupal/aggregator/ItemStorageControllerInterface.phpundefined
@@ -41,4 +41,42 @@ public function deleteCategories(array $entities);
+  public function loadFeedItemsByCategory($cid, $limit = 20);

loadByCategory

No need to have FeedItems in the method name, its the the Feed Item storage controller

kim.pepper’s picture

Status: Needs work » Needs review
FileSize
4.43 KB
7 KB

Thanks for the review.

This fixes all of #21.

ParisLiakos’s picture

Status: Needs review » Reviewed & tested by the community

thank you

alexpott’s picture

Title: Move aggregator_load_feed_items to the ItemStorageController, retain procedural wrapper » Change notice: Move aggregator_load_feed_items to the ItemStorageController, retain procedural wrapper
Status: Reviewed & tested by the community » Active
Issue tags: +Needs change record

Looks great.

Committed 91bd01a and pushed to 8.x. Thanks!

kim.pepper’s picture

Status: Active » Needs review

Proposed 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:


// Loads all feed items.
$items = aggregator_load_feed_items('sum');

// Loads feed items filtered by feed 'source'.  $feed object contains a fid property for the feed ID.
$items = aggregator_load_feed_items('source', $feed);

// Loads feed items filtered by feed category. $category array contains a cid key for the category ID.
$items = aggregator_load_feed_items('category', $category);

In Drupal 8


// Get the ItemStorageController from the dependency injection container.
$storage_controller = Drupal::entityManager()->getStorageController('aggregator_item');

// Loads all feed items.
$storage_controller->loadAll();

// Loads feed items by feed ID ($fid).
$storage_controller->loadByFeed($fid)

// Loads feed items by Category ID ($cid).
$storage_controller->loadByCategory($cid);

kim.pepper’s picture

Title: Change notice: Move aggregator_load_feed_items to the ItemStorageController, retain procedural wrapper » Move aggregator_load_feed_items to the ItemStorageController, retain procedural wrapper
Status: Needs review » Fixed
Issue tags: -Needs change record

Change notice added [#2059971]

kim.pepper’s picture

Issue summary: View changes

duplicate

kim.pepper’s picture

Issue summary: View changes

Added change notice

Status: Fixed » Closed (fixed)

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

Anonymous’s picture

Issue summary: View changes

removed change notice