Convert admin/structure/forum to a Controller

+++ b/core/modules/forum/lib/Drupal/forum/Form/OverviewForm.phpundefined
@@ -0,0 +1,115 @@
+    module_load_include('inc', 'taxonomy', 'taxonomy.admin');
...
+    $form = taxonomy_overview_terms($form, $form_state, $vocabulary);
...
+    // The form needs to have submit and validate handlers set explicitly.
+    $form['#theme'] = 'taxonomy_overview_terms';
+    $form['#submit'] = array('taxonomy_overview_terms_submit'); // Use the existing taxonomy overview submit handler.

inject moduleHandler and call loadInclude:)

or..convert admin/structure/taxonomy/%taxonomy_vocabulary
first and then extends its class:)

Both issues from #3 have landed.

#1: forum-overview.1.patch queued for re-testing.

Jumpstarting this issue back up. Did a reroll, but noticed that things were so different, that wasn't going to do it, so attaching an interdiff with the changes from after the reroll. Changed the form to extend OverviewTerms. I'm getting the base form using parent::buildForm within the buildForm function.

Removing forum.admin.inc (no longer exists) from hook_theme.

1. +++ b/core/modules/forum/forum.routing.yml
   @@ -1,36 +1,48 @@
      pattern: 'admin/structure/forum/delete/forum/{taxonomy_term}'
   ...
      pattern: 'admin/structure/forum/add/container'
   ...
      pattern: 'admin/structure/forum/add/forum'
   ...
      pattern: 'admin/structure/forum/edit/container/{taxonomy_term}'
   ...
      pattern: 'admin/structure/forum/edit/forum/{taxonomy_term}'
   ...
   +  pattern: 'admin/structure/forum'
   

   All these patterns needs starting /.

