Download & Extend
Issues

file_munge_filename() doc mentions watchdog but shouldn't

Posted by allain on October 2, 2009 at 8:05pm
Project:Drupal core
Version:6.x-dev
Component:documentation
Category:bug report
Priority:normal
Assigned:Unassigned
Status:closed (fixed)
Issue tags:Novice

Issue Summary

Documentation refers to watchdog in reference to its alerts parameter, but it's not called at all and the only other method it calls, drupal_set_message, doesn't call it either.

Maybe it should be removed?

Login or register to post comments

Comments

#1

Posted by jhodgdon on October 5, 2009 at 10:32pm
Title:Documentation problem with file_munge_filename» file_munge_filename() doc mentions watchdog but shouldn't
Version:6.x-dev» 7.x-dev

Agreed. watchdog() is not being called, and should be removed from the doc.

Please fix in Drupal 7 first, then backport patch to Drupal 6.

#2

Posted by jhodgdon on October 8, 2009 at 6:09pm
Status:active» needs review

Here's some better (I think) doc for file_munge_filename().

AttachmentSizeStatusTest resultOperations
594518.patch1.54 KBIdlePassed: 13256 passes, 0 fails, 0 exceptionsView details

#3

Posted by MichaelCole on October 18, 2009 at 5:50pm
Status:needs review» reviewed & tested by the community

Yes, patch applies on head.

Comment changes only. Seems thorough.

Watchdog was removed. Ship it!

Yeah! First patch review!

#4

Posted by jim0203 on October 18, 2009 at 6:03pm
Status:reviewed & tested by the community» needs review

Overall, a pass from me. Applies fine, solves the original problem by removing reference to watchdog(), and clarifies the rest of the documentation too. I might clarify the line which reads

If variable 'allow_insecure_uploads' evaluates to TRUE, no alterations will be made.

As it isn't immediately clear what this variable is, where it is set, etc.; but I would still pass the patch even if this line was kept as it currently is.

#5

Posted by jim0203 on October 18, 2009 at 6:05pm
Status:needs review» reviewed & tested by the community

Changing status to RTBC - accidentally changed away from this as we were both reviewing at the same time.

But FWIW, shouldn't patches only be set to RTBC when they've been received two positive reviews?

#6

Posted by Dries on October 18, 2009 at 6:36pm
Status:reviewed & tested by the community» fixed

Committed to CVS HEAD. Thanks!

#7

Posted by jhodgdon on October 19, 2009 at 3:17pm
Status:fixed» patch (to be ported)

Needs port to Drupal 6.

#8

Posted by jhodgdon on October 19, 2009 at 3:27pm
Version:7.x-dev» 6.x-dev
Status:patch (to be ported)» needs review

Here's a patch for Drupal 6.

AttachmentSizeStatusTest resultOperations
594518_D6.patch1.61 KBIgnored: Check issue status.NoneNone

#9

Posted by c960657 on February 8, 2010 at 5:38pm

The comment doesn't address the obvious questions: When should filenames be munged? Why is “exploit.php.pps” a dangerous name? How do I choose the extensions that should not be altered?

#10

Posted by jhodgdon on February 8, 2010 at 6:38pm

Please file this as a separate issue on Drupal 7. The original issue report here is that the doc mentions watchdog but shouldn't. If you think the doc needs more clarification in general, that is a separate issue.

Thanks!

#11

Posted by c960657 on February 10, 2010 at 8:15pm

Done: #710640: Improve documentation for file_munge_filename()

#12

Posted by jhodgdon on February 11, 2010 at 5:48pm
Status:needs review» reviewed & tested by the community

Since this D6 patch sets the doc to exactly what it is in D7, and that was all approved and committed, I'm going to go ahead and set this to RTBC.

#13

Posted by Gábor Hojtsy on March 1, 2010 at 10:59am
Status:reviewed & tested by the community» fixed

Thanks, committed to Drupal 6.

#14

Posted by System Message on March 15, 2010 at 11:00am
Status:fixed» closed (fixed)

Automatically closed -- issue fixed for 2 weeks with no activity.