While working on 'optional' fixes in #2608732: Some fixes for 'optional' parameter in core/modules, it became clear file module's file.module file needs some docblock love. In order to keep patch sizes reasonable, this issue was broken out and uses the work initiated there as a start. It now includes a number of other docblock changes, like proper formatting of a list of items.
Please limit any changes in this issue to docblock improvement only in this one file in the file module. Also to speed reviews, please create patches with enhanced context lines (using -U switch with git diff). Thanks.
| Comment | File | Size | Author |
|---|---|---|---|
| #17 | interdiff-15-17.txt | 4.18 KB | rang501 |
| #17 | 2610202-17-docblocks-file-module-file-d8.patch | 18.49 KB | rang501 |
| #15 | interdiff-10-15.txt | 3.8 KB | rang501 |
| #15 | 2610202-15-docblocks-file-module-file-d8.patch | 17.13 KB | rang501 |
| #10 | interdiff-8-10.txt | 4.47 KB | rang501 |
Comments
Comment #2
lars toomre commentedAttached is an initial patch for this issue. A second review of the file.module file (with this patch attached) would be appreciated. I am unsure yet if this catches all of the docblock changes that should be made. Thanks.
Comment #3
jhodgdonThanks for the patch! Sorry for the delay in reviewing -- I've been on vacation and no one else, unfortunately, ever reviews docs patches. :(
So most of the changes here look good! A few mostly minor things to fix:
Looks like this can be array|null ?
Can be string|null ?
Normally in a list like this, we like to instead of indicating the default above the list, put (default) into the list element that is the default value.
string|null then?
See above note about list and defaults
Um. This is talking about the return value of the callbacks. How does a return value have a default? I think that (the default) is incorrect here.
Can we fix the "e.g." punctuation, and preferably use "for example"? Should be:
... WIDTHxHEIGHT; for example, '640x480' ...
If 0 is the default, then the input can be string|int?
If 0 is the default, the value can be string|int?
So is it string[] or ... what does it contain if the requirements are satisfied?
Looks like this is string|null then?
See note above about defaults in lists of options.
See note above about defaults and lists.
Doesn't seem accurate. Says it can return also a single file object, and looks like it can also return FALSE. ???
Actually I think revisions is correct in this case, meaning "revisions of all entities that have references to this file".
Extra space after .
could leave out "text" here?
Comment #4
lars toomre commentedThanks for the review @jhodgdon. It was reassuring to see that I was consistent in making the same list type of error(s). Sorry for the trailing whitespace issue(s). I thought my editor was set-up to eliminate those, but it was not. That editor setting now has been changed so there should be no more issues of that type.
Attached is an interdiff and patch that should address each of your comments from #3. Let's see what the test bot thinks.
Comment #5
jhodgdonThanks for the new patch! I took a careful look through the patch (not interdiff) again... looking much better!
There are still a couple of things to fix:
Um. I think I mentioned this in the last review, but I guess I wasn't clear.
So take a look at how this reads... to summarize:
It's an optional array of callback functions.
The keys are function names. Then it documents these callbacks:
The values in the array are the parameters to be passed in, after the file entity.
The return value of the function tells whether the file passed validation.
Now here's the problem: the docs say:
An empty array [return value] ***(the default)*** indicates passing validation.
That "the default" is nonsensical -- a function return value does not have a default.
Also in this patch, we *lost* the important information that the return value should be an array of error messages, with empty array indicating it passed.
So I think the previous paragraph was better in this sentence than the patch; please restore it.
Also there are two spaces after the . before the "The hook..." sentence, and in the next line, there is a stray ' at the start of the line.
Then it documents how the
Given the previous discussion... Do you think this should say it's an array of validation error messages, with an empty array indicating no problems?
Maybe since we need another patch anyway, we should fix up a few nitpicks here:
The sentence about "If no extension..." is a bit weird. It says "it" will default to a limited safe set of extensions, but "it" refers to the entire array of callbacks, so this doesn't really make sense.
Really I think that sentence should say something like:
If the array is empty, it will be set up to call file_validate_extensions() with a safe list of extensions, as follows: "jpg .....".
And then the next sentence should be:
To allow all extensions, you must explicitly set this array to ['file_validatate_extensions' => ''].
And then in the last sentence, the . should be inside the ) at the end.
I think this is (default) and not (optional) ?
This is not actually accurate. If you look at
https://api.drupal.org/api/drupal/core!modules!file!file.module/function...
there is no related topic for "File interface".
And anyway, we're now returning file entities, not some generic object thing.
So.
Really @return should say something like FileInterface[] and not array, to indicate what is in the array (figure out the right interface/class and include the namespace).
And then I think this whole paragraph is irrelevant and should be removed.
Right?
Should be type |null right?
Extra space after .
Comment #6
rang501 commentedI took the liberty to fix the patch according to the review above. Hopefully I got it right.
Comment #7
jhodgdonThanks for the new patch -- still some problems though:
This section of documentation needs to be rewrapped to as close to 80 character lines as possible.
Also I wouldn't say "the hook hook_whatever() ... will be called" but instead "hook_whatever() will be invoked"
I think we can drop the words "the integer specifying" here to be much more concise.
This is a comma splice. make it into two sentences or use a ;
The "beware" sentence should not have a capital letter on "This", and the lines need to be rewrapped to closer to 80 characters.
Check the return value type again... doesn't agree with the text.
And if we're fixing this, we should also take out the "Functions returns" and punctuate this better.
False return value in case of an error is now mentioned twice in this doc area.
extra space in this line
Comment #8
rang501 commentedThanks for the review.
The issues you mentioned, should be fixed now. I hope I got the fifth one right.
Comment #9
jhodgdonAlmost! A few more things to fix still:
This whole area of the documentation needs to be rewrapped.
Documentation lines should go out to as close to 80 character lines as possible, without going over. Most of these lines are too short.
This should also be rewrapped.
And this.
The type here should include |null according to the text of this documentation.
The type here still does not match the words in this documentation.
I am also not sure how an entity can "contain FALSE" -- what does this mean? This doc block still needs some attention I think. It was extensively rewritten but I think it may still not be correct.
Comment #10
rang501 commentedHopefully it's better now.
Comment #11
rang501 commentedComment #13
rang501 commentedComment #14
jhodgdonGetting better!
So... I took a very careful look through this patch again. Item 5 in comment #9 is still a problem (see below), and because that still needs to be fixed, I also noted a few other things that I think would really improve this documentation:
This doc block is kind of problematic. How can you copy a file without a destination? Given that, it seems very odd that NULL should be allowed, unless NULL has some particular meaning -- in which case it needs to be explained what that meaning would be.
Same here.
When does the hook get called -- before or after the other validation functions?
Probably the validation errors here are strings? In which case we should say string[] not generic array for the type line.
See, this doc block tells what happens if the input is NULL, so this one is good (unlike the first two mentioned above). For the other two, you'll need to read the function and figure out what the default behavior is for the destination for those functions.
Let me try again on this one... I apparently haven't provided enough detail in my other reviews of this doc block (such as comment #9 item 5).
OK. Let's take a look at this in detail. The wording here (which I'm assuming is correct although I haven't looked at the function code) basically says this function can return:
a) If $delta is NULL, you get an array of file entities (this is of type FileInterface[] -- so far, so good).
b) If $delta is not NULL, you get a single file entity. This would be of type FileInterface, but that is NOT included in the @return type specification, so that is not so good.
c) FALSE if there was an error... that's shown in the @return type line, so that is good. However, if you look at the documentation that this patch is replacing, it says that this FALSE value can be put into **each array element** if there is an error for that one file. So for instance if one of the files failed, you might have an array (entity, FALSE, entity). This is not reflected in either the text in this patch or in the type information on the @return line, although the unpatched documentation has this information.
d) NULL if there are no files uploaded. This is documented and is in the type line.
So. Given (c), I think in the @return type information, instead of FileInterface[] we need to just say it's a generic array (because each element of the array is either a FileInterface or FALSE). And then that information needs to be put back into the documentation and not lost as it is in this patch.
So I think the type line needs to say:
array|\Drupal\file\FileInterface|null|false
and the documentation text needs adjustment. Right?
Here instead of generic array we should be specifying something like FileInterface[] right? Or can each entry be FALSE like in the above function (in which case generic "array" would be correct and the text would need to be updated)? I am not sure -- you need to look at the function body and see how it works. Either the type line or the documentation below it needs updating, and I'm not sure which.
If this defaults to NULL then its type line should be
int|null
right?
Comment #15
rang501 commentedThanks for the review!
About the points 1, 2 and 5 - first two functions require $destination to be a valid URI, if it's null, then error will be returned (default value seems to be wrong). The third one is correct, $destination will default to default file schema if null.
6 and 7. Hopefully got that right this time.
Comment #16
jhodgdonI agree with your analysis in #15.
So... For this review, in contrast to the others, besides just looking at the patch I looked at the code for all of these functions in file.module to see if the documentation was actually accurate.
It's mostly good, but we still need to fix a few things:
See below... [this is docs for file_copy()]
So in file_move() [here], the @param line says "string", but in file_copy() [above], the @param line says "string|null".
We should probably do one or the other for both of these functions, because they behave the same way, rather than having them documented differently. ... Which way? Hm...
Technically, the functions have a NULL default value so they must permit you to pass in NULL; however, they will both return an error if you do pass in NULL, so ... I guess my inclination is not to document that it can be NULL because for all *practical* purposes, it cannot be NULL if you want the functions to actually work, which presumably you do.
So let's make both of these say "string" only.
Nice. Verified it's correct also. :)
The text here for the return value docs is less specific now than it was in the original. Also, this one validation function @return doc was changed where the others were left the same... shouldn't they all be consistent? I think I would recommend a unified style for all of the "run one type of validation" functions. Maybe something like this:
An empty array if the file is a valid image, and an array containing an error message if it is not.
Or something similar? This would apply to all of the file_validate_* functions, like file_validate_is_image(), file_validate_name_length(), etc.
But I think file_validate() itself we should leave as it is in the patch, because that is returning an array of all error messages from a bunch of validation functions.
Thoughts?
Let's make this one just "array" instead of string[] so that all the validator return values are documented the same way?
So, this is interesting. Reading through the code for this function file_save_upload(), it appears to me that even if $delta is FALSE, actually all of the uploaded files that the user submitted are saved.
All that $delta seems to affect is the return value.
Right? Let's document it that way then, something like "The delta of the file to return..."
For this function file_managed_file_save_upload(), I looked at the code carefully...
So, unlike the unmanaged file_save_upload() function, in this one after getting the return value from file_save_upload(), it uses array_filter() to filter out the empty entries, so this statement saying the array can contain FALSE if there was an error with that particular file is incorrect. What really happens is that that one file will get tacitly omitted from the array. So the array contains only file entities, and only the ones that are successfully uploaded and validated.
Should document what NULL means here... see below...
This isn't really accurate. First off, it's not really a string, since it's the return value of t(), which isn't a string any more but \Drupal\Core\StringTranslation\TranslatableMarkup.
Second, if $choice is omitted, this function is actually returning an array of both the known options.
Actually, the only call to this function in Core doesn't pass in $choice, so this is the more common (or only, really) return value... and why it's a separate function instead of just being part of \Drupal\file\Plugin\views\filter\Status::getValueOptions() in the first place is beyond me... but fixing that is beyond the scope of this issue.
Meanwhile, we should fix the documentation so that it reflects the actual return value here, which is either a single \Drupal\Core\StringTranslation\TranslatableMarkup or an array of them.
Comment #17
rang501 commentedThanks for the detailed review again!
I have made another set of changes according your suggestions.
Comment #18
jhodgdonThanks! I think this is ready to go.
Issue scope note: I think this issue is well-scoped. It is fixing a bunch of doc blocks so that they are more accurate representations of the code. They are all in one file, and all related to file uploads. It's not just coding standards fixes, and couldn't be "sniffed" or fixed with a script, and the fixes aren't really related to fixes that we might need to make elsewhere. So it's one stand-alone issue that fixes up several related functions and I think it's well scoped.
Comment #19
catchCommitted/pushed to 8.1.x and cherry-picked to 8.0.x. Thanks!