I believe there is something wrong with the documentation for file_check_directory() regarding the $mode argument. The document states that this should be a boolean value, but should it not really be a bitmask? A boolean value is (as far as i know) either true or false, but this function accepts three different values for that argument (namely 0, FILE_CREATE DIRECTORY and FILE_MODIFY_PERMISSIONS).

So, if one wants to both have a directory created if it does not exist AND have its permissions changed if it does exist, then the following would be right:

$directory = file_directory_path() . '/some-folder/';
file_check_directory($directory, FILE_CREATE_DIRECTORY|FILE_MODIFY_PERMISSIONS);
CommentFileSizeAuthor
#22 690358-3.patch2.31 KBAnonymous (not verified)
#18 690358-retry.patch2.3 KBAnonymous (not verified)
#16 690358.patch2.3 KBAnonymous (not verified)
#14 patch.patch2.12 KBsbrattla
#5 patch.txt1.96 KBsbrattla
#3 690358-D6.patch1.57 KBjhodgdon

Comments

arianek’s picture

arianek commented
Project: Documentation » Drupal core
Version: » 6.15
Component: Correction/Clarification » documentation
Category: task » bug

moving to core queue - please update the version if you have it (i've just put 6 as it's required), thx!

jhodgdon’s picture

jhodgdon
she/her
Primary language English
commented
Issue tags: +Novice

The doc is here: http://api.drupal.org/api/function/file_check_directory/6 (function doesn't exist in Drupal 7)

Agreed, it should say bitmap rather than boolean.

jhodgdon’s picture

jhodgdon
she/her
Primary language English
commented
Status: Active » Needs review
StatusFileSize
new 690358-D6.patch1.57 KB

Here's a patch that adds the word "bitwise" and also cleans up the doc to conform to current doc header standards.

jhodgdon’s picture

jhodgdon
she/her
Primary language English
commented
Version: 6.15 » 6.x-dev
sbrattla’s picture

sbrattla commented
StatusFileSize
new patch.txt1.96 KB

Hi,

I've taken the liberty to further change the documentation a little bit. I believe it is called a bitmask, but operators working on bitmasks are called bitwise operators. Furthermore, a bitmask does not contain boolean values - but integral types.

I've attached a diff between the original file and my changes.

jhodgdon’s picture

jhodgdon
she/her
Primary language English
commented
Status: Needs review » Needs work

Please start with my patch, because your patch doesn't fix the other doc issues in this function, and doesn't conform to our documentation header standards.

Also, when you upload a patch, please name it .diff or .patch. Thanks!

jhodgdon’s picture

jhodgdon
she/her
Primary language English
commented

For reference, here are the doc standards: http://drupal.org/node/1354

jhodgdon’s picture

jhodgdon
she/her
Primary language English
commented

Also, your patch has numerous spelling errors.

sbrattla’s picture

sbrattla commented

What is the status on this documentation issue?

Jhogdon: you seemed quite eager to get your corrections applied to the documentation. Any news on that?

jhodgdon’s picture

jhodgdon
she/her
Primary language English
commented

When we commit a documentation patch, we want the documentation for the whole function to be cleaned up.

Your patch in #5 had spelling errrors and didn't clean up the documentation to comply with our standards. So it needs some work.

If you would like to make a new version of the patch that does clean up the documentation, then we can review it. Thanks!

sbrattla’s picture

sbrattla commented

That's all fine, but where do we go from here? We didn't really get any further with it.

What is the next step?

jhodgdon’s picture

jhodgdon
she/her
Primary language English
commented

The next step would be to make a patch that doesn't have spelling errors and that provides documentation that fully conforms to our documentation standards.

Ye’s picture

Ye commented

Without sbrattla & jhodgdon clarifying the documentation, file_check_directory()'s api page doesn't make sense at all. But would someone make that final stroke to get this patch into the core so that others wouldn't spend a few hours scratching my head to find out why $mode doesn't work?

Subscribing.

