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

Files: 
CommentFileSizeAuthor
#52 interdiff.txt2.17 KBkim.pepper
#52 2040199-page-category-form-52.patch14.93 KBkim.pepper
PASSED: [[SimpleTest]]: [MySQL] 58,209 pass(es).
[ View ]
#48 2040199-page-category-form-48.patch14.87 KBkim.pepper
PASSED: [[SimpleTest]]: [MySQL] 58,354 pass(es).
[ View ]
#48 interdiff.txt3.09 KBkim.pepper
#47 2040199-page-category-form-47.patch15.02 KBkim.pepper
PASSED: [[SimpleTest]]: [MySQL] 58,067 pass(es).
[ View ]
#47 interdiff.txt1.01 KBkim.pepper
#45 2040199-page-category-form-45.patch15.01 KBkim.pepper
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion.
[ View ]
#45 interdiff.txt1.27 KBkim.pepper
#42 2040199-page-category-form-42.patch15.2 KBkim.pepper
PASSED: [[SimpleTest]]: [MySQL] 58,277 pass(es).
[ View ]
#42 interdiff.txt5.32 KBkim.pepper
#39 2040199-page-category-form-39.patch15.09 KBParisLiakos
PASSED: [[SimpleTest]]: [MySQL] 58,042 pass(es).
[ View ]
#36 2040199-page-category-form-36.patch15.11 KBkim.pepper
PASSED: [[SimpleTest]]: [MySQL] 57,773 pass(es).
[ View ]
#36 interdiff.txt546 byteskim.pepper
#34 2040199-page-category-form-34.patch15.11 KBkim.pepper
PASSED: [[SimpleTest]]: [MySQL] 58,049 pass(es).
[ View ]
#34 interdiff.txt11.91 KBkim.pepper
#24 2040199-page-category-form-24.patch7.89 KBkim.pepper
PASSED: [[SimpleTest]]: [MySQL] 58,002 pass(es).
[ View ]
#24 interdiff.txt3.61 KBkim.pepper
#22 2040199-page-category-form-22.patch8.07 KBkim.pepper
PASSED: [[SimpleTest]]: [MySQL] 57,832 pass(es).
[ View ]
#22 interdiff.txt5.84 KBkim.pepper
#20 2040199-page-category-form-20.patch10.82 KBkim.pepper
PASSED: [[SimpleTest]]: [MySQL] 57,508 pass(es).
[ View ]
#20 interdiff.txt4.93 KBkim.pepper
#18 interdiff.txt5.61 KBkim.pepper
#17 2040199-page-category-form-17.patch10.36 KBkim.pepper
PASSED: [[SimpleTest]]: [MySQL] 57,760 pass(es).
[ View ]
#17 interdiff.txt2.41 KBkim.pepper
#15 2040199-page-category-form-15.patch10.07 KBkim.pepper
FAILED: [[SimpleTest]]: [MySQL] 56,815 pass(es), 59 fail(s), and 158 exception(s).
[ View ]
#13 Convert aggregator_page_category_form to a Controller-2040199-13.patch10.07 KBafeijo
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch Convert aggregator_page_category_form to a Controller-2040199-13.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#10 convert-BLOCK_LABEL_VISIBLE-to-a-constant--2029677-10.patch10.51 KBafeijo
PASSED: [[SimpleTest]]: [MySQL] 57,186 pass(es).
[ View ]
#8 aggregator-page-form-2040199.15.interdiff.txt6.13 KBlarowlan
#8 aggregator-page-form-2040199.15.patch11.9 KBlarowlan
PASSED: [[SimpleTest]]: [MySQL] 57,174 pass(es).
[ View ]
#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
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch issue-2038291-Convert language_admin_add_form to a Controller.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#2 Convert aggregator_page_category_form to a Controller-2040199-1.do-not-test.patch6.59 KBafeijo

Comments

Issue tags:+WSCCI-conversion

Status:Active» Needs work
StatusFileSize
new6.59 KB

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

StatusFileSize
new3.23 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch issue-2038291-Convert language_admin_add_form to a Controller.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

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

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.

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

Issue tags:-D8MI

Does not seem to be related to multilingual at all.

StatusFileSize
new11.9 KB
PASSED: [[SimpleTest]]: [MySQL] 57,174 pass(es).
[ View ]
new6.13 KB

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

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

