Follow up for #1971384: [META] Convert page callbacks to controllers

Convert /admin/structure/forum to a controller

Files: 
CommentFileSizeAuthor
#63 drupal8.forum-module.1974210-63.patch13.35 KBjibran
PASSED: [[SimpleTest]]: [MySQL] 58,930 pass(es).
[ View ]
#63 interdiff.txt1.45 KBjibran
#54 forum-overview.53.interdiff.txt957 byteslarowlan
#54 forum-overview.53.patch13.29 KBlarowlan
FAILED: [[SimpleTest]]: [MySQL] 59,409 pass(es), 24 fail(s), and 7 exception(s).
[ View ]
#51 forum-overview.51.interdiff.txt2.95 KBlarowlan
#51 forum-overview.51.patch13.28 KBlarowlan
FAILED: [[SimpleTest]]: [MySQL] 59,036 pass(es), 24 fail(s), and 7 exception(s).
[ View ]
#47 forum-overview.45.interdiff.txt1 KBlarowlan
#47 forum-overview.45.patch13.45 KBlarowlan
PASSED: [[SimpleTest]]: [MySQL] 59,091 pass(es).
[ View ]
#43 forum-overview.43.interdiff.txt735 byteslarowlan
#43 forum-overview.43.patch12.62 KBlarowlan
FAILED: [[SimpleTest]]: [MySQL] 58,668 pass(es), 1 fail(s), and 0 exception(s).
[ View ]
#39 drupal8.forum-module.1974210-39.patch13.16 KBdisasm
FAILED: [[SimpleTest]]: [MySQL] 58,424 pass(es), 1 fail(s), and 0 exception(s).
[ View ]
#39 interdiff.txt672 bytesdisasm
#38 drupal8.forum-module.1974210-38.patch13.21 KBdisasm
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion.
[ View ]
#38 interdiff.txt1005 bytesdisasm
#33 drupal8.forum-module.1974210-33.patch12.95 KBdisasm
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch drupal8.forum-module.1974210-33.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#33 interdiff.txt2.49 KBdisasm
#32 Screen Shot 2013-09-02 at 9.21.10 AM.png80.63 KBdisasm
#29 drupal8.forum-module.1974210-28.patch11.37 KBdisasm
FAILED: [[SimpleTest]]: [MySQL] 58,378 pass(es), 1 fail(s), and 0 exception(s).
[ View ]
#29 interdiff.txt685 bytesdisasm
#26 drupal8.forum-module.1974210-26.patch11.38 KBdisasm
FAILED: [[SimpleTest]]: [MySQL] 58,027 pass(es), 1 fail(s), and 0 exception(s).
[ View ]
#26 interdiff.txt1.67 KBdisasm
#24 drupal8.forum-module.1974210-24.patch11.17 KBdisasm
FAILED: [[SimpleTest]]: [MySQL] 58,376 pass(es), 25 fail(s), and 18 exception(s).
[ View ]
#24 interdiff.txt3.7 KBdisasm
#21 drupal8.forum-module.1974210-21.patch10.05 KBdisasm
FAILED: [[SimpleTest]]: [MySQL] 58,350 pass(es), 5 fail(s), and 2 exception(s).
[ View ]
#21 interdiff.txt600 bytesdisasm
#19 drupal8.forum-module.1974210-19.patch10.01 KBdisasm
FAILED: [[SimpleTest]]: [MySQL] 58,317 pass(es), 12 fail(s), and 15 exception(s).
[ View ]
#19 interdiff.txt8.99 KBdisasm
#15 drupal8.forum-module.1974210-15.patch9.72 KBdisasm
FAILED: [[SimpleTest]]: [MySQL] 58,019 pass(es), 14 fail(s), and 24 exception(s).
[ View ]
#15 interdiff.txt4.32 KBdisasm
#11 drupal8.forum-module.1974210-11.patch9.16 KBdisasm
FAILED: [[SimpleTest]]: [MySQL] 58,274 pass(es), 14 fail(s), and 24 exception(s).
[ View ]
#11 interdiff.txt352 bytesdisasm
#9 drupal8.forum-module.1974210-9.patch9 KBdisasm
FAILED: [[SimpleTest]]: [MySQL] 58,052 pass(es), 14 fail(s), and 156 exception(s).
[ View ]
#9 interdiff.txt4.92 KBdisasm
#1 forum-overview.1.patch7.9 KBlarowlan
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch forum-overview.1.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Comments

