in issue #2009580: Replace theme() with drupal_render() in image module we can't convert theme('file_upload_help') to drupal_render because "description" is an invalid render array key.

Files: 
CommentFileSizeAuthor
#51 2030243-51.patch8.5 KBpplantinga
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch 2030243-51.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#49 2030243-49.patch8.51 KBpplantinga
PASSED: [[SimpleTest]]: [MySQL] 59,281 pass(es).
[ View ]
#46 2030243-46.patch9.46 KBpplantinga
PASSED: [[SimpleTest]]: [MySQL] 58,056 pass(es).
[ View ]
#46 interdiff.txt4.55 KBpplantinga
#41 2030243-39-41-interdiff.txt992 bytesthedavidmeister
#41 2030243-41.patch9.25 KBthedavidmeister
PASSED: [[SimpleTest]]: [MySQL] 57,162 pass(es).
[ View ]
#39 2030243-39.patch8.56 KBthedavidmeister
PASSED: [[SimpleTest]]: [MySQL] 57,340 pass(es).
[ View ]
#38 2030243-38.patch8.54 KBthedavidmeister
PASSED: [[SimpleTest]]: [MySQL] 57,278 pass(es).
[ View ]
#37 2030243-37.patch8.54 KBthedavidmeister
PASSED: [[SimpleTest]]: [MySQL] 57,314 pass(es).
[ View ]
#36 2030243-36.patch8.54 KBthedavidmeister
PASSED: [[SimpleTest]]: [MySQL] 57,326 pass(es).
[ View ]
#6 file-help-description-2030243-6.patch5.14 KBpplantinga
PASSED: [[SimpleTest]]: [MySQL] 56,761 pass(es).
[ View ]
#1 file-help-description-2030243-1.patch5.14 KBpplantinga
PASSED: [[SimpleTest]]: [MySQL] 57,972 pass(es).
[ View ]

Comments

pplantinga’s picture

Status:Active» Needs review
StatusFileSize
new5.14 KB
PASSED: [[SimpleTest]]: [MySQL] 57,972 pass(es).
[ View ]

Proposed solution: change "description" to "user_description"

thedavidmeister’s picture

Renaming "description" does seem to be the right course of action here as it conflicts with the FAPI.

What does "user_description" mean though?

pplantinga’s picture

I had a hard time coming up with a replacement name, and you're right that user_description probably doesn't make the most sense.

The comment describing this field says it is - "The normal description for this field, specified by the user." Would "file_description" or "field_description" make more sense?

thedavidmeister’s picture

Status:Needs review» Needs work

file_description sounds a bit better to me. Lets roll with that for now.

pplantinga’s picture

Title:theme file_upload_help can't be converted to drupal_render because of "description" conflict» theme file_upload_help $variables['description'] conflicts with FAPI
Status:Needs work» Needs review

Update title to refer less to separate issue.

pplantinga’s picture

StatusFileSize
new5.14 KB
PASSED: [[SimpleTest]]: [MySQL] 56,761 pass(es).
[ View ]

'file_description' it is.

Status:Needs review» Needs work

The last submitted patch, file-help-description-2030243-6.patch, failed testing.

pplantinga’s picture

Status:Needs work» Needs review

#6: file-help-description-2030243-6.patch queued for re-testing.

thedavidmeister’s picture

Status:Needs review» Reviewed & tested by the community

This looks good to me.

Eric_A’s picture

The $file_upload_help element I saw in the other issue was a typeless render element. The drupal_render() API defines/ owns a few element properties, but description is not one of them. Could you please describe a little more where exactly the conflict is?

thedavidmeister’s picture

#description is a standard used by the FAPI: https://api.drupal.org/api/drupal/developer%21topics%21forms_api_referen...

Even though it isn't used by drupal_render(), it would be best if render arrays do not use #description unless they're implementing the #description attribute used by the FAPI.

This goes double for render arrays with a #type that is indented to be used in and around the FAPI.

thedavidmeister’s picture

thedavidmeister’s picture

also check out theme_form_element() which uses #description and is used as #theme_wrappers a lot in the FAPI

Eric_A’s picture

I don't think FAPI builders en processors and the like are actually tied to #description, but I did not check all the relevant functions today. Unlike a hander as #submit or #after_build, #description is just a plain data property to be used on certain types for use by theme functions.

A FAPI type like item has a default #theme_wrapper "theme_form_element()" and that theme hook will pick up the #description property from the type.
Just not put theme file_upload_help() as #theme on a type item, but on its own render array. I've commented a bit more in #2009580: Replace theme() with drupal_render() in image module.

