[drupal-devel] [bug] Sensible status messages for forum admin

[Morbus Iff]

Issue status update for http://drupal.org/node/19621

 Project:      Drupal
 Version:      cvs
 Component:    forum.module
-Category:     tasks
+Category:     bug reports
-Priority:     normal
+Priority:     critical
 Assigned to:  drumm
 Reported by:  drumm
 Updated by:   Morbus Iff
-Status:       active
+Status:       patch
 Attachment:   http://drupal.org/files/issues/_p_taxosaveobjs.patch (3.91 KB)

The committed patch breaks two things: the submission of new "free
tagging" terms (needed the $tid from taxonomy_save_term) and the
auto-creation of the "Forum" vocabulary (needed the $vid from
taxonomy_save_vocabulary). The attached patch fixes the above by
returning BOTH the $status and the $edit in taxonomy_save_term and
taxonomy_save_vocabulary. This was implemented per drumm's suggestion
in #10. Updating the issue to 'critical' and 'bug report'.




Morbus Iff



Previous comments:
------------------------------------------------------------------------

March 28, 2005 - 19:55 : drumm

Attachment: http://drupal.org/files/issues/forum.module_3.diff (6.73 KB)

We are making forums and forum containers here, not terms. This patch
returns a status so the calling code can set the appropriate message
(or no message at all). This has the happy side effect of preventing
any potential problems with calling this from when cron is running and
other similar situations. The define()s are done in common.inc to
encourage similarly architected code; separate UI from DB manipulation.




------------------------------------------------------------------------

March 29, 2005 - 03:48 : TDobes

+1... this might be used during module installation for modules that
require their own vocab or perhaps in the installer as a way to set up
some default categories for "instant" site creation.  Not cluttering
the screen with lots of drupal_set_messages would be a good thing in
these instances too.




------------------------------------------------------------------------

March 31, 2005 - 12:31 : drumm

Attachment: http://drupal.org/files/issues/forum.module_4.diff (7.35 KB)

Keeping up with CVS.




------------------------------------------------------------------------

March 31, 2005 - 16:05 : Dries

I'm going to put this on hold until after the Drupal 4.6 release.




------------------------------------------------------------------------

April 15, 2005 - 10:33 : Steven

Definite +1 for HEAD.




------------------------------------------------------------------------

April 15, 2005 - 11:15 : stefan nagtegaal

Indeed that this is a very nice patch!
+1 from me either..




------------------------------------------------------------------------

May 5, 2005 - 19:14 : Steven

This patch needs to be updated with the check plain patch. The term name
is not correctly escaped, but also, it should use theme('placeholder')
for putting the term name into a message, instead of hardcoding <em>
tags.




------------------------------------------------------------------------

May 6, 2005 - 20:47 : Steven

Committed to HEAD.




------------------------------------------------------------------------

May 10, 2005 - 16:34 : Morbus Iff

This patch caused the bug at http://drupal.org/node/22490. I am planning
a patch that sets $edit['status'] (and returns $edit, per the previous
behavior) instead of returning $status. Thoughts? Should be done
tomorrow.




------------------------------------------------------------------------

May 10, 2005 - 20:48 : drumm

That seems a bit ugly since we are returning something slightly
different from what was really created. Perhaps these sorts of
functions really need to standardize on returning something like
array('status' => ..., 'object' => ...);




------------------------------------------------------------------------

May 10, 2005 - 20:57 : Morbus Iff

Well, I had considered the multiple return values, but then decided on
the 'status' key mainly because of the nature of what an "object" is
(or, rather, could be). Are we returning a database object, which has
keys that specifically map to the term_data table (in which case, yes,
'status' is incorrect since it has no database comparison), or are we
returning a 'term' object, which simply has data related to it (such as
its last affected status)? I don't think either distinction has been
made in Drupal core in any sort of common or suggested way, and felt
that a modified $edit was "cleaner". Although, now that I think about
it, there ARE various Drupal functions that iterate over an object's
keys under the assumption that they DO relate to the table structure
(node_save, I believe, comes to mind). So, perhaps your array return is
the right idea after all. Either way, I'll be addressing it first thing
tomorrow.