Move database query out of \Drupal\aggregator\FeedFormController

+++ b/core/modules/aggregator/lib/Drupal/aggregator/FeedStorageController.phpundefined
@@ -140,4 +139,25 @@ public function deleteCategories(array $feeds) {
+  public function loadAllCategories() {
+    return $this->database->query('SELECT c.cid, c.title FROM {aggregator_category} c ORDER BY title')->fetchAllKeyed();
+  }

+++ b/core/modules/aggregator/lib/Drupal/aggregator/FeedStorageControllerInterface.phpundefined
@@ -42,4 +42,16 @@ public function saveCategories(Feed $feed, array $categories);
+  public function loadAllCategories();

the feed storage has nothing to do with categories..i think this is a correct way to solve this #15266: Replace aggregator category system with taxonomy

or #2046303: Convert aggregator_form_category to FormInterface

or it might be postponed till we get the category storage controller

so category storage is in #2046303: Convert aggregator_form_category to FormInterface, lets move those queries to methods:)

1. +++ b/core/modules/aggregator/lib/Drupal/aggregator/FeedFormController.php
   @@ -7,13 +7,52 @@
   -class FeedFormController extends EntityFormControllerNG {
   +class FeedFormController extends EntityFormControllerNG implements EntityFormControllerInterface {
   

   The implements isn't necessary, that's just the default entity form controller interface that's already implemented by the base class.

2. +++ b/core/modules/aggregator/lib/Drupal/aggregator/FeedFormController.php
   @@ -63,11 +102,11 @@ public function form(array $form, array &$form_state) {
   +      $options[$cid] = check_plain($title);
   

   When touching anyway, let's change this to String::checkPlain()

3. +++ b/core/modules/aggregator/lib/Drupal/aggregator/Form/OpmlFeedAdd.php
   @@ -51,22 +52,32 @@ class OpmlFeedAdd implements ControllerInterface, FormInterface {
   -   * @param \Guzzle\Http\Client
   +   * @param \Guzzle\Http\Client $http_client
   

   ClientInterface,as you make changes already anyway...

looks good
I just want to point one thing:

+++ b/core/modules/aggregator/lib/Drupal/aggregator/CategoryStorageController.php
@@ -98,4 +98,11 @@ public function isUnique($title, $cid = NULL) {
+  public function loadAllCategories() {
+    return $this->database->query('SELECT c.cid, c.title FROM {aggregator_category} c ORDER BY title')->fetchAllKeyed();

Maybe this name, should be changed to something more descriptive.
I would expect a method called "loadAllCategories" to return an array of
category objects.
Maybe name it loadAllKeyed() ?

Also no need to have the Categories part in the method name, it is the category storage

Looking good.

+++ b/core/modules/aggregator/lib/Drupal/aggregator/CategoryStorageControllerInterface.phpundefined
@@ -66,5 +66,10 @@ public function delete($cid);
+  /**
+   * Loads all categories.
+   */
+  public function loadAllCategories();

Needs @return docs. Plus what ParisLiakos said.

On a side note, why isn't categories defined on the Feed entity? Either as a field, a computed field, or at least a method so we can lazy load them. Also, FeedStorageController is doing looping db queries. These are unrelated to the patch though.

#14: issue-2041083-14.patch queued for re-testing.

+++ b/core/modules/aggregator/lib/Drupal/aggregator/FeedStorageController.php
@@ -39,7 +38,7 @@ public function loadCategories(array $feeds) {
-  public function saveCategories(Feed $feed, array $categories) {
+  public function saveCategories(EntityInterface $feed, array $categories) {

+++ b/core/modules/aggregator/lib/Drupal/aggregator/FeedStorageControllerInterface.php
@@ -7,7 +7,7 @@
-use Drupal\aggregator\Entity\Feed;
+use Drupal\Core\Entity\EntityInterface;

@@ -27,12 +27,12 @@ public function loadCategories(array $feeds);
-   * @param Feed $feed
+   * @param \Drupal\Core\Entity\EntityInterface $feed
...
-  public function saveCategories(Feed $feed, array $categories);
+  public function saveCategories(EntityInterface $feed, array $categories);

Nitpick:
this should be \Drupal\aggregator\FeedInterface

Besides this, it looks ready to fly, thanks!

+++ b/core/modules/aggregator/lib/Drupal/aggregator/Form/OpmlFeedAdd.php
@@ -59,14 +67,17 @@ class OpmlFeedAdd extends FormBase {
     $this->database = $database;

ah, also seems the databasse is not used anymore in OpmlFeedAdd class now right? we should remove it

1. +++ b/core/modules/aggregator/lib/Drupal/aggregator/FeedStorageControllerInterface.php
   @@ -42,4 +42,11 @@ public function saveCategories(Feed $feed, array $categories);
   +  /**
   +   * Provides a list of duplicate feeds.
   +   *
   +   * @param \Drupal\Core\Entity\EntityInterface $feed
   +   *   The feed entity.
   +   */
   +  public function getFeedDuplicates(EntityInterface $feed);
   

   needs @return docs

2. +++ b/core/modules/aggregator/lib/Drupal/aggregator/FeedStorageController.php
   @@ -62,4 +61,18 @@ public function deleteCategories(array $feeds) {
   +    return $query;
   

   which brings me to my other point:
   Instead of returning the $query we should better call fetchAll() on it before.

   i am also wondering whether its worth it to use entity query here

also i added this issue to #2068325: [META] Convert entity SQL queries to the Entity Query API

Are you still working on this? Otherwise tomorrow we should be able to bring it forward at the Prague extended sprint.

Applied changes suggested by @ParisLiakos

+++ b/core/modules/aggregator/lib/Drupal/aggregator/FeedFormController.php
@@ -17,6 +20,43 @@
+  protected $feedStorage;
...
+  protected $categoryStorageController;

just a nitpick, which shouldnt stop this from being committed..i would say for the sake of consistency either append on both properties Controller or to none.

RTBC either way though

#14: issue-2041083-14.patch queued for re-testing.

Something went wrong... Patch from #14 sent to retest to check if it is still valid.

failing, but just because of http://drupalcode.org/project/drupal.git/commitdiff/4bdcb12bf93362242d968ad65022c6cf342e3cf2

Reverting locally to my branch equivalent to comment #14 an starting working on @ParisLiakos suggestions step by step to check where is it breaking. Ευχαριστώ πολύ by the way!

Done, lets see...
Also fixed #24

I think this doesn't need tests since is like a refactor, right?

yeap, exactly.
looks awesome!
Παρακαλώ! ;)

Patch no longer applies.

rerolled.

Patch no longer applies.

let's do this again.

thanks!

#37: 2041083-move-db-query-out-feedformcontroller-37.patch queued for re-testing.

Committed/pushed to 8.x, thanks!