larowlan’s picture

Status:Active» Needs review
StatusFileSize
new7.9 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch forum-overview.1.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
ParisLiakos’s picture

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

larowlan’s picture

effulgentsia’s picture

Component:base system» forum.module
larowlan’s picture

Note this also should port the local actions to use forum.local_action.yml

effulgentsia’s picture

Status:Postponed» Needs review

Both issues from #3 have landed.

effulgentsia’s picture

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

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

The last submitted patch, forum-overview.1.patch, failed testing.

disasm’s picture

Status:Needs work» Needs review
StatusFileSize
new4.92 KB
new9 KB
FAILED: [[SimpleTest]]: [MySQL] 58,052 pass(es), 14 fail(s), and 156 exception(s).
[ View ]

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.

Status:Needs review» Needs work

The last submitted patch, drupal8.forum-module.1974210-9.patch, failed testing.

disasm’s picture

StatusFileSize
new352 bytes
new9.16 KB
FAILED: [[SimpleTest]]: [MySQL] 58,274 pass(es), 14 fail(s), and 24 exception(s).
[ View ]

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

disasm’s picture

Status:Needs work» Needs review

Status:Needs review» Needs work

The last submitted patch, drupal8.forum-module.1974210-11.patch, failed testing.

jibran’s picture

  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.

disasm’s picture

Status:Needs work» Needs review
StatusFileSize
new4.32 KB
new9.72 KB
FAILED: [[SimpleTest]]: [MySQL] 58,019 pass(es), 14 fail(s), and 24 exception(s).
[ View ]

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.

jibran’s picture

Status:Needs work» Needs review

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.

Status:Needs review» Needs work

The last submitted patch, drupal8.forum-module.1974210-15.patch, failed testing.

tim.plunkett’s picture

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

disasm’s picture

StatusFileSize
new8.99 KB
new10.01 KB
FAILED: [[SimpleTest]]: [MySQL] 58,317 pass(es), 12 fail(s), and 15 exception(s).
[ View ]

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!

Status:Needs review» Needs work

The last submitted patch, drupal8.forum-module.1974210-19.patch, failed testing.

disasm’s picture

StatusFileSize
new600 bytes
new10.05 KB
FAILED: [[SimpleTest]]: [MySQL] 58,350 pass(es), 5 fail(s), and 2 exception(s).
[ View ]

fixes missing use flag

disasm’s picture

Status:Needs work» Needs review

Status:Needs review» Needs work

The last submitted patch, drupal8.forum-module.1974210-21.patch, failed testing.

disasm’s picture

Status:Needs work» Needs review
StatusFileSize
new3.7 KB
new11.17 KB
FAILED: [[SimpleTest]]: [MySQL] 58,376 pass(es), 25 fail(s), and 18 exception(s).
[ View ]

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.

Status:Needs review» Needs work

The last submitted patch, drupal8.forum-module.1974210-24.patch, failed testing.

disasm’s picture

Status:Needs work» Needs review
StatusFileSize
new1.67 KB
new11.38 KB
FAILED: [[SimpleTest]]: [MySQL] 58,027 pass(es), 1 fail(s), and 0 exception(s).
[ View ]

This should fix remaining failures.

jibran’s picture

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

entity.manager now.

Status:Needs review» Needs work

The last submitted patch, drupal8.forum-module.1974210-26.patch, failed testing.

disasm’s picture

Status:Needs work» Needs review
StatusFileSize
new685 bytes
new11.37 KB
FAILED: [[SimpleTest]]: [MySQL] 58,378 pass(es), 1 fail(s), and 0 exception(s).
[ View ]

fixing #27.

Status:Needs review» Needs work

The last submitted patch, drupal8.forum-module.1974210-28.patch, failed testing.

andypost’s picture

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

disasm’s picture

StatusFileSize
new80.63 KB

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

disasm’s picture

Status:Needs work» Needs review
StatusFileSize
new2.49 KB
new12.95 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch drupal8.forum-module.1974210-33.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

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.

Status:Needs review» Needs work

The last submitted patch, drupal8.forum-module.1974210-33.patch, failed testing.

larowlan’s picture

This needs a reroll now that we have dropped the forum-> container and cmi variable, will link to change notice once done, but basically forum-> forum_container-> value instead.
This is the last non hook procedural function in forum. Lets kill it and declare forum ready to ship!

larowlan’s picture

Change notice is here https://drupal.org/node/2079767

larowlan’s picture

