Support from Acquia helps fund testing for Drupal Acquia logo

Comments

kim.pepper’s picture

Status: Active » Needs review
FileSize
6 KB

This 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...

ParisLiakos’s picture

kim.pepper’s picture

Status: Needs review » Postponed
kim.pepper’s picture

Issue summary: View changes

Linking to meta issue

lslinnet’s picture

Assigned: Unassigned » lslinnet
Status: Postponed » Needs work

As 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.

kim.pepper’s picture

Status: Needs work » Needs review
FileSize
3.86 KB

Re-roll

Status: Needs review » Needs work

The last submitted patch, 1974394-aggregator-category-controller-5.patch, failed testing.

ParisLiakos’s picture

+++ b/core/modules/aggregator/aggregator.moduleundefined
@@ -162,10 +162,7 @@ function aggregator_menu() {
     'title callback' => '_aggregator_category_title',
     'title arguments' => array(2),

@@ -242,13 +239,14 @@ function aggregator_menu() {
-function _aggregator_category_title($category) {
+function _aggregator_category_title($cid) {

lets just use directly drupal_set_title from the controller, and remove this callback

kim.pepper’s picture

Status: Needs work » Needs review
FileSize
2 KB
3.78 KB

Use drupal_set_title in controller as per #7

lslinnet’s picture

Assigned: lslinnet » Unassigned

As kim.pepper seems to have taken it up again i might as well unassign myself from it

dawehner’s picture

Status: Needs review » Needs work
+++ b/core/modules/aggregator/aggregator.moduleundefined
@@ -160,12 +160,7 @@ function aggregator_menu() {
-    'title callback' => '_aggregator_category_title',
-    'title arguments' => array(2),

Are you sure there shouldn't be a title callback to support menu titles?

+++ b/core/modules/aggregator/aggregator.routing.ymlundefined
@@ -67,3 +67,10 @@ aggregator_categories:
+    _controller: '\Drupal\aggregator\Controller\AggregatorController::category'

This should be certainly a _content instead.

+++ b/core/modules/aggregator/lib/Drupal/aggregator/Controller/AggregatorController.phpundefined
@@ -323,4 +323,28 @@ public function sources() {
+   * @return string
+   *   The rendered list of items for the feed.
...
+    return _aggregator_page_list($items, $cid);

This returns a render array

ygerasimov’s picture

Status: Needs work » Needs review
FileSize
3.74 KB
124.99 KB

Title 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.

kim.pepper’s picture

ygerasimov can we have an interdiff please?

ygerasimov’s picture

Here is interdiff.

larowlan’s picture

+++ b/core/modules/aggregator/lib/Drupal/aggregator/Controller/AggregatorController.phpundefined
@@ -323,4 +323,23 @@ public function sources() {
+    module_load_include('inc', 'aggregator', 'aggregator.pages');

This should inject the moduleHandler

+++ b/core/modules/aggregator/lib/Drupal/aggregator/Controller/AggregatorController.phpundefined
@@ -323,4 +323,23 @@ public function sources() {
+    return aggregator_page_category($category);

I thought the goal was to remove aggregator_page_category?

ygerasimov’s picture

@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.

ParisLiakos’s picture

Status: Needs review » Needs work

Lets remove aggregator_page_category in separate issue as main purpose of this one is new route.

we usually do that in conversions. there is no point on just introducing the routes if we keep the page callback functions

dawehner’s picture

As this is the main functionality of this controller and the only place where we use this controller, let's convert that as well.

kim.pepper’s picture

Status: Needs work » Needs review
FileSize
2.29 KB
4.43 KB

As this is the main functionality of this controller and the only place where we use this controller, let's convert that as well.

The aggregator_page_category_form() form builder in aggregator.pages.inc is the only place aggregator_page_category() gets called.

I've reverted the change in the controller to call aggregator_page_category(), inlined the logic in aggregator_page_category_form(), and removed aggregator_page_category() altogether.

Kim

larowlan’s picture

+++ b/core/modules/aggregator/aggregator.moduleundefined
@@ -153,12 +153,7 @@ function aggregator_menu() {
-    'title callback' => '_aggregator_category_title',
-    'title arguments' => array(2),

@@ -233,19 +228,6 @@ function aggregator_menu() {
- * Title callback: Returns a title for aggregator category pages.
- *
- * @param $category
- *   An aggregator category.
- *
- * @return
- *   A string with the aggregator category title.
- */
-function _aggregator_category_title($category) {
-  return $category->title;

+++ b/core/modules/aggregator/lib/Drupal/aggregator/Controller/AggregatorController.phpundefined
@@ -323,4 +323,26 @@ public function sources() {
+    drupal_set_title($category->title);

Title callbacks work again, so we can revert this

dawehner’s picture

I 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?

ParisLiakos’s picture

we 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..

kim.pepper’s picture

I 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 call menu_link_maintain('aggregator', $op, $link_path, $edit['title']);

kim.pepper’s picture

BTW, I don't we need the menu item at all, but that is another issue.

ParisLiakos’s picture

a, ha correct, thanks..
+1 on _aggregator_category_title removal then..

BTW, I don't we need the menu item at all, but that is another issue.

which issue then?

kim.pepper’s picture

No issue yet. I mean "but that is another topic". Bad choice of words. :-)

kim.pepper’s picture

This removes the menu entry entirely. Seems to still work, showing the menu links in the left sidebar.

categories-after.png

Crell’s picture

Status: Needs review » Needs work
+++ b/core/modules/aggregator/aggregator.pages.inc
@@ -58,31 +58,14 @@ function aggregator_page_source_form($form, $form_state, $feed) {
-function aggregator_page_category($category) {
+function aggregator_page_category_form($form, $form_state, $category) {

Why not just convert this to a form class?

+++ b/core/modules/aggregator/lib/Drupal/aggregator/Controller/AggregatorController.php
@@ -323,4 +323,26 @@ public function sources() {
+    $this->moduleHandler->loadInclude('aggregator', 'inc', 'aggregator.pages');

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.

ParisLiakos’s picture

maybe copy paste the buildPageList from #1974408: Convert aggregator_page_source() to a Controller, till its in.or postpone this one

kim.pepper’s picture

If we did convert aggregator_page_category_form() to a class that implements FormInterface, would it be ok to call AggregatorController:: 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.

ParisLiakos’s picture

no 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)

piyuesh23’s picture

Assigned: Unassigned » piyuesh23
kim.pepper’s picture

Assigned: piyuesh23 » kim.pepper

piyuesh23 I'm still working on this at the same time as #1974408: Convert aggregator_page_source() to a Controller

kim.pepper’s picture

Issue summary: View changes

Added blocked by link

kim.pepper’s picture

Issue summary: View changes

related issues

kim.pepper’s picture

Status: Needs work » Postponed

Currently 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.

kim.pepper’s picture

Status: Postponed » Closed (duplicate)
kim.pepper’s picture

Issue summary: View changes

updated blocked by