> This function should be used when the file to be deleted does not have an entry recorded in the files table.

Shouldn't that be {files_managed} table? Though I confess I'm not entirely sure what's going on with file tables now...

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

jhodgdon’s picture

Title: Documentation problem with file_unmanaged_delete » file_unmanaged_delete doc should be like the other file_unmanaged functions
Issue tags: +Novice

Actually, this function is called from file_delete(), which is the function that uses the File API. So I think that line should be struck from the documentation. The other file_unmanaged_* functions do not have this line, and I think it's misleading. They should all be documented the same way.

This is probably a good project for a novice -- just make sure all the file_unmanaged_* functions use consistent wording at the top to explain that they're not doing any database stuff.

jhodgdon’s picture

Status: Active » Needs review
FileSize
1.7 KB

I decided I liked the wording that was in file_unmanaged_copy:

Copies a file to a new location without invoking the file API.

So this patch makes all the other file_unmanaged_* functions have similar wording, and removes misleading and repetitive lines trying to say the same thing.

joachim’s picture

That all looks nice and succinct.

+1 from me.

+++ includes/file.inc	10 Feb 2011 17:11:31 -0000
@@ -1816,11 +1811,7 @@
- *
- * This function is identical to file_save_data() except the file will not be
- * saved to the {file_managed} table and none of the file_* hooks will be
- * called.

I think this bit could do to survive somewhere -- is there a page that gives an overview of the FileAPI? Something like:

"Functions for manipulating files such as file_save_data() have unmanaged counterparts such as file_unmanaged_save_data(). These are identical except that they do not invoke the File API -- that is, the {file_managed} table is not affected and none of the file_* hooks will be called."

Powered by Dreditor.

jhodgdon’s picture

Status: Needs review » Needs work

Do you think we should put this text on each of the functions? file_unmanaged_save_data was the only one with this paragraph. The rest just have links between the file_unmanaged_* and file_* equivalents, and the unmanaged ones say "without invoking the file API"... Maybe rather than just @see we could do:

See file_unmanaged_whatever() for the equivalent function that does not manage the file using the File API.
See file_whatever() for the equivalent function that does manage the file using the File API.

Thoughts?

Also, I guess all the file* functions are part of:
http://api.drupal.org/api/drupal/includes--file.inc/group/file/7
and we should probably put a note there at the least as to what "unmanaged" and "managed" files are.

jhodgdon’s picture

Title: file_unmanaged_delete doc should be like the other file_unmanaged functions » file_unmanaged_* doc should be consistent and clearer
Status: Needs work » Needs review
Issue tags: -Novice
FileSize
7.92 KB

Here's a new patch, which attempts to make things clearer, and further standardizes the file operations functions doc headers.

jhodgdon’s picture

FileSize
7.93 KB

Missed one spot. Try this one.

joachim’s picture

I've not gone over that patch with a fine toothcomb, but in general it looks good.

jhodgdon’s picture

Here's hoping you have time to get out your fine-toothed comb and review this sometime soon then. :)

joachim’s picture

Status: Needs review » Reviewed & tested by the community

Toothcomb time found :)

Very good clean-up and standardization of the file API and function docs. RTBC.

webchick’s picture

Status: Reviewed & tested by the community » Needs work

"+ * The File interface contains functions for handling "managed" and "unmanaged"
+ * files."

This could do with a bit more explanation, because my very next question reading this is "What's the difference between a managed and unmanaged file?" This is eventually explained, but not until I've read the remaining paragraph of information.

So I'd suggest adding a second sentence after that that lays out the difference summarized in a sentence (basically, stored in the database with full hook support e.g. for file fields on entities, vs. simply stored on the file system, e.g. the site logo), then go into more details in the following sentences.

It's also not clear to me why you're saying "See foo_function() for the other equivalent" rather than @see. We don't do that elsewhere. For example, we don't say "See foo_form_validate() for this form's validate function" we say "@see form_foo_validate()."

I think it would be better and more consistent to simply explain in the Common file handling functions defgroup that there are both managed and unmanaged file and that each function links to its opposite equivalent. Thoughts?

joachim’s picture

> It's also not clear to me why you're saying "See foo_function() for the other equivalent" rather than @see. We don't do that elsewhere. For example, we don't say "See foo_form_validate() for this form's validate function" we say "@see form_foo_validate()."

A simple @see would work if the functions were called file_unmanaged_dostuff and file_managed_dostuff. But when you're at file_unmanaged_dostuff, seeing a 'see file_dostuff' I just think 'Huh? What? What does that dostuff do that this dostuff doesn't? Argh am I using the wrong one!'

jhodgdon’s picture

Yes, I put in the text to explain why you would want to refer to the other functions. Just saying @see here seemed like not enough explanation of the difference. But I can take that back out if you feel strongly enough that it's not necessary.

I'll rearrange the explanation of managed/unmanaged if I can figure out a better wording.

jhodgdon’s picture

Status: Needs work » Needs review
FileSize
5.98 KB

OK, here's a new patch. I'm seeing webchick's point on the @see, so I both reworded the explanation about managed vs unmanaged, and make the See ... into just @see lines.

jhodgdon’s picture

Version: 7.x-dev » 8.x-dev

d8 at this point

jhodgdon’s picture

#13: 1029068-13.patch queued for re-testing.

joachim’s picture

Status: Needs review » Reviewed & tested by the community

Looks good to me.

jhodgdon’s picture

#13: 1029068-13.patch queued for re-testing.

catch’s picture

Issue tags: +Needs backport to D7

Docs only, so tagging for backport.

webchick’s picture

Status: Reviewed & tested by the community » Needs work

Great work! Looks good to me. However, no longer applies.

jhodgdon’s picture

Status: Needs work » Needs review
Issue tags: -Needs backport to D7

#13: 1029068-13.patch queued for re-testing.

Status: Needs review » Needs work

The last submitted patch, 1029068-13.patch, failed testing.

jhodgdon’s picture

Issue summary: View changes
Status: Needs work » Closed (cannot reproduce)

This documentation was long since fixed up in some other issue.