Support from Acquia helps fund testing for Drupal Acquia logo

Comments

kim.pepper’s picture

Assigned: Unassigned » kim.pepper
kim.pepper’s picture

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

ParisLiakos’s picture

the issue above is in

kim.pepper’s picture

Status: Active » Needs review
FileSize
2.22 KB

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

kim.pepper’s picture

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

kim.pepper’s picture

Issue tags: +WSCCI-conversion
FileSize
866 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.

kim.pepper’s picture

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

ParisLiakos’s picture

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

ParisLiakos’s picture

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

kim.pepper’s picture

Status: Needs work » Needs review
FileSize
1.71 KB
5.48 KB

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

dawehner’s picture

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

kim.pepper’s picture

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

dawehner’s picture

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

kim.pepper’s picture

Humans read these?? ;-)

kim.pepper’s picture

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.

ParisLiakos’s picture

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

kim.pepper’s picture

There ya go!

kim.pepper’s picture

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

ParisLiakos’s picture

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

kim.pepper’s picture

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

dawehner’s picture

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

kim.pepper’s picture

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

ParisLiakos’s picture

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.

ParisLiakos’s picture

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.

kim.pepper’s picture

Status: Needs review » Reviewed & tested by the community

Back to RTBC

alexpott’s picture

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.

alexpott’s picture

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.

kim.pepper’s picture

Status: Needs work » Needs review
FileSize
3.25 KB
5.63 KB

Reverts the coding standards issues in #29.

kim.pepper’s picture

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.

kim.pepper’s picture

Status: Needs work » Needs review

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

kim.pepper’s picture

kim.pepper’s picture

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

Added tests, so back to RTBC

dawehner’s picture

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.

dawehner’s picture

Issue tags: -Needs tests

ups.

alexpott’s picture

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.

Anonymous’s picture

Issue summary: View changes

Added link to meta issue