Support from Acquia helps fund testing for Drupal Acquia logo

Comments

marcingy’s picture

Assigned: Unassigned » marcingy
marcingy’s picture

Status: Active » Needs review
FileSize
9.59 KB
ParisLiakos’s picture

+++ 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

ParisLiakos’s picture

Status: Needs review » Closed (duplicate)
ParisLiakos’s picture

Status: Closed (duplicate) » Postponed

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

ParisLiakos’s picture

Status: Postponed » Needs work

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

marcingy’s picture

Status: Needs work » Needs review
FileSize
11.32 KB

Ok hopefully this is along the correct lines

Berdir’s picture

Status: Needs review » Needs work
  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...

marcingy’s picture

Status: Needs work » Needs review
FileSize
2.88 KB
11.4 KB

Ok should be done

ParisLiakos’s picture

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

twistor’s picture

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.

marcingy’s picture

FileSize
11.6 KB
3.04 KB

Added return docs and renamed method, also adds another couple of doc fixes.

Status: Needs review » Needs work

The last submitted patch, issue-2041083-12.patch, failed testing.

marcingy’s picture

Status: Needs work » Needs review
FileSize
726 bytes
11.59 KB

Doh...

Status: Needs review » Needs work

The last submitted patch, issue-2041083-14.patch, failed testing.

ParisLiakos’s picture

Status: Needs work » Needs review

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

ParisLiakos’s picture

Status: Needs review » Needs work
+++ 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!

ParisLiakos’s picture

+++ 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

ParisLiakos’s picture

  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

plach’s picture

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

marcingy’s picture

Assigned: marcingy » Unassigned

I don't have time atm so unassigning :)

ParisLiakos’s picture

Title: Move database query out of /core/modules/aggregator/lib/Drupal/aggregator/FeedFormController.php » Move database query out of \Drupal\aggregator\FeedFormController
grisendo’s picture

Applied changes suggested by @ParisLiakos

ParisLiakos’s picture

Status: Needs review » Reviewed & tested by the community
+++ 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

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 2041083-move-db-query-out-feedformcontroller-23.patch, failed testing.

grisendo’s picture

Status: Needs work » Needs review

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

grisendo’s picture

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

grisendo’s picture

Assigned: grisendo » Unassigned

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!

grisendo’s picture

Assigned: Unassigned » grisendo
grisendo’s picture

grisendo’s picture

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

ParisLiakos’s picture

Status: Needs review » Reviewed & tested by the community

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

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs reroll

Patch no longer applies.

herom’s picture

Status: Needs work » Needs review
Issue tags: -Needs reroll
FileSize
13.25 KB

rerolled.

ParisLiakos’s picture

Status: Needs review » Reviewed & tested by the community
alexpott’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs reroll

Patch no longer applies.

herom’s picture

Status: Needs work » Needs review
Issue tags: -Needs reroll
FileSize
13.24 KB

let's do this again.

ParisLiakos’s picture

Status: Needs review » Reviewed & tested by the community

thanks!

Xano’s picture

catch’s picture

Status: Reviewed & tested by the community » Fixed

Committed/pushed to 8.x, thanks!

plach’s picture

Status: Fixed » Closed (fixed)

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