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

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

kim.pepper’s picture

Assigned: Unassigned » kim.pepper
Status: Active » Needs review
FileSize
4.53 KB

First attempt.

ParisLiakos’s picture

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

kim.pepper’s picture

Status: Needs work » Needs review
FileSize
3 KB
6.08 KB

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

ParisLiakos’s picture

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();
kim.pepper’s picture

Status: Needs work » Needs review
FileSize
1.56 KB
5.99 KB

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

ParisLiakos’s picture

Status: Needs review » Reviewed & tested by the community

looks good to go now, thanks!

ParisLiakos’s picture

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

kim.pepper’s picture

Status: Needs work » Needs review
FileSize
660 bytes
5.97 KB

Re-roll and fixes issues in #7

ParisLiakos’s picture

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

kim.pepper’s picture

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

Fixes comment in #9

dawehner’s picture

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

ParisLiakos’s picture

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.

kim.pepper’s picture

Here's the comment change in #12.

ParisLiakos’s picture

Thank you!

ParisLiakos’s picture

reroll for admin overview conversion

alexpott’s picture

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

ParisLiakos’s picture

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

ParisLiakos’s picture

double-post:/

ParisLiakos’s picture

refactored it a bit..

ParisLiakos’s picture

Status: Needs work » Needs review

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

kim.pepper’s picture

dawehner’s picture

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

kim.pepper’s picture

Status: Needs work » Needs review
FileSize
777 bytes
6.13 KB

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.

kim.pepper’s picture

Fixed missing class imports, and added tests!

kim.pepper’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

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

kim.pepper’s picture

Status: Needs work » Needs review
FileSize
514 bytes
7.43 KB

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

ParisLiakos’s picture

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

kim.pepper’s picture

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.

kim.pepper’s picture

Status: Needs work » Needs review
FileSize
779 bytes
7.42 KB

better attach patch...

dawehner’s picture

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

kim.pepper’s picture

Thanks @dawehner. Fixes #33

dawehner’s picture

Status: Needs review » Reviewed & tested by the community

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

catch’s picture

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!

tstoeckler’s picture

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

Anonymous’s picture

Issue summary: View changes

Added link to meta issue