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.
This breaks the ability to use a different storage controller for feeds, or in other words, everyone implementing a different storage controller for feeds, needs to swap out this form controller.
Comments
Comment #1
marcingy CreditAttribution: marcingy commentedComment #2
marcingy CreditAttribution: marcingy commentedComment #3
ParisLiakos CreditAttribution: ParisLiakos commentedthe 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
Comment #4
ParisLiakos CreditAttribution: ParisLiakos commentedor #2046303: Convert aggregator_form_category to FormInterface
Comment #5
ParisLiakos CreditAttribution: ParisLiakos commentedor it might be postponed till we get the category storage controller
Comment #6
ParisLiakos CreditAttribution: ParisLiakos commentedso category storage is in #2046303: Convert aggregator_form_category to FormInterface, lets move those queries to methods:)
Comment #7
marcingy CreditAttribution: marcingy commentedOk hopefully this is along the correct lines
Comment #8
BerdirThe implements isn't necessary, that's just the default entity form controller interface that's already implemented by the base class.
When touching anyway, let's change this to String::checkPlain()
ClientInterface,as you make changes already anyway...
Comment #9
marcingy CreditAttribution: marcingy commentedOk should be done
Comment #10
ParisLiakos CreditAttribution: ParisLiakos commentedlooks good
I just want to point one thing:
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
Comment #11
twistor CreditAttribution: twistor commentedLooking good.
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.
Comment #12
marcingy CreditAttribution: marcingy commentedAdded return docs and renamed method, also adds another couple of doc fixes.
Comment #14
marcingy CreditAttribution: marcingy commentedDoh...
Comment #16
ParisLiakos CreditAttribution: ParisLiakos commented#14: issue-2041083-14.patch queued for re-testing.
Comment #17
ParisLiakos CreditAttribution: ParisLiakos commentedNitpick:
this should be \Drupal\aggregator\FeedInterface
Besides this, it looks ready to fly, thanks!
Comment #18
ParisLiakos CreditAttribution: ParisLiakos commentedah, also seems the databasse is not used anymore in OpmlFeedAdd class now right? we should remove it
Comment #19
ParisLiakos CreditAttribution: ParisLiakos commentedneeds @return docs
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
Comment #20
plachAre you still working on this? Otherwise tomorrow we should be able to bring it forward at the Prague extended sprint.
Comment #21
marcingy CreditAttribution: marcingy commentedI don't have time atm so unassigning :)
Comment #22
ParisLiakos CreditAttribution: ParisLiakos commentedComment #23
grisendo CreditAttribution: grisendo commentedApplied changes suggested by @ParisLiakos
Comment #24
ParisLiakos CreditAttribution: ParisLiakos commentedjust 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
Comment #26
grisendo CreditAttribution: grisendo commented#14: issue-2041083-14.patch queued for re-testing.
Comment #27
grisendo CreditAttribution: grisendo commentedSomething went wrong... Patch from #14 sent to retest to check if it is still valid.
Comment #28
grisendo CreditAttribution: grisendo commentedfailing, 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!
Comment #29
grisendo CreditAttribution: grisendo commentedComment #30
grisendo CreditAttribution: grisendo commentedDone, lets see...
Also fixed #24
Comment #31
grisendo CreditAttribution: grisendo commentedI think this doesn't need tests since is like a refactor, right?
Comment #32
ParisLiakos CreditAttribution: ParisLiakos commentedyeap, exactly.
looks awesome!
Παρακαλώ! ;)
Comment #33
alexpottPatch no longer applies.
Comment #34
herom CreditAttribution: herom commentedrerolled.
Comment #35
ParisLiakos CreditAttribution: ParisLiakos commentedComment #36
alexpottPatch no longer applies.
Comment #37
herom CreditAttribution: herom commentedlet's do this again.
Comment #38
ParisLiakos CreditAttribution: ParisLiakos commentedthanks!
Comment #39
Xano#37: 2041083-move-db-query-out-feedformcontroller-37.patch queued for re-testing.
Comment #40
catchCommitted/pushed to 8.x, thanks!
Comment #41
plach