Users can add forum topic when no forum exists

subscribe

A very hurried patch, (wife and child are in the car and waiting - I am in trouble!)

It appears form.inc doesn't check for a '_none' value in _form_validate() and I've made a small change to the forum_node_validate() function.

It's the change to form.inc I am most concerned about, I haven't had time to test other forms with required fields and required select fields, would appreciate some eyes on that.

I am still not entirely happy with this solution but I think it's cleaner.

This patch alters the forum.install file to make the forums taxonomy field 'required', this prevent the field from getting the '_none' => '- None -' option as it was previously.

This results in an empty select field when you visit node/add/forum (unless you've created your forums) - I've added a check to see whether or not that's the case and display a message if no containers or forums exist (I check the options on the taxonomy field in hook_form_alter()).

The reason I think this is lame is because you can still get to node/add/forum when no forums exist. There's a chance a site creator might just enable forum and think they are done, then the poor old user is left confused when they come to post a forum topic...

I would appreciate any ideas on how to handle/fix this better!

I think this is a duplicate of another critical issue in the queue. Please double-check prior working further on this.

@sun which issue is this a duplicate of?

I'm finding the problem with users creating forum topics and leaving the selection to "none" - and their post disappears. Shouldn't it force a choice of topic?

These the related forum/topic issues... which one is the duplicate?

#221527: Don't show Create Forum topic (node/add/forum) when there isn't a forum

this one?
#613272: Forums not useable out-of-the-box: disabled comments, no forums

Not sure where the dup is if there is one. The drupal_set_message() should only be shown to people with admin permissions though.

Reroll showing the dsm() to forum admins only. I think this is a reasonable approach. #221527: Don't show Create Forum topic (node/add/forum) when there isn't a forum and #613272: Forums not useable out-of-the-box: disabled comments, no forums would also be usability improvements, but I agree with heather that this is its own issue.

Hmm, I don't think this is quite right.

The instance for forums should be required, but we allow non-forum nodes to be posted to forums. In that case, the forum vocabulary will be optional, and forum_node_load() will still throw a notice.

So the patch here seems fine, but we could also change

if (_forum_node_check_node_type($node)) {
<code>
to
<code>
if (_forum_node_check_node_type($node) && !empty($node->forum_tid)) {

in forum_node_view().

Sorry, was going to mark it rtbc before I realised that...

Good point. I didn't even remember we allowed non-forum nodes in forums. New patch attached.

Looks good now.

Could we get some tests for this?

Ow yes this bit me...
This not only happens the first time, if you choose parent forum it will "crash" to, so hopefully this will fix it :)
Come on with those tests :p

Here is a small test that checks that the warning is displayed. I guess that is all there is to test? Or did I miss something?

The test should also try to submit that form with a title and body, but no forum selected I think.

The forum select field is required so all one gets is the An illegal choice has been detected. Please contact the site administrator.

The test now asserts that, but is that worth testing or should we do a more informative message? Not sure, but I am certainly not a fan of that message.

OK, forget patch from #28. That message is not what I get when I don't fill out a required field elsewhere. No wonder I didn't like it. I'll take a look at it later.

UPDATE: Redacted, I was confusing myself.

OK. So first off, forums is not a required field. The bug is not that forums should be a required field. The bug is just that forum_node_view assumes that all nodes with the taxonomy_forums field will have a forum_tid. This is not the case: nodes can be removed from forums without being deleted, in which case they will have a taxonomy_forums field without having a forum_tid. The patch is one line. I've included a test. The second patch is just the test, to show that it indeed fails without the patch to forum.module.

Need to Re-roll to correct test failures/exceptions.

Meant to upload two different files...

+++ modules/forum/forum.module
@@ -296,9 +296,8 @@ function forum_node_validate($node, $form) {
+    if (!empty($node->taxonomy_forums[$langcode]) && $containers = variable_get('forum_containers', array())) {

($containers = ...) needs to be wrapped in parenthesis.

+++ modules/forum/forum.test
@@ -51,6 +51,9 @@ class ForumTestCase extends DrupalWebTestCase {
+    // Create a forum node for the any forum user, without assigning the node to a forum.
+    $this->createForumTopic($this->forum, FALSE, FALSE);

"for the any forum user" (?) What's that?

+++ modules/forum/forum.test
@@ -258,9 +261,10 @@ class ForumTestCase extends DrupalWebTestCase {
    * @param array $forum Forum array.
    * @param boolean $container True if $forum is a container.
+   * @param boolean $assign_forum True to assign the topic to the forum.
    * @return object Topic node created.

While being there, let's fix the coding style here.

+++ modules/forum/forum.test
@@ -258,9 +261,10 @@ class ForumTestCase extends DrupalWebTestCase {
+  function createForumTopic($forum, $container = FALSE, $assign_forum = TRUE) {

@@ -271,8 +275,14 @@ class ForumTestCase extends DrupalWebTestCase {
+    if ($assign_forum) {
+      // Preselect the forum ID via a URL parameter.
+      $this->drupalPost('node/add/forum/' . $forum['tid'], $edit, t('Save'));

@@ -281,14 +291,19 @@ class ForumTestCase extends DrupalWebTestCase {
+    if ($assign_forum) {
+      $this->assertEqual($node->taxonomy_forums[LANGUAGE_NONE][0]['tid'], $forum['tid'], 'Saved forum topic was in the expected forum');

The argument variable should be named $tid.

116 critical left. Go review some!

forum.test has incomprehensible comments like "the own user" and "the any user". I don't think this issue should be burdened with fixing forum.test, which is poorly structured to begin with. These comments were committed along with the rest of the original test framework.

I fixed the other issues. I don't know what "While being there, let's fix the coding style here." refers to with respect to the PHPDoc.

+++ modules/forum/forum.test
@@ -51,6 +51,9 @@ class ForumTestCase extends DrupalWebTestCase {
+    // Create a forum node for the any forum user, without assigning the node to a forum.

I still don't grok that, sorry. Is the anonymous user meant here? Or is it trying to tell that it's creating a top-level forum? Or...?

+++ modules/forum/forum.test
@@ -258,9 +261,10 @@ class ForumTestCase extends DrupalWebTestCase {
    * @param array $forum Forum array.
    * @param boolean $container True if $forum is a container.
+   * @param boolean $assign_forum True to assign the topic to the forum.
    * @return object Topic node created.

See http://drupal.org/node/1354

+++ modules/forum/forum.test
@@ -258,9 +261,10 @@ class ForumTestCase extends DrupalWebTestCase {
+  function createForumTopic($forum, $container = FALSE, $assign_forum = TRUE) {

@@ -270,9 +274,16 @@ class ForumTestCase extends DrupalWebTestCase {
+    if ($assign_forum) {
+      // Preselect the forum ID via a URL parameter.
+      $this->drupalPost('node/add/forum/' . $tid, $edit, t('Save'));

Reading the code flow again, $preselect or similar would be a better argument variable name.

Powered by Dreditor.

Ok, brought some sanity to the variable names in forum.test. Thanks, sun.

Shoot, forgot to rename some of the variables.

Editing a node that does not have a forum_tid creates the same notice. Added a fix and a test.

When running tests, I get two notices when creating a node without a forum_tid that I cannot reproduce manually. Adding an if ($item['tid']) { in forum_node_validate gets rid of the notices, but I can't figure out why these notices happen.

I tested the patch and I got away with creating a forum node in no forum (not choosing anything in the "Forums" select box). Is that what we want? That node is somewhat orphaned.

Would it make more sense to find a way to tell the user that in order to create forum nodes one has to create a vocabulary to hold forum posts?

I think that is what we want. It's possible to remove a node from a forum without deleting it. In that case, we have an orphaned forum node, too. I think if there's a reason to orphan forum nodes by updating them, then it should be possible to create orphaned forum nodes, too. The system could have been more easily designed to prevent orphans, but it was not. So, I must assume this functionality was intentional.

I'm changing the issue title so that it's not about being able to create orphans, but about the notice that appears when you do.

@naxoc - Did you test from a fresh install? The patch should prevent you from creating an orphan node if you installed fresh after applying the patch.

I'm going to change the title back - the patch should be preventing orphans. If it doesn't, we need to fix the patch.

I would like to hear from someone who wrote forum.module before deciding whether orphans should never be allowed.

Earlier patches prevented creating orphans, but no patch in this thread prevented updating nodes such that they become orphans.

Forum module has no maintainer at the moment, so there's nobody to ask, I'm afraid. At any rate, it doesn't really matter what the original intent was - it's more a question of what we as a community want it to do right now.

Personally, I'm happy with the current behavior. We should be preventing orphan nodes from being created via node/add. I didn't mean to imply that we need to worry about what happens on update. That would be a separate issue anyway. I just wanted to verify that what naxoc observed isn't happening from a fresh install.

Drupal 6 has the forum vocabulary required for forum nodes, not required for non-forum nodes. Since we can do required / not-required with field instance settings, we should do the same in D7.

Okay, well the patch in #26 ought to be merged with the patch in #44 (and the tests updated) if we think that orphan nodes should be impossible to achieve.

The original intent may inform us of a scenario that we have not though of which would explain why orphaned nodes would be desirable. But I guess that intent will never be known.

To summarize the work so far: Without any patch, creating a forum node in no forum is possible, but viewing or editing that node creates PHP notices. With patch #44, the PHP notices are eliminated (and tested for). This should be in the final patch. With #26, creating a forum node in no forum is impossible, using field instance settings as mentioned in #50 (but this is not tested). This should be in the final patch, too (though perhaps people will want to change the dsm message).

@ksenzee It was on a fresh install. - I am setting the issue to needs work.

I am also still confused on how I managed to submit a node without filling in a required field. Maybe something weird is going on with the field settings?

New patch.

Remaining todo: Initially, when no forum terms exist yet, the form contains a forum term select widget with no options. Submitting the form therefore leads to the form API error message:

An illegal choice has been detected. Please contact the site administrator.

Not sure whether we need to fix that.

That's a general issue with Field API, it's worth a separate issue, but not worth holding up this one. Patch is missing the test changes from #44 though.

I don't have time to work on this patch right now. However, the last test changes from #44 look too convoluted to me. All the new patch needs to test is that a user is not able to submit a forum node directly after installation.

Yes, I agree that's all that's necessary for tests. Should be 2-3 lines added to one of the existing tests. Anyone got time to update?

Here is a quick test that just makes sure that the node is not saved. It does not assert the error message.

Doh, you beat me to it! The patch looks good to me, applied cleanly and the all the tests pass, good work!

Test addition looks good, RTBC.

In #39-#41, sun reviewed the patch that eventually became patch #44, and there he requested a cleanup of forum.test so as not to have ridiculous comments like "the any user" or "the own user", which aren't even English. I assume sun would still find such language unnecessarily confusing, and if the latest patch happened to include any lines with "the any user", I doubt he would have forgotten his original complaints and labelled #44 convoluted as he did in #56.

I'll re-roll this patch with the same fixes to the language in forum.test in a bit.

Those lines are no longer touched by this patch. This issue is critical, and we badly need to get that critical counter to zero. Other comment clean-ups can happen in a separate, non-critical issue.

Follow-up patch #771278: Clean-up forum.test.

Shortened up the PHPDoc comment on the test function so it fit on one line, and committed to HEAD. I've no idea why this weird edge-case bug was listed as critical, but hey. Another one bites the dust! Great work, folks!

#4: forum-form-652372.patch queued for re-testing.

why retesting this is fixed o_O