Add forum/container form descriptions refer to 'Terms' instead of 'Forum'

We already have a form alter here, so just need to expand what it is doing, and the tests

Altering the weight description is easy, but for example we already have in ForumForm.php :

    // Set the title and description of the name field.
    $form['name']['#title'] = $this->t('Forum name');
    $form['name']['#description'] = $this->t('Short but meaningful name for this collection of threaded discussions.');
    // Change the description.
    $form['description']['#description'] = $this->t('Description and guidelines for discussions within this forum.');

which are not working anymore. They all come from the baseFieldDefinition of the taxonomy term entity and I don't know what would be the cleaner way to alter this.

Fixed the basefield titles and descriptions.

Thanks @dpopdan, if this is the solution, then it must be added in the ContainerForm.php as well

I added some tests and made some refractoring on \Drupal\forum\Tests\ForumTest because it violated the coding standars very bad. I also changed the description for the parent field.

+++ b/core/modules/forum/src/Tests/ForumTest.php
@@ -113,11 +120,11 @@ protected function setUp() {
+  protected function testForum() {

@@ -247,7 +262,7 @@ function testForum() {
+  protected function testAddOrphanTopic() {

@@ -347,12 +362,16 @@ private function doAdminTests($user) {
+  protected function editForumVocabulary() {

@@ -387,15 +406,15 @@ function editForumVocabulary() {
+  protected function createForum($type, $parent = 0) {

@@ -436,10 +455,10 @@ function createForum($type, $parent = 0) {
+  protected function deleteForum($tid) {

@@ -475,7 +494,7 @@ private function doBasicTests($user, $admin) {
+  protected function testForumWithNewPost() {

@@ -511,13 +530,13 @@ function testForumWithNewPost() {
+  protected function createForumTopic($forum, $container = FALSE) {

this needs to be public. Can you revert the coding standards changes and file a new issue (post the link here). Will make it easier to review this and as a bonus, you'll get an extra commit mention. Put me down to review the other issue.

Wrong interdiff, sorry . The good one in the next comment.

I am uploading two patches one with name contains reproduce which reproduce the problem if already solved and another patch which contains name as solution is patch to resolve that bug.

Here is the screenshot of interface after applying patch.

@Shivam Agarwal, I don't understand what you did in #14
@larowlan the patch in #12 looks good for me

@prics #17 my first patch which failed in testing was created in order to reproduce the problem in case it is rectified on someone's localhost so as to understand where the actual problem was. Second patch is the solution of the problem i.e. the main motive for which issue was created for. I hope that is meaningful now.

1. +++ b/core/modules/forum/src/Form/ForumForm.php
   @@ -38,15 +38,10 @@ public function form(array $form, FormStateInterface $form_state) {
   +    $form['weight']['#description'] = $this->t('Forums are displayed in ascending order by weight.Forums are displayed in ascending order by weight.');
   

   Any reason why we don't do this one in prepareEntity too?

2. +++ b/core/modules/forum/src/Tests/ForumTest.php
   @@ -670,4 +674,34 @@ private function generateForumTopics() {
   +      default:
   

   No need for the default

@larowlan the description for the weight is hardcoded from TermForm, it doesn't came from the entity.

$form['relations']['weight'] = array(
      '#type' => 'textfield',
      '#title' => $this->t('Weight'),
      '#size' => 6,
      '#default_value' => $term->getWeight(),
      '#description' => $this->t('Terms are displayed in ascending order by weight.'),
      '#required' => TRUE,
    );

The same thing is on $form['parent']. So, I think that is OK.

I verified that the patch does what it is supposed to, but it adds a tab instead of a space in ForumTestphp at the start of line 355, and it adds an extra space at the end of line 358.

Based on last review

+++ b/core/modules/forum/src/Tests/ForumTest.php
@@ -341,8 +341,13 @@ private function doAdminTests($user) {
+	 // Test for forum specific descriptions and labels.
++    $this->verifyForumForm('forum');
++    // Test for container specific descriptions and labels.
++    $this->verifyForumForm('container');  ¶
...
+

Updated patch to fix tab, space, and extra +'s (noted in #21)

Tests are failing for the Forums (tested also with clean HEAD).
Failure should be unrelated to the patched code (seems to be about forum topic creation).

Here's the test report.

$ php core/scripts/run-tests.sh --class "Drupal\forum\Tests\ForumTest::testForum"

Drupal test run
---------------

Tests to be run:
  - Drupal\forum\Tests\ForumTest::testForum

Test run started:
  Friday, September 25, 2015 - 13:47

Test summary
------------


Fatal error: Call to a member function id() on boolean in core/modules/forum/src/Tests/ForumTest.php on line 560

Call Stack:
    0.0038     531328   1. {main}() core/scripts/run-tests.sh:0
    1.1101   17534888   2. simpletest_script_run_one_test() core/scripts/run-tests.sh:44
    1.1200   19145520   3. Drupal\simpletest\TestBase->run() core/scripts/run-tests.sh:658
   37.6016   59970600   4. Drupal\forum\Tests\ForumTest->testForum() core/modules/simpletest/src/TestBase.php:1023
   37.9739   60936032   5. Drupal\forum\Tests\ForumTest->generateForumTopics() core/modules/forum/src/Tests/ForumTest.php:158
   37.9740   60936600   6. Drupal\forum\Tests\ForumTest->createForumTopic() core/modules/forum/src/Tests/ForumTest.php:678

FATAL Drupal\forum\Tests\ForumTest::testForum: test runner returned a non-zero error code (255).
- Found database prefix 'simpletest119866' for test ID 1.
[25-Sep-2015 13:48:33 Europe/Rome] PHP Fatal error:  Call to a member function id() on boolean in core/modules/forum/src/Tests/ForumTest.php on line 560
[25-Sep-2015 13:48:33 Europe/Rome] PHP Stack trace:
[25-Sep-2015 13:48:33 Europe/Rome] PHP   1. {main}() core/scripts/run-tests.sh:0
[25-Sep-2015 13:48:33 Europe/Rome] PHP   2. simpletest_script_run_one_test() core/scripts/run-tests.sh:44
[25-Sep-2015 13:48:33 Europe/Rome] PHP   3. Drupal\simpletest\TestBase->run() core/scripts/run-tests.sh:658
[25-Sep-2015 13:48:33 Europe/Rome] PHP   4. Drupal\forum\Tests\ForumTest->testForum() core/modules/simpletest/src/TestBase.php:1023
[25-Sep-2015 13:48:33 Europe/Rome] PHP   5. Drupal\forum\Tests\ForumTest->generateForumTopics() core/modules/forum/src/Tests/ForumTest.php:158
[25-Sep-2015 13:48:33 Europe/Rome] PHP   6. Drupal\forum\Tests\ForumTest->createForumTopic() core/modules/forum/src/Tests/ForumTest.php:678

- Removed test site directory.
- Removed 52 leftover tables.

Test run duration: 38 sec

Patch doesn't apply to 8.2.x. Setting to unassigned since this hasn't been touched for about one year.

Rerolling the patch in #23. Removing 'Needs Review' tag as it was vague.

Done in a live webinar on Drupal contributions.

Thanks. Applied that patch and found 'Forums are displayed in ascending order by weight.' is duplicated on screen. Screen shots follow.

And found a few things.

1. +++ b/core/modules/forum/src/Form/ContainerForm.php
   @@ -23,14 +23,10 @@ public function form(array $form, FormStateInterface $form_state) {
   +     $this->forumFormType = $this->t('forum container');
   +   // Update the description.
   

   Indentation errors/

2. +++ b/core/modules/forum/src/Form/ContainerForm.php
   @@ -42,5 +38,24 @@ public function buildEntity(array $form, FormStateInterface $form_state) {
   +  }
   +  /**
   

   Blank link needed here, I think.

3. +++ b/core/modules/forum/src/Form/ForumForm.php
   @@ -33,15 +33,11 @@ public function form(array $form, FormStateInterface $form_state) {
   +// But update the description.
   
   @@ -164,5 +160,16 @@ protected function forumParentSelect($tid, $title) {
      }
   +  /**
   

   Indentation

4. +++ b/core/modules/forum/src/Form/ForumForm.php
   @@ -164,5 +160,16 @@ protected function forumParentSelect($tid, $title) {
      }
   +  /**
   

   Blank line.

Screenshots after applying patch in #28

Settings to NW.

Looking good - couple of minor observations

1. +++ b/core/modules/forum/src/Form/ForumForm.php
   @@ -33,15 +33,11 @@ public function form(array $form, FormStateInterface $form_state) {
   +// But update the description.
   

   nit, indent is wrong

2. +++ b/core/modules/forum/src/Tests/ForumTest.php
   @@ -686,4 +690,32 @@ private function generateForumTopics() {
   +  protected function verifyForumForm($type) {
   

   this feels like it should be two different methods, verifyForumForm and verifyContainerForm - thoughts?

Will try to work on this so assigning this to myself :)

I am looking at this as part of SprintWeekend2017

Tested in 8.2 and looks good.

Add Forum before

Add Forum after

Add Container before

Add Container after

1. +++ b/core/modules/forum/src/Form/ContainerForm.php
   @@ -23,14 +23,10 @@ public function form(array $form, FormStateInterface $form_state) {
        $form = parent::form($form, $form_state, $taxonomy_term);
   +     $this->forumFormType = $this->t('forum container');
   +   // Update the description.
   +    $form['weight']['#description'] = $this->t('Containers are displayed in ascending order by weight.');
   

   The alignment is wrong here, these should all line up.

2. +++ b/core/modules/forum/src/Form/ContainerForm.php
   @@ -42,5 +38,24 @@ public function buildEntity(array $form, FormStateInterface $form_state) {
   +    $this->entity->get('name')->getDataDefinition()->setLabel('Container name');
   +    $this->entity->get('name')->getDataDefinition()->setDescription('Short but meaningful name for this collection of related forums.');
   

   This is the first time that we are changing base field definitions in a form and I don't think it's really a good idea. Is there absolutely no way we can do this by actually altering the form array?

Hello,

Fixed the alignment.

For the base field definitions, i don't know of another solution.
So we leave it like this ?
Thanks,

#2861022: ForumForm and ContainerForm forms are rendered using wrong texts

So after 2 yers this issue was created, it was merged in another issue created last week ?

Yeah, that's what I'm trying to ascertain.

What I think has happened, is that issue appears to be 'fixed' based on the commit message, but actually, the commit message refers to the wrong issue, and the patch is for something else.

I'm going to close the other one as a duplicate.

Closed the other one and left a tell for lauriii to confirm it was just an issue number mismatch

The problem seems to be resolved. Can we change the status into RTBC ?

patch #42 not applying with my core files. So, I have rerolled this patch with my version.

Rerolled patch with proper coding standard

It looks like you are adding the verifyForumForm() method into the ForumForm class and not the FormFormTest class.
Because the method is not in the test class, the tests fail with "Call to undefined method ..."
I will take a look at it.

Re-rolled patch.
Original name field descriptions were not even showing up anymore.
Now all seems to be working fine.

Found two small things.

+++ b/core/modules/forum/src/Form/ContainerForm.php
@@ -43,4 +39,23 @@ public function buildEntity(array $form, FormStateInterface $form_state) {
+  }
+  /**

Needs a blank line after the method.

+++ b/core/modules/forum/src/Form/ForumForm.php
@@ -165,4 +160,17 @@ protected function forumParentSelect($tid, $title) {
+

Remove extra blank line.

Made changes as per the comment #58. Applying the patch, please review.

Munavijayalakshmi, thanks for your patch. I have reviewed this patch and seems working, moreover, the duplicate string issue has also been fixed.

@akashkrishnan, thanks for review. Can you explain what you mean by 'seems working'? I'm wondering if there is something you are not sure about? Did you install the patch and look at the addforum page? If you did, can you add a screenshot to confirm that this patch fixes the issue?

add_forum_container-2453495-61.patch successfully applied and the issues are fixed now, here I have uploaded a few screenshots showing the fixed areas.

So my point 2. in #39 was never addressed as far as I can tell. I am going to take a stronger stance now and say that we definitely should not be altering the property definitions at runtime. Let's just override the form directly where we need to.

As seen that #61 is no longer compatible with the current version so Reroll the patch for the version 9.4.x and attaching the reroll_diff file for the same.

Refactoring the CCF erros at above patch

Sorry, As my PHPCS is not working on my local so i address the fixes manually though

Fixed CCF for #77.
Please review.

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.