thedavidmeister’s picture

I'd like to see any #foo values used by theme(), drupal_render() or documented as part of the FAPI to be not used by core theme functions if we can avoid it (in this case we can).

Just not put theme file_upload_help() as #theme on a type item

Well, yeah... but that's just a workaround for a bug, that you can't use #theme_wrappers => array('form_element') with #theme => 'file_upload_help' because of the conflict with #description.

Eric_A’s picture

There is no real conflict. Elements of different types should just not be merged into one type on one level. They must live as a sibling or a child. But of course, when early rendering into a string every type can be stuffed into #markup.
Once FAPI and render API arrays are moved to OO I suppose this issue will not exist anymore.

thedavidmeister’s picture

Elements of different types should just not be merged into one type on one level.

I don't think 'file_upload_help' is an element #type. There's a proposed change to start moving/using/treating many more #theme hooks as element #types in core but it's very early stages still. For now, 'file_upload_help' is just a #theme hook used by the FAPI and it should be compatible with FAPI #theme_wrappers I think.

Once FAPI and render API arrays are moved to OO I suppose this issue will not exist anymore.

I've heard of a lot of talk about this, I haven't seen anyone put forward even a proof-of-concept patch for it yet. I'd be pretty surprised if this was a D8 thing.

Eric_A’s picture

Yeah, I should have said:
Different elements cannot just be merged into one element on one level.
Every single element has its own properties, be it #theme, #theme_wrappers, #description. Merging elements will lead to conflicts.
So they must live as a siblings or a children. Of course, when early rendering into a string every type can be stuffed into #markup.

thedavidmeister’s picture

well "early rendering" is something we're trying to avoid wherever possible, so let's continue under the assumption that it's not the preferred solution or anybody's end goal.

I don't see the harm in wanting to make more form element #theme hooks compatible with standard FAPI #theme_wrappers within core. What negative side effects would come of the patch in #6?

#theme and #theme_wrappers do not have to be siblings or children. If that were the case then #theme_wrappers would be completely redundant as a concept and everything would just be #theme hooks with lots of nested children.

Of course in the current system there will always be conflicts, array('#type' => 'table', '#theme' => 'table') would make no sense, for example. But there is a lot we can do to at least make standard, well documented attributes of #theme and #theme_wrappers mean consistent things across the FAPI, RAPI and theme system and that can only be a good thing.

This isn't about whether all #theme will be compatible with all #type or all #theme_wrappers in general, it's about whether we can simplify a specific part of the FAPI with a simple namespace change, which we can.

Eric_A’s picture

Just not put theme file_upload_help() as #theme on a type item, but on its own render array

This is where I was mixing up two problems from the image issue. theme_image_style_preview() was the one that was used there to replace the item default (theme_form_element()). The comment (in corrected form) should have been on #2009580: Replace theme() with drupal_render() in image module.

The current title of this issue says there is a conflict with FAPI. The image issue is postponed on this one. Shouldn't this be marked as a bug, then? But, In this issue there is no test demonstrating a bug. In the other issue I did not find a pointer to any failing test related to the #description property.

thedavidmeister’s picture

Category:task» bug
Status:Reviewed & tested by the community» Needs work
Issue tags:+Needs tests

Yup, adding a test for this is fine.

We'd be looking for errors or unexpected behaviour caused by:

array(
  '#theme' => 'file_upload_help',
  '#theme_wrappers' => array('form_element'),
  '#description' => 'foo',
);

The other issue probably doesn't need to be postponed on this, it could work with child elements in the meantime.

pplantinga’s picture

When I tried it, drupal wouldn't let me use #description, giving an error. Do we still need to test for that behavior?

thedavidmeister’s picture

Category:bug» task
Status:Needs work» Reviewed & tested by the community

#description definitely falls under the umbrella of #2024743: Document "reserved variables" for renderables to avoid conflicts with drupal_render(), the FAPI, and _theme() so I don't really see why we need tests for this element but not the myriad of other theme functions that have no test coverage for exactly the same problem. Maybe the test coverage for core theme hooks in general should go in that issue instead.

If we write tests for this it needs to be in a framework format for testing a lot of related functionality and not just shivved in ad-hoc every time someone happens to ask for it on a particular hook.

For example, tests were not requested for #2010672: Rename 'type' variable of theme_mark to 'status' or #1828536: Rename 'type' variable of theme_item_list() to 'list_type' and it's almost an identical situation here.

