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

Files: 
CommentFileSizeAuthor
#32 1974370-aggregator-page-last-controller-test-only.patch949 byteskim.pepper
PASSED: [[SimpleTest]]: [MySQL] 55,630 pass(es).
[ View ]
#32 1974370-aggregator-page-last-controller-32.patch6.55 KBkim.pepper
PASSED: [[SimpleTest]]: [MySQL] 56,742 pass(es).
[ View ]
#31 1974370-aggregator-page-last-controller-31.patch5.63 KBkim.pepper
PASSED: [[SimpleTest]]: [MySQL] 55,662 pass(es).
[ View ]
#31 interdiff.txt3.25 KBkim.pepper
#24 1974370-aggregator-page-last-controller-24.patch8.34 KBkim.pepper
PASSED: [[SimpleTest]]: [MySQL] 55,944 pass(es).
[ View ]
#24 interdiff.txt6.37 KBkim.pepper
#22 1974370-aggregator-page-last-controller-22.patch3.48 KBkim.pepper
PASSED: [[SimpleTest]]: [MySQL] 55,914 pass(es).
[ View ]
#22 interdiff.txt1.39 KBkim.pepper
#20 1974370-aggregator-page-last-controller-20.patch3.49 KBkim.pepper
PASSED: [[SimpleTest]]: [MySQL] 56,105 pass(es).
[ View ]
#12 1974370-aggregator-page-last-controller-12.patch5.48 KBkim.pepper
PASSED: [[SimpleTest]]: [MySQL] 55,591 pass(es).
[ View ]
#12 interdiff.txt1.71 KBkim.pepper
#6 interdiff.txt866 byteskim.pepper
#5 1974370-aggregator-page-last-controller-5.patch3.06 KBkim.pepper
PASSED: [[SimpleTest]]: [MySQL] 55,297 pass(es).
[ View ]
#5 interdiff.txt866 byteskim.pepper
#4 1974370-aggregator-page-last-controller-4.patch2.22 KBkim.pepper
PASSED: [[SimpleTest]]: [MySQL] 55,446 pass(es).
[ View ]

Comments

Assigned:Unassigned» kim.pepper

We need the AggregatorController for in #1972262: Convert aggregator_feed_add to a new style controller to do this.

the issue above is in

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

This patch is a straight router conversion. It still calls functions in aggregator.pages.inc which will be moved to the controller eventually.

Issue tags:-WSCCI-conversion
StatusFileSize
new866 bytes
new3.06 KB
PASSED: [[SimpleTest]]: [MySQL] 55,297 pass(es).
[ View ]

Forgot to remove the old function from aggregator.pages.inc

Issue tags:+WSCCI-conversion
StatusFileSize
new866 bytes

Forgot to remove the old function from aggregator.pages.inc

Status:Needs review» Needs work
Issue tags:-WSCCI-conversion

The last submitted patch, 1974370-aggregator-page-last-controller-5.patch, failed testing.

Status:Needs work» Needs review

Status:Needs review» Needs work

