Follow up for #1951268: Convert /forum and /forum/% to new router, remove forum_forum_load(), forum_get_topics(), create Forum service

Problem/Motivation

#1951268: Convert /forum and /forum/% to new router, remove forum_forum_load(), forum_get_topics(), create Forum service will add a primitive form controller for forum module. This issue is to flesh it out and remove forum_form_forum() and forum_form_container() to instead use a first-class entity form controller, moving the menu callback to the new routing system in the process.

Proposed resolution

Flesh out ForumFormController to replace forum_form_forum() and forum_form_container()

Remaining tasks

Write patch

User interface changes

None

API changes

forum_form_forum(), forum_form_submit() and forum_form_container() removed.

Files: 
CommentFileSizeAuthor
#33 1951278-forum-patch.patch25.72 KBtim-e
PASSED: [[SimpleTest]]: [MySQL] 57,039 pass(es).
[ View ]
#33 1951278-forum-patch.interdiff.txt3.04 KBtim-e
#28 forum-form-controller.1951278.28.interdiff.txt427 byteslarowlan
#28 forum-form-controller.1951278.28.patch25.61 KBlarowlan
PASSED: [[SimpleTest]]: [MySQL] 57,963 pass(es).
[ View ]
#25 forum-form-controller.1951278.25.patch25.6 KBlarowlan
PASSED: [[SimpleTest]]: [MySQL] 57,934 pass(es).
[ View ]
#22 forum-form-controller.1951278.22.interdiff.txt1.21 KBlarowlan
#22 forum-form-controller.1951278.22.patch25.58 KBlarowlan
PASSED: [[SimpleTest]]: [MySQL] 58,012 pass(es).
[ View ]
#19 forum-form-controller.1951278.19.interdiff.txt3.76 KBlarowlan
#19 forum-form-controller.1951278.19.patch25.58 KBlarowlan
FAILED: [[SimpleTest]]: [MySQL] 57,528 pass(es), 81 fail(s), and 20 exception(s).
[ View ]
#17 forum-form-controller.1951278.17.interdiff.txt4.36 KBlarowlan
#17 forum-form-controller.1951278.17.patch25.45 KBlarowlan
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion.
[ View ]
#14 forum-form-controller.1951278.14.interdiff.txt5.72 KBlarowlan
#14 forum-form-controller.1951278.14.patch25.17 KBlarowlan
FAILED: [[SimpleTest]]: [MySQL] 56,481 pass(es), 61 fail(s), and 24 exception(s).
[ View ]
#12 forum-form-controller.1951278.12.interdiff.txt2.05 KBlarowlan
#12 forum-form-controller.1951278.12.patch24 KBlarowlan
PASSED: [[SimpleTest]]: [MySQL] 57,345 pass(es).
[ View ]
#10 forum-form-controller.1951278.10.interdiff.txt4.61 KBlarowlan
#10 forum-form-controller.1951278.10.patch23.96 KBlarowlan
FAILED: [[SimpleTest]]: [MySQL] 57,704 pass(es), 2 fail(s), and 16 exception(s).
[ View ]
#8 forum-form-controller.1951278.1.patch23.18 KBlarowlan
PASSED: [[SimpleTest]]: [MySQL] 55,638 pass(es).
[ View ]

Comments

