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));
CommentFileSizeAuthor
#16 2063671-category-loop-16.patch7.1 KBkim.pepper
#16 interdiff.txt2.18 KBkim.pepper
#14 2063671-category-loop-14.patch7.14 KBlinl
#11 2063671-category-loop-11.patch7.32 KBkim.pepper
#11 interdiff.txt5.76 KBkim.pepper
#7 aggregator-category-db-calls.txt4.36 KBParisLiakos
#4 2063671-category-loop-4.patch4.38 KBkim.pepper
#4 interdiff.txt788 byteskim.pepper
#2 2063671-category-loop-2.patch4.33 KBkim.pepper

Comments

ParisLiakos’s picture

ParisLiakos commented

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

kim.pepper’s picture

kim.pepper
Primary language English
Location 🏄‍♂️🇦🇺Sydney, Australia
commented
Status: Active » Needs review
StatusFileSize
new 2063671-category-loop-2.patch4.33 KB

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

ParisLiakos’s picture

ParisLiakos commented
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

kim.pepper
Primary language English
Location 🏄‍♂️🇦🇺Sydney, Australia
commented
Status: Needs work » Needs review
StatusFileSize
new interdiff.txt788 bytes
new 2063671-category-loop-4.patch4.38 KB

Oops. Fixes docs issues in #3

kim.pepper’s picture

kim.pepper
Primary language English
Location 🏄‍♂️🇦🇺Sydney, Australia
commented

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

kim.pepper
Primary language English
Location 🏄‍♂️🇦🇺Sydney, Australia
commented

double post

ParisLiakos’s picture

ParisLiakos commented
StatusFileSize
new aggregator-category-db-calls.txt4.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

kim.pepper’s picture

kim.pepper
Primary language English
Location 🏄‍♂️🇦🇺Sydney, Australia
commented
Assigned: Unassigned » kim.pepper

Sounds good. I can take a look later tonight.

ParisLiakos’s picture

ParisLiakos commented
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

ParisLiakos commented

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

kim.pepper
Primary language English
Location 🏄‍♂️🇦🇺Sydney, Australia
commented
StatusFileSize
new interdiff.txt5.76 KB
new 2063671-category-loop-11.patch7.32 KB

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

ParisLiakos commented
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

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
commented
Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs reroll

Patch no longer applies.

linl’s picture

linl commented
Status: Needs work » Needs review
Issue tags: -Needs reroll
StatusFileSize
new 2063671-category-loop-14.patch7.14 KB

Rerolled.

ParisLiakos’s picture

ParisLiakos commented
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

kim.pepper
Primary language English
Location 🏄‍♂️🇦🇺Sydney, Australia
commented
Status: Needs work » Needs review
StatusFileSize
new interdiff.txt2.18 KB
new 2063671-category-loop-16.patch7.1 KB

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

ParisLiakos’s picture

ParisLiakos commented
Status: Needs review » Reviewed & tested by the community

yeap:) thanks!

kim.pepper’s picture

kim.pepper
Primary language English
Location 🏄‍♂️🇦🇺Sydney, Australia
commented

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

catch’s picture

catch
he/him
Primary language English
commented
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.