@thedavidmeister summed up the reasoning for this patch nicely in #22:
I had a chat with @tim.plunkett in IRC and we decided removing #type 'markup' was probably the best way to go as it doesn't do anything, and the fact that it doesn't do anything seems to be poorly understood in the community in general - people believe that it is required for #markup to be rendered, but this is not the case.
I don't know what the original use-case/scenario behind this was but currently drupal_render() will not render #markup if #theme is set.
// If #theme was not set, but the element has raw #markup, prepend the content
// in #markup to #children. #children may contain the rendered content
// supplied by #theme, or the rendered child elements, as processed above. If
// both #theme and #markup are set, then #theme is responsible for rendering
// the element. Eventually assigned #theme_wrappers will expect both the
// element's #markup and the rendered content of child elements in #children.
if (!isset($elements['#theme']) && isset($elements['#markup'])) {
$elements['#children'] = $elements['#markup'] . $elements['#children'];
}
This isn't documented anywhere other than in the inline docs.
This has *nothing* to do with #type 'markup' and drupal_render() never checks #type 'markup' although it does set it for everything with #markup set, regardless of whether #markup will be rendered in the final output, which is useless.
Example
renderable array A:
array(
'#theme' => 'foo',
'#markup' => 'bar',
);
array B:
array(
'#type' => 'foo',
'#markup' => 'bar,
);
Assuming that element_info() does not define #theme = #type by default for 'foo' (this is what core generally does do but is not guaranteed or required by the current API).
I would expect both arrays to be treated similarly, at least for the purposes of deciding what to do with the #markup.
Renderable array A in HEAD will have its #type set to 'markup' (as #type is not set) but #markup will not be rendered.
Renderable array B in HEAD will have its #type left as 'foo' but #markup will be rendered.
This behaviour seems counter-intuitive to me.
Proposed resolution
Remove #type 'markup' completely as it does nothing and if you try to use it to do something mildly complex you get weird behaviour and false positives for renderable elements that don't use #markup. This is not saying we should get rid of #markup, just #type 'markup'.
Comment | File | Size | Author |
---|---|---|---|
#26 | Double_content_in_overlay.png | 40.85 KB | alexpott |
#21 | 2012818-21.patch | 5.67 KB | thedavidmeister |
#20 | 2012818-20.patch | 1.06 KB | thedavidmeister |
#14 | 2012818-14-test.patch | 2.75 KB | thedavidmeister |
#13 | 2012818-13-test.patch | 2.3 KB | thedavidmeister |
Comments
Comment #1
thedavidmeister CreditAttribution: thedavidmeister commentedComment #2
tim.plunkettCan you currently use #type and #theme at the same time on the same render array?
Comment #3
thedavidmeister CreditAttribution: thedavidmeister commented#2 - yes, you can. many of the defaults that come back from hook_element_info() include #theme definitions for types. See #type => 'table' for an example.
Comment #4
tim.plunkettI meant before the element_info is merged in.
Given:
What happens?
Because as I've said, when you have #markup and no #type or #theme, your #type is 'markup'. That's what I've tried to point out in many of these issues.
Comment #5
thedavidmeister CreditAttribution: thedavidmeister commented#4 - Yes, your type is #markup if there is no #type set. In HEAD #theme is currently irrelevant for determining whether #type is 'markup'.
The patch doesn't change that, I don't think.
array('#markup' => 'foo') internally becomes array('#type' => 'markup', '#markup' => 'foo') and this patch doesn't change that default type being selected.
Currently array('#markup' => 'foo', '#theme' => 'table') internally becomes array('#type' => 'markup', '#theme' => 'table', '#markup' => 'foo') but then #markup is ignored.
This patch doesn't change what would happen for array('#theme' => 'item_list', '#type' => 'table') at all.
This patch only affects something like:
Which would load the defaults for 'table' from element_info() then pass what it has to theme('item_list') and then with the result of that:
A. in HEAD ignore #markup
B. with patch, prepend #markup to the result of theme() which I believe is better as it is consistent with drupal_render()'s behaviour when #theme is not set - this is what drupal_render() currently does for *all* #types (not just #type => 'markup') when #theme is not set.
Comment #5.0
thedavidmeister CreditAttribution: thedavidmeister commentedUpdated issue summary.
Comment #6
thedavidmeister CreditAttribution: thedavidmeister commented@tim.plunkett - I updated the issue summary to hopefully make things more specific :)
Comment #7
tim.plunkettGiven these four render arrays
I would expect $build1 and $build2 to be identical, and $build3 and $build4 to match each other.
This change would instead leave $build1 as is, but $build2, $build3, and $build4 would all be the same.
I think this should be won't fix.
@thedavidmeister said "yeah, but i'm telling you why somebody would *expect* it to work based on the docs"
To which I respond, let's fix the docs.
Comment #8
thedavidmeister CreditAttribution: thedavidmeister commentedTo which I responded, "Why on earth is #type set to 'markup' if #theme precludes it ever being used for anything". Also, yeah the docs need updating.
Comment #9
thedavidmeister CreditAttribution: thedavidmeister commentedThis patch prevents:
array('#theme' => 'table');
being converted into:
array('#theme' => 'table', '#type' => 'markup')
Inside drupal_render() meaning that altering element info for #type => 'markup' in contrib won't ever have weird name conflicts with the parameters of unrelated theme functions. It also means that #markup and #type => 'markup' won't be counter intuitive internally.
Comment #10
tim.plunkettThe title was getting pretty long.
Comment #11
thedavidmeister CreditAttribution: thedavidmeister commentedDocs go here #2015307: \Drupal\Core\Render\RendererInterface::render() docs do not mention most of the base # attributes, adding default attributes to elements or how rendering works
Comment #12
thedavidmeister CreditAttribution: thedavidmeister commentedIf this needs tests, it needs work.
Comment #13
thedavidmeister CreditAttribution: thedavidmeister commentedHere's a test that I'm not sure how to make pass yet.
Comment #14
thedavidmeister CreditAttribution: thedavidmeister commentedTwo tests that I'm not sure how to make pass at the same time.
Comment #16
thedavidmeister CreditAttribution: thedavidmeister commented#14: 2012818-14-test.patch queued for re-testing.
Comment #18
thedavidmeister CreditAttribution: thedavidmeister commentedComment #19
thedavidmeister CreditAttribution: thedavidmeister commentedI don't actually know why we need #type 'markup' at all. Couldn't get a decent answer from IRC either when I asked around.
The entry in system_element_info() is:
Which achieves nothing AFAICS since #markup will presumably always be overwritten when #markup is set.
I'll put up a patch and see if the testbots can tell the difference :)
Comment #20
thedavidmeister CreditAttribution: thedavidmeister commentedsee what happens...
Comment #21
thedavidmeister CreditAttribution: thedavidmeister commentedremoving existing explicit #type 'markup' arrays.
Comment #22
thedavidmeister CreditAttribution: thedavidmeister commentedI had a chat with @tim.plunkett in IRC and we decided removing #type 'markup' was probably the best way to go as it doesn't do anything, and the fact that it doesn't do anything seems to be poorly understood in the community in general - people believe that it is required for #markup to be rendered, but this is not the case.
Comment #22.0
thedavidmeister CreditAttribution: thedavidmeister commentedUpdated issue summary.
Comment #22.1
thedavidmeister CreditAttribution: thedavidmeister commentedUpdated issue summary.
Comment #22.2
thedavidmeister CreditAttribution: thedavidmeister commentedUpdated issue summary.
Comment #23
star-szrAsked @thedavidmeister about this patch on IRC and he made some updates to the issue summary to reflect the latest patch.
I think this is ready to go now, #22 is a good justification in my opinion and I've quoted that comment in the issue summary for reference.
Should probably have a small change notification as well.
Comment #24
star-szrAnd there are existing docs that say #type => markup is not needed: https://api.drupal.org/api/drupal/developer%21topics%21forms_api_referen...
Comment #25
alexpottCommitted 9873c74 and pushed to 8.x. Thanks!
Comment #26
alexpott[EDIT] Posted to wrong issue...
Comment #27
alexpottComment #28
alexpottOops... posted on the wrong patch this has not been reverted
Comment #29
star-szrHow's this for a change notice?
#type 'markup' removed
In Drupal 7,
#type
'markup' was a distinction without a difference and caused confusion because including'#type' => 'markup'
in a render array was always optional and did not affect the rendering of markup added using the#markup
render array key in any way.In Drupal 8, you can still add arbitrary markup to render arrays using the
#markup
render array key, but'#type' => 'markup'
is no longer a valid type.Unneeded #type specification:
Correct (for both Drupal 7 and Drupal 8):
In Drupal 7, drupal_render() set
#type
to markup internally before rendering if#markup
was set but#type
was not set. In Drupal 8, we no longer specify a#type
at all for markup, since'#type' => 'markup'
never affected the behaviour of the#markup
render array key.Affects:
Comment #30
thedavidmeister CreditAttribution: thedavidmeister commentedseems fair to me.
Comment #31
star-szrThanks @thedavidmeister! Change notice created:
https://drupal.org/node/2036237
See also the issue to update the Form API Reference docs (in the Documentation project): #2036209: #type markup has been removed, let's remove it from the form/render API documentation.
And I came across this issue in the Documentation queue as well, crosslinking here for future reference and maybe as a good example of the confusion in action: #1261040: Document that #tree property has no effect on #markup elements
Comment #32
David_Rothstein CreditAttribution: David_Rothstein commentedFrom the change notice:
The "never affected" part of this isn't right; in Drupal 7 it was definitely necessary for drupal_render() to do this or otherwise drupal_pre_render_markup() (defined in system_element_info() as a #pre_render function for markup elements) would never be called. For now, I've fixed that in the change notice by changing "never affected" to "no longer affects".
However, for similar reasons I'd like to suggest we reconsider this change. Without markup elements being an actual #type, it means you can no longer add a #pre_render function (or anything else) to them in hook_element_info_alter().... In short, Drupal 8 is treating them differently from any other element for no apparent reason.
According to the issue summary here, the motivation for this issue was some confusing code in drupal_render(), code which Git blame says was originally added in #891112: Replace theme_item_list()'s 'data' items with render elements (the same issue that removed drupal_pre_render_markup() from Drupal 8). I haven't fully gotten my head around the motivation for that change, but this makes me think we should consider whether we are applying fixes on top of fixes rather than fixing the underlying problem.
Comment #33
thedavidmeister CreditAttribution: thedavidmeister commented@David_Rothstein, have you reviewed the failing tests in #14?
If you want to put #type 'markup' back in, could you suggest how we make both of those tests pass?
Comment #34
thedavidmeister CreditAttribution: thedavidmeister commented@David_Rothstein, I think I see what you're saying.
You are suggesting #markup should just be an attribute of a #type 'markup' that otherwise functions exactly the same as any other type, right (ie. its behaviour isn't hard-coded into drupal_render())?
In that case, the tests in #14 wouldn't be a problem as #markup wouldn't mean anything for any element that didn't have a #type of 'markup'.
How would that approach handle existing #types that have no #theme hook set and render #markup in one of their #pre_process functions?
Comment #35
thedavidmeister CreditAttribution: thedavidmeister commentedSimply rolling back #21 won't address the concerns in #32.
If we want #markup to function exactly as an attribute of a new/resurrected #type 'markup' and not effect anything else, there's a fair bit more that needs to be done than putting back in those 3 lines in drupal_render() and system_element_info().
Comment #36
thedavidmeister CreditAttribution: thedavidmeister commentedComment #37
thedavidmeister CreditAttribution: thedavidmeister commentedI'm going to tentatively set this back to fixed. It's been nearly a month since the last update from @David_Rothstein.
Comment #38.0
(not verified) CreditAttribution: commentedAdd quote to summary