Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
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
.
Comment | File | Size | Author |
---|---|---|---|
#51 | 2030243-51.patch | 8.5 KB | pplantinga |
#49 | 2030243-49.patch | 8.51 KB | pplantinga |
#46 | 2030243-46.patch | 9.46 KB | pplantinga |
#46 | interdiff.txt | 4.55 KB | pplantinga |
#41 | 2030243-39-41-interdiff.txt | 992 bytes | thedavidmeister |
Comments
Comment #1
pplantinga CreditAttribution: pplantinga commentedProposed solution: change "description" to "user_description"
Comment #2
thedavidmeister CreditAttribution: thedavidmeister commentedRenaming "description" does seem to be the right course of action here as it conflicts with the FAPI.
What does "user_description" mean though?
Comment #3
pplantinga CreditAttribution: pplantinga commentedI 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?
Comment #4
thedavidmeister CreditAttribution: thedavidmeister commentedfile_description sounds a bit better to me. Lets roll with that for now.
Comment #5
pplantinga CreditAttribution: pplantinga commentedUpdate title to refer less to separate issue.
Comment #6
pplantinga CreditAttribution: pplantinga commented'file_description' it is.
Comment #8
pplantinga CreditAttribution: pplantinga commented#6: file-help-description-2030243-6.patch queued for re-testing.
Comment #9
thedavidmeister CreditAttribution: thedavidmeister commentedThis looks good to me.
Comment #10
Eric_A CreditAttribution: Eric_A commentedThe $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?Comment #11
thedavidmeister CreditAttribution: thedavidmeister commented#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.
Comment #12
thedavidmeister CreditAttribution: thedavidmeister commentedform_builder() references #description - https://api.drupal.org/api/drupal/core%21includes%21form.inc/function/fo...
Comment #13
thedavidmeister CreditAttribution: thedavidmeister commentedalso check out theme_form_element() which uses #description and is used as #theme_wrappers a lot in the FAPI
Comment #14
Eric_A CreditAttribution: Eric_A commentedI 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.
Comment #15
thedavidmeister CreditAttribution: thedavidmeister commentedI'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).
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.
Comment #16
Eric_A CreditAttribution: Eric_A commentedThere 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.
Comment #17
thedavidmeister CreditAttribution: thedavidmeister commentedI 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.
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.
Comment #18
Eric_A CreditAttribution: Eric_A commentedYeah, 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.
Comment #19
thedavidmeister CreditAttribution: thedavidmeister commentedwell "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.
Comment #20
Eric_A CreditAttribution: Eric_A commentedThis 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.
Comment #21
thedavidmeister CreditAttribution: thedavidmeister commentedYup, adding a test for this is fine.
We'd be looking for errors or unexpected behaviour caused by:
The other issue probably doesn't need to be postponed on this, it could work with child elements in the meantime.
Comment #22
pplantinga CreditAttribution: pplantinga commentedWhen I tried it, drupal wouldn't let me use #description, giving an error. Do we still need to test for that behavior?
Comment #23
thedavidmeister CreditAttribution: thedavidmeister commented#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.
Comment #24
Eric_A CreditAttribution: Eric_A commentedPlease 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.
Comment #25
thedavidmeister CreditAttribution: thedavidmeister commentedTests should focus on unexpected behaviour in render arrays structured similarly to #21.
Comment #26
Eric_A CreditAttribution: Eric_A commentedWhy 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:
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.
Comment #27
thedavidmeister CreditAttribution: thedavidmeister commentedCross 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).
Comment #28
Eric_A CreditAttribution: Eric_A commentedNo, 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.
Comment #29
Eric_A CreditAttribution: Eric_A commentedYou 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.
Comment #30
thedavidmeister CreditAttribution: thedavidmeister commentedWhere is it documented that we can't use whatever #theme_wrappers and #theme hooks we want to on a given element, in general?
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.
Comment #31
pplantinga CreditAttribution: pplantinga commentedI can't reproduce the error anymore. Either I'm crazy or someone else fixed it, so we can probably mark this as fixed.
Comment #32
thedavidmeister CreditAttribution: thedavidmeister commentedI can confirm that:
produces:
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:
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.
Comment #33
Eric_A CreditAttribution: Eric_A commented#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.
Comment #34
thedavidmeister CreditAttribution: thedavidmeister commentedAren'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'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 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.
Comment #35
thedavidmeister CreditAttribution: thedavidmeister commentedyknow, 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().
Comment #36
thedavidmeister CreditAttribution: thedavidmeister commentedsomething like this?
Comment #37
thedavidmeister CreditAttribution: thedavidmeister commentedfixed a typo.
Comment #38
thedavidmeister CreditAttribution: thedavidmeister commentedsorry.
Comment #39
thedavidmeister CreditAttribution: thedavidmeister commented>.< it's really late here. I should get some sleep.
Comment #40
pplantinga CreditAttribution: pplantinga commentedLooks good to me!
Comment #41
thedavidmeister CreditAttribution: thedavidmeister commentedActually, I missed the docs in the last patch.
Comment #42
pplantinga CreditAttribution: pplantinga commentedThis one looks even better.
Comment #43
alexpottI;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!
Comment #44
thedavidmeister CreditAttribution: thedavidmeister commentedlol, sure, good point.
Maybe this could wait on #2044105: Introduce #alters for \Drupal\Core\Render\Renderer::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 :(
Comment #45
thedavidmeister CreditAttribution: thedavidmeister commentedDon'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.
Comment #46
pplantinga CreditAttribution: pplantinga commentedWhat about changing the function name to file_upload_description()
Something like this?
Comment #47
pplantinga CreditAttribution: pplantinga commentedComment #48
areke CreditAttribution: areke commentedI think that's a good name change; unfortunately, the patch no longer applies.
Comment #49
pplantinga CreditAttribution: pplantinga commentedHere's a rerolled patch
Comment #50
areke CreditAttribution: areke commentedThe patch no longer applies and needs to be re-rolled.
Comment #51
pplantinga CreditAttribution: pplantinga commentedre-roll
Comment #52
sunComment #53
jibran51: 2030243-51.patch queued for re-testing.
Comment #55
andypostThere's no such function anymore