Skip to content
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

Add an attr function to make outputting HTML attributes easier #3930

Draft
wants to merge 8 commits into
base: 3.x
Choose a base branch
from
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
96 changes: 96 additions & 0 deletions extra/html-extra/HtmlExtension.php
Original file line number Diff line number Diff line change
Expand Up @@ -12,8 +12,10 @@
namespace Twig\Extra\Html;

use Symfony\Component\Mime\MimeTypes;
use Twig\Environment;
use Twig\Error\RuntimeError;
use Twig\Extension\AbstractExtension;
use Twig\Runtime\EscaperRuntime;
use Twig\TwigFilter;
use Twig\TwigFunction;

Expand All @@ -30,6 +32,7 @@ public function getFilters(): array
{
return [
new TwigFilter('data_uri', [$this, 'dataUri']),
new TwigFilter('html_attr_merge', [self::class, 'htmlAttrMerge']),
];
}

Expand All @@ -38,6 +41,7 @@ public function getFunctions(): array
return [
new TwigFunction('html_classes', [self::class, 'htmlClasses']),
new TwigFunction('html_cva', [self::class, 'htmlCva']),
new TwigFunction('html_attr', [self::class, 'htmlAttr'], ['needs_environment' => true, 'is_safe' => ['html']]),
];
}

Expand Down Expand Up @@ -124,4 +128,96 @@ public static function htmlCva(array|string $base = [], array $variants = [], ar
{
return new Cva($base, $variants, $compoundVariants, $defaultVariant);
}

public static function htmlAttrMerge(...$arrays): array
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can maybe have a @param to better describe $arrays?

{
$result = [];

foreach ($arrays as $argNumber => $array) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not guaranteed that we have a number here.

if (!$array) {
continue;
}

if (!is_iterable($array)) {
throw new RuntimeError(sprintf('The "attr_merge" filter only works with mappings or "Traversable", got "%s" for argument %d.', \gettype($array), $argNumber + 1));
Copy link

@leevigraham leevigraham Oct 22, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this be " The html_attr_merge filter only…" . What if the exception is thrown when using the html_attr function.

}

$array = (array) ($array);

foreach (['data', 'aria'] as $flatOutKey) {
if (!isset($array[$flatOutKey])) {
continue;
}
$values = (array) $array[$flatOutKey];
foreach ($values as $key => $value) {
$result[$flatOutKey.'-'.$key] = $value;
}
unset($array[$flatOutKey]);
}

foreach (['class', 'style'] as $deepMergeKey) {
if (!isset($array[$deepMergeKey])) {
continue;
}

$value = $array[$deepMergeKey];
unset($array[$deepMergeKey]);

if (!is_iterable($value)) {
$value = (array) $value;
}

$result[$deepMergeKey] = [...$result[$deepMergeKey] ?? [], ...$value];
}

$result = [...$result, ...$array];
}

return $result;
}

public static function htmlAttr(Environment $env, ...$args): string
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe a @param for $args?

{
$attr = self::htmlAttrMerge(...$args);

if (isset($attr['class'])) {
$attr['class'] = trim(implode(' ', $attr['class']));
}

if (isset($attr['style'])) {
$style = '';
foreach ($attr['style'] as $name => $value) {
if (is_numeric($name)) {
$style .= $value.'; ';
} else {
$style .= $name.': '.$value.'; ';
}
Comment on lines +190 to +194
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure why this specific case for style.. i may have missed the reason earlier in the discussion ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In case style is something like ['color: red', 'font-weight: bold'].

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have two other questions then (sorry)

What if "aria" is something like ['aria-disabled', 'aria-current'] ?

Does that mean i can do something like this ?

{{ html_attr(
   { style: ['color: red;'] },
   { style: {'color': 'blue'} },
   { style: {'color': 'green'} },
   { style: ['', 'color: black;'] },
) }}

I'm shared between "i understand each of these usages" and "this could be very un-predictable when/if args are assembled from different layers / templates :|

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, definetly many edge cases I haven't thought through. Make sure you don't miss the inital idea in #3907, fwiw.

I think we should aim for behaviour similar to array_merge and not try to do any kind of parsing/interpretation. For example:

  • html_attr_merge( { style: {'color': 'blue'} }, { style: {'color': 'green'} } } ) }} would be { style: {'color': 'green'} }
  • html_attr_merge( { style: ['color: blue'] }, { style: { ['color: green']} } } ) }} would be { style: ['color: blue', 'color: green'] } – if you want "real" overrides, use semantic keys
  • html_attr_merge( { aria: { 'aria-label': 'that' }, {'label': 'this'} } ) would be { 'aria-label': 'this' } – I'd rewrite keys from the aria and data sub-array to the "flat" values first and then merge those.

I hope I can come up with reasonable test cases for all those situations so we can discuss them before merge.

}
$attr['style'] = trim($style);
}

if (isset($attr['data'])) {
foreach ($attr['data'] as $name => $value) {
$attr['data-'.$name] = $value;
}
unset($attr['data']);
}

$result = '';
$runtime = $env->getRuntime(EscaperRuntime::class);

foreach ($attr as $name => $value) {
if ($value === false) {
continue;
}

if ($value === true) {
$value = $name;
}

$result .= $runtime->escape($name, 'html_attr').'="'.$runtime->escape($value).'" ';
mpdude marked this conversation as resolved.
Show resolved Hide resolved
}

