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.
For instructions on how to convert a page callback into a controller, see the WSCCI Conversion Guide.
Part of #1971384: [META] Convert page callbacks to controllers
Blocked By
#2040199: Convert aggregator_page_category_form and aggregator_page_source_form to a Controller
Related Issues
#2043581: Move aggregator_load_feed_items to the ItemStorageController, retain procedural wrapper
#1974408: Convert aggregator_page_source() to a Controller
#2040199: Convert aggregator_page_category_form and aggregator_page_source_form to a Controller
Comment | File | Size | Author |
---|---|---|---|
#27 | 1974394-aggregator-category-controller-27.patch | 4.45 KB | kim.pepper |
#27 | interdiff.txt | 598 bytes | kim.pepper |
#27 | categories-after.png | 38.09 KB | kim.pepper |
#19 | 1974394-aggregator-category-controller-19.patch | 4.43 KB | kim.pepper |
#19 | interdiff.txt | 2.29 KB | kim.pepper |
Comments
Comment #1
kim.pepperThis isn't pretty. There is a form that wraps this which has not been covered as part of this issue, and the autoloading of category as an argument is not supported with the new routing as category is not an entity.
Seeing what the bot says...
Comment #2
ParisLiakos CreditAttribution: ParisLiakos commentedmaybe we should wait for #15266: Replace aggregator category system with taxonomy here
Comment #3
kim.pepperComment #3.0
kim.pepperLinking to meta issue
Comment #4
lslinnet CreditAttribution: lslinnet commentedAs it is very unlikely that #15266: Replace aggregator category system with taxonomy will make it into core before july 1st, we might as well get this polished off and ready for being commited.
Comment #5
kim.pepperRe-roll
Comment #7
ParisLiakos CreditAttribution: ParisLiakos commentedlets just use directly drupal_set_title from the controller, and remove this callback
Comment #8
kim.pepperUse drupal_set_title in controller as per #7
Comment #9
lslinnet CreditAttribution: lslinnet commentedAs kim.pepper seems to have taken it up again i might as well unassign myself from it
Comment #10
dawehnerAre you sure there shouldn't be a title callback to support menu titles?
This should be certainly a _content instead.
This returns a render array
Comment #11
ygerasimov CreditAttribution: ygerasimov commentedTitle callback looks like not needed for menus. See screenshot.
Changed to
_content
Changed documentation.
Removed duplicate code in controller by using aggregator_page_category() function.
Comment #12
kim.pepperygerasimov can we have an interdiff please?
Comment #13
ygerasimov CreditAttribution: ygerasimov commentedHere is interdiff.
Comment #14
larowlanThis should inject the moduleHandler
I thought the goal was to remove aggregator_page_category?
Comment #16
ygerasimov CreditAttribution: ygerasimov commented@larowlan, thank you very much for the review. I have corrected patch to use moduleHandler. Lets remove aggregator_page_category in separate issue as main purpose of this one is new route.
Comment #17
ParisLiakos CreditAttribution: ParisLiakos commentedwe usually do that in conversions. there is no point on just introducing the routes if we keep the page callback functions
Comment #18
dawehnerAs this is the main functionality of this controller and the only place where we use this controller, let's convert that as well.
Comment #19
kim.pepperThe
aggregator_page_category_form()
form builder in aggregator.pages.inc is the only placeaggregator_page_category()
gets called.I've reverted the change in the controller to call
aggregator_page_category()
, inlined the logic inaggregator_page_category_form()
, and removedaggregator_page_category()
altogether.Kim
Comment #20
larowlanTitle callbacks work again, so we can revert this
Comment #21
dawehnerI think it is good to not have title callback anymore ... unless we have an actual menu item for this appearing on the page, is this the case? If not we could maybe drop the full menu entry?
Comment #22
ParisLiakos CreditAttribution: ParisLiakos commentedwe do have menu items for categories (although i admit, i never liked it) which is actually broken atm.
i should actually open an issue for that one time..so if the title callback works, lets keep it...for now..
Comment #23
kim.pepperI just tested this locally, and it looks like aggregator module is creating the menu items when the category is created, so there is no need to use a title callback at all.
In
aggregator_save_category()
we callmenu_link_maintain('aggregator', $op, $link_path, $edit['title'])
;Comment #24
kim.pepperBTW, I don't we need the menu item at all, but that is another issue.
Comment #25
ParisLiakos CreditAttribution: ParisLiakos commenteda, ha correct, thanks..
+1 on _aggregator_category_title removal then..
which issue then?
Comment #26
kim.pepperNo issue yet. I mean "but that is another topic". Bad choice of words. :-)
Comment #27
kim.pepperThis removes the menu entry entirely. Seems to still work, showing the menu links in the left sidebar.
Comment #28
Crell CreditAttribution: Crell commentedWhy not just convert this to a form class?
Totally not acceptable to do from within a controller method. Either inline the function we need as another method or just move it to the .module file.
Comment #29
ParisLiakos CreditAttribution: ParisLiakos commentedmaybe copy paste the buildPageList from #1974408: Convert aggregator_page_source() to a Controller, till its in.or postpone this one
Comment #30
kim.pepperIf we did convert
aggregator_page_category_form()
to a class that implements FormInterface, would it be ok to callAggregatorController:: buildPageList()
?? Seems to me that buildPageList() a render helper function, and we would need to inject AggregatorController into the new form class. Seems a bit strange to me.Comment #31
ParisLiakos CreditAttribution: ParisLiakos commentedno need to worry about the category form, we have a seperate issue for that #2040199: Convert aggregator_page_category_form and aggregator_page_source_form to a Controller (it actually doesnt need the buildPageLsit method at all, see the issue for more details)
Comment #32
piyuesh23 CreditAttribution: piyuesh23 commentedComment #33
kim.pepperpiyuesh23 I'm still working on this at the same time as #1974408: Convert aggregator_page_source() to a Controller
Comment #33.0
kim.pepperAdded blocked by link
Comment #33.1
kim.pepperrelated issues
Comment #34
kim.pepperCurrently blocked by #2040199: Convert aggregator_page_category_form and aggregator_page_source_form to a Controller as the form callbacks wrap page callbacks. Once that is in, we should combine this issue with #1974408: Convert aggregator_page_source() to a Controller as they both use common code.
Comment #35
kim.pepperThis is being combined with #1974408: Convert aggregator_page_source() to a Controller
Comment #35.0
kim.pepperupdated blocked by