'Forums' term defaults to 'None' value

1. The 'Forums' taxonomy term field is required, but has a 'None' value.

I think this is the correct behavior... the system needs to display something when the the user hasn't yet chosen a forum. I don't think it would be bad to call that "choose one" (or whatever flavor of that message people like), but it would be *much* worse if the first option of the list were simply chosen by default for lack of a "none"..

2. The field should default to the forum the user was visiting when they clicked 'Add new forum topic'.

This I think is an excellent idea nd should be implemented. I think it could be implemented pretty easily using a $_REQUEST variable passed in when the user clicks the "add" link ... I'll give it whirl in the next few days, but I'd love for someone to re-affirm that this strategy has no weird security issues I'm not thinking of.

Yes, but look what happens when you go to: http://drupal.org/node/add/forum

I think we are both on the same page: The forums field should default to the correct forum when possible, otherwise a "choose this" option.

this patch sets the default forum ... if available in the URL

Looks and works fine.

While I hesitate to spend an awful long time on this bug, generally speaking, we don't like to make assumptions about the URL where forms will be displayed. The framework should allow me to arbitrarily call the forum node form function from the URL /superforum/add/purple/cat/monkey/dishwasher, which would break if the form assumed arg(3).

If this really is the only way around it, please re-write the code slightly so that its more like:

 $form['taxonomy_forums'][$langcode]['#default_value'] = is_numeric(arg(3)) ? arg(3) : '';	

This allows someone to easily form_alter it to arg(6) instead.

However, in looking at the D6 version I don't really see any code that uses arg(). Could you investigate further to see how D6 solves this issue?

And while it's exceptionally annoying, this bug doesn't prevent forum module from working (though lots of other stuff does, apparently ;P) so I'm downgrading from critical.

I'd also like if we could provide a test for this.

Turns out that this is the same method that was used in d6:

    //--forum.module:217--//
    case 'prepare':
      if (empty($node->nid)) {
        // New topic
        $node->taxonomy[arg(3)]->vid = $vid;
        $node->taxonomy[arg(3)]->tid = arg(3);
      }
      break;

I rerolled the patch with the minor change you (rightly) suggested. This certainly isn't a huge deal but I think it would be good not to lose the functionality from d6

Note that Drupal 7 currently contains this code, which looks like it's trying to do what the patch here is doing, but failing miserably in the process:

function forum_node_prepare($node) {
  if (_forum_node_check_node_type($node)) {
    if (empty($node->nid)) {
      // New topic
      $node->taxonomy_forums[0]['tid'] =  arg(3);
    }
  }
}

As part of this patch, we should probably just delete that code. Especially since it gets called in node_object_prepare(), so it is also causing some problems when trying to programmatically create nodes in certain situations (e.g., Devel Generate), which is how I came across it.

testbot seems to be ignoring me ... lets try this (same patch as #8 - I cant fathom why it would fail)

cycling status for testbot

cycling status for testbot

here testbot ... here boy.

It seems David's comment in #10 was ignored in the latest patch?

@dries: the patch in #8 had inexplicably failed according to testbot (but passed everything locally) .... Everything subsequent was me just trying to figure out why that failed so I could then go back to #10

Since testbot is stil out of commission (I think) I'll just roll a new patch with #10 and cross my fingers :)

This patch is the same as #8 while incorporating David's suggestion in #10.

Note:
With this patch I'm able to manually create forum posts correctly, and the default value of "forum" is correctly filled out. However, I tried generating some forum posts using devel generate and I get an error (below) ... BUT I get the same error with or without this patch.

// I get this error when I view a forum topic node generated by devel generate
Notice: Undefined property: stdClass::$forum_tid in forum_node_view() (line 246 of /Users/alex/Sites/d7/modules/forum/forum.module).

Ok testbot ... its you and me. THROWDOWN!!

(trying to get testbot to test... this is the same patch as #17)

I got it!! Take this testbot!

I feel as though testbot has something against this particular issue. It hates & hates ... lets try this patch (from #24) again since the retest aint happening

Testbot still seems a bit confused :)

In any case, here's a version with a test (actually just fixes some bizarre stuff that was in an existing test), so that the forum tests now correctly fail without the patched code but pass with it.

I also did some minor cleanup to the new code and comments to make it a bit easier to understand. Ideally, we would find a way to do this whole thing via hook_menu() or hook_menu_alter() rather than having to rely on arg() magic, but I couldn't think of any simple way to do that.... and since this is the same approach that was taken before, it should certainly be good enough for now.

I'm not seeing where this test is asserting that the correct category is being used after the form is submitted.

But then, there are a lot of tests like that.

Otherwise sound.

-J

So much for the testbot - the patch does not even apply :) Should be a simple reroll though.

I think you're right, the test should probably check that the correct category was saved. (The test right now will fail when we want it to - however, that probably relies on the fact that "None" is selected by default on the form and therefore prevents the form from being submitted.)

Also I just realized that this code comment is incredibly confusing, since I made the bad decision to put "3" in the 3rd spot of the URL ... too many 3's :)

+      // If there is no default forum already selected, try to get the forum
+      // ID from the URL (e.g., if we are on a page like node/add/forum/3, we
+      // expect "3" to be the ID of the forum that was requested).
+      $requested_forum_id = arg(3);

Should probably use node/add/forum/2 => "2" as the example instead.

subscribing

Here's one with a test to check that it worked.

Committed to CVS HEAD. Thanks!

Let's fix the confusing code comment I mentioned above :)

Committed to HEAD. Thanks!