Problem/Motivation

As part of #1971384: [META] Convert page callbacks to controllers we are converting the aggregator form callbacks to FormInterface. The two forms at /aggregator/categories/{cid}/categorize and /aggregator/sources/{aggregator_feed}/categorize share much of the same code, only the list of feed items is filtered differently: one by the source feed it comes from, and one by category.

The existing code uses form callbacks that wrap page callbacks, that then call common protected code that either builds a render array or a form array depending on a switch... It gets confusing.

Proposed resolution

Break out the two forms into a FormInterface implementations with common abstract base class that has all the common code. The left-over procedural code will be handled with the controller conversions in #1974408: Convert aggregator_page_source() to a Controller

Remaining tasks

Follow ups:

User interface changes

None.

API changes

None.

For instructions on how to convert a page callback into a controller, see the WSCCI Conversion Guide.

Blocks the following issues

#1974394: Convert aggregator_page_category() to a Controller
#1974408: Convert aggregator_page_source() to a Controller

#2043581: Move aggregator_load_feed_items to the ItemStorageController, retain procedural wrapper
#2046303: Convert aggregator_form_category to FormInterface
#2003642: Convert aggregator_page_source_form to the new form interface

Follow ups

#2063671: Use the aggregator category storage in AggregatorCategorizeFormBase

CommentFileSizeAuthor
#52 interdiff.txt2.17 KBkim.pepper
#52 2040199-page-category-form-52.patch14.93 KBkim.pepper
#48 2040199-page-category-form-48.patch14.87 KBkim.pepper
#48 interdiff.txt3.09 KBkim.pepper
#47 2040199-page-category-form-47.patch15.02 KBkim.pepper
#47 interdiff.txt1.01 KBkim.pepper
#45 2040199-page-category-form-45.patch15.01 KBkim.pepper
#45 interdiff.txt1.27 KBkim.pepper
#42 2040199-page-category-form-42.patch15.2 KBkim.pepper
#42 interdiff.txt5.32 KBkim.pepper
#39 2040199-page-category-form-39.patch15.09 KBParisLiakos
#36 2040199-page-category-form-36.patch15.11 KBkim.pepper
#36 interdiff.txt546 byteskim.pepper
#34 2040199-page-category-form-34.patch15.11 KBkim.pepper
#34 interdiff.txt11.91 KBkim.pepper
#24 2040199-page-category-form-24.patch7.89 KBkim.pepper
#24 interdiff.txt3.61 KBkim.pepper
#22 2040199-page-category-form-22.patch8.07 KBkim.pepper
#22 interdiff.txt5.84 KBkim.pepper
#20 2040199-page-category-form-20.patch10.82 KBkim.pepper
#20 interdiff.txt4.93 KBkim.pepper
#18 interdiff.txt5.61 KBkim.pepper
#17 2040199-page-category-form-17.patch10.36 KBkim.pepper
#17 interdiff.txt2.41 KBkim.pepper
#15 2040199-page-category-form-15.patch10.07 KBkim.pepper
#13 Convert aggregator_page_category_form to a Controller-2040199-13.patch10.07 KBafeijo
#10 convert-BLOCK_LABEL_VISIBLE-to-a-constant--2029677-10.patch10.51 KBafeijo
#8 aggregator-page-form-2040199.15.interdiff.txt6.13 KBlarowlan
#8 aggregator-page-form-2040199.15.patch11.9 KBlarowlan
#6 Convert aggregator_page_category_form to a Controller-2040199-6.do-not-test.patch12.44 KBafeijo
#3 issue-2038291-Convert language_admin_add_form to a Controller.patch3.23 KBafeijo
#2 Convert aggregator_page_category_form to a Controller-2040199-1.do-not-test.patch6.59 KBafeijo
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

larowlan’s picture

Issue tags: +WSCCI-conversion
afeijo’s picture

here is my initial code

Thanks a lot for timplunkett, Crell and larowlan (so far :] )

AggregatorCategoryForm.php need a lot of work, it is a copy of ActionAdminManagerForm.php

afeijo’s picture

so far code attached
it has this error:
Fatal error: Call to undefined method Drupal\aggregator\Form\AggregatorCategoryForm::pageCategory() in /www/web/drupal/core/modules/aggregator/lib/Drupal/aggregator/Form/AggregatorCategoryForm.php on line 54

