Split ForumForm into ForumAddForm and ForumEditForm

So there is this function

  /**
   * Returns a select box for available parent terms.
   *
   * @param int $tid
   *   ID of the term that is being added or edited.
   * @param string $title
   *   Title for the select box.
   *
   * @return array
   *   A select form element.
   */
  protected function forumParentSelect($tid, $title) {
    // @todo Inject a taxonomy service when one exists.
    $parents = taxonomy_term_load_parents($tid);
    if ($parents) {
      $parent = array_shift($parents);
      $parent = $parent->id();
    }
    else {
      $parent = 0;
    }

    $vid = $this->config('forum.settings')->get('vocabulary');
    // @todo Inject a taxonomy service when one exists.
    $children = taxonomy_get_tree($vid, $tid, NULL, TRUE);

    // A term can't be the child of itself, nor of its children.
    foreach ($children as $child) {
      $exclude[] = $child->tid;
    }
    $exclude[] = $tid;

    // @todo Inject a taxonomy service when one exists.
    $tree = taxonomy_get_tree($vid, 0, NULL, TRUE);
    $options[0] = '<' . $this->t('root') . '>';
    if ($tree) {
      foreach ($tree as $term) {
        if (!in_array($term->id(), $exclude)) {
          $options[$term->id()] = str_repeat(' -- ', $term->depth) . $term->getName();
        }
      }
    }

    $description = $this->t('Forums may be placed at the top (root) level, or inside another container or forum.');

    return array(
      '#type' => 'select',
      '#title' => $title,
      '#default_value' => $parent,
      '#options' => $options,
      '#description' => $description,
      '#required' => TRUE,
    );
  }

It was in the old ForumForm file, and both the ForumAdd and ForumEdit will need it. Where is a good place to put it? I'd imagine I wouldn't want to duplicate the same function in both places.

class ForumBaseForm {
  protected function formParentSelect
}
class ForumAddForm extends ForumBaseForm {}
class ForumEditForm extends ForumBaseForm {}

Thanks Larowlan!

My first attempt at this one.

assuming bot agrees I think this is +1 for rtbc

oh, and thanks @Michael Hodge Jr :)

You're welcome. Thanks for the help!

@Tstoeckler, I'll get those changes taken care of. It won't likely be until Saturday as I'm traveling this weekend, but they are on my to-do list. Any hints on why the test failed? I had this fail locally, but I thought it may have been a local thing. I couldn't figure out why this particular error was being triggered.

In which case we need a trait?

Alright, so I was able to get this worked out in a way that reduces the duplicated code, but didn't require a trait in order to do so. The test was failing because of the save() method in the container class. The message being output wasn't what the test expected.

I also refactored the ForumAdd/Edit classes some more per comment #2 to reduce some of the duplicated code. I also split the Container into ContainerAdd and ContainerEdit as appropriate.

Let me know what you guys think of this patch.

Forgot a space between one of my functions. New patch and interdiff attached.

+++ b/core/modules/forum/src/Form/ForumFormBase.php
@@ -0,0 +1,125 @@
+    // @todo Inject a taxonomy service when one exists.
...
+    // @todo Inject a taxonomy service when one exists.
...
+    $tree = taxonomy_get_tree($vid, 0, NULL, TRUE);

FYI the issue is #1976298: Move taxonomy_get_tree() and associated functions to Taxonomy storage, deprecate procedural wrappers. which is RTBC, so might need a re-roll if that goes in first.

Screenshots from manual testing

git ac https://www.drupal.org/files/issues/Drupal-Split_ForumForm_Into_AddEdit_Classes-2324871-17.patch
  % Total    % Received % Xferd  Average Speed   Time    Time     Time  Current
                                 Dload  Upload   Total   Spent    Left  Speed
100 22371  100 22371    0     0  21821      0  0:00:01  0:00:01 --:--:-- 41504
error: patch failed: core/modules/forum/src/Form/ContainerForm.php:1
error: core/modules/forum/src/Form/ContainerForm.php: patch does not apply

Here is the rerolled patch

ta