Issue tags:+Needs reroll
disasm’s picture

Status:Needs work» Needs review
StatusFileSize
new1005 bytes
new13.21 KB
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion.
[ View ]
disasm’s picture

StatusFileSize
new672 bytes
new13.16 KB
FAILED: [[SimpleTest]]: [MySQL] 58,424 pass(es), 1 fail(s), and 0 exception(s).
[ View ]

Missed a reference to containers.

Status:Needs review» Needs work

The last submitted patch, drupal8.forum-module.1974210-39.patch, failed testing.

larowlan’s picture

+++ b/core/modules/forum/forum.module
@@ -214,18 +198,6 @@ function forum_menu_local_tasks(&$data, $router_item, $root_path) {
- * Implements hook_menu_local_tasks_alter().
- *
- * Remove the 'Add Forum' and 'Add container' local tasks on the delete form.
- */
-function forum_menu_local_tasks_alter(&$data, $router_item, $root_path) {
-  if ($root_path == 'admin/structure/forum' && !empty($router_item['map'][3]) &&
-      $router_item['map'][3] == 'delete') {
-    $data = array();
-  }
-}
-
-/**

This hunk is the cause of the fail

xjm’s picture

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.

larowlan’s picture

Status:Needs work» Needs review
StatusFileSize
new12.62 KB
FAILED: [[SimpleTest]]: [MySQL] 58,668 pass(es), 1 fail(s), and 0 exception(s).
[ View ]
new735 bytes

Lets' get this RTBC before the 14th.

Status:Needs review» Needs work
Issue tags:-WSCCI, -Needs reroll, -FormInterface, -WSCCI-conversion

The last submitted patch, forum-overview.43.patch, failed testing.

disasm’s picture

Status:Needs work» Needs review

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

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

The last submitted patch, forum-overview.43.patch, failed testing.

larowlan’s picture

Assigned:larowlan» Unassigned
Status:Needs work» Needs review
StatusFileSize
new13.45 KB
PASSED: [[SimpleTest]]: [MySQL] 59,091 pass(es).
[ View ]
new1 KB

The local tasks still rely on menu_get_item, ensuring we have a hook_menu entry for the delete pages means the action shouldn't show.
So removed the old alter.

Status:Needs review» Needs work
Issue tags:-WSCCI, -Needs reroll, -FormInterface, -WSCCI-conversion

The last submitted patch, forum-overview.45.patch, failed testing.

larowlan’s picture

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

#47: forum-overview.45.patch queued for re-testing.

jibran’s picture

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?

larowlan’s picture

StatusFileSize
new13.28 KB
FAILED: [[SimpleTest]]: [MySQL] 59,036 pass(es), 24 fail(s), and 7 exception(s).
[ View ]
new2.95 KB

Fixes #50

jibran’s picture

Issue tags:-Needs reroll+FormBase

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.

Status:Needs review» Needs work

The last submitted patch, forum-overview.51.patch, failed testing.

larowlan’s picture

Status:Needs work» Needs review
StatusFileSize
new13.29 KB
FAILED: [[SimpleTest]]: [MySQL] 59,409 pass(es), 24 fail(s), and 7 exception(s).
[ View ]
new957 bytes

First one changed, I think the second one is ok, value will be set by then.

jibran’s picture

Status:Needs review» Reviewed & tested by the community

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

webchick’s picture

Status:Reviewed & tested by the community» Fixed
Issue tags:+Needs followup
+++ 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!

larowlan’s picture

webchick’s picture

Status:Fixed» Needs review

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

Rolling back and re-testing.

webchick’s picture

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

larowlan’s picture

Yeah saw the same fails in another issue

Status:Needs review» Needs work

The last submitted patch, forum-overview.53.patch, failed testing.

alexpott’s picture

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

jibran’s picture

Status:Needs work» Needs review
StatusFileSize
new1.45 KB
new13.35 KB
PASSED: [[SimpleTest]]: [MySQL] 58,930 pass(es).
[ View ]

Fixed #62.

larowlan’s picture

Status:Needs review» Reviewed & tested by the community

Thanks jibran, assuming bot likes it

larowlan’s picture

(phew) it was c4dd26c022c02532ee63ac2caf37c3f1b9c04628 that went in just before that broke it, knew I wasn't mad, getUrlGenerator() became urlGenerator().

webchick’s picture

Status:Reviewed & tested by the community» Fixed

Trying that again. :)

Committed and pushed to 8.x. Thanks!

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