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
#34 1974372-aggregator-sources-controller-34.patch7.59 KBkim.pepper
PASSED: [[SimpleTest]]: [MySQL] 57,352 pass(es).
[ View ]
#34 1974372-interdiff.txt1.09 KBkim.pepper
#32 1974372-aggregator-sources-controller-30.patch7.42 KBkim.pepper
PASSED: [[SimpleTest]]: [MySQL] 55,812 pass(es).
[ View ]
#32 interdiff.txt779 byteskim.pepper
#28 1974372-aggregator-sources-controller-28.patch7.43 KBkim.pepper
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion.
[ View ]
#28 interdiff.txt514 byteskim.pepper
#25 1974372-aggregator-sources-controller-25.patch7.41 KBkim.pepper
FAILED: [[SimpleTest]]: [MySQL] 55,824 pass(es), 3 fail(s), and 0 exception(s).
[ View ]
#25 interdiff.txt3.24 KBkim.pepper
#23 1974372-aggregator-sources-controller-23.patch6.13 KBkim.pepper
FAILED: [[SimpleTest]]: [MySQL] 56,181 pass(es), 3 fail(s), and 45 exception(s).
[ View ]
#23 interdiff.txt777 byteskim.pepper
#21 1974372-aggregator-sources-controller-21.patch6.81 KBkim.pepper
PASSED: [[SimpleTest]]: [MySQL] 55,731 pass(es).
[ View ]
#19 1974372-aggregator-sources-controller-19.patch6.34 KBParisLiakos
PASSED: [[SimpleTest]]: [MySQL] 56,405 pass(es).
[ View ]
#19 interdiff.txt899 bytesParisLiakos
#15 1974372-aggregator-sources-controller-15.patch6.26 KBParisLiakos
PASSED: [[SimpleTest]]: [MySQL] 55,625 pass(es).
[ View ]
#13 1974372-aggregator-sources-controller-13.patch5.96 KBkim.pepper
PASSED: [[SimpleTest]]: [MySQL] 55,944 pass(es).
[ View ]
#13 interdiff.txt633 byteskim.pepper
#10 1974372-aggregator-sources-controller-10.patch5.98 KBkim.pepper
PASSED: [[SimpleTest]]: [MySQL] 56,005 pass(es).
[ View ]
#10 interdiff.txt653 byteskim.pepper
#8 1974372-aggregator-sources-controller-8.patch5.97 KBkim.pepper
PASSED: [[SimpleTest]]: [MySQL] 55,390 pass(es).
[ View ]
#8 interdiff.txt660 byteskim.pepper
#5 1974372-aggregator-sources-controller-5.patch5.99 KBkim.pepper
PASSED: [[SimpleTest]]: [MySQL] 55,386 pass(es).
[ View ]
#5 interdiff.txt1.56 KBkim.pepper
#3 1974372-aggregator-sources-controller-4.patch6.08 KBkim.pepper
PASSED: [[SimpleTest]]: [MySQL] 55,369 pass(es).
[ View ]
#3 interdiff.txt3 KBkim.pepper
#1 1974372-aggregator-sources-controller-1.patch4.53 KBkim.pepper
PASSED: [[SimpleTest]]: [MySQL] 55,377 pass(es).
[ View ]

Comments

Assigned:Unassigned» kim.pepper
Status:Active» Needs review
StatusFileSize
new4.53 KB
PASSED: [[SimpleTest]]: [MySQL] 55,377 pass(es).
[ View ]

First attempt.

Status:Needs review» Needs work

