This is practically undocumented.

Files: 
CommentFileSizeAuthor
#24 doc-hook_theme-1021568-24.patch3.59 KBsven.lauer
PASSED: [[SimpleTest]]: [MySQL] 33,325 pass(es).
[ View ]
#22 doc-hook_theme-1021568-22.patch3.6 KBsven.lauer
PASSED: [[SimpleTest]]: [MySQL] 33,336 pass(es).
[ View ]
#18 doc-hook_theme-1021568-18.patch3.07 KBsven.lauer
PASSED: [[SimpleTest]]: [MySQL] 33,314 pass(es).
[ View ]
#14 hook_permission_1021568_14.patch3.18 KBskottler
PASSED: [[SimpleTest]]: [MySQL] 33,278 pass(es).
[ View ]
#13 hook_permission_1021568_13.patch3.18 KBskottler
PASSED: [[SimpleTest]]: [MySQL] 33,265 pass(es).
[ View ]
#10 hook_permission_1021568.patch3.18 KBskottler
PASSED: [[SimpleTest]]: [MySQL] 33,077 pass(es).
[ View ]
#9 hook_permission_1021568.patch3.18 KBskottler
PASSED: [[SimpleTest]]: [MySQL] 33,073 pass(es).
[ View ]
#2 1021568.patch3.32 KBjhodgdon
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1021568.patch. This may be a -p0 (old style) patch, which is no longer supported by the testbots.
[ View ]

Comments

Assigned:Unassigned» jhodgdon

The idea here is that in
hook_theme()
http://api.drupal.org/api/drupal/modules--system--system.api.php/functio...
in the Return Values section, variables vs. renderable elements makes no sense.

chx said in IRC:
* [it should explain what] is a renderable element, when should i use one instead the other and so on
* a form is a renderable element. the page array. it has children, it has # properties.if you pass it as a variable then it wont fly cos whatever you pass as a variable is extracted, ie array('foo' => 1) is passed into the template as $foo = 1;

I will write a better doc, just a moment...

Assigned:jhodgdon» Unassigned
Status:Active» Needs review
StatusFileSize
new3.32 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1021568.patch. This may be a -p0 (old style) patch, which is no longer supported by the testbots.
[ View ]

Here's a patch for review...

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

d8 at this point

#2: 1021568.patch queued for re-testing.

#2: 1021568.patch queued for re-testing.

Status:Needs review» Needs work

The last submitted patch, 1021568.patch, failed testing.

I know it isn't applying, but those docs are far more understandable than before.

Looks like this needs a reroll. Probably a good novice project?

StatusFileSize
new3.18 KB
PASSED: [[SimpleTest]]: [MySQL] 33,073 pass(es).
[ View ]

Here is the rerolled patch.

Status:Needs work» Needs review
StatusFileSize
new3.18 KB
PASSED: [[SimpleTest]]: [MySQL] 33,077 pass(es).
[ View ]

Sorry, initiating a test run.

Status:Needs review» Reviewed & tested by the community

Looks like we have a viable patch (thanks for the reroll!), and Everett reviewed the text. Let's get it in (8.x/7.x).

Status:Reviewed & tested by the community» Needs work