sbrattla’s picture

sbrattla commented
StatusFileSize
new patch.patch2.12 KB

Hey,

I've attached a patch which is a combination of both my and jhodgdon's suggestions. The changes are as follows:

1. It is explicitly explained that the bitmask is optional. In other words, the function can be used to merely check wether a directory exists and is writable - without any actions being taken unless requested.
2. The $form_item argument is explained in more detail.
3. the return value is also explained a little more in detail. The existing docs does not really explain which consequences the optional actions has on the returned value.

Anyways, i've attached my and jhodgdon corrections. Would anyone have the chance to have a look at it. I'll make a proper patch file with correct headers when we've sorted out all spelling mistakes and explanatory faults.

jhodgdon: not trying to 'overrule' your patch suggestion. Just trying to combine things so that the docs are becoming as explanatory as possible.

Would anyone have a look at the patch please, and comment upon all errors and spelling mistakes?

jhodgdon’s picture

jhodgdon
she/her
Primary language English
commented

Your patch is not formed correctly -- it is lacking the stuff at the top that tells what file the patch applies to. Oh... I guess you knew that...

Other problems:
- "whether" is misspelled in the first line.
- The first line of any function doc should be one sentence, followed by a blank line. See
http://drupal.org/node/1354#functions
- The param and return sections are not properly formatted. See link above.
- This paragraph has grammar problems or at least is very awkward (second sentence):

+ *            An optional string containing the name of a form item that
+ *            any errors will be attached to. This is useful when the function is 
+ *            used to validate a directory which path has been entered in a form. 
+ *            An error will consequently prevent form submit handlers from running,
+ *            and instead display the form along with the error messages.
Anonymous’s picture

Anonymous (not verified) commented
Status: Needs work » Needs review
StatusFileSize
new 690358.patch2.3 KB

Used mostly sbrattla's work to create a new doxygen compliant patch. Also fixed the spelling and gave the grammar some love. Hope its good!

jhodgdon’s picture

jhodgdon
she/her
Primary language English
commented
Status: Needs review » Needs work

The doc in the patch is great, but the patch doesn't apply to the 6.x dev branch for me.

Anonymous’s picture

Anonymous (not verified) commented
Status: Needs work » Needs review
StatusFileSize
new 690358-retry.patch2.3 KB

My bad, checked out wrong version at the desktop comp.

jhodgdon’s picture

jhodgdon
she/her
Primary language English
commented

Missed this last time:
ftp -> FTP (acronyms should be capitalized).

I'm still not getting this patch to apply, but I'm not sure why. Maybe it's a line endings problem, or maybe something local to my machine? It looks to me as though the patch you made matches what I'm seeing in the file, so I don't get it.

Status: Needs review » Needs work

The last submitted patch, 690358-retry.patch, failed testing.

jhodgdon’s picture

jhodgdon
she/her
Primary language English
commented

Ignore the test failure (all D6 patches are failing).

Anonymous’s picture

Anonymous (not verified) commented
StatusFileSize
new 690358-3.patch2.31 KB

Here it is again, capitalized FTP and fixed it so it should apply to dev now!

Anonymous’s picture

Anonymous (not verified) commented
Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 690358-3.patch, failed testing.

Anonymous’s picture

Anonymous (not verified) commented
Status: Needs work » Needs review
jhodgdon’s picture

jhodgdon
she/her
Primary language English
commented
Status: Needs review » Reviewed & tested by the community

Looks fine (and the patch applies fine for me). Thanks!

drewish’s picture

drewish commented

Yeah looks real good.

gábor hojtsy’s picture

gábor hojtsy
he/him
Primary language Hungarian
Location Hungary
commented
Status: Reviewed & tested by the community » Fixed

Great, thank you. Committed, pushed.

Status: Fixed » Closed (fixed)
Issue tags: -Novice

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

Issue tags: +Novice

Restoring issue tags, see #2125755: System messages removed all issue tags during D7 upgrade.