Title:Convert admin/structure/forum/edit/{forum|container}/% to use ForumFormController and new routing systemCreate taxonomy term form controller for forum terms (ie not default) and use that for admin/structure/forum/edit/{forum|contain

New title "Create taxonomy term form controller for forum terms (ie not default) and use that for admin/structure/forum/edit/{forum|container}/%"

Status:Postponed» Active

back on this

Ok patch is 75% done, pretty sure this blocks moving term description to field too, so will make this my to priority. Let me know if you want me to push my wip to sandbox.

Oh there are like 20 other issues blocking that. Seriously it will be a month til they're all done.

Status:Active» Needs review
StatusFileSize
new23.18 KB
PASSED: [[SimpleTest]]: [MySQL] 55,638 pass(es).
[ View ]

StatusFileSize
new23.96 KB
FAILED: [[SimpleTest]]: [MySQL] 57,704 pass(es), 2 fail(s), and 16 exception(s).
[ View ]
new4.61 KB

Injects config factory now #1909418: Allow Entity Controllers to have their dependencies injected is in.
Removes refs to _values in @todo's as that looks unlikely to get in.

Looks good!

Just a few minor nickpicks.

+++ b/core/modules/forum/forum.routing.ymlundefined
@@ -10,3 +10,27 @@ forum_settings:
+    _controller: 'Drupal\forum\ForumController::addContainer'

Should be _content?

+++ b/core/modules/forum/forum.routing.ymlundefined
@@ -10,3 +10,27 @@ forum_settings:
+    _controller: 'Drupal\forum\ForumController::addForum'

_content?

+++ b/core/modules/forum/lib/Drupal/forum/ContainerFormController.phpundefined
@@ -0,0 +1,56 @@
+    $insert = $this->entity->isNew();

Can this be called isNew? I think that would make it clearer.

StatusFileSize
new24 KB
PASSED: [[SimpleTest]]: [MySQL] 57,345 pass(es).
[ View ]
new2.05 KB

Fixes nitpicks and missing use.

+++ b/core/modules/forum/forum.moduleundefined
@@ -120,21 +120,11 @@ function forum_menu() {
-    'parent' => 'admin/structure/forum',
...
-    'parent' => 'admin/structure/forum',

Just wondering whether we still need 'parent' here.

+++ b/core/modules/forum/lib/Drupal/forum/ForumFormController.phpundefined
@@ -0,0 +1,199 @@
+    $status = $term->save();

Not sure whether we have discussed before, but as the storage controller is not injected into the term object it might be better to call save on the storage controller instead.

+++ b/core/modules/forum/lib/Drupal/forum/ForumFormController.phpundefined
@@ -0,0 +1,199 @@
+        cache_invalidate_tags(array('content' => TRUE));

This can also just use Cache::invalidateCache

+++ b/core/modules/forum/lib/Drupal/forum/ForumFormController.phpundefined
@@ -0,0 +1,199 @@
+    if (isset($_GET['destination'])) {
...
+      unset($_GET['destination']);

Is it possible to use Request $request instead?

+++ b/core/modules/forum/lib/Drupal/forum/ForumFormController.phpundefined
@@ -0,0 +1,199 @@
+    $parents = taxonomy_term_load_parents($tid);

There should be a follow up to replace this by a injected service using EQ

StatusFileSize
new25.17 KB
FAILED: [[SimpleTest]]: [MySQL] 56,481 pass(es), 61 fail(s), and 24 exception(s).
[ View ]
new5.72 KB

Re-roll with feedback from #13

+++ b/core/modules/forum/forum.moduleundefined
@@ -120,21 +120,13 @@ function forum_menu() {
+    'tab_parent' => 'admin/structure/forum',
...
+    'tab_parent' => 'admin/structure/forum',

Do you know whether this is actually required?

+++ b/core/modules/forum/forum.moduleundefined
@@ -226,6 +212,15 @@ function forum_menu_local_tasks_alter(&$data, $router_item, $root_path) {
+function forum_entity_info_alter(&$info) {
+  // Register forum specific form controllers.
+  $info['taxonomy_term']['controllers']['form']['forum'] = 'Drupal\forum\ForumFormController';
+  $info['taxonomy_term']['controllers']['form']['container'] = 'Drupal\forum\ContainerFormController';

I guess we want to use hook_entity_info instead, as we add new entries, not replace some.

+++ b/core/modules/forum/lib/Drupal/forum/ForumController.phpundefined
@@ -0,0 +1,86 @@
+  public static function create(ContainerInterface $container) {
+    return new static($container->get('plugin.manager.entity'), $container->get('config.factory'));

Let's break the new static in multiple lines to make it easier to add new entries later, if needed (we did that in many other patches already).

+++ b/core/modules/forum/lib/Drupal/forum/ForumController.phpundefined
@@ -0,0 +1,86 @@
+    // @todo use $this->entityManager->getform() when
+    //   http://drupal.org/node/1982980 is in.
+    return entity_get_form($taxonomy_term, 'forum');
...
+    // @todo use $this->entityManager->getform() when
+    //   http://drupal.org/node/1982980 is in.
+    return entity_get_form($taxonomy_term, 'container');

entityManger->getForm() exists now.

+++ b/core/modules/forum/lib/Drupal/forum/ForumFormController.phpundefined
@@ -0,0 +1,227 @@
+  }

Let's have an empty line after "}"

Status:Needs review» Needs work

The last submitted patch, forum-form-controller.1951278.14.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new25.45 KB
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion.
[ View ]
new4.36 KB

Do you know whether this is actually required?

It doesn't seem to change functionality, but it does provide extra context for alter implementations, so I say lets leave it, its consistent with other modules.

+++ b/core/modules/forum/forum.moduleundefined
index 670cca9..00ebbf0 100644
--- a/core/modules/forum/lib/Drupal/forum/ForumController.php

The controllers should be placed in the "Controller" folder.

+++ b/core/modules/forum/lib/Drupal/forum/ForumController.phpundefined
index 9480f8a..253efc1 100644
--- a/core/modules/forum/lib/Drupal/forum/ForumFormController.php

The form controller should be placed in the form folder.

StatusFileSize
new25.58 KB
FAILED: [[SimpleTest]]: [MySQL] 57,528 pass(es), 81 fail(s), and 20 exception(s).
[ View ]
new3.76 KB

Moves namespaces as per #18

Status:Needs review» Reviewed & tested by the community

Thank you very much!

Status:Reviewed & tested by the community» Needs work

The last submitted patch, forum-form-controller.1951278.19.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new25.58 KB
PASSED: [[SimpleTest]]: [MySQL] 58,012 pass(es).
[ View ]
new1.21 KB

Stuffed the namespaces in #19

Status:Needs review» Reviewed & tested by the community

I hope this is green this time. (latest thoughts)

Status:Reviewed & tested by the community» Needs work
Issue tags:+Needs reroll

Needs a reroll

curl https://drupal.org/files/forum-form-controller.1951278.22.patch | git a
  % Total    % Received % Xferd  Average Speed   Time    Time     Time  Current
                                 Dload  Upload   Total   Spent    Left  Speed
100 26194  100 26194    0     0   6328      0  0:00:04  0:00:04 --:--:--  8543
error: patch failed: core/modules/forum/forum.module:1338
error: core/modules/forum/forum.module: patch does not apply

Status:Needs work» Needs review
StatusFileSize
new25.6 KB
PASSED: [[SimpleTest]]: [MySQL] 57,934 pass(es).
[ View ]

straight re-roll for removal of rdf stuff.

+++ b/core/modules/forum/forum.moduleundefined
@@ -1298,3 +1293,19 @@ function _forum_update_forum_index($nid) {
+ * @param $variables
...
+function theme_forum_form($variables) {

do we typehint for arrays?

do we typehint for arrays?

Straight copy from existing function, just moved files - out of scope?

StatusFileSize
new25.61 KB
PASSED: [[SimpleTest]]: [MySQL] 57,963 pass(es).
[ View ]
new427 bytes

Fixes #26

Status:Needs review» Reviewed & tested by the community

It is ready to go.

This issue was RTBC and passing tests on July 1, the beginning of API freeze.

Issue tags:-RTBC Feb 18

Issue tags:+RTBC July 1

StatusFileSize
new3.04 KB
new25.72 KB
PASSED: [[SimpleTest]]: [MySQL] 57,039 pass(es).
[ View ]

Reroll

Issue tags:+Needs followup

+++ b/core/modules/forum/lib/Drupal/forum/Form/ForumFormController.phpundefined
@@ -0,0 +1,229 @@
+    // @todo Inject a taxonomy service when one exists.
+    $children = taxonomy_get_tree($vid, $tid, NULL, TRUE);

So this looks like a reference to #1976298: Move taxonomy_get_tree() and associated functions to Taxonomy storage, retain procedural wrappers., correct? Let's make sure we have converting these lines as part of the scope of that issue. (Ideally I'd prefer to see the issue linked in the @todo, but it's not a big deal.)

Status:Reviewed & tested by the community» Fixed
Issue tags:-Needs followup

Committed to 8.x. Thanks!

Issue tags:+Needs followup

Dries didn't reload. ;)

Status:Fixed» Needs work

+++ b/core/modules/forum/lib/Drupal/forum/Form/ForumFormController.phpundefined
@@ -0,0 +1,229 @@
+   * Constructs a new MenuLinkFormController object.

Copy/paste error here :-)

Status:Needs work» Fixed

d.o ate my comment.

@kim.pepper, can you file a followup novice issue? Thanks!

Issue tags:-Needs followup

Removing follow up tag.

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