Come together with the global Drupal community in Rotterdam, 28 Sept – 1 Oct 2026. Sessions, contribution, connection, and Early Bird savings until 8 June.Hello,
One year ago, the Drupal 8 html_tag has been updated and it's now able to render nested tags, see the CR here #2887146.
I backported the functionnality to Drupal 7 so no, we can render this render array properly, including the children.
To test the functionnality, run this code before and after applying the patch in /devel/php:
$render_array = array(
'#type' => 'html_tag',
'#tag' => 'h1',
'#attributes' => array('class' => 'title'),
'children' => array(
array(
'#type' => 'link',
'#title' => 'Link title',
'#href' => '/',
'#attributes' => array('class' => 'inner'),
),
array(
'#theme' => 'link',
'#text' => 'Link title',
'#path' => '/',
'#options' => array(
'attributes' => array(),
'html' => FALSE,
)
),
)
);
$html = render($render_array);
debug($html);
Without the patch: <h1 class="title" />
With the patch: <h1 class="title"><a href="/" class="inner">Link title</a><a href="/">Link title</a></h1>
Another example:
$render_array = array(
'#type' => 'html_tag',
'#tag' => 'h1',
'#attributes' => array('class' => 'title'),
'#value' => 'value',
'children' => array(
array(
'#type' => 'link',
'#title' => 'Link title',
'#href' => '/',
'#attributes' => array('class' => 'inner'),
),
array(
'#theme' => 'link',
'#text' => 'Link title',
'#path' => '/',
'#options' => array(
'attributes' => array(),
'html' => FALSE,
)
),
)
);
$html = render($render_array);
debug($html);
Without the patch: <h1 class="title">value</h1>
With the patch: <h1 class="title">value<a href="/" class="inner">Link title</a><a href="/">Link title</a></h1>
Change record
Proposed change record: https://www.drupal.org/node/2982025
| Comment | File | Size | Author |
|---|---|---|---|
| #72 | 2981726.patch | 22.91 KB | pol |
Comments
Comment #2
polComment #3
polComment #4
polComment #5
polComment #6
polComment #7
andypostbit cleaner title
Comment #9
polUpdating the patch.
Comment #11
polUpdate the failing test.
Comment #12
polUpdate the failing test.
Comment #14
polUpdate code and failing test.
Comment #17
polFix the test.
Comment #19
polFor some reasons, the tests are passing locally, but not on the Drupal CI. Investigating.
Comment #20
polI'm unable to reproduce the issue from the Drupal CI.
Here's the result locally:
Comment #21
polUpdate the tests.
Comment #23
polUpdate the tests.
Comment #24
samuel.mortensonThis looks good to me after a short review.
Comment #25
polUpdate patch codestyle.
Comment #26
polFix test.
Comment #29
polFix the test.
Comment #31
manuel garcia commented+1 to doing this, thanks @Pol!
I think a CR for this would be great :)
Comment #32
polComment #33
polComment #34
polComment #35
polThe last patch:
<br />to<br/>)Ready for review again.
Comment #36
manuel garcia commentedThanks for the CR, I've reworded and clarified it a bit, in the spirit of making it clearer.
Comment #37
ieguskiza commentedComment #38
ieguskiza commentedReviewed and tested the added functionality and tests, looks ready for me.
Comment #39
ieguskiza commentedComment #40
donquixote commentedAs an initial reaction, the proposed change appears to be an improvement and is consistent with Drupal 8.
I personally have run into situations where I did wish that 'html_tag' in D7 would work the way that is proposed here.
BC concerns
On second thought, the proposed change does introduce BC issues in some edge cases.
Here is a line from drupal_render():
This means the following will produce a different result with this patch than it does now:
Without the patch, I expect the following html:
<div><strong>Hello</strong></div>With the patch, I expect the following html:
<div>Hello</div>Why?
The #theme_wrappers will still be just
array('container'), notarray('container', 'tag_name')orarray('tag_name', 'container'), due to the way drupal_render() works.Was this a BC break in D8 too?
The D8 change record was on 29 June 2017, on 8.4.x branch.
At this time D8 itself was already considered stable, but 8.4.0 was not released yet (this was on 4 October 2017).
The D8 change is implemented somewhat differently, using a preRenderHtmlTag() method. So, I don't know if this is comparable anyway.
The BC break I am pointing out here might not be the only one. We are changing 'html_tag' to work as a theme wrapper, this might cause other side effects.
Alternatives
We could look at different implementations with #pre_render, but this could also cause edge case BC issues, for similar reasons.
A safer alternative would be to introduce a new element, or extend the 'container' element to have a dynamic '#container_tag_name' property. I am saying #container_tag_name or #container_tag, not just #tag_name or #tag, to reduce possible conflict with other elements that support a #tag property.
Contrib
There is at least one contrib implementation I know of,
'themekit_container'.
(this reminds me I need to publish a stable version of this module)
Perhaps there are others who have attempted the same.
I am NOT proposing that everyone use this or another contrib solution. Adding a dependency just for one theme hook usually is not worth it.
Conclusion
The original proposal has potential BC issues, but is more consistent with D8.
We might conclude that the BC issues are survivable or can be somehow mitigated.
The alternative has less BC issues, but is not consistent with D8.
Comment #41
polHello @donquixote,
At first I had exactly the same idea, and I've filled in an issue regarding this as well... #2981632: Let the 'container' element use a different HTML tag than the default "div".
Then only I proposed this patch when I saw how Drupal 8 did the thing.
I've updated your example:
Without the patch:
<div><strong>Hello</strong></div>With the patch:
<div></div>Comment #42
donquixote commentedYou are right.
Comment #43
donquixote commentedCode review for #32:
Perhaps this should read "The rendered html" or something like that.
I wonder, what should happen if
$element['#children'] === ''vs!isset($element['#children'])?I think the latter happens if 'tag_name' is used with #theme, the other if used with #theme_wrappers with no children.
We can output either this
<TAGNAME></TAGNAME>(with empty contents)or this
<TAGNAME/>I think if used with #theme_wrappers, we always want the former. Or do we?
For a browser they might be equivalent, but we still want a clearly defined outcome in each case.
NULL !== $element['#value']I like the yoda condition, but probably somebody will complain about it :)
Perhaps the #value_prefix and #value_suffix should only wrap the #value, not the #children?
So, more like this,
For the overall structure, I would usually propose "onion style", where you first build the inner html and then wrap it into a container.
But in this case it actually seems good enough to me in your patch, I don't see how changing it would greatly improve things.
Comment #44
polThanks for the suggestions @donquixote.
I've updated the patch:
#valueonly with#value_prefixand#value_suffix,Comment #45
polPatch updated:
Comment #46
polHere's the interdiff with #32.
Comment #47
_KurT_ commentedHI, shouldn't we check if
$element['#tag']is present and is not NULL ?I understand that it is created for this, to render what exactly you need, but currently it renders
< class="title">value<a href="/" class="inner">Link title</a><a href="/">Link title</a></>if #tag is empty.Comment #48
polHello,
Yes we could check for the existence of that property. But then, we have to decide what to do in each case.
If the
$element['#tag']is not present ? What tag to use ?The current implementation of theme_html_tag() does not check for that.
Because of those black spots, I didn't add the check.
Comment #49
donquixote commented#tag is considered a required property for #theme = 'html_tag' or #type = 'html_tag'.
A number of theme hooks have required properties, and in most cases there are no checks in place, they are just expected to be there. Omitting e.g. $image['#path'] would lead to a PHP notice, and it would not work properly.
Exactly.
I think we should keep it that way.
I'd say this counts as incorrect use of the 'html_tag', so the some kind of error handling should apply.
In a different life, we might let theme functions throw a specific type of exception.
In this life, we just let the PHP notice happen.
Comment #50
polComment #51
markhalliwellThe
#value_prefixand#value_suffixproperties hadisset()checks around them because these properties are not defined insystem_element_info().If we want to remove the checks, then these properties need to be added to the element info as default values (that's why element_info exists in the first place).
I suspect that this is an attempt to deal with the fact that this is technically a theme hook, which can be invoked outside of the render system using
theme()(which is not possible in 8.x and, now, considered extremely bad practice in 7.x).I would almost argue that this should deprecate the theme hook implementation entirely and move all this code to a
#pre_rendercallback that populates#markuplike HtmlTag::preRenderHtmlTag does (which would be a true 8.x backport BTW, unlike what the current issue title suggests).We can then replace the code in
theme_html_tagwith something that constructs a new element using the correct#typeand populating the appropriate properties, spit out a deprecation notice (perhaps with the ability to silience it via a variable) and then render the new element usingdrupal_render().There wouldn't be any risk of recursion either since the
html_tagelement would get rid of the#themeproperty and simply rely on the new#pre_rendercallback that populates#markup.Pretty sure this could be considered a BC break. While newer browsers don't care about this space, I don't think changing this now is worth breaking existing contrib/site tests as well.
---
Regardless of whatever is done here, this issue will likely need a CR.
Comment #52
markhalliwellThis should also take into account changing the logic of the self-closing tags to only apply to void tags, not based on content (see related 8.x issue).
Comment #53
polHere's a patch with a complete different approach - I'm putting this here to test it first, of course, feedback is welcome.
Comment #55
markhalliwellCloser, I'm not entirely sure why only certain parts were backported. The majority of the code, documentation, and tests could more or less be a 1:1 copy and paste (adjusting for 7.x APIs/simpletest of course).
---
This is a weird name, in the early days of 8.x, it was just named:
drupal_pre_render_html_tag.This is not the same list as 8.8.x.
These overwrite potential
#prefixand#suffixproperties from previous#pre_rendercallbacks (i.e. conditional comments, see D8's implementation).This needs to be removed as it is now handled by the new
#pre_rendercallback.Also,
theme_html_tagneeds to be changed to something like the following:Shouldn't these be
#value_prefixand#value_suffix? These properties aren't handled in 8.x (because it's a proper render array and #weight can be used for child elements), but these will need to be supported in 7.x for BC reasons. I suspect they can just be prepended and appended to #value respectively. Core already handles#prefixand#suffixproperties defined any any renderable element automatically.Comment #56
polThanks Mark,
I updated the patch, fixing most of your comments.
As you said, we can almost copy the doc from D8 and the tests as well, but it's not yet done.
Comment #57
polLooks like I uploaded the wrong patch, going to update that now.
Comment #58
polHere's the good patch with the interdiff against #53.
Comment #59
polComment #60
polChange record is done: https://www.drupal.org/node/2982025
Comment #61
markhalliwellShould be
$element, a#pre_rendercallback is invoked on a single element (regardless if it has children).The reference needs to be changed to
drupal_attributes().The
#valueneeds to be ran throughfilter_xss_admin(), which also means there is a test missing.There should be an underscore (_) between html and tag.
Add the corresponding 8.x comment explaining what void elements are via URLs above this please.
This should only work in non-void elements (else statement above).
I would also argue that these properties should be deprecated and a notice triggered (via watchdog) when they're used informing developers to use child elements in the render array instead.
Thus, both of these need independent
isset()checks.This should also backport the
#noscriptfeature as well as check for existing properties, please just use the same 8.x code:This should indicate when this was deprecated. Also, the last line needs to start with an asterisk, not an inline comment.
Also, at the very least, we should trigger a watchdog deprecation notice here I think?
Not necessary. This is automatically handled by drupal_render().
---
Change record should also mention the deprecation of
theme_html_tag().Comment #62
markhalliwellManually setting
#childrenshouldn't be necessary... that is why#markupis used.Although, that being said... I suspect this is because
#markupworks in quite differently in 8.x compared to its previous 7.x counterpart. (edit: yes, Renderer::doRender() checks for#markupafter#pre_rendercallbacks are invoked and child elements have been rendered and then prepends it to the existing#childrenproperty)We have drupal_pre_render_markup() that can be used as a callback. This essentially does the same thing, but as a separate callback which would allow other
#pre_rendercallbacks to be injected between this process should the need arise.I'm still a bit confused however, as manually setting
#childrento something other than an empty string actually sort circuits drupal_render() from rendering any children though...So I'm not entirely sure how this test is "passing" and seems like it's a false positive (BTW, no short array syntax).
We may have to convert
#value(as well as#value_prefixand#value_suffixrespectively) to child render array elements for this to work properly in 7.x.Comment #63
polMassive feedback from @Mark, thanks!!! Very much appreciated :-)
I've updated the patch, fixing most of the issue I think, I will post it here to trigger the tests.
As discussed on Slack, I added an extra pre_render callback to render the children elements.
As this patch is quite big, I've created a temporary branch on Gitlab, I think it will be easier if we follow the dev on a specific branch on Gitlab.
Interdiff #59 to #62: https://gitlab.com/drupol/drupal/compare/9af4f5892f337160660f339b30c1562...
Comment #64
polSee interdiffs: https://gitlab.com/drupol/drupal/merge_requests/8/commits
Comment #66
markhalliwellSeparate line.
These belong in the else statement above as they should only work on non-void elements (same as #value). This also likely needs a test.
Comments are what should respect the 80 character limit rule. Code (aside from arrays) all belong on one line (this needs to be applied to the two other warnings).
Should wrap at 80 characters.
"Ensure #theme is not set to prevent recursion."
Comment #67
polComments from #66 are fixed.
The commit related to that is here: https://gitlab.com/drupol/drupal/merge_requests/8/diffs?commit_id=63ff7b...
Important remarks
Tests were failing with the previous version of the patch because of entries in the watchdog when clearing the cache. It was quite easy to guess where it was coming from.
The issues were in
drupal_pre_render_styles()anddrupal_pre_render_scripts(), so I made modifications to prevent the use of#value_prefixand#value_suffixand prevent warnings in watchdog (dblog).I also had to do two calls to
render()indrupal_pre_render_html_tag()because contrib modules like devel uses the#prefixand#suffixkeys as render arrays.render()had to be used to convert the render array into a string and I added a condition and watchdog message to stay BC while warning the user of that bad practice.Let me know if it's the proper approach.
See the commit: https://gitlab.com/drupol/drupal/merge_requests/8/diffs?commit_id=c6e016...
Comment #68
markhalliwellChanging the structure of this at this point is a BC break, especially for contrib modules that might modify styles/scripts (i.e. AdvAgg).
Instead, perhaps we should introduce a silencing property that won't trigger the deprecation warnings.
Something like
$element['#ignore_deprecation_warning'] = TRUE;?This allows code that must use it (for BC reasons) to safely bypass the warnings while providing the originally intended functionality.
We can then just wrap the watchdog calls to check for this:
Almost, arrays that have an opening parenthesis past the 80 character limit do get a new line after them (https://www.drupal.org/docs/develop/standards/coding-standards#array).
This should be:
These properties have never allowed arrays which means this is really an issue with devel specifically, not core.
Also, the current
html_tagimplementation does not handle these either,drupal_render()does.So this doesn't belong here. It is completely out of scope of this issue.
This needs the correct code style still.
Now that there's another line of code below this, remove "and then render the element" from the comment. Besides,
drupal_render($variables['element'])is already quite self explanatory.---
Change record also needs to explicitly mention the deprecation of properties as well as
html_tagtheme hook (i.e.theme_html_tag()).Comment #69
polComments from #66 are fixed.
Diff: https://gitlab.com/drupol/drupal/merge_requests/8/diffs?commit_id=96a8cc...
Comment #70
polChange record updated:
Link: https://www.drupal.org/node/2982025
Comment #71
markhalliwellUsing
empty($element['#ignore_deprecation_warning'])as the conditional is preferred as this doesn't require the value to be eplicitlyFALSE(should allow any empty value: 0, NULL, '').Thus, defining this in the element info isn't necessary, nor do we really want to advertise it there.
This is really just an optional property intended mainly for use internally (due to BC reasons only).
Comment #72
polPatch has been updated.
Diff: https://gitlab.com/drupol/drupal/merge_requests/8
Comment #73
markhalliwellLGTM, let's get @Fabianx's review on this now.
Comment #74
fabianx commentedThere is standard way for deprecation and that is to use trigger_error() with E_DEPRECATED, which will never trigger on prod (if properly configured) and always trigger on dev.
I am okay with a kill-switch for that, but core special casing itself but not allowing contrib the same without code changes feels weird.
Have to think about the deprecation part of the patch.
Appreciate the thinking of security, but it's a larger behaviour change.
It could be okay as security hardening, but I would want it in it's own issue.
I assume this is the same in D8 - Why is it needed / wanted?
drupal_render() is perfectly capable of rendering children by itself. Why do we want / need to pre_render them?
This is fine in a way, but should core not just do the same (if it's a simple switch-over) instead of doing so.
Why can't we transparently map '#theme => ...' to '#type' => '...' and just say the behavior changed - if in essence we do that ...
That's not acceptable unfortunately - needs to use trigger_error.
See Drupal 8 and Symfony.
Currently this would lead to even more deprecation warnings (if you use value_prefix and value_suffix right now).
Should we not just leave '#theme' => 'html_tag' alone and maybe put a deprecation warning - if something wants to know that it's deprecated?
In that way we would only improve the #type => 'html_tag'.
A larger problem and that also applies to unset '#theme' and set 'type' instead in the theme_html_tag function is that:
- There is #theme_wrappers that run directly after the children are rendered.
Sorry for spoiling the party, but with my conservative core committer hat on, I have to decline this for this iteration.
We - and I know that sucks process wise - need to add a deprecation policy to Drupal 7 first and also get agreement from the Community that using E_DEPRECATED is okay and maybe rather have a global kill switch for it.
As PHP 7 uses it, I think that should be fine.
As for the approach itself - I like it much in parts, but we need to be careful to not break existing usages. The patch already takes great care of that, but I unfortunately feel that we still have problems with some edge cases.
So sadly setting to CNW (or rather: Approach needs discussion)
Comment #75
joseph.olstadYou guys are doing great work here.
Trying to follow along; E_DEPRECATED sounds like a good idea, as long as it's compatible with the various versions of PHP (or maybe add a conditional based on minimum php version if it helps compatibility or is needed) otherwise a change record suffice if it's only for a far out edge case.
Passing all tests is very encouraging!
Comment #76
fabianx commentedI appreciate all the hard work that did go into this, but I feel a safer way would be to just do:
Even that is a BC break though in a way, because if you added some children somewhere and then converted to type html_tag then it would work (as #value is not set), but as soon as above code would run the void element would suddenly have content. It's a real edge case, but could imagine we break something somewhere.
Yes, the D8 code and backport are way nicer, but it's a hard BC break - too, because if you use "my-custom-element" and expected
"<my-custom-element />"then this would suddenly break as it's not white-listed -- even if invalid.We could introduce a new 'd8_html_tag' though OR conditionally enable the functionality for new installs with old installs having to opt-in to using that.
OR we could enable it per render array, e.g. #type => 'html_tag', '#nested_html_tag' => TRUE,
I know BC is problematic, but I don't think just potentially breaking sites would be good.
Comment #77
donquixote commentedI have not commented on this issue since #49, but I did think about it since.
My current view is we should leave existing render element types and theme hooks unchanged in Drupal 7.
(I already mentioned this to Pol, just never wrote it down in this issue)
The functionality we hope to provide here can easily be achieved in other ways.
The safer solution would be to define a new theme hook and/or element type, instead of changing an existing one.
This was already done in contrib (theme_themekit_container).
We could decide to do something similar in core, if that makes people happy, and someone makes a strong and convincing case why it is needed.
People who already use 'html_tag', be it with '#type' or '#theme' or theme(), want their code to remain valid and the behavior unchanged.
People who need something else can choose the new theme hook or element type.
When designing such a new theme hook and/or element type, the correct mechanism, as already pointed out by Fabianx, is using '#theme_wrappers' for this.
By doing this, Drupal will already do all the work for us, and we don't need additional code to render the children.
In Drupal 7 (and still in Drupal 8 I guess) we have two categories of theme hooks:
So what we try to accomplish here is closer in its behavior to 'container' than to 'html_tag'. The only thing missing in 'container' is a property to specify the tag name.
But this does not mean we should modify the 'container' theme hook or element type.
Imo we should leave both 'html_tag' and 'container' alone.
If we absolutely want this in core we should create something new.
Comment #78
markhalliwelltrigger_error()andE_USER_DEPRECATED(which is typically what's used overE_DEPRECATED). I'm ok with following 8.x here if it actually works... I just thought it wouldn't (granted I didn't really try).#pre_rendercallback instead of modifying drupal_render() as a whole (which could affect more than just this). In hindsight, I think the way this is actually intended to work (even in 8.x) is if#valueis not empty, it doesn't render child elements. IMO, I think this is technically a bug in both 7.x and 8.x. Perhaps #value should just be converted into a #weight neutral child element and#value_prefix=> -1 and#value_suffix=> 1?Then we wouldn't have to worry about#markupat all and just let#childrencarry it?theme(). In fact, that is what is prominately "suggested" in the comments of theme_html_tag(). Despite this being bad practice, it's quite common to see calls totheme(), specifically for the purpose of usinghtml_tag. The fact that there has been both a theme hook and an element is what has led to this confusion (nevermind the performance implications) and why the theme hook was ultimately removed in 8.x.theme()invocation out there and can potentially cause the most peformance issues when doing so. It's almost akin to use#markupeverywhere. Iftheme()weren't invocable directly and developers forced to use the Render API to invoke theme hooks (like 8.x), this would be less of an issue. As it stands now, these calls can cause huge performance hits when used inappropriately (which is quite frequent) because it bypasses the Render API entirely, which of course prevents things like render caching from being used. This, by no means, "forces" developers to start using render arrays... but it at least warns them this theme hook is now deprecated and really shouldn't be used directly (because it was removed in 8.x).#theme => html_tag__nested__suggestion. In which case, simply removing this and then forcing#type => html_tag, this theme hook suggestion would become lost. No. This happens directly in the theme function forhtml_tag. If a [sub/base]theme implements a theme hook suggestion, the preprocess functions would still run prior to this function being invoked. This function is only ever called when we know it is the absolute last function to call (as it is supposed to return a rendered string). This means that if this function were to be invoked, it is done so with the knowledge that it is literally supposed to behtml_tag, regardless of the suggestions prior. That being said, I wouldn't be opposed to perhaps save the theme hook in something like#context][theme_hookprior to unsetting#theme. This way we can at least have some context when it hits the Render API.#76:
I don't like the idea of introducing a new "d8_html_tag" specific element. That wouldn't be a backport.
While, yes, introducing the void element list is technically a BC change from previous behavior, I think we could get away with providing BC support using something like
#void_element => TRUE.This would allow default behavior to function in the backport way of 8.x, while also providing developers a property to manually set (override default behavior) should they need their super special hypothetical custom element to behave as it did before.
Optionally we could, in theory, provide this void element list as a variable that developers could extend. However, I'm not a big fan of this as 1) 8.x does not do this and 2) introduces this list as something developers "should" customize, lending to the idea that "they know better".
Arguably, whether or not there is a closing tag or not for custom elements really boils down to supporting existing custom tests as they would expect very specific markup. Browsers really don't care about this, one way or the other.
The only reason I said what I did in #51.2 about the space before the closing self tag is because this would be a much broader change that would affect all of core/custom/contrib testing code alike.
If a few custom elements are suddenly now not void elements, I would imagine that the tests for these would be far fewer in number and a relatively easy enough of a change to implement.
But these, again, are just tests. This doesn't actually "break" the functionality of real-world production level code interpreted by the browser.
Comment #79
markhalliwellOh, regarding
E_USER_DEPRECATEDI was also under the impression we couldn't use this because these were only introduced in PHP 5.3 and 7.x still technically supports a minimum of PHP 5.2.4.Comment #80
markhalliwellNo, we shouldn't.
Which is what this does. As I stated in #78, the "changes" made here actually do not affect functionality (behavior), just tests. It would also be a very small subset of tests compared to the change I mentioned in #51.2.
Modern browsers do not care one way or the other if a tag self closes or has a dedicated closing tag; this is merely a convention from XHTML.
This does not prevent developers from using
#theme => html_tagortheme('html_tag'). It still functions the same way, but it would let the developers know that it's been deprecated.Which brings me to the next point: this should be about backporting the changes for
html_tagfrom 8.x. This has absolutely nothing to do with containers. They're two intentionally separate elements designed to do different tasks.Except that isn't how
drupal_render()actually works with#markup(try it, step through it).The entire point of backporting the 8.x implementation is to deprecate and stop using the theme hook
html_tagequivalent. There is no#theme_wrappersthan can or should be used here.The
html_tagelement isn't supposed to be "themeable" (theme hook). It's a low level render element intended to create custom markup without actually using#markupspecifically to allow it to remain alterable prior to rendering.This is why the theme hook was removed in 8.x; along with many, many other reasons.
It's also why we tell people to never use
#markupdirectly and instead use and appropritate theme hook (template) or, at the very least,#type => html_tag.---
Overall, the discussion around this topic kind of proves my point: developers don't know what the use case of
html_tagreally is and have been abusing it for years.This change is necessary because it moves 7.x code inline with what is expected in 8.x.
It gives developers the opportunity to easily locate where these "bad practice" implementations are so they can change their code prior to migrating to future versions of Drupal.
This isn't about and should never be about "introducing a new element" so developers can feel better.
This should be about backporting expected behavior for future proofing site migration.
How we deal with BC, while important, is less of an issue.
We can find non-destructive ways to deal with BC without having to introduce entirely new concepts.
---
That all being said, I do think there is still a semi "bug" in that if it's a void element, then it shouldn't have any
#valueor child elements.The patch above probably needs to unset child elements when a void element is detected. The 8.x implementation kind of handles this automatically due to how
#markupoperates, but in 7.x we'll need to be explicit about that indrupal_pre_render_html_tag()before it moves onto the other#pre_rendercallbacks.The way
drupal_render()works in 7.x is not the same as 8.x.This divergence happened with the "safe markup" initiative happened in 8.x and changed... well, everything. The
#markupproperty became an actual "standard" if you will, whereas in 7.x it's mostly there as a backup and for specific cases like this.This is why these extra
#pre_rendercallbacks are necessary, but also so we can separate out the specific "flattening" aspect (if you will) should the need arise where contrib/custom code wishes to interject their own#pre_rendercallback before it does so.Comment #81
markhalliwellI also feel like I need to further clarify why the void element change isn't really that big of a deal.
This will use the example in #76:
<my-custom-element />This means that to get this output in the current implementation, all one really has to do this:
The reason this outputs a "void element" here is that it has no
#valueof any kind.The entire logic of the current implementation is based on whether
#valueis empty or not.That's it.
By changing the logic to detect specific void elements, the output would suddenly become
<my-custom-element></my-custom-element>.Functionality (behaviorally) this is the exact same in the browser. The only difference is for specific custom tests that check for exact string literal output matches. As I explained above, this would likely be far and few in between and a relatively minor change to "fix". Because this only affects custom tests, this "change" is highly worth it.
We also don't have to worry about "child elements" because the never worked before so it is highly unlikely there would be any code out there that actually had this.
Regardless, let's play this out hypothetically.
Current implementation ignores any child element artifacts:
Now, with the patch applied:
This artifact child element actually works correctly because it's placed inside the closing tag.
Whether this is intentional or not (it suddenly shows up) is really up to the developer/site that implemented it.
One could argue that, while technically a "change", it actually serves to highlight code that was originally intended to work this way in the first place (perhaps their theme added child support, in which case this would likely still work the same).
Or maybe it highlights code that should be removed.
Regardless, this is all very hypothetical and likely not happening in any real-world code since children were never supported by
html_tagin the first place. Why would anyone just leave this there when it didn't work in the first place?Note: this would also effectively be the same output it wasn't a child and instead
'#value' => '<strong>child</strong>',(i.e. nothing "breaks").Now let's use
<hr />, current implementation:With patch:
This is the "bug" I mentioned at the end of #80. When it's determined that it should be a void element, we need to prevent all child elements from rendering. We could unset them, but perhaps we should just set
#access => FALSEon them instead. I'm fairly certain this is a bug in 8.x as well... but I haven't really tried it out (this is all hypothetical and likely not happening in real-world code at all).---
Despite all that, we can still add a
#void_elementboolean property to check for (for BC reasons). By default (NULL), it could determine this value base on#tag. If explicitly set to TRUE or FALSE, then it gives control over this back to the developer.Supporting BC does not inherently mean we cannot change default output/behavior. Sure, we try to avoid this at all costs. However, in cases like this (backporting), the benefits outweigh the tiny fraction of developers that just might be affected by this.
All it means is that we have to provide some way for developers to easily get the previous (one could argue "desired") output/behavior.
That's all.
Comment #82
donquixote commented@markcarver (#80)
Perhaps I have my priorities different, and "future proofing" is not that important to me.
Even with the perspective of upgrading to D8, I would still prefer my existing D7 site to remain stable, and then do all the work as a rewrite in D8, instead of having to fix the D7 site after core upgrades.
Or even worse, patch old contrib modules.
The tests only cover a small subset of the permutations that are possible with render arrays.
Perhaps some of these don't make sense, but the developer who did it might no longer be around to be told that.
In the following example the difference would be less subtle.
Then the other example with '#pre_render' in combination with '#type' => 'tag_name'.
I think this is what Fabianx meant above, perhaps in reference to my own comment #40. I think I misunderstood him earlier.
Perhaps there are more cases with funny combinations.
Is this relevant?
With all the existing Drupal 7 sites with adventurous custom code out there, I'd say yes.
Another thing that would no longer work is theme overrides for theme('html_tag').
I personally think it is a really bad idea to target html_tag in your theme, but I am sure that are sites that do it.
To me, in D7, a '#type' => 'container' is a html element around element children but limited to div, whereas '#type' => 'html_tag' allows arbitrary tag name but does not support children. '#type' => 'container' might have uses beyond that, but the basic functionality is just that.
The D8 version of 'html_tag' provides the best of both worlds, but takes away the theme layer - which is good.
In D7 I do the same with 'themekit_container', which provides arbitrary tag name and rendering of children.
I think I am lost. I don't know which behavior you assume that I expect, which behavior you expect, if that is the same or different, and which exact combination we are even talking about :)
If we would want to design a new element where children are rendered and then wrapped into a html tag, then '#theme_wrappers' would do the job.
If we want to skip the theme layer, we could use '#post_render' instead.
However, if the goal is to change the behavior of an existing element type (which it is, if the priority is backporting), then perhaps both methods have their problems.
But this said: I don't know what kind of problem with '#markup' vs '#theme_wrappers' you are trying to point out here. I could only guess what you mean.
I support this sentiment, Theme overrides of 'html_tag' are a bad idea.
As mentioned above I think there is a difference in priorities or philosophies.
Should D7 sites make gradual changes to make the transition to D8 easier? Should they even be nudged to do so?
Or is D8 so far gone that we should rather leave the D7 sites in peace?
Comment #83
donquixote commented@Fabianx (#76)
I find it hard to follow these hypothetical examples. It would help if they were more explicit.
Perhaps we should even add a range of different examples to the issue summary, with the different outputs from different implementations.
Comment #84
markhalliwellPossibly. I see issues like this (and others) where there seems to be an insurgence of attempting to sudden add new features into 7.x.
There's at least two major issues with this:
The only reason this issue has any grounding at all is due to #552478-193: Restrict "self-closing" tags to only void elements in drupal_pre_render_html_tag explicitly stating that a separate backport issue should be used for 7.x.
And that's what this is and should only be: a backport of upstream code.
This is not about arbitrarily fixing unique (entirely custom) use cases or the desire to introduce the ability to suddenly change container tags.
Nor is this about #theme_wrappers. This isn't about the theme system at all, it's about the Render API and fixing a blatant mistake of introducing a theme hook for html_tag when there should have never been one in the first place.
To actually make the backported code function properly (due to the way drupal_render() actually works in 7.x) some additional #pre_render callbacks are necessary. This is all explained above in the reviews/diff (including GitLab).
Not entirely true.
Themes that implement their own
THEMENAME_html_tag()functions would still be picked up by the theme system API and overuse cores default implementation.This is the very foundation of how the theme system works.
If you're referring to the removal of #theme in the element info definition (which was mainly removed for performance reasons), this could be supported via a variable to "add this back in" (for BC purposes) as the core theme function explicitly removes this to prevent recursion. This way any custom implementation could still theoretically work as it did before.
It is, but has been a necessary "evil" since the element has always invoked the theme hook. I know of several sites I have done this personally actually. This is exactly why it was removed in 8.x and should be deprecated and discouraged here now.
---
Please note it is not my intention for this to come across as a personal attack. I'm just quoting for context and reference. I get your desire here, but there is clear precedence with how this should proceed. It's either a complete backport or nothing at all.
I believe we can backport this fairly easily enough with extremely few disruptions.
We can place existing variable toggles in areas we're not 100% sure all use cases would be applied properly.
Again, I will state, the purpose of backporting is not to provide a one click update and your site remains the same. It's to provide BC options for sites that need previous functionality.
Whether you're upgrading core or contrib, you should always read release notes and change records. Then thoroughly test it prior to actually deploying it to prod.
Sometimes, this includes making some very minor changes because your very specific site needs them.
This isn't the case with every site.