return trim($result);
}
}
19 changes: 19 additions & 0 deletions extra/html-extra/Tests/Fixtures/html_attr_merge.test
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
--TEST--
"html_attr_merge" function
--TEMPLATE--
{% autoescape false %}
#1 - {{ { foo: 'bar' } | html_attr_merge({ bar: 'baz' }) | json_encode }}
#2 - {{ { class: 'foo' } | html_attr_merge({ class: 'bar' }) | json_encode }}
#3 - {{ { class: 'foo' } | html_attr_merge({ class: ['bar', 'baz'] }) | json_encode }}
#4 - {{ { class: { special: 'foo' } }
| html_attr_merge({ class: ['bar', 'baz'] })
| html_attr_merge({ class: { special: 'qux' } })
| json_encode }}
{% endautoescape %}
--DATA--
return []
--EXPECT--
#1 - {"foo":"bar","bar":"baz"}
#2 - {"class":["foo","bar"]}
#3 - {"class":["foo","bar","baz"]}
#4 - {"class":{"special":"qux","0":"bar","1":"baz"}}
184 changes: 184 additions & 0 deletions extra/html-extra/Tests/HtmlAttrMergeTest.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,184 @@
<?php

namespace Twig\Extra\Html\Tests;

use PHPUnit\Framework\TestCase;
use Twig\Extra\Html\HtmlExtension;

class HtmlAttrMergeTest extends TestCase
{
/**
* @dataProvider htmlAttrProvider
*/
public function testMerge(array $expected, array $inputs)
{
$result = HtmlExtension::htmlAttrMerge(...$inputs);

self::assertSame($expected, $result);
}

public function htmlAttrProvider(): \Generator
{
yield 'merging different attributes from two arrays' => [
['id' => 'some-id', 'label' => 'some-label'],
[
['id' => 'some-id'],
['label' => 'some-label'],
]
];

yield 'merging different attributes from three arrays' => [
['id' => 'some-id', 'label' => 'some-label', 'role' => 'main'],
[
['id' => 'some-id'],
['label' => 'some-label'],
['role' => 'main'],
]
];

yield 'merging different attributes from Traversables' => [
['id' => 'some-id', 'label' => 'some-label', 'role' => 'main'],
[
new \ArrayIterator(['id' => 'some-id']),
new \ArrayIterator(['label' => 'some-label']),
new \ArrayIterator(['role' => 'main']),
]
];

yield 'later keys override previous ones' => [
['id' => 'other'],
[
['id' => 'this'],
['id' => 'that'],
['id' => 'other'],
]
];

yield 'ignore empty strings or arrays passed as arguments' => [
['some' => 'attribute'],
[
['some' => 'attribute'],
[], // empty array
'', // empty string
]
];

yield 'keep "true" and "false" boolean values' => [
['disabled' => true, 'enabled' => false],
[
['disabled' => true],
['enabled' => false],
]
];

yield 'consolidate values for the "class" key' => [
['class' => ['foo', 'bar', 'baz']],
[
['class' => ['foo']],
['class' => 'bar'], // string, not array
['class' => ['baz']],
]
];

yield 'class values can be overridden when they use names (array keys)' => [
['class' => ['foo', 'bar', 'importance' => 'high']],
[
['class' => 'foo'],
['class' => ['bar', 'importance' => 'low']],
['class' => ['importance' => 'high']],
]
];

yield 'inline style values with numerical keys are merely collected' => [
['style' => ['font-weight: light', 'color: green', 'font-weight: bold']],
[
['style' => ['font-weight: light']],
['style' => ['color: green', 'font-weight: bold']],
]
];

yield 'inline style values can be overridden when they use names (array keys)' => [
['style' => ['font-weight' => 'bold', 'color' => 'red']],
[
['style' => ['font-weight' => 'light']],
['style' => ['color' => 'green', 'font-weight' => 'bold']],
['style' => ['color' => 'red']],
]
];

yield 'no merging happens when mixing numerically indexed inline styles with named ones' => [
['style' => ['color: green', 'color' => 'red']],
[
['style' => ['color: green']],
['style' => ['color' => 'red']],
]
];

yield 'turning aria attributes from array to flat keys' => [
['aria-role' => 'banner'],
[
['aria' => ['role' => 'main']],
['aria' => ['role' => 'banner']],
]
];

yield 'using aria attributes from a sub-array' => [
['aria-role' => 'main', 'aria-label' => 'none'],
[
['aria' => ['role' => 'main', 'label' => 'none']],
]
];

yield 'merging aria attributes, where the array values overrides the flat one' => [
['aria-role' => 'navigation'],
[
['aria-role' => 'main'],
['aria' => ['role' => 'banner']],
['aria' => ['role' => 'navigation']],
]
];

yield 'merging aria attributes, where the flat ones overrides the array' => [
['aria-role' => 'navigation'],
[
['aria' => ['role' => 'main']],
['aria-role' => 'banner'],
['aria-role' => 'navigation'],
]
];

yield 'using data attributes in a sub-array' => [
['data-foo' => 'this', 'data-bar' => 'that'],
[
['data' => ['foo' => 'this']],
['data' => ['bar' => 'that']],
]
];

yield 'turning data attributes from array to flat keys' => [
['data-test' => 'bar'],
[
['data' => ['test' => 'foo']],
['data' => ['test' => 'bar']],
]
];

yield 'merging data attributes, where the array values overrides the flat one' => [
['data-test' => 'baz'],
[
['data-test' => 'foo'],
['data' => ['test' => 'bar']],
['data' => ['test' => 'baz']],
]
];

yield 'merging data attributes, where the flat ones overrides the array' => [
['data-test' => 'baz'],
[
['data' => ['test' => 'foo']],
['data-test' => 'bar'],
['data-test' => 'baz'],
]
];
}
}
Loading