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));
Files: 
CommentFileSizeAuthor
#16 2063671-category-loop-16.patch7.1 KBkim.pepper
PASSED: [[SimpleTest]]: [MySQL] 58,961 pass(es).
[ View ]
#16 interdiff.txt2.18 KBkim.pepper
#14 2063671-category-loop-14.patch7.14 KBLinL
PASSED: [[SimpleTest]]: [MySQL] 59,072 pass(es).
[ View ]
#11 2063671-category-loop-11.patch7.32 KBkim.pepper
PASSED: [[SimpleTest]]: [MySQL] 58,465 pass(es).
[ View ]
#11 interdiff.txt5.76 KBkim.pepper
#7 aggregator-category-db-calls.txt4.36 KBParisLiakos
#4 2063671-category-loop-4.patch4.38 KBkim.pepper
PASSED: [[SimpleTest]]: [MySQL] 58,433 pass(es).
[ View ]
#4 interdiff.txt788 byteskim.pepper
#2 2063671-category-loop-2.patch4.33 KBkim.pepper
PASSED: [[SimpleTest]]: [MySQL] 58,029 pass(es).
[ View ]

Comments

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

Status:Active» Needs review
StatusFileSize
new4.33 KB
PASSED: [[SimpleTest]]: [MySQL] 58,029 pass(es).
[ View ]

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

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

Status:Needs work» Needs review
StatusFileSize
new788 bytes
new4.38 KB
PASSED: [[SimpleTest]]: [MySQL] 58,433 pass(es).
[ View ]

Oops. Fixes docs issues in #3

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

Can we expand this issue to include those too?

double post

StatusFileSize
new4.36 KB

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

Assigned:Unassigned» kim.pepper

Sounds good. I can take a look later tonight.

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

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!

StatusFileSize
new5.76 KB
new7.32 KB
PASSED: [[SimpleTest]]: [MySQL] 58,465 pass(es).
[ View ]

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

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

looks great:)
thanks. fixing title too:)

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

Patch no longer applies.

Status:Needs work» Needs review
Issue tags:-Needs reroll
StatusFileSize
new7.14 KB
PASSED: [[SimpleTest]]: [MySQL] 59,072 pass(es).
[ View ]

Rerolled.

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

Status:Needs work» Needs review
StatusFileSize
new2.18 KB
new7.1 KB
PASSED: [[SimpleTest]]: [MySQL] 58,961 pass(es).
[ View ]

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

Status:Needs review» Reviewed & tested by the community

yeap:) thanks!

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

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.