Issue status update for http://drupal.org/node/19621 Project: Drupal Version: cvs Component: forum.module Category: tasks Priority: normal Assigned to: drumm Reported by: drumm Updated by: TDobes Status: patch +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. TDobes Previous comments: ------------------------------------------------------------------------ March 28, 2005 - 18: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.

Issue status update for http://drupal.org/node/19621 Project: Drupal Version: cvs Component: forum.module Category: tasks Priority: normal Assigned to: drumm Reported by: drumm Updated by: Dries Status: patch I'm going to put this on hold until after the Drupal 4.6 release. Dries Previous comments: ------------------------------------------------------------------------ March 29, 2005 - 02: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 - 10: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 - 19:31 : drumm Attachment: http://drupal.org/files/issues/forum.module_4.diff (7.35 KB) Keeping up with CVS.

Issue status update for http://drupal.org/node/19621 Project: Drupal Version: cvs Component: forum.module Category: tasks Priority: normal Assigned to: drumm Reported by: drumm Updated by: stefan nagtegaal Status: patch Indeed that this is a very nice patch! +1 from me either.. stefan nagtegaal Previous comments: ------------------------------------------------------------------------ March 29, 2005 - 02: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 - 10: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 - 19:31 : drumm Attachment: http://drupal.org/files/issues/forum.module_4.diff (7.35 KB) Keeping up with CVS. ------------------------------------------------------------------------ March 31, 2005 - 23:05 : Dries I'm going to put this on hold until after the Drupal 4.6 release. ------------------------------------------------------------------------ April 15, 2005 - 17:33 : Steven Definite +1 for HEAD.

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.

Issue status update for http://drupal.org/node/19621 Project: Drupal Version: cvs Component: forum.module Category: bug reports Priority: critical Assigned to: drumm Reported by: drumm Updated by: Morbus Iff Status: patch Attachment: http://drupal.org/files/issues/_p_19621_taxosaveobjs.patch (3.52 KB) New patch to use list() and array_values() instead, per Dries. 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. ------------------------------------------------------------------------ May 11, 2005 - 11:39 : Morbus Iff 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'. ------------------------------------------------------------------------ May 12, 2005 - 16:48 : killes@www.drop.org The patch looks ok to me.