Files: 
CommentFileSizeAuthor
#27 1974394-aggregator-category-controller-27.patch4.45 KBkim.pepper
PASSED: [[SimpleTest]]: [MySQL] 57,225 pass(es).
[ View ]
#27 interdiff.txt598 byteskim.pepper
#27 categories-after.png38.09 KBkim.pepper
#19 1974394-aggregator-category-controller-19.patch4.43 KBkim.pepper
PASSED: [[SimpleTest]]: [MySQL] 57,121 pass(es).
[ View ]
#19 interdiff.txt2.29 KBkim.pepper
#16 1974394-aggregator-category-controller-15.patch3.7 KBygerasimov
PASSED: [[SimpleTest]]: [MySQL] 57,175 pass(es).
[ View ]
#16 1974394-aggregator-category-controller-15-interdiff.txt816 bytesygerasimov
#13 1974394-aggregator-category-controller-11-interdiff.txt2.37 KBygerasimov
#11 First aggregator category Site Install.png124.99 KBygerasimov
#11 1974394-aggregator-category-controller-11.patch3.74 KBygerasimov
PASSED: [[SimpleTest]]: [MySQL] 57,102 pass(es).
[ View ]
#8 1974394-aggregator-category-controller-8.patch3.78 KBkim.pepper
PASSED: [[SimpleTest]]: [MySQL] 56,576 pass(es).
[ View ]
#8 interdiff.txt2 KBkim.pepper
#5 1974394-aggregator-category-controller-5.patch3.86 KBkim.pepper
FAILED: [[SimpleTest]]: [MySQL] 56,812 pass(es), 2 fail(s), and 0 exception(s).
[ View ]
#1 1974394-aggregator-category-controller-1.patch6 KBkim.pepper
PASSED: [[SimpleTest]]: [MySQL] 55,403 pass(es).
[ View ]

Comments

Status:Active» Needs review
StatusFileSize
new6 KB
PASSED: [[SimpleTest]]: [MySQL] 55,403 pass(es).
[ View ]

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

Status:Needs review» Postponed

Issue summary:View changes

Linking to meta issue

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.

Status:Needs work» Needs review
StatusFileSize
new3.86 KB
FAILED: [[SimpleTest]]: [MySQL] 56,812 pass(es), 2 fail(s), and 0 exception(s).
[ View ]

Re-roll

Status:Needs review» Needs work

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

+++ 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

Status:Needs work» Needs review
StatusFileSize
new2 KB
new3.78 KB
PASSED: [[SimpleTest]]: [MySQL] 56,576 pass(es).
[ View ]

Use drupal_set_title in controller as per #7

Assigned:lslinnet» Unassigned

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

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

Status:Needs work» Needs review
StatusFileSize
new3.74 KB
PASSED: [[SimpleTest]]: [MySQL] 57,102 pass(es).
[ View ]
new124.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.

ygerasimov can we have an interdiff please?

Here is interdiff.

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

StatusFileSize
new816 bytes
new3.7 KB
PASSED: [[SimpleTest]]: [MySQL] 57,175 pass(es).
[ View ]

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

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

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

Status:Needs work» Needs review
StatusFileSize
new2.29 KB
new4.43 KB
PASSED: [[SimpleTest]]: [MySQL] 57,121 pass(es).
[ View ]

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

+++ 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

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?

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

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']);

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

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?

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

StatusFileSize
new38.09 KB
new598 bytes
new4.45 KB
PASSED: [[SimpleTest]]: [MySQL] 57,225 pass(es).
[ View ]

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

categories-after.png

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.

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

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.

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)

Assigned:Unassigned» piyuesh23

Assigned:piyuesh23» kim.pepper

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

Issue summary:View changes

Added blocked by link

Issue summary:View changes

related issues

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.

Status:Postponed» Closed (duplicate)

Issue summary:View changes

updated blocked by