-
-
Notifications
You must be signed in to change notification settings - Fork 1.3k
RFC / WIP - html_attributes function #4405
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: 3.x
Are you sure you want to change the base?
Conversation
35668a6 to
bcba2bc
Compare
ab96d70 to
93fadaa
Compare
html_attributes function
html_attributes function
|
|
I suppose that the implementation of |
|
@fabpot Yep it could do. I still haven't fully considered the twig methods yet. Given the existing static methods the public static function htmlClasses(...$args): string
{
$attributes = HtmlAttributes::merge(['class' => $args]);
return HtmlAttributes::renderAttributes($attributes);
}I'm also interested in your (and @stof) opinion on merging vs replacing on some attributes. I thought using the
|
| if (is_int($k)) { | ||
| $styles = array_filter(explode(';', $v)); | ||
| foreach ($styles as $style) { | ||
| $style = explode(':', $style); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is broken in case CSS values contains : (which can happen, for instance in the if function or in strings)
| $value = (array)$value; | ||
| foreach ($value as $k => $v) { | ||
| if (is_int($k)) { | ||
| $styles = array_filter(explode(';', $v)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is broken if a CSS string contains a ;. You cannot perform CSS parsing based on naive explode calls. The CSS syntax is not meant to allow that.
| // data and aria arrays are expanded into data-* and aria-* attributes | ||
| $expanded = []; | ||
| foreach ($attributes as $key => $value) { | ||
| if (in_array($key, ['data', 'aria'])) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should we really add such support ? Right now, those data and aria keys will be merged with the explicit aria-* and data-* keys, which can be a confusing behavior. I'm not sure this special case is worth it.
| // Handle class, style, data-controller value coercion | ||
| // array[] -> concatenate string | ||
| if (in_array($key, ['class', 'style', 'data-controller'])) { | ||
| $value = array_filter($value); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what if the value is not an array there ?
| } elseif ($value === false) { | ||
| $value = 'false'; | ||
| } elseif(is_array($value)) { | ||
| $value = join(" ", array_filter($value)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| $value = join(" ", array_filter($value)); | |
| $value = implode(' ', array_filter($value)); |
| } | ||
|
|
||
| // Everything else gets added as an encoded value | ||
| $return[] = $key . '="' . htmlspecialchars($value) . '"'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
keys must be escaped as well (and with the html_attr strategy)
| * @see ./Tests/HtmlAttributesTest.php for usage examples | ||
| * | ||
| */ | ||
| public static function merge(...$attributeGroup): array |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this signature does not correspond to the PR description.
no it cannot. |

An alternative implementation to #3930
Addresses:
This WIP pr demonstrates a html attribute merging strategy for html attributes, aria-attributes with special handling for
class,styledataandariaattributes.I've currently focussed on the
HtmlAttributes::mergefunction which returns the merged attributes array.Examples
See
./Tests/HtmlAttributesTest.phpfor usage examplesThe return value of
HtmlAttributes::mergeshould also be able to be used as an input forHtmlAttributes::mergeas demonstrated here:https://github.com/twigphp/Twig/pull/4405/files#diff-210291f849102679e6c0a1050d5bcd3076cf55b3cc9677c20fab26dcd15f543cR27-R75
Rendering Attributes
The
HtmlAttributes::renderAttributesmethod that takes the output ofHtmlAttributes::mergeand creates the attribute string.This method:
nullattribute valuesclass,styleanddata-controllerattribute valuesaria-*boolean attribute values to'true''false'strings.data-*array valuesfalsevalues (seearia-*coercion above)trueboolean values./Tests/HtmlAttributesTest.phpdemonstrate the return value ofHtmlAttributes::mergeandHtmlAttributes::renderAttributesThere was some consideration made to coerce
data-*boolean values to string'true'and'false'but this was decided against. StimulusJs uses the following conditional to determine if adata-*-valueattribute istrueorfalsewhen internally coercing to a javascriptBoolean.Illustrated here: https://codepen.io/leevigraham/pen/MWNOyLr
Challenges
The challenge with a
html_attributeslike function is that the merging strategy is arbitrary.classandstyleattributes are usually merged together. Other attributes override the previous values.Symfony UX twig components recommend Stimulus for interaction. Stimulus uses
data-controllerattributes for functionality. Thedata-controllervalue can be a space delimited list of strings. In this case should the multipledata-controllervalues be merged or overridden? For StimulusJs also applies todata-targetanddata-actionAlternative implementation ideas
Merge strategy options
Given the challenges with
data-controllerabove maybe the method should take 2 arguments:In the example above the
$optionsargument would be used to determine which values to merge. Other values would replace.Given the return value of
HtmlAttributes::mergecan also be used as the$attributesargument ofHtmlAttributes::mergethe developer could callHtmlAttributes::mergemultiple times which would be the equivalent of multiple attribute arrays / argument unpacking.References in other platforms / frameworks:
Yii2 has a similar function: https://github.com/yiisoft/yii2/blob/master/framework/helpers/BaseHtml.php#L1966-L2046
The renderTagAttributes method has the following rules:
'aria' => ['role' => 'checkbox', 'value' => 'true']would be rendered asaria-role="checkbox" aria-value="true".'data' => ['params' => ['id' => 1, 'name' => 'yii']]would be rendered asdata-params='{"id":1,"name":"yii"}'.CraftCMS uses twig and provides an attr() twig method that implements renderTagAttributes. I've used this helper many times and the rules above are great. Especially the "Attributes whose values are null will not be rendered".
Vuejs v2 -> v3 also went through some changes for false values https://v3-migration.vuejs.org/breaking-changes/attribute-coercion.html. This aligns with "Attributes whose values are null will not be rendered." above.
TODO