ParisLiakos’s picture

Welcome to aggregator's pages.inc!

this is a _form callback and should be converted as such. But, in order to figure out what happens here you need to check _aggregator_page_list the very first conditional...

so, here a pretty hacky thing is done:)
You should copy the logic from aggregator_categorize_items and aggregator_categorize_items_submit after doing the aggregator_load_feed_items call.
Ignore everything else.

ParisLiakos’s picture

afeijo’s picture

Here it is my latest changes

the categories page now loads, but with just 1 button. It is not usable. And I guess the original code before my changes isnt good ether.

http://drupal8.dev/aggregator/categories/1/categorize

if I reset --hard it, that page display 3 notices !?

2 a.m., not a good time to confirm it :) tomorrow I will do a fresh review

thanks once more larowlan

Gábor Hojtsy’s picture

Issue tags: -D8MI

Does not seem to be related to multilingual at all.

larowlan’s picture

So this passes all aggregator tests locally.
But it seems to include removing aggegator_page_category too.
Is it possible that your patch from above contains extra cruft @aFeijo?

Also added #2043581: Move aggregator_load_feed_items to the ItemStorageController, retain procedural wrapper

larowlan’s picture

+++ b/core/modules/aggregator/aggregator.routing.ymlundefined
@@ -67,3 +67,17 @@ aggregator_categories:
+aggregator_page_category:
+  pattern: '/aggregator/categories/{category}'
+  defaults:
+    _content: '\Drupal\aggregator\Controller\AggregatorController::pageCategory'
+  requirements:

