@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'.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

thedavidmeister’s picture

Status: Active » Needs review
Issue tags: +theme system cleanup
FileSize
1.73 KB
tim.plunkett’s picture

Can you currently use #type and #theme at the same time on the same render array?

thedavidmeister’s picture

#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.

tim.plunkett’s picture

I meant before the element_info is merged in.

Given:

array(
  '#theme' => 'item_list',
  '#type' => 'table',
);

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.

thedavidmeister’s picture

#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:

array(
  '#theme' => 'item_list',
  '#type' => 'table',
  '#markup' => 'foo',
);

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.

thedavidmeister’s picture

Issue summary: View changes

Updated issue summary.

thedavidmeister’s picture

@tim.plunkett - I updated the issue summary to hopefully make things more specific :)

tim.plunkett’s picture

Given these four render arrays

$build1 = array(
  '#theme' => 'table',
  '#rows' => array(array(
    'foo',
  )),
);

$build2 = array(
  '#markup' => 'wtf',
  '#theme' => 'table',
  '#rows' => array(array(
    'foo',
  )),
);

$build3 = array(
  'wtf' => array(
    '#markup' => 'wtf',
  ),
  'table' => array(
    '#theme' => 'table',
    '#rows' => array(array(
      'foo',
    )),
  ),
);

$build4 = array(
  '#prefix' => 'wtf',
  '#theme' => 'table',
  '#rows' => array(array(
    'foo',
  )),
);

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.

thedavidmeister’s picture

Title: Setting #theme in a renderable array shouldn't change the way that #markup is treated. » Setting #theme in a renderable array should prevent #type => 'markup' being added as a default in drupal_render().

To 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.

thedavidmeister’s picture

FileSize
1003 bytes

This 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.

tim.plunkett’s picture

Title: Setting #theme in a renderable array should prevent #type => 'markup' being added as a default in drupal_render(). » #type => 'markup' should not be set if #theme is set
Issue tags: +Needs tests

The title was getting pretty long.

thedavidmeister’s picture

Status: Needs review » Needs work

If this needs tests, it needs work.

thedavidmeister’s picture

Status: Needs work » Needs review
FileSize
2.3 KB

Here's a test that I'm not sure how to make pass yet.

thedavidmeister’s picture

FileSize
2.75 KB

Two tests that I'm not sure how to make pass at the same time.

Status: Needs review » Needs work
Issue tags: -Needs tests, -theme system cleanup

The last submitted patch, 2012818-14-test.patch, failed testing.

thedavidmeister’s picture

Status: Needs work » Needs review

#14: 2012818-14-test.patch queued for re-testing.

Status: Needs review » Needs work
Issue tags: +Needs tests, +theme system cleanup

The last submitted patch, 2012818-14-test.patch, failed testing.

thedavidmeister’s picture

Title: #type => 'markup' should not be set if #theme is set » #type => 'markup' should not be set if #theme is an implemented theme hook
thedavidmeister’s picture

Title: #type => 'markup' should not be set if #theme is an implemented theme hook » Remove #type 'markup' or document a decent use-case for keeping it

I 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:

  $types['markup'] = array(
    '#markup' => '',
  );

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 :)

thedavidmeister’s picture

Status: Needs work » Needs review
FileSize
1.06 KB

see what happens...

thedavidmeister’s picture

FileSize
5.67 KB

removing existing explicit #type 'markup' arrays.

thedavidmeister’s picture

Title: Remove #type 'markup' or document a decent use-case for keeping it » Remove #type 'markup'
Issue tags: -Needs tests

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.

thedavidmeister’s picture

Issue summary: View changes

Updated issue summary.

thedavidmeister’s picture

Issue summary: View changes

Updated issue summary.

thedavidmeister’s picture

Issue summary: View changes

Updated issue summary.

star-szr’s picture

Status: Needs review » Reviewed & tested by the community
Issue tags: +Needs change record

Asked @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.

star-szr’s picture

And there are existing docs that say #type => markup is not needed: https://api.drupal.org/api/drupal/developer%21topics%21forms_api_referen...

alexpott’s picture

Title: Remove #type 'markup' » Change notice: Remove #type 'markup'
Priority: Normal » Critical
Status: Reviewed & tested by the community » Active

Committed 9873c74 and pushed to 8.x. Thanks!

alexpott’s picture

Priority: Critical » Normal
Status: Active » Needs work
FileSize
40.85 KB

[EDIT] Posted to wrong issue...

alexpott’s picture

Title: Change notice: Remove #type 'markup' » Remove #type 'markup'
Priority: Critical » Normal
Status: Active » Needs work
alexpott’s picture

Title: Remove #type 'markup' » Change notice: Remove #type 'markup'
Priority: Normal » Critical
Status: Needs work » Active

Oops... posted on the wrong patch this has not been reverted

star-szr’s picture

Priority: Normal » Critical
Status: Needs work » Needs review

How'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:

array(
  '#type' => 'markup',
  '#markup' => 'Some arbitrary markup.',
);

Correct (for both Drupal 7 and Drupal 8):

array(
  '#markup' => 'Some arbitrary markup.',
);

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:

  • Module developers
  • Themers
thedavidmeister’s picture

Status: Needs review » Reviewed & tested by the community

seems fair to me.

star-szr’s picture

Title: Change notice: Remove #type 'markup' » Remove #type 'markup'
Priority: Critical » Normal
Status: Reviewed & tested by the community » Fixed
Issue tags: -Needs change record

Thanks @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

David_Rothstein’s picture

Title: Remove #type 'markup' » Remove #type 'markup' (rollback?)
Status: Fixed » Active

From the change notice:

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.

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.

thedavidmeister’s picture

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()....

@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?

thedavidmeister’s picture

@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?

thedavidmeister’s picture

Title: Remove #type 'markup' (rollback?) » Remove #type 'markup'

Simply 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().

thedavidmeister’s picture

Status: Active » Postponed (maintainer needs more info)
thedavidmeister’s picture

Status: Postponed (maintainer needs more info) » Fixed

I'm going to tentatively set this back to fixed. It's been nearly a month since the last update from @David_Rothstein.

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

Anonymous’s picture

Issue summary: View changes

Add quote to summary