The Doxygen documentation for file_validate_extensions looks to be incomplete for one of the parameters. In includes/file.inc for Drupal 6.20 at line 660:
* @param $extensions
* A string with a space separated
This line is incomplete and is missing a key descriptor for what is expected for this parameter. A simple patch is attached to correct this with suggested verbiage for the documentation line.
| Comment | File | Size | Author |
|---|---|---|---|
| #10 | file_validate_extensions.patch | 661 bytes | disturbedmime |
| #3 | file.inc_.patch | 661 bytes | disturbedmime |
| file.inc_.patch | 450 bytes | disturbedmime |
Comments
Comment #1
jhodgdonThanks - good catch.
The patch is not OK as-is though, as it violates our doc standards and English grammar/punctuation (see http://drupal.org/node/1354):
a) Lines in doxygen must wrap at 80 characters or less.
b) i.e 'jpg gif png'. -> For example, 'jpg gif png'.
c) If you are fixing this, you might as well insert a blank line between the @param and the @return.
This problem is not present in D7, by the way.
Good issue for a Novice contributor... and it's not necessary to add so many tags to an issue, in general.
Comment #2
jhodgdonForgot status change. By the way, when you attach a patch to an issue, please change the status to "Needs Review".
Comment #3
disturbedmime commentedAttached an updated patch per recommendations from jhodgdon (thank you for the constructive feedback, by the way). Note that this patch also adds a space between the last @param and @return. The @return string also did not wrap at 80 characters, and is fixed as well.
Comment #4
disturbedmime commentedAn afterthought...
The documentation at http://drupal.org/node/1354#general where the line-wrapping restrictions are mentioned is slightly ambiguous and does not specify whether the 80 character limit should include the first 6 characters needed for the comment syntax and indenting. I interpreted this as a total of 80 characters for the entire line (assuming compatibility with an 80 character terminal), including the formatting characters, thus resulting in the change to the @return string. If I'm wrong about this, please let me know and I'll update the patch accordingly.
Comment #6
disturbedmime commentedIn regards to http://drupal.org/node/961172 (All D6 patches are failing),
Is the automated patch-test system still broken as it is stated in this link?
Also, changing the ticket version to 6.20, although this issues has existed since the first beta release of 6.0. I also verified that includes/file.inc has not changed in HEAD, so it should be applicable there as well.
Comment #7
disturbedmime commented#3: file.inc_.patch queued for re-testing.
Comment #9
jhodgdonYes, the d6 patch system is still failing. Sorry.
Yes, the line wrapping is the total characters in the line. This is a general programming practice that is common in many projects.
Please leave the version at 6.x-dev, which means it is a problem that exists in the 6.x development branch (as well as released versions).
So... The patch in #3 has a space at the end of the first line of the parameter description. That needs to be removed, to comply with general Drupal coding standards.
Thanks for the other fixups... patch is nearly ready to go!
Comment #10
disturbedmime commentedAh, good catch. Another revision of the same patch attached with the space removed.
Comment #12
jhodgdonThanks for your willingness to fix the patch up! Looks good to me, and applies fine. I wish they'd fix that d6 testing issue...
Comment #13
disturbedmime commented#10: file_validate_extensions.patch queued for re-testing.
Comment #18
disturbedmime commentedAttempted a re-test to see if the automated patch test system had been fixed. Apparently not. I'll leave it alone now and let the core developers do their job without me pestering their systems.
Comment #19
gábor hojtsyThanks, committed, pushed.