Convert aggregator_page_last() to a Controller

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

the issue above is in

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

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

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

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

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

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

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

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

nothing you can do with module_load_include

There ya go!

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

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 ?

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

perfect, thanks!

2 tracker fails? seems irrelevant

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

Back to RTBC

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.

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.

Reverts the coding standards issues in #29.

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

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

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

Added tests, so back to RTBC

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

ups.

Committed c9b5abc and pushed to 8.x. Thanks!