Follow up to #2040199: Convert aggregator_page_category_form and aggregator_page_source_form to a Controller

We are making a db query in a controller, and it is inside a for loop.

Suggest moving this to a storage controller, and refactoring the query so it only needs to be executed once.

foreach (element_children($form_items) as $iid) {
        $categories_result = $this->database->query('SELECT c.cid, c.title, ci.iid FROM {aggregator_category} c LEFT JOIN {aggregator_category_item} ci ON c.cid = ci.cid AND ci.iid = :iid', array(':iid' => $iid));
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

ParisLiakos’s picture

could we move this database query to the category storage controller too, please?

kim.pepper’s picture

Status: Active » Needs review
FileSize
4.33 KB

This creates a new method on CategoryStorageController::loadByItem() that loads categories for an individual item.

ParisLiakos’s picture

Status: Needs review » Needs work

Thanks!

  1. +++ b/core/modules/aggregator/lib/Drupal/aggregator/CategoryStorageControllerInterface.php
    @@ -66,5 +66,16 @@ public function delete($cid);
    +   * @param array $item_id
    

    isnt this int, not array?:)

  2. +++ b/core/modules/aggregator/lib/Drupal/aggregator/CategoryStorageControllerInterface.php
    @@ -66,5 +66,16 @@ public function delete($cid);
    +   *   An array of
    

    incomplete sentence

kim.pepper’s picture

Status: Needs work » Needs review
FileSize
788 bytes
4.38 KB

Oops. Fixes docs issues in #3

kim.pepper’s picture

ParisLiakos, I also noticed we are calling the database directly in AggregatorCategorizeFormBase::submitForm()

Can we expand this issue to include those too?

kim.pepper’s picture

double post

ParisLiakos’s picture

well, running:
grep -r 'query(' core/modules/aggregator/ | grep 'category'
and found 19 results..attached in file

maybe we do them all here? so we get rid of db_* calls for categories, thus making their storage really swappable

kim.pepper’s picture

Assigned: Unassigned » kim.pepper

Sounds good. I can take a look later tonight.

ParisLiakos’s picture

Assigned: kim.pepper » Unassigned

but yes, of course, my grep above misses the db_insert and db_delete calls..so, yes lets just fix this one in the FormBase

ParisLiakos’s picture

ah! go ahead;) if you end up thinking that its too much of work for a single issue...let it be.
up to you. thanks!

kim.pepper’s picture

This moves the updating of categories in the submit handler to CategoryStorageController. It also means we can remove the database dependency from AggregatorCategorizeFormBase. Yay!

ParisLiakos’s picture

Title: Follow up: replace aggregator db_query in for loop with storage controller method » Use the aggregator category storage in AggregatorCategorizeFormBase
Status: Needs review » Reviewed & tested by the community

looks great:)
thanks. fixing title too:)

webchick’s picture

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

Patch no longer applies.

LinL’s picture

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

Rerolled.

ParisLiakos’s picture

Status: Needs review » Needs work
+++ b/core/modules/aggregator/lib/Drupal/aggregator/CategoryStorageController.php
@@ -98,4 +98,31 @@ public function isUnique($title, $cid = NULL) {
+  public function updateCategoriesForItem($iid, array $cids) {

i think we should name this updateByItem()
The Categories part is redundant

kim.pepper’s picture

Status: Needs work » Needs review
FileSize
2.18 KB
7.1 KB

Renamed to updateItem() Seems to make more sense to me!

ParisLiakos’s picture

Status: Needs review » Reviewed & tested by the community

yeap:) thanks!

kim.pepper’s picture

#16: 2063671-category-loop-16.patch queued for re-testing.

catch’s picture

Status: Reviewed & tested by the community » Fixed

Committed/pushed to 8.x, thanks!

Status: Fixed » Closed (fixed)

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