+++ b/core/modules/aggregator/lib/Drupal/aggregator/Controller/AggregatorController.phpundefined
@@ -323,4 +322,31 @@ public function sources() {
+  /**
+   * Form constructor to list items aggregated in a category.
+   *
+   * @param $category
+   *   The category for which to list all of the aggregated items.
+   *
+   * @return string
+   *   The rendered list of items for the feed.
+   *
+   * @see aggregator_menu()
+   * @ingroup forms
+   */
+  public function pageCategory($category) {
+    // @todo Refactor this once all controller conversions are complete.
+    $this->moduleHandler->loadInclude('aggregator', 'inc', 'aggregator.pages');
+
+    // @todo improve how the $category is handled, should be an object already
+    $category = aggregator_category_load($category);
+
+    drupal_add_feed('aggregator/rss/' . $category->cid, config('system.site')->get('name') . ' ' . t('aggregator - @title', array('@title' => $category->title)));
+
+    // It is safe to include the cid in the query because it's loaded from the
+    // database by aggregator_category_load().
+    $items = aggregator_load_feed_items('category', $category);
+
+    return _aggregator_page_list($items, arg(3));

Does this belong in this patch?

+++ b/core/modules/aggregator/lib/Drupal/aggregator/Controller/AggregatorController.phpundefined
@@ -265,8 +265,7 @@ public function categories() {
     $items = aggregator_load_feed_items('sum');

can we add a reference to https://drupal.org/node/2043581

+++ b/core/modules/aggregator/lib/Drupal/aggregator/Controller/AggregatorController.phpundefined
@@ -323,4 +322,31 @@ public function sources() {
+    $category = aggregator_category_load($category);

Can we add a @todo for https://drupal.org/node/15266

+++ b/core/modules/aggregator/lib/Drupal/aggregator/Form/AggregatorCategoryForm.phpundefined
@@ -0,0 +1,182 @@
+use Drupal\Core\Entity\EntityManager;
...
+  /**
+   * Stores the Entity manager.
+   *
+   * @var \Drupal\Core\Entity\EntityManager
+   */
+  protected $entityManager;
...
+   * @param \Drupal\Core\Entity\EntityManager $entity_manager
...
+    $this->entityManager = $entity_manager;
...
+      $container->get('plugin.manager.entity'),
...
+    if ($items && ($form_items = $this->entityManager->getRenderController('aggregator_item')->viewMultiple($items, 'default'))) {

We should explicity inject the render controller and not the entity manager.

afeijo’s picture

Status: Needs work » Needs review
FileSize
10.51 KB

ok larowlan,

Uploading the patch with the changes we've talked

larowlan’s picture

+++ b/core/modules/aggregator/aggregator.moduleundefined
@@ -242,6 +236,8 @@ function aggregator_menu() {
+  // @todo improve how the $category is handled, should be an object already

Can we add a reference to https://drupal.org/node/15266 and a trailing .

+++ b/core/modules/aggregator/aggregator.routing.ymlundefined
@@ -67,3 +67,17 @@ aggregator_categories:
+aggregator_page_category:
+  pattern: '/aggregator/categories/{category}'
+  defaults:
+    _content: '\Drupal\aggregator\Controller\AggregatorController::pageCategory'
+  requirements:

this is being handled in https://drupal.org/node/1974394 and needs to be removed from this patch

+++ b/core/modules/aggregator/lib/Drupal/aggregator/Controller/AggregatorController.phpundefined
@@ -265,9 +265,10 @@ public function categories() {
+    // @todo remove this once aggregator_load_feed_items() is refactored after

we can remove 'is refactored' now

+++ b/core/modules/aggregator/lib/Drupal/aggregator/Controller/AggregatorController.phpundefined
@@ -265,9 +265,10 @@ public function categories() {
+    // https://drupal.org/node/2043581 is in.

needs indent

+++ b/core/modules/aggregator/lib/Drupal/aggregator/Form/AggregatorCategoryForm.phpundefined
@@ -0,0 +1,183 @@
+use Drupal\aggregator\Controller\AggregatorController;

No longer needed that I can see

+++ b/core/modules/aggregator/lib/Drupal/aggregator/Form/AggregatorCategoryForm.phpundefined
@@ -0,0 +1,183 @@
+   * Stores the Render controller.
...
+   * Stores the Config Factory.

These comments aren't consistent with the other others (which don't use Stores), should we make them consistent?

+++ b/core/modules/aggregator/lib/Drupal/aggregator/Form/AggregatorCategoryForm.phpundefined
@@ -0,0 +1,183 @@
+      // $container->get('plugin.manager.entity'),

Should be removed

ygerasimov’s picture

Status: Needs review » Needs work
+++ b/core/modules/aggregator/aggregator.moduleundefined
@@ -155,10 +155,7 @@ function aggregator_menu() {
     'title arguments' => array(2),
-    'page callback' => 'aggregator_page_category',
-    'page arguments' => array(2),
-    'access arguments' => array('access news feeds'),
-    'file' => 'aggregator.pages.inc',
+    'route_name' => 'aggregator_page_category',

This should be out of this patch as it is not related to the form.

+++ b/core/modules/aggregator/aggregator.routing.ymlundefined
@@ -67,3 +67,17 @@ aggregator_categories:
+
+aggregator_page_category:
+  pattern: '/aggregator/categories/{category}'
+  defaults:
+    _content: '\Drupal\aggregator\Controller\AggregatorController::pageCategory'
+  requirements:
+    _permission: 'access news feeds'

This is also not related to the form.

The issue that addresses aggregator/categories/{category} is #1974394: Convert aggregator_page_category() to a Controller. Lets keep these two issues separate.

afeijo’s picture

Status: Needs work » Needs review
FileSize
10.07 KB

ok friends, new patch

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

Status: Needs work » Needs review
FileSize
10.07 KB

Re-roll.

I've created a new CategoryStorageController for categories in #2046303: Convert aggregator_form_category to FormInterface. Waiting for this issue to go in first.

kim.pepper’s picture

Issue summary: View changes

related issues

Status: Needs review » Needs work

The last submitted patch, 2040199-page-category-form-15.patch, failed testing.

kim.pepper’s picture

Status: Needs work » Needs review
FileSize
2.41 KB
10.36 KB

This fixes a couple of the failures in #15 and adds some documentation fixes along the way.

kim.pepper’s picture

FileSize
5.61 KB

Oops. Wrong interdiff. This one is correct.

larowlan’s picture

+++ b/core/modules/aggregator/lib/Drupal/aggregator/Controller/AggregatorController.phpundefined
@@ -265,9 +265,9 @@ public function categories() {
+    // @todo remove this once aggregator_load_feed_items() after https://drupal.org/node/2043581 is in.

More than 80 chars and not a well-structured sentence. Also lower case r. Perhaps Remove once aggregator_load_feed_items() is .... See https://drupal.org/node/2043581. replace the ... with whatever its going to be (eg method on storage controller).

+++ b/core/modules/aggregator/lib/Drupal/aggregator/Controller/AggregatorController.phpundefined
@@ -322,5 +322,4 @@ public function sources() {
-

Can we add this back (nitpick)

+++ b/core/modules/aggregator/lib/Drupal/aggregator/Form/AggregatorCategoryForm.phpundefined
@@ -0,0 +1,181 @@
+use Drupal\Core\Config\ConfigFactory;
+use Drupal\Component\Utility\String;
+use Drupal\Core\Controller\ControllerInterface;
+use Drupal\Core\Database\Connection;
+use Drupal\Core\Entity\EntityManager;
+use Drupal\Core\Entity\EntityRenderControllerInterface;
+use Drupal\Core\Extension\ModuleHandlerInterface;

Can we get this in alpha order? (nitpick)

+++ b/core/modules/aggregator/lib/Drupal/aggregator/Form/AggregatorCategoryForm.phpundefined
@@ -0,0 +1,181 @@
+   * @param \Drupal\Core\Entity\EntityManager $entity_manager

Can we change this to a hard dependency on a EntityRenderControllerInterface? Then move the ->getRenderController() to the ::create method? Then the dependency is more transparent.

+++ b/core/modules/aggregator/lib/Drupal/aggregator/Form/AggregatorCategoryForm.phpundefined
@@ -0,0 +1,181 @@
+          '#type' => $this->configFactory->get('aggregator.settings')->get('source.category_selector'),

We're injecting the whole factory for just one config item, I think its preferred to just grab the single Config item (see comments from @alexpott on #1951268-61: Convert /forum and /forum/% to new router, remove forum_forum_load(), forum_get_topics(), create Forum service)

kim.pepper’s picture

Thanks @larowlan!

This includes fixes as per #19.

ParisLiakos’s picture

Status: Needs review » Needs work
+++ b/core/modules/aggregator/aggregator.moduleundefined
@@ -242,6 +239,7 @@ function aggregator_menu() {
+  // @todo improve how the $category is handled (https://drupal.org/node/15266), should be an object already.

this is more than 80 chars..also i think we should just leave it alone, it reduces the possibilities to break other patches, and also not so much in scope here

+++ b/core/modules/aggregator/aggregator.routing.ymlundefined
@@ -67,3 +67,10 @@ aggregator_categories:
diff --git a/core/modules/aggregator/lib/Drupal/aggregator/Controller/AggregatorController.php b/core/modules/aggregator/lib/Drupal/aggregator/Controller/AggregatorController.php

+++ b/core/modules/aggregator/lib/Drupal/aggregator/Controller/AggregatorController.phpundefined
@@ -64,14 +64,14 @@ class AggregatorController implements ControllerInterface {
-   * @param \Drupal\Core\Config\ConfigFactory $config_factory
-   *   The config factory.
...
+   * @param \Drupal\Core\Routing\PathBasedGeneratorInterface $url_generator
+   *   The url generator.

@@ -265,13 +265,13 @@ public function categories() {
-    // @todo Refactor this function once after all controller conversions are
-    // done.
+    // @todo Refactor this function once all controller conversions are done.
...
+    // @todo Replace this with ItemStorageController::loadFeedItems() from
+    //  https://drupal.org/node/2043581 when it is in.
...
-    // @todo Refactor this function once after all controller conversions are
-    // done.
+    // @todo Refactor this function once all controller conversions are done.

@@ -282,7 +282,6 @@ public function pageLast() {
-

@@ -322,5 +321,4 @@ public function sources() {
-

more out of scope changes.
lets not touch the Controller at all here, since its not needed

+++ b/core/modules/aggregator/lib/Drupal/aggregator/Form/AggregatorCategoryForm.phpundefined
@@ -0,0 +1,181 @@
+    $this->config = $config_factory->get('aggregator.settings');
...
+      $container->get('config.factory'),

if we only need this particular config, why inject the whole factory?
Lets inject directly just the aggregator.settings config

+++ b/core/modules/aggregator/lib/Drupal/aggregator/Form/AggregatorCategoryForm.phpundefined
@@ -0,0 +1,181 @@
+    // @todo Remove once categories are taxonomy terms - see
+    //   https://drupal.org/node/15266.

+1 for the @todo

+++ b/core/modules/aggregator/lib/Drupal/aggregator/Form/AggregatorCategoryForm.phpundefined
@@ -0,0 +1,181 @@
+        $this->database->delete('aggregator_category_item')->condition('iid', $iid)->execute();
+        $insert = $this->database->insert('aggregator_category_item')->fields(array('iid', 'cid'));

multiple lines please

kim.pepper’s picture

Status: Needs work » Needs review
FileSize
5.84 KB
8.07 KB

Thanks for the review @ParisLiakos.

This fixes all issues raised in #21.

ParisLiakos’s picture

Status: Needs review » Needs work

seems ready, but #2043581: Move aggregator_load_feed_items to the ItemStorageController, retain procedural wrapper went in, so we need to account for the changes there

kim.pepper’s picture

Status: Needs work » Needs review
FileSize
3.61 KB
7.89 KB

Refactored to use ItemStorageController. This allows us to remove loading aggregator.pages.inc and therefore the module loader too.

afeijo’s picture

Glad to see this issue going!
Thanks for the help, guys
I have 2 hard weeks with sickness upon my family and myself. Damn winter :)

Status: Needs review » Needs work

The last submitted patch, 2040199-page-category-form-24.patch, failed testing.

kim.pepper’s picture

Status: Needs work » Needs review

LocaleUpdateCronTest.php failed. Re-testing.

kim.pepper’s picture

Issue tags: -sprint, -WSCCI-conversion

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

The last submitted patch, 2040199-page-category-form-24.patch, failed testing.

kim.pepper’s picture

Status: Needs work » Needs review

"Failed to write configuration file". re-testing.

kim.pepper’s picture

Issue tags: -sprint, -WSCCI-conversion

Status: Needs review » Needs work

The last submitted patch, 2040199-page-category-form-24.patch, failed testing.

kim.pepper’s picture

Status: Needs work » Needs review
Issue tags: +sprint, +WSCCI-conversion
kim.pepper’s picture

Issue summary: View changes

more related issues

kim.pepper’s picture

This patch combines aggregator_page_source_form() and aggregator_page_category_form() into a new base form class that takes care of the common form building functionality provided by aggregator_categorize_items() and two sub-classes.

As aggregator_categorize_items() is no longer called inside _aggregator_page_list(), this make it much easier to move the remaining common render array building functionality into AggregatorController for #1974394: Convert aggregator_page_category() to a Controller and #1974408: Convert aggregator_page_source() to a Controller.

Marking #2003642: Convert aggregator_page_source_form to the new form interface as a duplicate of this issue since it combines the two.

kim.pepper’s picture

Issue summary: View changes

more related issues

dawehner’s picture

Some manual testing on simplytest.me worked totally perfect, here are some comments + one nitpick.

@@ -166,11 +166,8 @@ function aggregator_menu() {
   );
   $items['aggregator/categories/%aggregator_category/categorize'] = array(
...
     'type' => MENU_LOCAL_TASK,

@@ -195,11 +192,8 @@ function aggregator_menu() {
   $items['aggregator/sources/%aggregator_feed/categorize'] = array(
...
     'type' => MENU_LOCAL_TASK,

Just as a site-node: We can't convert them really easy yet to local task plugins...

@@ -0,0 +1,179 @@
+ * Contains \Drupal\aggregator\Form\AggregatorCategoryForm.
...
+abstract class CategorizeFormBase implements FormInterface, ControllerInterface {

One of these two lines is wrong, guess which of those.

@@ -0,0 +1,179 @@
+   * @param \Drupal\aggregator\ItemInterface[] $items

Fuck yeah!

@@ -0,0 +1,179 @@
+      foreach (element_children($form_items) as $iid) {
+        $categories_result = $this->database->query('SELECT c.cid, c.title, ci.iid FROM {aggregator_category} c LEFT JOIN {aggregator_category_item} ci ON c.cid = ci.cid AND ci.iid = :iid', array(':iid' => $iid));

a db query in a foreach always sounds wrong ... maybe open a followup?

dawehner’s picture

Issue summary: View changes

added issues blocked by this

kim.pepper’s picture

Thanks dawehner!

I've fixed the typo in comments and created a follow up here #2063671: Use the aggregator category storage in AggregatorCategorizeFormBase

dawehner’s picture

Status: Needs review » Reviewed & tested by the community

Thank you!

ParisLiakos’s picture

Title: Convert aggregator_page_category_form to a Controller » Convert aggregator_page_category_form and aggregator_page_source_form to a Controller
Assigned: afeijo » Unassigned

looks awesome, +1 for the followup. re-titling to reflect the scope change in #34

ParisLiakos’s picture

reroll

effulgentsia’s picture

tim.plunkett’s picture

Status: Reviewed & tested by the community » Needs work

#2059245: Add a FormBase class containing useful methods should be going in soon, FYI.

  1. +++ b/core/modules/aggregator/aggregator.routing.yml
    @@ -95,3 +95,18 @@ aggregator_category_add:
    +    entity_type: 'aggregator_feed'
    

    What's this for?

  2. +++ b/core/modules/aggregator/lib/Drupal/aggregator/Form/CategorizeCategoryForm.php
    @@ -0,0 +1,30 @@
    +  public function buildForm(array $form, array &$form_state, $cid = NULL) {
    +    $items = $this->storageController->loadByCategory($cid);
    +    return $this->prepareFormItems($items);
    
    +++ b/core/modules/aggregator/lib/Drupal/aggregator/Form/CategorizeFeedForm.php
    @@ -0,0 +1,32 @@
    +  public function buildForm(array $form, array &$form_state, FeedInterface $aggregator_feed = NULL) {
    +    $items = $this->storageController->loadByFeed($aggregator_feed->id());
    +    return $this->prepareFormItems($items, $aggregator_feed);
    
    +++ b/core/modules/aggregator/lib/Drupal/aggregator/Form/CategorizeFormBase.php
    @@ -0,0 +1,179 @@
    +  protected function prepareFormItems($items, $feed = NULL) {
    

    Why isn't this just buildForm(), and these could call parent::buildForm()?

    Or, even better.
    ImageEffectFormBase has a similar need to this, but the base class specifies abstract protected function prepareImageEffect($image_effect);, and just calls that in buildForm in the base, so each child is required to specify its effect loading.

  3. +++ b/core/modules/aggregator/lib/Drupal/aggregator/Form/CategorizeFormBase.php
    @@ -0,0 +1,179 @@
    +  protected $renderController;
    ...
    +  protected $storageController;
    ...
    +    $this->renderController = $render_controller;
    ...
    +    $this->storageController = $storage_controller;
    

    These should say what they're for, like aggregatorItemStorage

  4. +++ b/core/modules/aggregator/lib/Drupal/aggregator/Form/CategorizeFormBase.php
    @@ -0,0 +1,179 @@
    +   * @param \Drupal\aggregator\ItemInterface[] $items
    +   *   An array of items.
    +   * @param \Drupal\aggregator\FeedInterface|null $feed
    +   *   (optional) The feed we are displaying.
    +   *
    +   * @return array
    +   *   A form builder array.
    +   */
    +  protected function prepareFormItems($items, $feed = NULL) {
    

    This should be typehinted

kim.pepper’s picture

Status: Needs work » Needs review
FileSize
5.32 KB
15.2 KB

Thanks Tim.

This fixes 1) 3) and 4) from #41

For 2) We have 2 different form routes that take different parameters, so need buildForm() in our subclasses. That means we can't have one buildForm in the abstract base class.

If accept that, then I don't think it makes sense to have a buildForm() in the base class that doesn't actually get called by a route like all other buildForm() methods do.

kim.pepper’s picture

Also, not sure what FormBase will give us. We don't need anything that is provided by FormBase.

jibran’s picture

Status: Needs review » Needs work
  1. +++ b/core/modules/aggregator/aggregator.module
    @@ -163,11 +163,8 @@ function aggregator_menu() {
         'title' => 'Categorize',
    
    @@ -189,11 +186,8 @@ function aggregator_menu() {
         'title' => 'Categorize',
    

    Let's move this to routes.yml

  2. +++ b/core/modules/aggregator/aggregator.module
    @@ -163,11 +163,8 @@ function aggregator_menu() {
         'type' => MENU_LOCAL_TASK,
    
    @@ -189,11 +186,8 @@ function aggregator_menu() {
         'type' => MENU_LOCAL_TASK,
    

    Should we fix this using Local tasks are provided by annotated plugins implementing LocalTaskInterface instead of type MENU_LOCAL_TASK in hook_menu()? Or should we wait for #2050919: Replace local task plugin discovery with YamlDiscovery

  3. +++ b/core/modules/aggregator/aggregator.pages.inc
    @@ -144,111 +110,17 @@ function aggregator_load_feed_items($type, $data = NULL, $limit = 20) {
     function _aggregator_page_list($items, $op, $feed_source = '') {
    ...
    +  // Assemble output.
    +  $build = array(
    +    '#type' => 'container',
    +    '#attributes' => array('class' => array('aggregator-wrapper')),
       );
    ...
    +  $build['feed_source'] = is_array($feed_source) ? $feed_source : array('#markup' => $feed_source);
    +  if ($items) {
    +    $build['items'] = entity_view_multiple($items, 'default');
    +    $build['pager'] = array('#theme' => 'pager');
       }
    ...
    +  return $build;
     }
    

    Any reason why this is not a list controller?

  4. +++ b/core/modules/aggregator/lib/Drupal/aggregator/Form/CategorizeFormBase.php
    @@ -0,0 +1,180 @@
    +use Drupal\Core\Config\Config;
    +use Drupal\Core\Controller\ControllerInterface;
    +use Drupal\Core\Database\Connection;
    +use Drupal\Core\Form\FormInterface;
    +use Symfony\Component\DependencyInjection\ContainerInterface;
    ...
    +abstract class CategorizeFormBase implements FormInterface, ControllerInterface {
    ...
    +
    +  /**
    +   * The aggregator config.
    +   *
    +   * @var \Drupal\Core\Config\Config
    +   */
    +  protected $config;
    +
    +  /**
    +   * The database connection.
    +   *
    +   * @var \Drupal\Core\Database\Connection;
    +   */
    +  protected $database;
    +
    ...
    +   * @param \Drupal\Core\Database\Connection $database
    +   *   The database connection.
    +   * @param \Drupal\Core\Config\Config $config
    +   *   The aggregator config.
    ...
    +    $this->database = $database;
    +    $this->config = $config;
    ...
    +      $container->get('database'),
    +      $container->get('config.factory')->get('aggregator.settings'),
    ...
    +  /**
    +   * {@inheritdoc}
    +   */
    +  public function validateForm(array &$form, array &$form_state) {
    +  }
    

    Because of FormBase we can remove all the boilerplate code.

kim.pepper’s picture

Status: Needs work » Needs review
FileSize
1.27 KB
15.01 KB

1) We need to have a title on our tab links. Moving to the route definition doesn't fix that.

2) I hope we don't need hold this up on the local tasks issue?

3) We're removing the form building part of this function. This is also used by a couple of other page callbacks and will be removed in #1974408: Convert aggregator_page_source() to a Controller (currently blocked by this issue.

4) valdiateForm() is the only boiler plate code that can be removed by using FormBase, so I've refactored this to use it.

tim.plunkett’s picture

FormBase also means t() can become $this->t()

kim.pepper’s picture

Coverts t() -> $this->t()

kim.pepper’s picture

This addresses timplunkett's concerns in #41. We now use buildForm() in the base form controller.

tim.plunkett’s picture

Status: Needs review » Reviewed & tested by the community

This looks great, thanks for bearing with me @kim.pepper!

webchick’s picture

Status: Reviewed & tested by the community » Needs review
+++ b/core/modules/aggregator/lib/Drupal/aggregator/Form/CategorizeFormBase.php
@@ -0,0 +1,171 @@
+abstract class CategorizeFormBase extends FormBase {

Hm. A bit concerned about this. The generic "SomethingSomethigBase" name makes it sound like it's a class intended to be extended by other modules, but it's extremely Aggregator module specific. Can we throw "Aggregator" in the class name? (Yes, I realize it's namespaced to aggregator module, but I'm not sure people will get that when it comes up as a class they see in their IDE.)

kim.pepper’s picture

How about AggregatorCategorizeItemForm? Dunno.

kim.pepper’s picture

Renamed to AggregatorCategorizeFormBase

ParisLiakos’s picture

Status: Needs review » Reviewed & tested by the community
ParisLiakos’s picture

Issue summary: View changes

Added follow up

webchick’s picture

Status: Reviewed & tested by the community » Fixed

Committed and pushed to 8.x. Thanks!

HyperGlide’s picture

@afeijo++

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

Anonymous’s picture

Issue summary: View changes

Updated summary