Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
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));
Comment | File | Size | Author |
---|---|---|---|
#16 | 2063671-category-loop-16.patch | 7.1 KB | kim.pepper |
#16 | interdiff.txt | 2.18 KB | kim.pepper |
#14 | 2063671-category-loop-14.patch | 7.14 KB | LinL |
#11 | 2063671-category-loop-11.patch | 7.32 KB | kim.pepper |
#11 | interdiff.txt | 5.76 KB | kim.pepper |
Comments
Comment #1
ParisLiakos CreditAttribution: ParisLiakos commentedcould we move this database query to the category storage controller too, please?
Comment #2
kim.pepperThis creates a new method on CategoryStorageController::loadByItem() that loads categories for an individual item.
Comment #3
ParisLiakos CreditAttribution: ParisLiakos commentedThanks!
isnt this int, not array?:)
incomplete sentence
Comment #4
kim.pepperOops. Fixes docs issues in #3
Comment #5
kim.pepperParisLiakos, I also noticed we are calling the database directly in AggregatorCategorizeFormBase::submitForm()
Can we expand this issue to include those too?
Comment #6
kim.pepperdouble post
Comment #7
ParisLiakos CreditAttribution: ParisLiakos commentedwell, 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
Comment #8
kim.pepperSounds good. I can take a look later tonight.
Comment #9
ParisLiakos CreditAttribution: ParisLiakos commentedbut yes, of course, my grep above misses the db_insert and db_delete calls..so, yes lets just fix this one in the FormBase
Comment #10
ParisLiakos CreditAttribution: ParisLiakos commentedah! go ahead;) if you end up thinking that its too much of work for a single issue...let it be.
up to you. thanks!
Comment #11
kim.pepperThis moves the updating of categories in the submit handler to CategoryStorageController. It also means we can remove the database dependency from AggregatorCategorizeFormBase. Yay!
Comment #12
ParisLiakos CreditAttribution: ParisLiakos commentedlooks great:)
thanks. fixing title too:)
Comment #13
webchickPatch no longer applies.
Comment #14
LinL CreditAttribution: LinL commentedRerolled.
Comment #15
ParisLiakos CreditAttribution: ParisLiakos commentedi think we should name this updateByItem()
The Categories part is redundant
Comment #16
kim.pepperRenamed to updateItem() Seems to make more sense to me!
Comment #17
ParisLiakos CreditAttribution: ParisLiakos commentedyeap:) thanks!
Comment #18
kim.pepper#16: 2063671-category-loop-16.patch queued for re-testing.
Comment #19
catchCommitted/pushed to 8.x, thanks!