Status:Needs work» Needs review
StatusFileSize
new10.51 KB
PASSED: [[SimpleTest]]: [MySQL] 57,186 pass(es).
[ View ]

ok larowlan,

Uploading the patch with the changes we've talked

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

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.

Status:Needs work» Needs review
StatusFileSize
new10.07 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch Convert aggregator_page_category_form to a Controller-2040199-13.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

ok friends, new patch

Status:Needs review» Needs work

Status:Needs work» Needs review
StatusFileSize
new10.07 KB
FAILED: [[SimpleTest]]: [MySQL] 56,815 pass(es), 59 fail(s), and 158 exception(s).
[ View ]

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.

Issue summary:View changes

related issues

Status:Needs review» Needs work

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

Status:Needs work» Needs review
StatusFileSize
new2.41 KB
new10.36 KB
PASSED: [[SimpleTest]]: [MySQL] 57,760 pass(es).
[ View ]

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

StatusFileSize
new5.61 KB

Oops. Wrong interdiff. This one is correct.

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

StatusFileSize
new4.93 KB
new10.82 KB
PASSED: [[SimpleTest]]: [MySQL] 57,508 pass(es).
[ View ]

Thanks @larowlan!

This includes fixes as per #19.

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

Status:Needs work» Needs review
StatusFileSize
new5.84 KB
new8.07 KB
PASSED: [[SimpleTest]]: [MySQL] 57,832 pass(es).
[ View ]

Thanks for the review @ParisLiakos.

This fixes all issues raised in #21.

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

Status:Needs work» Needs review
StatusFileSize
new3.61 KB
new7.89 KB
PASSED: [[SimpleTest]]: [MySQL] 58,002 pass(es).
[ View ]

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

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.

Status:Needs work» Needs review

LocaleUpdateCronTest.php failed. Re-testing.

Issue tags:-sprint, -WSCCI-conversion

#24: 2040199-page-category-form-24.patch queued for re-testing.

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

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

Status:Needs work» Needs review

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

Issue tags:-sprint, -WSCCI-conversion

#24: 2040199-page-category-form-24.patch queued for re-testing.

Status:Needs review» Needs work

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

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

#24: 2040199-page-category-form-24.patch queued for re-testing.

Issue summary:View changes

more related issues

StatusFileSize
new11.91 KB
new15.11 KB
PASSED: [[SimpleTest]]: [MySQL] 58,049 pass(es).
[ View ]

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.

Issue summary:View changes

more related issues

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?

Issue summary:View changes

added issues blocked by this

StatusFileSize
new546 bytes
new15.11 KB
PASSED: [[SimpleTest]]: [MySQL] 57,773 pass(es).
[ View ]

Thanks dawehner!

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

Status:Needs review» Reviewed & tested by the community

Thank you!

Title:Convert aggregator_page_category_form to a ControllerConvert 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

StatusFileSize
new15.09 KB
PASSED: [[SimpleTest]]: [MySQL] 58,042 pass(es).
[ View ]

reroll

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

Status:Needs work» Needs review
StatusFileSize
new5.32 KB
new15.2 KB
PASSED: [[SimpleTest]]: [MySQL] 58,277 pass(es).
[ View ]

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.

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

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.

Status:Needs work» Needs review
StatusFileSize
new1.27 KB
new15.01 KB
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion.
[ View ]

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.

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

StatusFileSize
new1.01 KB
new15.02 KB
PASSED: [[SimpleTest]]: [MySQL] 58,067 pass(es).
[ View ]

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

StatusFileSize
new3.09 KB
new14.87 KB
PASSED: [[SimpleTest]]: [MySQL] 58,354 pass(es).
[ View ]

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

Status:Needs review» Reviewed & tested by the community

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

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

How about AggregatorCategorizeItemForm? Dunno.

StatusFileSize
new14.93 KB
PASSED: [[SimpleTest]]: [MySQL] 58,209 pass(es).
[ View ]
new2.17 KB

Renamed to AggregatorCategorizeFormBase

Status:Needs review» Reviewed & tested by the community

Issue summary:View changes

Added follow up

Status:Reviewed & tested by the community» Fixed

Committed and pushed to 8.x. Thanks!

@afeijo++

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

Issue summary:View changes

Updated summary