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.

Sign up for GitHub

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

Jump to bottom

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

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

Changes from 1 commit
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
03a5de1
Add an `attr` function to make outputting HTML attributes easier
mpdude Dec 6, 2023
e3a74bd
Support boolean attribute values
mpdude Sep 30, 2024
aef2551
Add a first test POC
mpdude Oct 1, 2024
9722108
Add tests for key merging
mpdude Oct 7, 2024
717f872
Use `[... ]` instead of `array_merge`#
mpdude Oct 7, 2024
09ff3b4
Update extra/html-extra/HtmlExtension.php
mpdude Oct 7, 2024
799fb28
Fix CS
mpdude Oct 7, 2024
0f694fd
Add initial functional test for the `html_attr_merge` filter
mpdude Oct 7, 2024
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
79 changes: 79 additions & 0 deletions extra/html-extra/HtmlExtension.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,8 +12,12 @@
namespace Twig\Extra\Html;

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

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

Expand All @@ -38,6 +43,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 +130,77 @@ 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 arrays or "Traversable", got "%s" for argument %d.', \gettype($array), $argNumber + 1));
mpdude marked this conversation as resolved.
Show resolved Hide resolved
}

$array = CoreExtension::toArray($array);

foreach (['class', 'style', 'data', 'aria'] as $deepMergeKey) {
if (isset($array[$deepMergeKey])) {
$value = $array[$deepMergeKey];
unset($array[$deepMergeKey]);

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

$value = CoreExtension::toArray($value);
mpdude marked this conversation as resolved.
Show resolved Hide resolved

$result[$deepMergeKey] = array_merge($result[$deepMergeKey] ?? [], $value);
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we use [... ] when we can ? As it's slightly more performant than array_merge

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@smnandre Like this? 717f872

}
}

$result = array_merge($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) {
$result .= $runtime->escape($name, 'html_attr').'="'.$runtime->escape($value).'" ';
mpdude marked this conversation as resolved.
Show resolved Hide resolved
}

return trim($result);
}
}