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.
Part of #1971384: [META] Convert page callbacks to controllers
As #15266: Replace aggregator category system with taxonomy has been postponed, we can go ahead with this conversion.
Related
#1974394: Convert aggregator_page_category() to a Controller
Comment | File | Size | Author |
---|---|---|---|
#29 | 2046303-29-aggregator-un-use-d.patch | 688 bytes | tstoeckler |
#22 | 2046303-aggregator-category-form-22.patch | 27.8 KB | kim.pepper |
#22 | interdiff.txt | 776 bytes | kim.pepper |
#21 | 2046303-aggregator-category-form-21.patch | 27.69 KB | kim.pepper |
#21 | interdiff.txt | 683 bytes | kim.pepper |
Comments
Comment #1
kim.pepperThis is quite a bit more than a form conversion.
Comment #3
kim.pepperRemoved leftover references to non-existant aggregator.admin.inc file.
Comment #4
kim.pepperNo need for entityManager in CategoryAdminForm any more, so removed.
Comment #4.0
kim.pepperAdded link to taxonomy issue
Comment #5
ParisLiakos CreditAttribution: ParisLiakos commentedclosed #2041083: Move database query out of \Drupal\aggregator\FeedFormController as duplicate
Comment #6
ParisLiakos CreditAttribution: ParisLiakos commentedThanks, looks good so far
Needs a newline
not sure whats that? is there such a path?
One less newline:)
Any reason we couldnt have those methods merged in a single save() method?
can we have those in multiple lines?
Could we have methods for those too?
lets use the Request object and not arg()
inject this?
same here. lets use the Request
Comment #7
kim.pepperThanks for the review @ParisLiakos.
I would prefer to keep them separate as insert has a return value of the new id, and they are running completely different queries in the implementation. I'd like to avoid passing in $op params wherever possible.
I've added a new method
isUnique()
on the CategoryStorageControllerInterface, that takes an optional $cid param to exclude for checking for uniqueness if we are renaming an existing category.Injecting Request in both forms now via buildForm().
I wasn't sure this was ok, since we don't know if the service exists? We are checking if the module is enabled first.
Comment #9
kim.pepper#7: 2046303-aggregator-category-form-7.patch queued for re-testing.
Comment #11
ParisLiakos CreditAttribution: ParisLiakos commentedisUnique sounds great thanks:)
fetchCol?
Well, you could make the constructor argument optional, and perform the same check with the module handler in the create() method, right?
Comment #12
kim.pepperFixes fetchCol() and conditionally injects the BlockManager as suggested in #11.
Comment #13
kim.pepperTag monster.
Comment #14
ParisLiakos CreditAttribution: ParisLiakos commentedwell, looks great..two more nitpicks and sth else which if its too much of work we could leave for a followup
Connection needs the full namespace
how much more work would it be, to make those arrays an stdClass object instead, so we avoid those (object) conversions..lets standarise to stdClass for now..till they become taxonomy terms
needs a newline in between
Comment #15
ParisLiakos CreditAttribution: ParisLiakos commentedalso..i was now checking aggregator_category_load() and well, i think that this function should be marked deprecated and just be an one line wrapper for
CategoryStorageControllerInterface::load()
so i noticed that currently we do fetchObject() not fetchAssoc(). also the query does
SELECT *
which i like better, because it means, it will load any additional fields added by hook_schema_alter.Finally this method should similarly take care of static cache, probably storing loaded category stdClasses in a class property, not drupal_static ofc
Comment #16
kim.pepperThanks for the review.
This takes care of all changes in #14 and #15 except for:
Can this be a follow up?
Comment #17
kim.pepperdouble-post
Comment #18
ParisLiakos CreditAttribution: ParisLiakos commentedThank you:)
so you got rid of the unused "use" statements but we should fix the arguments here:)
you updated the @return but not the description:)
when i was asking about standarizing to stdClass i meant those too:) before it was consistent to array, but now load() gives you an object save and update take an array.
so we typecast (object) on $form_state['values'] before passing to storage controller.
oops i missed that in the previous review
Well, if we want to make aggregator_category_load() deprecated, we need to make it one-liner wrapper:) i dont think it is committable with keeping the same logic in two places in this point of release cycle
Comment #19
kim.pepperThanks again.
Here are the fixes for #18.
Comment #20
ParisLiakos CreditAttribution: ParisLiakos commentedbesides killing the legacy sql in the deprecated function:
we could still typehint this with \stdClass i guess:)
Comment #21
kim.pepperKills legacy sql as per #20.
Agreed with ParisLiakos in IRC to leave out stdClass type hinting.
Comment #22
kim.pepperGot confused with the incorrect docblock comments on aggregator_category_load(). We actually return an stdClass and not an associative array. Updated the function and the comments.
Comment #23
ParisLiakos CreditAttribution: ParisLiakos commentedthank you!
Comment #24
tstoecklerSo the last patch actually makes this an API change, technically. For a non-existing $cid aggregator_category_load() now returns NULL, where it returned an empty array before. Because the previous behavior wasn't very semantic anyway I think it's OK do make this change, but this needs committer approval. It would also be possible to retain the current behavior but in this case, I really don't see why that would be necessary.
Comment #25
ParisLiakos CreditAttribution: ParisLiakos commentedNot really. Before the patch:
After the patch
Which is exact the same behavior
Its the docs that were inaccurate before, which @kim.pepper fixed:)
Comment #26
tstoecklerAhh, I see. How naive of me to trust our API documentation. Sorry for the noise and thanks for clearing that up!
Comment #27
Dries CreditAttribution: Dries commentedCommitted to 8.x. Thanks.
Comment #28
ParisLiakos CreditAttribution: ParisLiakos commentedfollowup for forgotten aggregator_save_category() :)
#2068393: Mark aggregator_save_category() as deprecated
Comment #29
tstoecklerThis seems to be unused.
Comment #30
kim.pepperNice catch.
Comment #31
ParisLiakos CreditAttribution: ParisLiakos commentedlets take this to the followup #2068393: Mark aggregator_save_category() as deprecated shall we?
its already fixed there.
Comment #32.0
(not verified) CreditAttribution: commentedAdded related issues