+++ b/core/modules/aggregator/lib/Drupal/aggregator/Routing/AggregatorController.phpundefined
@@ -56,4 +56,44 @@ public function feedAdd() {
+    $feeds = entity_load_multiple('aggregator_feed');
...
+          $summary_items = entity_view_multiple($items, 'summary');

Use the already injected entityManager here, instead of the procedural helpers

+++ b/core/modules/aggregator/lib/Drupal/aggregator/Routing/AggregatorController.phpundefined
@@ -56,4 +56,44 @@ public function feedAdd() {
+      $aggregator_summary_items = config('aggregator.settings')->get('source.list_max');

same..we need config.entity service injected

Status:Needs work» Needs review
StatusFileSize
new3 KB
new6.08 KB
PASSED: [[SimpleTest]]: [MySQL] 55,369 pass(es).
[ View ]

Injected all the dependencies. Seems a lot more code just to have an injected config factory :-(

Status:Needs review» Needs work

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

should be @todo and a better message, eg:
@todo remove this once all callbacks are converted.

+++ b/core/modules/aggregator/lib/Drupal/aggregator/Routing/AggregatorController.phpundefined
@@ -56,4 +70,50 @@ public function feedAdd() {
+      $aggregator_summary_items = $this->configFactory
+                                       ->get('aggregator.settings')
+                                       ->get('source.list_max');
...
+          $summary_items = $this->entityManager
+                                ->getRenderController('aggregator_item')
+                                ->viewMultiple($items, 'summary');

weird indentation.should be like:

$var = $this->sth
  ->getSth()
  ->doIt();

Status:Needs work» Needs review
StatusFileSize
new1.56 KB
new5.99 KB
PASSED: [[SimpleTest]]: [MySQL] 55,386 pass(es).
[ View ]

Thanks for the review @ParisLiakos Updated with feedback in #4

Status:Needs review» Reviewed & tested by the community

looks good to go now, thanks!

Status:Reviewed & tested by the community» Needs work
Issue tags:+Needs reroll

No longer applies
Since you are rerolling it

+++ b/core/modules/aggregator/aggregator.routing.ymlundefined
@@ -25,3 +25,10 @@ aggregator_feed_add:
     _controller: '\Drupal\aggregator\Routing\AggregatorController::feedAdd'
...
+    _controller: '\Drupal\aggregator\Routing\AggregatorController::sources'

i think the rest of core is using _content here..i just noticed..so maybe we should do so instead of _controller..and i am not sure why there are two ways of doing the same thing

+++ b/core/modules/aggregator/lib/Drupal/aggregator/Routing/AggregatorController.phpundefined
@@ -56,4 +70,50 @@ public function feedAdd() {
+   * @return string
+   *   An HTML-formatted string.

it is an array now should be:
* @return array
* A render array as expected by drupal_render().

Status:Needs work» Needs review
StatusFileSize
new660 bytes
new5.97 KB
PASSED: [[SimpleTest]]: [MySQL] 55,390 pass(es).
[ View ]

Re-roll and fixes issues in #7

+++ b/core/modules/aggregator/lib/Drupal/aggregator/Routing/AggregatorController.phpundefined
@@ -73,8 +73,8 @@ public function feedAdd() {
+   *   A form array as expected by drupal_render().

oops, not a form array:) a render array

StatusFileSize
new653 bytes
new5.98 KB
PASSED: [[SimpleTest]]: [MySQL] 56,005 pass(es).
[ View ]

Damn. I need to actually stop copy paste from reviews. :-)

Fixes comment in #9

+++ b/core/modules/aggregator/lib/Drupal/aggregator/Routing/AggregatorController.phpundefined
@@ -24,20 +25,33 @@ class AggregatorController implements ControllerInterface {
+    $this->configFactory = $config_factory;

I tend to like to not store the full config factory but just the config object needed below: 'aggregator.settings'. Do you have an opinion about that?

Status:Needs review» Reviewed & tested by the community

at least aggregator_page_last() needs the system config, so we should store the configFactory so it can be reused.
i think it looks good. thanks a lot @kim.pepper!

Really minor that shouldn't hold this patch:

+++ b/core/modules/aggregator/lib/Drupal/aggregator/Routing/AggregatorController.phpundefined
@@ -24,20 +25,33 @@ class AggregatorController implements ControllerInterface {
+   * The config factory to get aggregator settings.

The comment could be more general maybe:

The config factory to retrieve settings from.

Or just:

The configuration factory.

StatusFileSize
new633 bytes
new5.96 KB
PASSED: [[SimpleTest]]: [MySQL] 55,944 pass(es).
[ View ]

Here's the comment change in #12.

Thank you!

StatusFileSize
new6.26 KB
PASSED: [[SimpleTest]]: [MySQL] 55,625 pass(es).
[ View ]

reroll for admin overview conversion

Status:Reviewed & tested by the community» Needs work

+++ b/core/modules/aggregator/lib/Drupal/aggregator/Routing/AggregatorController.phpundefined
@@ -153,4 +165,50 @@ public function adminOverview() {
+    // @todo remove this once all callbacks are converted.
+    module_load_include('inc', 'aggregator', 'aggregator.pages');
...
+        if ($items = aggregator_load_feed_items('source', $feed, $aggregator_summary_items)) {

I think we should to the conversion of this function in this patch... so that this conversion is done and the other conversion can use it... then we don't have to redo everyone one but the last one. We just have to remember to remove aggregator_load_feed_items on the last one :)

actually this function does not belong to the controller
it is also being rewritten to use entity_query in #15266: Replace aggregator category system with taxonomy
then it could be moved to the ItemStorageController or a static method depending on #1742850: Follow-up for entity_load_multiple_by_properties() outcome

double-post:/

StatusFileSize
new899 bytes
new6.34 KB
PASSED: [[SimpleTest]]: [MySQL] 56,405 pass(es).
[ View ]

refactored it a bit..

Status:Needs work» Needs review

we could also move this function to the .module file as well temporarily

StatusFileSize
new6.81 KB
PASSED: [[SimpleTest]]: [MySQL] 55,731 pass(es).
[ View ]

Re-roll.

Status:Needs review» Needs work
Issue tags:-Needs reroll+Needs tests

+++ b/core/modules/aggregator/lib/Drupal/aggregator/Routing/AggregatorController.phpundefined
@@ -185,4 +197,51 @@ public function adminOverview() {
+        if ($items = aggregator_load_feed_items('source', $feed, $aggregator_summary_items)) {

Just in general: this code could be replaced by an EQ, but yeah this is out of scope of this issue.

While manually testing the patch, I recognized the following bug.

+++ b/core/modules/aggregator/lib/Drupal/aggregator/Routing/AggregatorController.phpundefined
@@ -185,4 +197,51 @@ public function adminOverview() {
+    Drupal::moduleHandler()->loadInclude('aggregator', 'inc', 'aggregator.pages');

This code breaks in objects, so we clearly have no test coverage :(

Status:Needs work» Needs review
StatusFileSize
new777 bytes
new6.13 KB
FAILED: [[SimpleTest]]: [MySQL] 56,181 pass(es), 3 fail(s), and 45 exception(s).
[ View ]

Thanks for the review @dawehner.

This is a re-roll that fixes the Drupal => \Drupal issue and some missing imported. The patch was manually re-tested successfully locally.

Status:Needs review» Needs work

The last submitted patch, 1974372-aggregator-sources-controller-23.patch, failed testing.

StatusFileSize
new3.24 KB
new7.41 KB
FAILED: [[SimpleTest]]: [MySQL] 55,824 pass(es), 3 fail(s), and 0 exception(s).
[ View ]

Fixed missing class imports, and added tests!

Status:Needs work» Needs review

Status:Needs review» Needs work

The last submitted patch, 1974372-aggregator-sources-controller-25.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new514 bytes
new7.43 KB
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion.
[ View ]

Hmmm. Somehow managed to remove permissions requirement from aggregator_page_last in a merge. Added now.

+++ b/core/modules/aggregator/lib/Drupal/aggregator/Routing/AggregatorController.phpundefined
@@ -227,4 +229,54 @@ public function pageLast() {
+    \Drupal::moduleHandler()->loadInclude('aggregator', 'inc', 'aggregator.pages');

we have the moduleHandler injected so lets use that instead of \Drupal

once more, thanks for the tests!

Status:Needs review» Needs work

The last submitted patch, 1974372-aggregator-sources-controller-28.patch, failed testing.

Using the injected modulehandler insteadas per #29.

ParisLiakos, I assume this is a temporary step, as most of that code will be inlined or moved to a manager, and we won't need moduleHandler anymore.

Status:Needs work» Needs review
StatusFileSize
new779 bytes
new7.42 KB
PASSED: [[SimpleTest]]: [MySQL] 55,812 pass(es).
[ View ]

better attach patch...

+++ b/core/modules/aggregator/lib/Drupal/aggregator/Tests/AggregatorRenderingTest.phpundefined
@@ -96,6 +96,16 @@ public function testFeedPage() {
+    $this->assert(isset($links[0]), format_string('Link to href %href found.', array('%href' => $href)));

format_string should be certainly String::format() but i'm not sure whether ->assert instead of ->assertTrue is the way to go

StatusFileSize
new1.09 KB
new7.59 KB
PASSED: [[SimpleTest]]: [MySQL] 57,352 pass(es).
[ View ]

Thanks @dawehner. Fixes #33

Status:Needs review» Reviewed & tested by the community

Manually testing works fine.I guess the tag can be removed now as well.

Status:Reviewed & tested by the community» Fixed

+++ b/core/modules/aggregator/lib/Drupal/aggregator/Routing/AggregatorController.phpundefined
@@ -227,4 +229,53 @@ public function pageLast() {
+
+    // @todo remove this once aggregator_load_feed_items() is refactored after
+    // http://drupal.org/node/15266 is in.

I had to double-check the nid, but it really is that old! Committed/pushed to 8x., thanks!

+++ b/core/modules/aggregator/aggregator.pages.inc
@@ -319,46 +319,6 @@ function template_preprocess_aggregator_item(&$variables) {
-    '#type' => 'container',
...
-    '#sorted' => TRUE,
+++ b/core/modules/aggregator/lib/Drupal/aggregator/Routing/AggregatorController.php
@@ -227,4 +229,53 @@ public function pageLast() {
+      '#type' => 'container',
...
+      '#sorted' => TRUE,

Can anyone tell me what #sorted TRUE does? I couldn't find it anywhere on api.drupal.org

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

Issue summary:View changes

Added link to meta issue