+++ b/core/modules/aggregator/lib/Drupal/aggregator/Routing/AggregatorController.phpundefined
@@ -56,4 +56,21 @@ public function feedAdd() {
+    drupal_add_feed('aggregator/rss', config('system.site')->get('name') . ' ' . t('aggregator'));

Lets inject config

+++ b/core/modules/aggregator/lib/Drupal/aggregator/Routing/AggregatorController.phpundefined
@@ -56,4 +56,21 @@ public function feedAdd() {
+    // TODO move this function to controller.
...
+    // TODO move this function to controller.

should be @todo

actually lets wait for #1974372: Convert aggregator_page_sources() to a Controller, it will take care inject the configFactory

Status:Needs work» Needs review
StatusFileSize
new1.71 KB
new5.48 KB
PASSED: [[SimpleTest]]: [MySQL] 55,591 pass(es).
[ View ]

Re-roll against head. Added fixes from #10.

@kim.pepper
Please try to use git diff instead of git format-patch, because you end up with multiple commits in the patch file which is really confusing!

Hi @dawehner. I usually use git diff, but @tim.plunkett requested I follow the instructions on http://drupal.org/documentation/git/configure which uses format patch! So confusing...

Well, http://drupal.org/node/1054616 is also an additional page which is helpful to read. Multiple diffs in one patch is simply not scannable by a human :)

Humans read these?? ;-)

As an aside, what real benefit do we get from calling Drupal::moduleHandler()->loadInclude() instead of module_load_include(). Neither of them can be mocked out for unit tests.

Actually you can mock module handler and then call

<?php
Drupal
::getContainer()->set('module_handler', $mocked_module_handler);
?>

nothing you can do with module_load_include

There ya go!

StatusFileSize
new3.49 KB
PASSED: [[SimpleTest]]: [MySQL] 56,105 pass(es).
[ View ]

Reformatted patch to use just diff instead of format-patch as per #13.

+++ b/core/modules/aggregator/lib/Drupal/aggregator/Routing/AggregatorController.phpundefined
@@ -7,10 +7,11 @@
+use Drupal;
@@ -153,4 +154,23 @@ public function adminOverview() {
+    Drupal::moduleHandler()->loadInclude('aggregator', 'inc', 'aggregator.pages');

We dont use ; global classes inside class context. just prefix with \
\Drupal::moduleHandler()

StatusFileSize
new1.39 KB
new3.48 KB
PASSED: [[SimpleTest]]: [MySQL] 55,914 pass(es).
[ View ]

Fixes from #21 and others provided by @ParisLiakos in IRC.

+++ b/core/modules/aggregator/lib/Drupal/aggregator/Routing/AggregatorController.phpundefined
@@ -153,4 +153,22 @@ public function adminOverview() {
+    drupal_add_feed('aggregator/rss', config('system.site')->get('name') . ' ' . t('aggregator'));

What about injecting config() or at least use \Drupal::config ?

StatusFileSize
new6.37 KB
new8.34 KB
PASSED: [[SimpleTest]]: [MySQL] 55,944 pass(es).
[ View ]

Injected the configFactory and the moduleHandler. A few other code style cleanups as well.

Status:Needs review» Reviewed & tested by the community

perfect, thanks!

Status:Reviewed & tested by the community» Needs work
Issue tags:-WSCCI-conversion

The last submitted patch, 1974370-aggregator-page-last-controller-24.patch, failed testing.

Status:Needs work» Needs review
Issue tags:+WSCCI-conversion

2 tracker fails? seems irrelevant

#24: 1974370-aggregator-page-last-controller-24.patch queued for re-testing.

Status:Needs review» Reviewed & tested by the community

Back to RTBC

Status:Reviewed & tested by the community» Needs work

So the big thing is that there is 0 test coverage for this route in simpletest... so a green test is not enough to commit. Ideally this issue should add tests to confirm that the refactor is successful.

Additionally the following changes are unrelated...

+++ b/core/modules/aggregator/aggregator.moduleundefined
@@ -134,10 +134,8 @@ function aggregator_menu() {
   $items['aggregator'] = array(
+++ b/core/modules/aggregator/lib/Drupal/aggregator/Routing/AggregatorController.phpundefined
@@ -90,8 +114,8 @@ public function feedAdd() {
-    //   callbacks, because access callbacks are also invoked during menu link
-    //   generation. Add token support to routing: http://drupal.org/node/755584.
+    // callbacks, because access callbacks are also invoked during menu link
+    // generation. Add token support to routing: http://drupal.org/node/755584.

This change is not according to coding standards... the indentation is correct. See https://drupal.org/coding-standards/docs#todo

+++ b/core/modules/aggregator/lib/Drupal/aggregator/Routing/AggregatorController.phpundefined
@@ -31,21 +33,41 @@ class AggregatorController implements ControllerInterface {
-   * @var \Drupal\Core\Database\Connection;
+   * @var \Drupal\Core\Database\Connection
@@ -111,7 +135,13 @@ public function feedRefresh(FeedInterface $aggregator_feed, Request $request) {
-    $header = array(t('Title'), t('Items'), t('Last update'), t('Next update'), t('Operations'));
+    $header = array(
+      t('Title'),
+      t('Items'),
+      t('Last update'),
+      t('Next update'),
+      t('Operations'),
+    );
@@ -150,7 +180,7 @@ public function adminOverview() {
-      '#empty' =>  t('No feeds available. <a href="@link">Add feed</a>.', array('@link' => url('admin/config/services/aggregator/add/feed'))),
+      '#empty' => t('No feeds available. <a href="@link">Add feed</a>.', array('@link' => url('admin/config/services/aggregator/add/feed'))),
@@ -179,10 +209,28 @@ public function adminOverview() {
-      '#empty' =>  t('No categories available. <a href="@link">Add category</a>.', array('@link' => url('admin/config/services/aggregator/add/category'))),
+      '#empty' => t('No categories available. <a href="@link">Add category</a>.', array('@link' => url('admin/config/services/aggregator/add/category'))),

These are unrelated changes and unnecessary.

Issue tags:+Needs tests

Adding tags due to...

So the big thing is that there is 0 test coverage for this route in simpletest... so a green test is not enough to commit. Ideally this issue should add tests to confirm that the refactor is successful.

Status:Needs work» Needs review
StatusFileSize
new3.25 KB
new5.63 KB
PASSED: [[SimpleTest]]: [MySQL] 55,662 pass(es).
[ View ]

Reverts the coding standards issues in #29.

StatusFileSize
new6.55 KB
PASSED: [[SimpleTest]]: [MySQL] 56,742 pass(es).
[ View ]
new949 bytes
PASSED: [[SimpleTest]]: [MySQL] 55,630 pass(es).
[ View ]

Added a very basic test for the 'aggregator' path, which mirrors existing tests.

Status:Needs review» Needs work

The last submitted patch, 1974370-aggregator-page-last-controller-32.patch, failed testing.

Status:Needs work» Needs review

Drupal\translation_entity\Tests\EntityTranslationUITest->testTranslationUI() looks like an unrelated failure. Re-testing

Status:Needs review» Reviewed & tested by the community
Issue tags:-Needs tests

Added tests, so back to RTBC

Issue tags:+Needs tests

Even I personally think that this listing pages are actually should be views out of the box, this page works pretty well.

Issue tags:-Needs tests

ups.

Status:Reviewed & tested by the community» Fixed

Committed c9b5abc and pushed to 8.x. Thanks!

Automatically closed -- issue fixed for 2 weeks with no activity.

Issue summary:View changes

Added link to meta issue