+++ b/modules/system/system.api.phpundefined
@@ -1994,21 +1994,27 @@ function hook_permission() {
+ *   containing information about the hook. Each information array must contain
+ *   either a 'variables' element or a 'render element' element, but not both.

This is great, instead of the required if not construct.

+++ b/modules/system/system.api.phpundefined
@@ -1994,21 +1994,27 @@ function hook_permission() {
+ *   Use 'variables' if you are theming an item that has inputs for the data and
+ *   style; in this case, the variables not supplied by the calling function
+ *   will be given default values and passed to the template or theme function.
+ *   Use 'render element' if you are theming a single element or element tree
+ *   composed of elements defined in hook_element() implementations, such as a
+ *   form array, a page array, or a single checkbox element. The theme

However couldn't this be the first sentence of the two descriptions that follow - this would save the cross-referencing/see above.

+++ b/modules/system/system.api.phpundefined
@@ -1994,21 +1994,27 @@ function hook_permission() {
+ *     to determine how to render its inputs, such as the data to be rendered

Inputs looks a bit odd here. I don't have a better word for it, but at the same time I don't think I've ever written 'inputs' anywhere. Maybe stick to using argument for the functions and variables for the template?

-1 days to next Drupal core point release.

Status:Needs work» Needs review
StatusFileSize
new3.18 KB
PASSED: [[SimpleTest]]: [MySQL] 33,265 pass(es).
[ View ]

StatusFileSize
new3.18 KB
PASSED: [[SimpleTest]]: [MySQL] 33,278 pass(es).
[ View ]

The last patch had a space at the end of one of the lines. Re-roll attached.

This is a bit odd as it is, in my opinion.

The information about when to use render element is in the main description, but the information about when to use variables is in the variables list item only. I think both should be treated the same (i.e., move the "use variables" phrase up to just after the "use render element" sentence)? maybe? I found it a bit jarring as it was, because I read that "use render elements" sentence, and immediately thought "and what are variables for"? and it wasn't answered until later.

Also, there is no "hook_element" (and there never has been). The hook used to be called "hook_elements" in D6, but now is called "hook_element_info".

Status:Needs review» Needs work

Status:Needs work» Needs review
StatusFileSize
new3.07 KB
PASSED: [[SimpleTest]]: [MySQL] 33,314 pass(es).
[ View ]

Attaching a conservative re-roll of @skottler's #14, fixing the hook name and addressing jhodgdon's #15.

The explanation of 'variables' (which I now pulled up to be part of the intro text before the list of array key/value pairs) might still need some work to be as clear as possible ...

Elaborating on my uneasiness with the 'variables' explanation mentioned above: The reworked text is much clearer that what we used to have, but it still does not quite tell someone who does his first hook_theme()-implementation what he should do.

Perhaps we should insert a paragraph that makes clear what the registered theme functions do, esp. since in D7, there are two rather distinct use cases. Something like

The functions declared by this hook have one of two purposes: Either they specify how a particular render array is to be rendered as HTML (this is usually the case if the theme function is assigned to the render array's #theme property), or they specify how an invocation of theme() should be handled ...

Of course, this would need quite some work, but such an explanation could be very helpful. Might be something for a follow-up issue, though.

Seems like a reasonable plan that would clarify this exact issue rather than a follow-up? But ... are they really two separate uses for hook_theme()? I guess so...

StatusFileSize
new3.6 KB
PASSED: [[SimpleTest]]: [MySQL] 33,336 pass(es).
[ View ]

Okay, re-rolled a version that includes a clarification of the kind suggested in #19. I thought that the proper place for this is at the top of the doc, as it specifies the general purpose of the function. Perhaps we could also pull more of the paragraph from the beginning of the return value description up.

Status:Needs review» Needs work

I like this version's structure and wording as it is!

A couple of minor grammar/formatting/punctuation stuff that needs to be fixed:

a)

two purposes: Either

Either should not be capitalized here, I think?

b) You cannot have a blank line in the middle of a @return section, or the API module will think the rest is not part of the @return.

StatusFileSize
new3.59 KB
PASSED: [[SimpleTest]]: [MySQL] 33,325 pass(es).
[ View ]

Rerolling. No wording change.

As far as I understand, there is no way to force a line break in the @return directive? I would have thought that the API module uses the indentation to figure out where the @return ends, but according to the doxygen standards, it does not ...

Patch applies cleanly to 7.x, as well.

Status:Needs work» Needs review

Status:Needs review» Reviewed & tested by the community

Right, as far as I know there is no way to have a paragraph break inside a @return. This is usually not a huge limitation, because the vast majority of functions do not need them.

Anyway, thanks for all the iterations -- this looks good to go!

Nota bene, committers, that I joined this issue rather late. @jhodgdon and @skottler deserve at least as much credit for this patch as I do.

Status:Reviewed & tested by the community» Fixed

Great improvement! Committed and pushed to 8.x and 7.x. Thanks!

Status:Fixed» Closed (fixed)
Issue tags:-Novice, -needs backport to D7

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