Use the entity storage controller in aggregator module

i have no clue what to do with

  $result = db_query('SELECT fid FROM {aggregator_feed} WHERE queued = 0 AND checked + refresh < :time AND refresh <> :never', array(
    ':time' => REQUEST_TIME,
    ':never' => AGGREGATOR_CLEAR_NEVER
  ));

The other change in aggregator_cron() could use some benchmarking?
see #1930274-53: Convert aggregator processors and parsers to plugins

oops

sigh..status

EntityFilteringThemeTest.php line 122
random failure?

#3: drupal-aggregator_entity_storage-1957312-3.patch queued for re-testing.

entity_delete_multiple() needs the id array..so i looped the items instead...hmm but now that you say so, i think entity_load_multiple() returns the array keyed by ids so i could use array_keys i think, will check later

this is a lot better, you are right:)

1. +++ b/core/modules/aggregator/lib/Drupal/aggregator/Plugin/aggregator/processor/DefaultProcessor.php
   @@ -190,9 +190,11 @@ public function process(Feed $feed) {
   +    $result = \Drupal::entityQuery('aggregator_item')
   
   @@ -209,13 +211,12 @@ public function postProcess(Feed $feed) {
   +      $result = \Drupal::entityQuery('aggregator_item')
   

   we can now inject stuff to plugins so entity query should be injected.

2. +++ b/core/modules/aggregator/lib/Drupal/aggregator/Plugin/aggregator/processor/DefaultProcessor.php
   @@ -190,9 +190,11 @@ public function process(Feed $feed) {
   +      entity_delete_multiple('aggregator_item', $result);
   
   @@ -209,13 +211,12 @@ public function postProcess(Feed $feed) {
   +        entity_delete_multiple('aggregator_item', $result);
   

   and the item storage controller

There is also one more query in \Drupal\aggregator\Plugin\Block\AggregatorFeedBlock::build() to get rid of and one in \Drupal\aggregator\Controller\AggregatorController::adminOverview()

1. +++ b/core/modules/aggregator/lib/Drupal/aggregator/Controller/AggregatorController.php
   @@ -173,33 +173,42 @@ public function feedRefresh(FeedInterface $aggregator_feed, Request $request) {
   +    $result = \Drupal::entityQuery('aggregator_feed')
   +      ->execute();
   ...
   +    $feeds = entity_load_multiple('aggregator_feed', $result);
   

   you dont have to use the entityQuery at all. i think just calling entity_load_mutiple without the second parameter, gives you all the entities

2. +++ b/core/modules/aggregator/lib/Drupal/aggregator/Entity/Feed.php
   @@ -170,6 +170,16 @@ public function label($langcode = NULL) {
   +  public function getItemCount() {
   

   this method should be moved to the FeedStorageController instead (and also added to its interface)

3. +++ b/core/modules/aggregator/lib/Drupal/aggregator/Plugin/Block/AggregatorFeedBlock.php
   @@ -131,31 +131,33 @@ public function blockSubmit($form, &$form_state) {
   +      $result = \Drupal::entityQuery('aggregator_item')
   

   to inject this:

   in the class::create() method, add as parameter $container->get('entity.query')->get('aggregator_item') to the new static() call.

   This will pass it to the class constructor as additional argument, where you will take it and store it to a property. eg $this->itemEntityQuery.

   then instead of using \Drupal::entityQuery('aggregator_item') use the property instead;)

4. +++ b/core/modules/aggregator/lib/Drupal/aggregator/Plugin/Block/AggregatorFeedBlock.php
   @@ -131,31 +131,33 @@ public function blockSubmit($form, &$form_state) {
   +      $items = entity_load_multiple('aggregator_item', $result);
   

   same for this but in the create method you will add

   $container->get('entity.manager')->getStorageController('aggregator_item')

5. +++ b/core/modules/aggregator/lib/Drupal/aggregator/Plugin/Block/AggregatorFeedBlock.php
   @@ -131,31 +131,33 @@ public function blockSubmit($form, &$form_state) {
   -      $result = $this->connection->queryRange("SELECT * FROM {aggregator_item} WHERE fid = :fid ORDER BY timestamp DESC, iid DESC", 0, $this->configuration['block_count'], array(':fid' => $feed->id()));
   +      $result = \Drupal::entityQuery('aggregator_item')
   +        ->condition('fid', $feed->id())
   +        ->range(0, $this->configuration['block_count'])
   +        ->execute();
   

   we should add the ORDER BY part in the entity query too

1. +++ b/core/modules/aggregator/lib/Drupal/aggregator/Controller/AggregatorController.php
   @@ -173,33 +173,42 @@ public function feedRefresh(FeedInterface $aggregator_feed, Request $request) {
   +    $feeds = entity_load_multiple('aggregator_feed');
   ...
   +    $storage_controller = \Drupal::entityManager()->getStorageController('aggregator_feed');
   

   lets retrieve the storage controller before the entity_load_multiple call, and use it there too.
   also it should be retrieved with $this->entityManager()

2. +++ b/core/modules/aggregator/lib/Drupal/aggregator/FeedStorageController.php
   @@ -62,4 +62,21 @@ public function deleteCategories(array $feeds) {
   +  public function getItemCount(Feed $feed) {
   
   +++ b/core/modules/aggregator/lib/Drupal/aggregator/FeedStorageControllerInterface.php
   @@ -42,4 +42,20 @@ public function saveCategories(Feed $feed, array $categories);
   +  public function getItemCount(Feed $feed);
   

   should be typehinted with FeedInterface

3. +++ b/core/modules/aggregator/lib/Drupal/aggregator/FeedStorageController.php
   @@ -62,4 +62,21 @@ public function deleteCategories(array $feeds) {
   +  public function getFeedsToRefresh() {
   +    return $this->database->query('SELECT fid FROM {aggregator_feed} WHERE queued = 0 AND checked + refresh < :time AND refresh <> :never', array(
   ...
   +    ));
   

   so this actually retrieves the feed IDs not the whole feed objects.
   So i would say, name it getFeedIdsToRefresh().

   Also it shouldnt return the query, but rather it should fetchCol() first

4. +++ b/core/modules/aggregator/lib/Drupal/aggregator/Plugin/Block/AggregatorFeedBlock.php
   @@ -130,32 +149,36 @@ public function blockSubmit($form, &$form_state) {
   -    if ($feed = $this->storageController->load($this->configuration['feed'])) {
   ...
   +    if ($feed = $this->itemStorage->load($this->configuration['feed'])) {
   

   i think this should be feedStroage not itemStorage

5. +++ b/core/modules/aggregator/lib/Drupal/aggregator/Plugin/Block/AggregatorFeedBlock.php
   @@ -130,32 +149,36 @@ public function blockSubmit($form, &$form_state) {
   +      $items = entity_load_multiple('aggregator_item', $result);
   

   this should also use the itemStorage to load and not the procedural function

6. +++ b/core/modules/aggregator/lib/Drupal/aggregator/Plugin/aggregator/processor/DefaultProcessor.php
   @@ -190,9 +190,11 @@ public function process(Feed $feed) {
   +    $result = \Drupal::entityQuery('aggregator_item')
   ...
   +      entity_delete_multiple('aggregator_item', $result);
   
   @@ -209,13 +211,12 @@ public function postProcess(Feed $feed) {
   +      $result = \Drupal::entityQuery('aggregator_item')
   ...
   +        entity_delete_multiple('aggregator_item', $result);
   

   lets inject the entity_query and the item storage here as described in #21

looks great to me, thanks!

Just one thing

+++ b/core/modules/aggregator/lib/Drupal/aggregator/Plugin/aggregator/processor/DefaultProcessor.php
@@ -190,9 +212,12 @@ public function process(Feed $feed) {
+    $result = $this->entityQuery->get('aggregator_item')
+      ->condition('fid', $feed->id())
+      ->execute();

we should use the loadByFeed() method in ItemStorageController..By making the method there to default to NULL $limit rather than 20.So one can use it to fetch all items too.. (Change that would affect loadByCategory() too)

and also a reroll

@peximo Yes exactly:) thanks for rolling this

Thanks! Looks almost done.
Just a few docs issues i could find

1. +++ b/core/modules/aggregator/lib/Drupal/aggregator/FeedStorageControllerInterface.php
   @@ -26,4 +26,19 @@
   +  /**
   +   * Returns the fids of feed that need to be refreshed.
   

   Probably feed here should be plural. ie feeds

2. +++ b/core/modules/aggregator/lib/Drupal/aggregator/ItemStorageController.php
   @@ -23,7 +23,7 @@ class ItemStorageController extends FieldableDatabaseStorageController implement
   -  public function loadAll($limit = 20) {
   +  public function loadAll($limit = FALSE) {
   
   @@ -33,7 +33,7 @@ public function loadAll($limit = 20) {
   -  public function loadByFeed($fid, $limit = 20) {
   +  public function loadByFeed($fid, $limit = FALSE) {
   

   it should default to NULL, not FALSE :)

3. ItemStorageControllerInterface::loadAll() docblock still says it defaults to 20

Thank you!

ah, yes, good point. there is a count() method in QueryInterface.
also i found a couple forgotten spots that need to be converted as well

yes, its a lot better now:)

getItemCount should be in the Item storage controller not the Feed one.
also got rid the database from aggregator controller

ah

reroll

lets also use the methods now that #2028037: Expand FeedInterface with methods is in.
this should be good to go now

Reroll plus some docs fixes/additions

reroll

and of course, we dont need that hack anymore:)

one more reroll

reroll

yay, thanks!
but aggregator goes faaaast those days:D

rerolls++

67: 1957312-drupal-aggregator_entity_storage-67.patch queued for re-testing.