Remove theme_html_tag() from the theme system

+1 from me. According to git blame, this was added in #552478-142: Restrict "self-closing" tags to only void elements in drupal_pre_render_html_tag. It'd probably be good to get some of those folks to chime in on this issue.

Please get your search right:

drupal_add_html_head()
_drupal_default_html_head()
drupal_pre_render_styles()
drupal_pre_render_scripts()
config_admin_sync_form()

Also, postponing on #1833920: [META] Markup Utility Functions

I don't understand why we have this function.

@jenlampton: I believe you are asking the wrong question. Don't you mean to ask: "Why does this function need to go through the theme layer?" If so, then I believe it qualifies as a candidate for #1833920: [META] Markup Utility Functions and I would consider retitling this issue.

@sun: Thanks for finding some examples fo where this is used. I took a look at these use cases and it seems to me that the benefit is primarily having a renderable abstraction of a tag that can be digested by drupal_alter (rather than being things that themers want to touch when theming).

This discussion brings up a question in my mind with regard to renderables. I am pondering the following:

Hypothesis: Some renderables are useful for developers via drupal_alter, but do not necessarily have a theming purpose.

Corollary: Callbacks in renderables that produce markup may be of two types: Some simply need to construct HTML that does not have a use case for being overridden by the theme layer, others need to construct HTML that does have a use case for being overridden by the theme layer.

I'm interested to hear what others think about this. Let's just focus on goals for now, not specific code implementations.

+1 for this, turning this theme function into a simple, alterable tag() function like l() - the "auto-closing tag" thing is a nice convenience thing for developers so it would be good to not get rid of it entirely.

This might be all that's needed here.

Profiled a completely default (fresh install) Drupal site with #5:

Warning: Must specify directory location for XHProf runs. Trying /tmp as default. You can either pass the directory location as an argument to the constructor for XHProfRuns_Default() or set xhprof.output_dir ini param.
=== 8.x..8.x compared (51a358e1a4ca6..51a359108f98c):

ct  : 43,046|43,046|0|0.0%
wt  : 249,945|249,619|-326|-0.1%
cpu : 228,236|234,159|5,923|2.6%
mu  : 13,397,384|13,397,384|0|0.0%
pmu : 13,495,048|13,495,048|0|0.0%

<a href="http://127.0.0.1:8888/xhprof-kit/xhprof/xhprof_html/index.php?source=drupal-perf&url=%2F&run1=51a358e1a4ca6&run2=51a359108f98c&extra=8.x..8.x">Profiler output</a>

Warning: Must specify directory location for XHProf runs. Trying /tmp as default. You can either pass the directory location as an argument to the constructor for XHProfRuns_Default() or set xhprof.output_dir ini param.
=== 8.x..1825090-5 compared (51a358e1a4ca6..51a3592d3a3b7):

ct  : 43,046|42,168|-878|-2.0%
wt  : 249,945|240,496|-9,449|-3.8%
cpu : 228,236|223,497|-4,739|-2.1%
mu  : 13,397,384|13,334,672|-62,712|-0.5%
pmu : 13,495,048|13,437,480|-57,568|-0.4%

<a href="http://127.0.0.1:8888/xhprof-kit/xhprof/xhprof_html/index.php?source=drupal-perf&url=%2F&run1=51a358e1a4ca6&run2=51a3592d3a3b7&extra=8.x..1825090-5">Profiler output</a>

---
ct = function calls, wt = wall time, cpu = cpu time used, mu = memory usage, pmu = peak memory usage
You need to specify an alias or run this command within a drupal site.    

This is a very interesting approach to use #type => x for the "markup utility functions" and just re-use form API definitions.

Or would it still be helpful to be able to use #theme => x consistently, but the #theme system / render API internally would know that this is a function that should just return the markup / a structured variables array?

Nice speed improvement!

Would be RTBC once comments are addressed and tests are fixed.

Love, love, love this issue.

#9 - I know c4rl retracted what he wrote up here https://github.com/c4rl/renderapi in general, but there was one bit that made sense to me:

A further point of confusion was that the #type key was only used for form elements, and yet #theme was used for render arrays. From an API perspective, we could instead apply default theme callbacks to particular #type values that can be altered or overridden. In this way #type provides semantic information about the render array whereas #theme does not (it's just a callback).

Therefore, let us define a render array using the hash-prefixed key #type.

Axiom vi: A render array requires the key #type.

So you'd just use '#type' consistently *everywhere* and the render/theme system would know when '#type' mapped to a theme function internally, rather than vice-versa where the developer needing to know ahead of time whether what they're implementing is "type" or "theme" or in the case of the proposed MUF, a "theme-type".

Yes, there's always seemed that there's some overlap with #type with respect to hook_element_info() and hook_theme(), but we need to make sure and not confuse what folks would expect from #type.

