Support from Acquia helps fund testing for Drupal Acquia logo

Comments

sven.lauer’s picture

Status: Active » Needs review
FileSize
21.88 KB

This one was rather quick.

I had to get creative with the various Form/Render API callbacks, though. Generalizing from the new callback standards, I went with "#property callback: ..." for the introductory line (e.g. "#process callback: ...").

jhodgdon’s picture

Status: Needs review » Needs work

Hm. I am not sure I like that:

 /**
- * Element validate callback for the maximum upload size field.
+ * #element_validate callback: Validates the maximum uplodad size field.

All of our other callback terminology uses plain English rather than coder-speak (e.g., Access callback not 'access callback' as in hook_menu()).

The rest looks pretty good. A few things to fix:

a) Typo (form->from):

+ * Retrieves the file description form a field field element.

b) This should have a form function name instead of description:

+ * Form submission handler for upload and remove buttons of file_generic fields.

(should say Form submission handler for form_function_name(), or Form submission handler for upload and remove buttons on form_func_name() or something like that).

c) Typo (handler misspelled) plus form function instead of description:

+ * Form submission hanlder for upload / remove buttons of managed_file elements.

d) Verb tense (saves not save):

+ * Save any files that have been uploaded into a managed_file element.

e) Typo (retrieves):

+ * Retrives a list of references to a file.

That word is misspelled several other times in your patch.

sven.lauer’s picture

Attached patch straight-forwardly fixes (a), (d), and (e, I think I caught all of those. At least the ones that had the same misspelling).

For (b), I went with:

 * Form submission handler for upload and remove buttons of field_widget_form().

Seeing as it is not really a submit handler for the whole form, but rather for the file field specific stuff---which should be mentioned in the summary line. Relatedly, I was not sure what to do with (except fixing the embarrassing "hanlder" typo):

 * Form submission hanlder for upload / remove buttons of managed_file elements.

so I left it as is for now. The problem is that there is no single form constructor where these are used---rather, they are registered by the #process function for file_managed form element types.

Also, I left the #-callbacks untouched until we resolve what should be done:

I think that "Element validate callback:" is quite unclear. Even as someone who knows about the #element_validate property, it is not immediately clear to me what this means. By contrast "Access/Title/Page callback" is likely to be familiar to anyone who ever implemented hook_menu().

Two suggestions:
(i) Don't mention that this is a callback in the summary at all, but include a line later, saying "This is assigned as a #element_validate callback in foo_bar()".
(ii) Have a generic "Render API callback: ..." as the summary, and include a line as mentioned above.

I think (ii) is better. Generally, the hope is that once we figure out a way to automatically document the #-properties, things like '#element_validate' can be auto-linked.

jhodgdon’s picture

I like your suggestion (ii) too.

Regarding the "Form submission hanlder for upload / remove buttons of managed_file elements." -- thanks for the explanation. I agree it can be left as-is except for the typo.

sven.lauer’s picture

Status: Needs work » Needs review
FileSize
23.73 KB

Changed all the "#foo callback: ..." lines to "Render API callback: ..." and added a line to the docblock saying where this is assigned and to what property.

Only remaining caveat: I've changed all of those to "Render API callback: ..." though many of these would be considered Form API callbacks, if one makes such a distinction. There does seem to be a building consensus, however, that Form API really should be seen as subset of Render API (and, after all, all forms go through
drupal_render() at the end, so the form specific stuff that is done prior to that can be seen as preprocessing the elements for rendering).

However, if people disagree, I could change those to "Form API callback: ...".

jhodgdon’s picture

Status: Needs review » Needs work

Looks pretty good! I like how those element validate callbacks are documented -- seems very clear, and points people to where the items are being used/defined/added to arrays. Excellent!

So I gave this a thorough read-through, and found a few typos -- it's pretty close!

a)

+ * Ensures that a size has been entered and that it can be parse by

parse -> parsed

b) Probably the word "multiple" is redundant here:
+ * Tests upload and remove buttons for multiple multi-valued File field.

c) Verb tense on 2nd clause (do -> does, and there should be an "and" after , too):
+ * Tests the file extension, do additional checks if mimedetect is installed.

sven.lauer’s picture

Fixing these, but:

(b)

 + * Tests upload and remove buttons for multiple multi-valued File field.

The "multiple is in fact not redundant, as the test creates two multi-value fields and tests those:

$this->createFileField($field_name, $type_name, array('cardinality' => 3));
$this->createFileField($field_name2, $type_name, array('cardinality' => 3));

But, of course, that means that it should be "multiple multi-valued File fields" (not "field").

(c) With "and" and "does", this went over 80 characters ... but looking at the test, I actually don't see any additional, mimedetect-dependent tests. So I just changed this to "Tests file extension checking."

sven.lauer’s picture

Status: Needs work » Needs review
jhodgdon’s picture

Just a thought: multiple multi-valued --> several multi-valued -- might be more readable?

sven.lauer’s picture

Hm ... I agree that "multiple multi-value fields" is not the finest prose, but "several" tends to get a "specific" reading in English ("There are several multi-value fields. This functions tests those."), which is not what is meant here.

I tried variants of "multiple file fields with multiple values", but could not fit those into 80 characters.

sven.lauer’s picture

Oh, and:

Looks pretty good! I like how those element validate callbacks are documented -- seems very clear, and points people to where the items are being used/defined/added to arrays. Excellent!

Should we make this a standard? Between our (newish) standards for callbacks that get directly passed to functions, menu and form callbacks, and hook implementations, this would close a gap (and this way of documenting those is most likely to be forward compatible to whatever gets decided in #100680: [meta] Make API module generate Form API documentation.

xjm’s picture

I guess the only thing is that the patch uses "Render API callback" generically for any Render API property, whereas for menus we're naming the specific type of callback it is (access, ajax, page, etc.) I agree we should probably add it to our docs one way or another. It's not so much a separate standard as a special case of the other.

jhodgdon’s picture

Page/access callbacks are callbacks from hook_menu(), whereas these are callbacks for things related to elements, so I don't think there is a conflict? Not sure how to word this standard...

xjm’s picture

Well, not so much a conflict as just an inconsistency. In both cases, the callback is automatically called when it is set in a certain array key (for the hook_menu() array or the render array, respectively.) Either is fine by me really; we''d just need to change a couple "Pre-render callback: " to "Render API callback" in other patches.

jhodgdon’s picture

So... I'm not too concerned about this conflict or lack of standards actually. I think calling something a "pre-render callback" is pretty clear, and the docs that Sven did above calling it "render API callback" and in the next line explaining exactly what it is are also pretty clear. I am not convinced that either way is necessarily better, or that they all need to be exactly the same across Drupal.... Do you think we need to adopt a definite standard for this?

jhodgdon’s picture

Status: Needs review » Reviewed & tested by the community

Meanwhile, the patch in #7 looks good. :)

sven.lauer’s picture

Hm, I was thinking of making this a standard for the sake of consistency. If some (many) functions use the "Render API callback: ..." + separate specification format for #pre_render callbacks, this might well lead someone to think that "Pre-render callback: ..." must means something else.

I like the "Render API: ..." format a little better, as it is more explicit (and also specifies where the callback is assigned). At the same time, #pre_render (and #post_render, #theme, #theme_wrapper callbacks) are special, as they are invoked by drupal_render() itself---meaning (a) that these properties always work and (b) that there is no specialized function invoking them. So it might make sense to (optionally) document these with a more menu-callback-like pattern. In any case, I think that, ideally, these should get an @see drupal_render() (and an @see to the function assigning the callback, if it is not mentioned in the doc text).

xjm’s picture

Yeah, I agree with pretty much every point in #17.

jhodgdon’s picture

OK. Can one of you please file a separate issue on the standard and write up a proposal for what it should say? Thanks!

catch’s picture

Status: Reviewed & tested by the community » Fixed

Looks good. Committed/pushed to 8.x. Thanks!

Status: Fixed » Closed (fixed)

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

xjm’s picture

Version: 8.x-dev » 7.x-dev
Assigned: sven.lauer » Unassigned
Status: Closed (fixed) » Needs review
Issue tags: +Needs backport to D7
FileSize
13.51 KB
17.11 KB

With the render and menu callback stuff removed. The name of the CSS file had also changed.

jhodgdon’s picture

Status: Needs review » Reviewed & tested by the community

These changes all look fine for D7, assuming the test bot agrees. Thanks!

webchick’s picture

Status: Reviewed & tested by the community » Fixed

Looks great, thanks!

Committed and pushed to 7.x.

jhodgdon’s picture

Version: 7.x-dev » 8.x-dev
Status: Fixed » Needs work

Follow-up: I just noticed a typo in the patch that was committed to 8.x (and probably 7.x). This line from file.field.inc:
* Render API callback: Validates the maximum uplodad size field.

uploadad -> upload

jhodgdon’s picture

Issue tags: +Novice
hansyg’s picture

Patch for fixing typo from #25

hansyg’s picture

Status: Needs work » Needs review
xjm’s picture

Status: Needs review » Reviewed & tested by the community

That looks good. Thanks @hansyg!

jhodgdon’s picture

Version: 8.x-dev » 7.x-dev
Status: Reviewed & tested by the community » Fixed

I committed the patch in #28 to Drupal 8 - thanks hansyg!

7.x does not appear to have this typo, so I'm marking this back to fixed.

Status: Fixed » Closed (fixed)

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

Lars Toomre’s picture

For reference, I created a follow-up issue #1811674: Further clean up of API docs for File module.