Eric_A’s picture

When I tried it, drupal wouldn't let me use #description, giving an error.

Please give us a link to the patch or PIFR report. The form_builder() function will add some WAI-ARIA attributes on some elements with a #description property (which I think FAPI should not, because it's not its responsibility), but it should only do this for elements that are recognized as FAPI elements. Did you move the #description from a non-FAPI context to a FAPI element? Then that is the bug.

If there is no actual evidence mentioned here of a problem then there is no point in keeping this particular issue active for much longer.

thedavidmeister’s picture

Status:Reviewed & tested by the community» Needs work

Tests should focus on unexpected behaviour in render arrays structured similarly to #21.

Eric_A’s picture

Why the theme wrapper when the goal was to convert a direct call to theme('file_upload_help') to a render element? The matching render array should look something like this:

$file_upload_help = array(
  '#theme' => 'file_upload_help',
  '#description' => $description,
  '#upload_validators' => $upload_validators,
  '#cardinality' => $cardinality,
);

When the call to theme() was inside another render element array (usually that's on #markup) you can integrate it by:
a) directly rendering the element to string,
b) lazily rendering to string, i.e. #pre_render,
c) adding a separate (child or sibling) element.

Merging properties directly into the same level would logically always cause conflicts. There can be only one theme, one theme wrapper, one type, one description, etc. on an element.

Regarding #23: the #type property defines the element type and makes it possible to automatically provide default values on the *same* level as where the type property sits. Here you cannot fix the conflict by cleanly separating elements.

thedavidmeister’s picture

Why the theme wrapper when the goal was to convert a direct call to theme('file_upload_help') to a render element

Cross post? I think you're talking about #2009580: Replace theme() with drupal_render() in image module, for which I believe we've already said we were going to render the relevant array directly into #markup.

I was under the impression that here we're just discussing whether the theme hook 'file_upload_help', which is used in the FAPI, is compatible with #type 'item'.

The reason I'm mentioning #theme_wrappers array('form_element') is because that's almost the only property that #type 'item' sets, so rather than get bogged down in a discussion about the roles of #type and #theme I thought it would be more "apples to apples" to discuss compatibility for #theme and #theme_wrappers (which #type in this particular instance is just a proxy for).

Eric_A’s picture

No, I was replying to #25, which refers to #21, which introduces #theme_wrappers out of nowhere. But if it had said '#type' => 'item' I would have responded the same: Type item does not belong on the array element that is supposed to match the direct call to theme_file_upload_help().

Type item sets defaults for #input, #markup and #theme_wrappers. The first is set to TRUE which signals form_builder() to give it the special treatment. But all in all, this is not really relevant to the point I'm trying to make here.

Eric_A’s picture

compatibility for #theme and #theme_wrappers

You can use both on one element, if it makes sense for that element. But you cannot just merge two different elements of which one has #theme_wrapper and one #theme. If if works on occasion than that's just luck. To illustrate: #theme being empty has special meaning to drupal_render(). Only then will it take responsibility for rendering child elements. So when there are child elements, you can only use a theme callback that knows about the structure and renders the child elements.

thedavidmeister’s picture

Assigned:pplantinga» Unassigned

You can use both on one element, if it makes sense for that element. But you cannot just merge two different elements of which one has #theme_wrapper and one #theme.

Where is it documented that we can't use whatever #theme_wrappers and #theme hooks we want to on a given element, in general?

#theme being empty has special meaning to drupal_render(). Only then will it take responsibility for rendering child elements. So when there are child elements, you can only use a theme callback that knows about the structure and renders the child elements.

That's not quite true any more, #theme will now only "take charge" if the hook isn't implemented, not just if it is empty. This has nothing to do with #theme_wrappers though, that happens after #theme or drupal_render() recursively rendering children in a completely different step within drupal_render() and it doesn't care about how children are handled at all, it just assumes that they've been correctly handled and now exist in #children.

pplantinga’s picture

I can't reproduce the error anymore. Either I'm crazy or someone else fixed it, so we can probably mark this as fixed.

thedavidmeister’s picture

Status:Needs work» Needs review
Issue tags:-Needs tests

I can confirm that:

$build = array(
  '#theme' => 'file_upload_help',
  '#description' => 'foo',
  '#cardinality' => -1,
  '#theme_wrappers' => array('form_element'),
);
drupal_render($build);

produces:

<div class="form-item">
foo<br />Unlimited number of files can be uploaded to this field.
<div class="description">foo</div>
</div>

Which is what you expect with the current code, so I don't know how an automated test will help, but that's not really useful to someone trying to build a render array like this.

The problem as I see it is twofold:

- #description is used twice in the rendering for #theme and #theme_wrappers so it is duplicated in the markup.
- #description means two different things in the #theme and #theme_wrappers

For me it comes down to this, given:

- #theme does not care about #theme_wrappers
- #theme_wrappers do not care about #theme, they only care about #children
- #theme_wrappers do not even care about other #theme_wrappers
- IIRC it is not documented anywhere that certain #theme hooks are incompatible with theme hooks intended to be used as #theme_wrappers

Any theme function/template in core that is intended to be used as a wrapper (respects/renders the #children attribute) should not define variables with the same name as any other theme function in core and we should take reasonable measures (whether documentation or obscurity) to ensure that the variable names of the wrappers are unlikely to conflict with contrib/custom theme function variable names too.

Additionally, the documentation of drupal_render() specifically mentions the theme wrapper for form elements and says this:

For example, for the form element type, by default only the #theme_wrappers property is set, which adds the form markup around the rendered child elements of the form. This allows you to set the #theme property on a specific form to a custom theme function, giving you complete control over the placement of the form's children while not at all having to deal with the form markup itself.

Which strongly implies to me that I should be able to use *any* theme function defined by core within theme_form_wrappers and not expect a conflict, on the same element as the #theme_wrappers property, i.e., not requiring a nested element to hold #theme.

Eric_A’s picture

#theme_wrappers callbacks have no access to element children, but they do have access to a computed #children property (computed from "nested elements") *and* any other element property, for example #attributes. The most popular callback for #theme_wrappers is probably theme_container(), which serves as a nice example.
It is perfectly fine to define your own data attributes and not care about what other elements do. The only properties you should stay away from are the ones owned by the applicable API.

thedavidmeister’s picture

Status:Needs review» Needs work
Issue tags:+Needs issue summary update

#theme_wrappers callbacks have no access to element children, but they do have access to a computed #children property (computed from "nested elements") *and* any other element property, for example #attributes.

Aren't you now saying the same thing as me? #theme is rendered into #children before #theme_wrappers is handled. I never said that #theme_wrappers theme hooks have access to element children. Regardless, how is this relevant to the patch in #6 or the issue summary/title?

It is perfectly fine to define your own data attributes and not care about what other elements do.

It's fine to say that but.. the Drupal Render API does not allow you to set attributes for your #theme function and have them only available to your #theme function (invisible to #theme_wrappers) so you currently can't satisfy the following use case (and can't write a test for it either - the conflict prevents you writing a test for the conflict):

- I want to render an array themed with 'file_upload_help', following the drupal_render() documentation that states I can set #theme_wrappers to wrap it in the correct form markup without needing to have a parent element setting #theme_wrappers and a nested child element setting #theme
- I want the user's set description for my file upload field to be displayed, alongside the cardinality description (let's assume it is 'foo')
- I want to give the form element a description of 'bar'

Really, the best fix would be if drupal_render() let you set data attributes for #theme and each #theme_wrappers callback individually so there would be no chance of name collisions but that's not how the API works and there's no chance of making the API work that way in D8 this long after API freeze.

The only properties you should stay away from are the ones owned by the applicable API.

The crux of this issue and this discussion is that I disagree with this point because #theme interacts with more than just the API, it also shares attributes with #theme_wrappers so property names defined by potentially appropriate wrapper hooks for a given theme function are just as important to avoid as the core API ones.

The points against committing the patch in #6 aren't really reasons why it shouldn't be allowed in core but rehashes of either "there's no conflict because you can work around it by nesting elements" or "don't use #theme and #theme_wrappers together if there's a conflict" but that's not what the docs say - which explicitly encourage using #theme and #theme_wrappers on a single element wherever an appropriate wrapper hook is available.

So really, the only points against the patch are because @Eric_A doesn't personally see the value in it because he has discovered a workaround, not because the patch introduces any bugs, coding standards issues, bad API design or DX problems.

The patch in #6 improves core by allowing for the use-case outlined above, passes tests, has been reviewed, and fixes a Drupal WTF by making the Render API behave slightly more like its documentation says it should. The patch should be RTBC, not bikeshedded over whether it should be possible to put a given #theme and #theme_wrappers on a given render element (a decision which Drupal core has no reason to be opinionated about).

That said, the issue summary does need an update as it currently says the reason for the cleanup is that we "can't" do something that we clearly can do simply by nesting elements or rendering one element into the #markup of another.

thedavidmeister’s picture

Title:theme file_upload_help $variables['description'] conflicts with FAPI» Remove theme_file_upload_help() from the theme system

yknow, another option would be to pull file_upload_help out of the theme system completely, in a similar way to how theme_comment_post_forbidden() is slated to be removed in #1987396: Refactor & Clean-up comment.module theme functions.

- There's no markup except a couple of <strong> tags in this theme function so there shouldn't be a use-case to need template/theme function overrides
- All the descriptions are run through t() so they shouldn't be too hard to override
- If we do think we need to preserve DX for messing with this text we can just introduce a drupal_alter('file_upload_help', $descriptions, $variables) on the internal array just before it gets imploded.

theme_file_upload_help() could just become file_upload_help().

thedavidmeister’s picture

Status:Needs work» Needs review
StatusFileSize
new8.54 KB
PASSED: [[SimpleTest]]: [MySQL] 57,326 pass(es).
[ View ]

something like this?

thedavidmeister’s picture

StatusFileSize
new8.54 KB
PASSED: [[SimpleTest]]: [MySQL] 57,314 pass(es).
[ View ]

fixed a typo.

thedavidmeister’s picture

StatusFileSize
new8.54 KB
PASSED: [[SimpleTest]]: [MySQL] 57,278 pass(es).
[ View ]

sorry.

thedavidmeister’s picture

StatusFileSize
new8.56 KB
PASSED: [[SimpleTest]]: [MySQL] 57,340 pass(es).
[ View ]

>.< it's really late here. I should get some sleep.

pplantinga’s picture

Status:Needs review» Reviewed & tested by the community

Looks good to me!

thedavidmeister’s picture

Status:Reviewed & tested by the community» Needs review
StatusFileSize
new9.25 KB
PASSED: [[SimpleTest]]: [MySQL] 57,162 pass(es).
[ View ]
new992 bytes

Actually, I missed the docs in the last patch.

pplantinga’s picture

Status:Needs review» Reviewed & tested by the community

This one looks even better.

alexpott’s picture

Status:Reviewed & tested by the community» Needs work

I;m not sure that this function has been well named since this will cause interesting issue if someone creates a module called file_upload as this will suddenly become an implementation of hook_help!

thedavidmeister’s picture

lol, sure, good point.

Maybe this could wait on #2044105: Introduce #alters for drupal_render() and become a #render callback. At the moment there's not really a clear guide as to what to do with functions that we're de-theme-ifying, so it's easy to make mistakes like this :(

thedavidmeister’s picture

Don't wait on the issue referenced in #44.

#2052253: [META] Add #render property to drupal_render() and convert #type "#pre_render -> #markup" calls to use it is more likely to happen as it has smaller scope for the initial patch.

It also raises the point that *currently* the way to "pull something out of the theme layer" is to convert it to a #pre_render -> #markup style element.

The way that we're using drupal_render() on an element that's the value of the #file_upload_description key is kind of against #2046881: [meta] Avoid "early rendered" strings where beneficial to do so, build structured data to pass to drupal_render() once instead though.

I think this kind of needs to be re-thought a bit.

pplantinga’s picture

StatusFileSize
new4.55 KB
new9.46 KB
PASSED: [[SimpleTest]]: [MySQL] 58,056 pass(es).
[ View ]

What about changing the function name to file_upload_description()

Something like this?

pplantinga’s picture

Status:Needs work» Needs review
areke’s picture

Issue summary:View changes
Status:Needs review» Needs work
Issue tags:+Needs reroll

I think that's a good name change; unfortunately, the patch no longer applies.

pplantinga’s picture

Status:Needs work» Needs review
Issue tags:-Needs reroll
StatusFileSize
new8.51 KB
PASSED: [[SimpleTest]]: [MySQL] 59,281 pass(es).
[ View ]

Here's a rerolled patch

areke’s picture

Status:Needs review» Needs work
Issue tags:+Needs reroll

The patch no longer applies and needs to be re-rolled.

pplantinga’s picture

Status:Needs work» Needs review
Issue tags:-Needs reroll
StatusFileSize
new8.5 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch 2030243-51.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

re-roll

sun’s picture

jibran’s picture

51: 2030243-51.patch queued for re-testing.

Status:Needs review» Needs work

The last submitted patch, 51: 2030243-51.patch, failed testing.