Though, it seems that #type sometimes invoking some #theme callback and sometimes invoking a MUF seems plausible. I have a feeling that some of this discussion pertains to bigger-picture ideas in #1843798: [meta] Refactor Render API to be OO.

sometimes invoking a MUF seems plausible.

#type => 'link' already invokes l() to generate '#markup', so there's that.

@c4rl, what exactly made you change your mind about wanting to use the type for all renderable arrays? I thought that would make things less confusing, not more.

#13 @thedavidmeister The direction here seems fine at the moment; I do like the idea that #type may or may not map to associated theme callbacks. Seems to me that we'd move toward combining purposes of hook_element_info() and hook_theme() as a consistent registry. In the OO-driven architecture I'd like to move toward, what we are calling "#type" is actually instead a class definition implementing a common interface.

Carry on :)

#13: 1825090-13.patch queued for re-testing.

#14 - cool. I'll open an actual issue sometime in the next few days for the '#theme' vs. '#type' thing and link to it from the render API refactor issue.

Looks very good to me, #type is appropriate here, passes tests => RTBC

This will need a change notification.

I would love to see this in, great work @thedavidmeister! I think HtmlTagUnitTest should be renamed and docs slightly updated since it is a WebTestBase test.

how's this?

i missed a spot.

try this.

Looking much better, just a few more doc tweaks and then I think this is ready to go.

+++ b/core/modules/system/lib/Drupal/system/Tests/Common/HtmlTagTest.phpundefined
@@ -0,0 +1,47 @@
+ * Definition of Drupal\system\Tests\Common\HtmlTagTest.

Contains \Drupal… per https://drupal.org/node/1354#file. The standard used to be 'Definition of…'.

+++ b/core/modules/system/lib/Drupal/system/Tests/Common/HtmlTagTest.phpundefined
@@ -0,0 +1,47 @@
+ * Unit tests for theme_html_tag().

Comment needs to be updated :)

+++ b/core/modules/system/lib/Drupal/system/Tests/Common/HtmlTagTest.phpundefined
@@ -0,0 +1,47 @@
+   * Test #type 'html_tag'.

Tests (plural) per https://drupal.org/node/1354#functions.

Edited to clarify last point.

make link work?

Update issue summary to use template, reflect latest patch and include code samples

Just rolling in a one-line docs update.

Also updated the issue summary. RTBC!

Add missing code tags

Remove unneeded code samples

Add back theme('html_tag') example, missed the element.

More fiddling and fixing, I think I'm done now :)

Remove drupal.org URLs from code samples because they get linked

Update API change section intro

So, so pleased to see this one in the RTBC queue. Committed/pushed to 8.x, thanks!

https://drupal.org/contributor-tasks/draft-change-notification
is instructions for drafting a change notice.
Anyone can try the first draft. :)
Just make a comment saying you are going to try, and ask questions if you have any.

I am going to work on the change notice.

Summary

html_tag is no longer available as a theme function. It has been converted to a pre-render callback instead to simplify and improve performance.

API change

Any calls to theme('html_tag'); or within a render array '#theme' => 'html_tag' need to be converted to '#type' => 'html_tag'. Drupal 8 core does not use this, but contributed modules need to note this.

Before:

$output = array(
  '#theme' => 'html_tag',
  '#tag' => 'meta',
  '#attributes' => array(
    'name' => 'Generator',
    'content' => 'Drupal 8',
  ),
);

After:

$output = array(
  '#type' => 'html_tag',
  '#tag' => 'meta',
  '#attributes' => array(
    'name' => 'Generator',
    'content' => 'Drupal 8',
  ),
);

Before:

$meta_generator = array(
  '#tag' => 'meta',
  '#attributes' => array(
    'name' => 'Generator',
    'content' => 'Drupal 8',
  ),
);
$output = theme('html_tag', array('element' => $meta_generator));

After:

$meta_generator = array(
  '#type' => 'html_tag',
  '#tag' => 'meta',
  '#attributes' => array(
    'name' => 'Generator',
    'content' => 'Drupal 8',
  ),
);
$output = drupal_render($meta_generator);

@Shyamala - looks good, thank you! The only thing is I would change the summary to something more along these lines:

html_tag is no longer available as a theme function. It has been converted to a pre-render callback instead to simplify and improve performance.

I don't think the performance is the main reason why we removed it, and I don't think we need to say where the pre-render callback lives :)

@Cottser edited #29 as suggested.

Great, I'd say that's ready to post. When you post I would also wrap these in <code> tags:

• theme('html_tag');
• '#theme' => 'html_tag'
• '#type' => 'html_tag'

Thank you!

updated change notice at: https://drupal.org/node/2019739

Great, thanks @Shyamala! Reverting priority.