2. +++ b/core/modules/forum/lib/Drupal/forum/Form/OverviewForm.php
   @@ -0,0 +1,101 @@
   +  public function __construct(ConfigFactory $config_factory, EntityManager $entity_manager) {
   

   I think we have to call parent::__construct here.

3. +++ b/core/modules/forum/lib/Drupal/forum/Form/OverviewForm.php
   @@ -0,0 +1,101 @@
   +        if (in_array($form[$key]['#term']['tid'], $this->forumConfig->get('containers'))) {
   

   We can move the config array outside the loop.

Fixed #1 and #3.
For #2, would need to pass all the dependencies needed by __construct, so makes more sense to set the dependencies here than call the parent. Added module_handler as a param to the constructor.

Thank you @disasm for fixing that. I think for 2

+++ b/core/modules/forum/lib/Drupal/forum/Form/OverviewForm.php
@@ -30,9 +31,11 @@ class OverviewForm extends OverviewTerms {
+    $this->config = $config_factory->get('taxonomy.settings');
...
+    $this->moduleHandler = $module_handler;

Theses attributes are not defined here so calling parent::__construct($config_factory, $module_handler) will be enough. it seems more OO that __construct is calling its parent but anyways it is just my opinion.

You should call parent:: here, or else you will introduce hard-to-find bugs later on if OverviewTerms is changed.

In debugging these tests, most of the form array in buildForm was fairly different than the one being replaced in the callback. Whacked most of the form and redid the conversion. Also renamed to Overview since it's already in the Form namespace, no need to duplicate the name (The form it's extending does the same thing, OverviewTerms in taxonomy namespace).

constructor is also now calling parent.

Another bug was the way load() method was used to get the entity. It was passing an array, which loadMultiple expects, so switched it to just pass the vid. Enabled the module on my local, and the page is displaying now with no errors. I was able to get to both the new Forum, and new container forms. Lets see what testbot thinks!

fixes missing use flag

This patch adds the protected $entityManager variable to the Overview class. It also changes it and it's parent to store configFactory instead of individual config variables.

This should fix remaining failures.

+++ b/core/modules/forum/lib/Drupal/forum/Form/Overview.php
@@ -0,0 +1,114 @@
+      $container->get('plugin.manager.entity'),

entity.manager now.

fixing #27.

To fix the test you need to check taxonomy.local_actions.yml for proper route

+++ b/core/modules/taxonomy/lib/Drupal/taxonomy/Form/OverviewTerms.php
@@ -23,7 +23,7 @@ class OverviewTerms extends FormBase {
-  protected $config;
+  protected $configFactory;

@@ -41,7 +41,7 @@ class OverviewTerms extends FormBase {
-    $this->config = $config_factory->get('taxonomy.settings');
+    $this->configFactory = $config_factory;

@@ -87,7 +87,7 @@ public function buildForm(array $form, array &$form_state, VocabularyInterface $
-    $page_increment = $this->config->get('terms_per_page_admin');
+    $page_increment = $this->configFactory->get('taxonomy.settings')->get('terms_per_page_admin');

As I remember Crell suggested to use config object if possible to make unit testing easy, because it's easy to pass then factory

@andypost The test that's failing isn't a proper route issue, It's that the Add Forum button is showing up on the delete page. I'm not sure how this is even possible, because when the delete but is clicked, it sets the redirect to the correct path of the delete page, which should be independent of this form controller. Attached is a screenshot of the verbose output where the test is failing.

I converted the local actions to yml, but my local testing is still showing these on the delete form, although I'm not sure how.

Missed a reference to containers.

Thanks for your work on this issue! Please see #1971384-43: [META] Convert page callbacks to controllers for an update on the routing system conversion process.

#43: forum-overview.43.patch queued for re-testing.

This is ready to fly just minor things.

1. +++ b/core/modules/forum/lib/Drupal/forum/Form/Overview.php
   @@ -0,0 +1,113 @@
   +  protected $urlGenerator;
   

   We have $urlGenerator in FormBase we can remove this and use it.

2. +++ b/core/modules/forum/lib/Drupal/forum/Form/Overview.php
   @@ -0,0 +1,113 @@
   +    $form['terms']['#empty'] = $this->t('No containers or forums available. <a href="@container">Add container</a> or <a href="@forum">Add forum</a>.', array('@container' => $this->urlGenerator->generateFromPath('admin/structure/forum/add/container'), '@forum' => $this->urlGenerator->generateFromPath('admin/structure/forum/add/forum')));
   

   Can we add $argument array separately for readability?

Thanks for fixing it.

+++ b/core/modules/forum/lib/Drupal/forum/Form/Overview.php
@@ -106,7 +98,10 @@ public function buildForm(array $form, array &$form_state) {
+      '@container' => $this->urlGenerator->generateFromPath('admin/structure/forum/add/container'),
+      '@forum' => $this->urlGenerator->generateFromPath('admin/structure/forum/add/forum')

This should be $this->getUrlGenerator(). Sorry for not being clear first time.

Consistence-- :D. It is ready to fly.

+++ b/core/modules/forum/lib/Drupal/forum/Form/Overview.php
@@ -0,0 +1,108 @@
+class Overview extends OverviewTerms {

This is missing PHPDoc. Lee gave me the string 'Provides forum overview form for the forum vocabulary' in IRC ... fixed on commit.

+++ b/core/modules/taxonomy/lib/Drupal/taxonomy/Form/OverviewTerms.php
@@ -87,7 +87,7 @@ public function buildForm(array $form, array &$form_state, VocabularyInterface $
-    $page_increment = $this->config->get('terms_per_page_admin');
+    $page_increment = $this->configFactory->get('taxonomy.settings')->get('terms_per_page_admin');

That's unfortunate. :( Let's get a follow-up issue to add a config() method to FormBase, ok?

Committed and pushed to 8.x. Thanks!

Hm, shoot. I think this might've caused testbot failures.

Rolling back and re-testing.

#54: forum-overview.53.patch queued for re-testing.

+      '@container' => $this->getUrlGenerator()->generateFromPath('admin/structure/forum/add/container'),
+      '@forum' => $this->urlGenerator->generateFromPath('admin/structure/forum/add/forum')

Should be using $this->urlGenerator() here on both lines - this is why head broke.

+/**
+ * @file
+ * Contains \Drupal\forum\Form\OverviewForm.
+ */

Should be \Drupal\forum\Form\Overview

+
+class Overview extends OverviewTerms {

Missing docblock

Fixed #62.

Trying that again. :)

Committed and pushed to 8.x. Thanks!