1. +++ b/core/modules/forum/forum.module
   @@ -163,8 +163,10 @@ function forum_entity_type_build(array &$entity_types) {
   +    ->setFormClass('forum-add', 'Drupal\forum\Form\ForumAddForm')
   +    ->setFormClass('forum-edit', 'Drupal\forum\Form\ForumEditForm')
   +    ->setFormClass('container-add', 'Drupal\forum\Form\ContainerAddForm')
   +    ->setFormClass('container-edit', 'Drupal\forum\Form\ContainerEditForm')
   
   +++ b/core/modules/forum/forum.routing.yml
   @@ -49,7 +49,7 @@ forum.add_forum:
   -    _entity_form: 'taxonomy_term.container'
   +    _entity_form: 'taxonomy_term.container-edit'
   
   @@ -57,7 +57,7 @@ forum.edit_container:
   -    _entity_form: 'taxonomy_term.forum'
   +    _entity_form: 'taxonomy_term.forum-edit'
   
   +++ b/core/modules/forum/src/Controller/ForumController.php
   @@ -163,7 +163,7 @@ public function addForum() {
   -    return $this->entityFormBuilder()->getForm($taxonomy_term, 'forum');
   +    return $this->entityFormBuilder()->getForm($taxonomy_term, 'forum-add');
   
   @@ -178,7 +178,7 @@ public function addContainer() {
        ));
   -    return $this->entityFormBuilder()->getForm($taxonomy_term, 'container');
   +    return $this->entityFormBuilder()->getForm($taxonomy_term, 'container-add');
   

   We don't use hyphens for form classes, please use underscores.

2. +++ b/core/modules/forum/src/Form/ContainerFormBase.php
   @@ -0,0 +1,51 @@
   +class ContainerFormBase extends ForumFormBase {
   
   +++ b/core/modules/forum/src/Form/ForumFormBase.php
   @@ -0,0 +1,125 @@
   +class ForumFormBase extends TermForm {
   

   Why aren't these abstract? Also this just means yet another parent class. They could instead use the "template method pattern", like EntityStorageBase and BlockBase do, by declaring new abstract protected methods, calling those, and thereby forcing the subclasses to just implement their functionality without the need for overriding/parent::

Most importantly, I wonder if we're actually gaining anything here.

As I commented on #2324877-3: Split ShortcutSetForm into ShortcutSetAddForm and ShortcutSetEditForm:

Why are we doing this? Is there a meta issue for these where this was decided to be a good idea?

Re-roll of #22 plus a fix for #24 1)

I'm at DrupalCon LA Sprint. I'll try to re-roll the last working version of the patch for this issue.

I've rerolled the patch from #27

Added namespaces to a few calls to l() and re-rolled the patch.

Marking for review to kick off test bot for updated patch in #32

Rerolled patch from #32.

Used entityManager to retrieve the taxonomy items and vocabulary.
Passed an URL object instead of a link to Drupal::l().
This is part of a Drupal code sprint, i've noticed that some tests have failed. Work in progress.

Missing conversions to \Drupal::l() and passing the Url object using routing instead of a direct link.
ForumForm extends from ForumFormBase now and the duplicate code has been removed from ForumForm.
Maybe ForumForm can be removed now ?

Let's remove ForumForm as well, I don't think we don't need it anymore?

Remove @todo comments and removal of ForumForm in favor of ForumFormBase.

A few unrelated changes but those are fairly straightforward. I think this looks OK now, just needs a beta evaluation.

pinged maintainer to see if this is still possible or is now a BC break

Could we deprecated the existing forms in 8.2/3.x and remove it in 9.x like here https://www.drupal.org/node/2428003#comment-11688399

Yep we can't remove the classes in D8 but we can deprecate and use the new stuff.

Just discussed this with @xjm. https://www.drupal.org/core/d8-bc-policy wasn't 100% clear, but form classes are in the same category as controllers, hence @internal, hence we don't need a bc layer here at all.

If this is going to affect form alters at all we should have a change record though.

The patch in #52 does not apply anymore.
We don't need beta evaluation anymore for 8.3.x & 8.2.x

Here is the rerolled the patch against 8.3.x

More cleanup, less weight patch :)

1. +++ b/core/modules/forum/src/Form/ContainerFormBase.php
   @@ -5,9 +5,9 @@
   +class ContainerFormBase extends ForumFormBase {
   
   +++ b/core/modules/forum/src/Form/ForumEditForm.php
   rename from core/modules/forum/src/Form/ForumForm.php
   rename to core/modules/forum/src/Form/ForumFormBase.php
   
   +++ b/core/modules/forum/src/Form/ForumFormBase.php
   @@ -2,28 +2,25 @@
   +class ForumFormBase extends TermForm {
   

   should this be abstract?

2. +++ b/core/modules/forum/src/Form/ForumFormBase.php
   @@ -135,7 +101,7 @@ protected function forumParentSelect($tid, $title) {
   +    $children = $this->entityManager->getStorage('taxonomy_term')->loadTree($vid, $tid, null, TRUE);
   
   @@ -143,7 +109,7 @@ protected function forumParentSelect($tid, $title) {
   +    $tree = $this->entityManager->getStorage('taxonomy_term')->loadTree($vid, 0, null, TRUE);
   

   Why did we remove the local variable?

#66.1
yup we can make it abstract
#66.2
Reverted. This was in the rerolled patch above :)

More cleanup, less weight patch :)

Replaced the deprecated functions.

+++ b/core/modules/forum/src/Form/ForumAddForm.php
@@ -0,0 +1,38 @@
+  /**
+   * Reusable type field to use in status messages.
+   *
+   * @var string
+   */
+  protected $forumFormType;
+
+  /**
+   * Reusable url stub to use in watchdog messages.
+   *
+   * @var string
+   */
+  protected $urlStub = 'forum';

+++ b/core/modules/forum/src/Form/ForumEditForm.php
@@ -0,0 +1,56 @@
+   * Reusable type field to use in status messages.
+   *
+   * @var string
+   */
+  protected $forumFormType;
+
+  /**
+   * Reusable url stub to use in watchdog messages.
+   *
+   * @var string
+   */
+  protected $urlStub = 'forum';

This is duplicated in the add and edit form, but both of them extend the base form. Could we move them into the parent and remove the duplication?

Removed the duplicated code to the ForumBase Abstract class.

looks good, the only thing is BC - this form class names can be used in contrib

+++ b/core/modules/forum/forum.module
@@ -107,8 +107,10 @@ function forum_entity_type_build(array &$entity_types) {
-    ->setFormClass('forum', 'Drupal\forum\Form\ForumForm')
-    ->setFormClass('container', 'Drupal\forum\Form\ContainerForm')
+    ->setFormClass('forum_add', 'Drupal\forum\Form\ForumAddForm')
+    ->setFormClass('forum_edit', 'Drupal\forum\Form\ForumEditForm')
+    ->setFormClass('container_add', 'Drupal\forum\Form\ContainerAddForm')
+    ->setFormClass('container_edit', 'Drupal\forum\Form\ContainerEditForm')

that's makes me think about BC for contrib

#71, AFAIK, This is not BC as per the bc-policy because the form classes and controllers are internal unless they are tagged with @api https://www.drupal.org/core/d8-bc-policy#controllers
// Naveen

I think this is ready, but over to alexpott to confirm the BC question/otherwise

Proper status

The bc issue here seems fine.

One thing I thought of was deprecating the old classes deprecated for removal in 8.4.x - then if someone really is using them, it gives them a minor release cycle to update.

+++ b/core/modules/forum/src/Form/ContainerEditForm.php
similarity index 93%
rename from core/modules/forum/src/Form/ContainerForm.php

We should deprecate the old classes instead of removing them. Our policy on these is best effort and leaving them in core but unused and deprecated seems fine to me - especially as the deprecation message is to extend the new abstract classes - which seem to have all the original functionality.

I added the Needs issue summary update as I think that there is still a Novice task for the issue to re-scope the issue based on @alexpott's note about deprecating the old class and adding the new ones.

Forum is approved for removal. See #1898812: [policy] Deprecate forum module for removal in Drupal 11

This is now Postponed. The status is set according to two policies. The Remove a core extension and move it to a contributed project and the Extensions approved for removal policies.

It will be moved to the contributed extension once the Drupal 11 branch is open.