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.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

larowlan’s picture

Title: Convert admin/structure/forum/edit/{forum|container}/% to use ForumFormController and new routing system » Create 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}/%"

larowlan’s picture

larowlan’s picture

Status: Postponed » Active

back on this

tim.plunkett’s picture

larowlan’s picture

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.

tim.plunkett’s picture

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

larowlan’s picture

Status: Active » Needs review
FileSize
23.18 KB
larowlan’s picture

larowlan’s picture

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.

kim.pepper’s picture

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.

larowlan’s picture

Fixes nitpicks and missing use.

dawehner’s picture

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

larowlan’s picture

dawehner’s picture

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

larowlan’s picture

Status: Needs work » Needs review
FileSize
25.45 KB
4.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.

dawehner’s picture

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

larowlan’s picture

dawehner’s picture

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.

larowlan’s picture

Status: Needs work » Needs review
FileSize
25.58 KB
1.21 KB

Stuffed the namespaces in #19

dawehner’s picture

Status: Needs review » Reviewed & tested by the community

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

alexpott’s picture

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
larowlan’s picture

Status: Needs work » Needs review
FileSize
25.6 KB

straight re-roll for removal of rdf stuff.

dawehner’s picture

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

larowlan’s picture

do we typehint for arrays?

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

larowlan’s picture

jibran’s picture

Status: Needs review » Reviewed & tested by the community

It is ready to go.

YesCT’s picture

Issue tags: -Needs reroll +RTBC Feb 18

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

larowlan’s picture

Issue tags: -RTBC Feb 18
larowlan’s picture

Issue tags: +RTBC July 1
tim-e’s picture

xjm’s picture

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

Dries’s picture

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

Committed to 8.x. Thanks!

xjm’s picture

Issue tags: +Needs followup

Dries didn't reload. ;)

kim.pepper’s picture

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

xjm’s picture

Status: Needs work » Fixed

d.o ate my comment.

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

kim.pepper’s picture

vijaycs85’s picture

Issue tags: -Needs followup

Removing follow up tag.

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