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 3 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
87 changes: 87 additions & 0 deletions extra/html-extra/HtmlExtension.php
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,85 @@ 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) {
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);
}
}
30 changes: 30 additions & 0 deletions extra/html-extra/Tests/HtmlAttrMergeTest.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,30 @@
<?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): void
{
$result = HtmlExtension::htmlAttrMerge(...$inputs);

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

public function htmlAttrProvider(): \Generator
{
yield 'simple test' => [
['id' => 'some-id', 'class' => ['some-class']],
[
['id' => 'some-id'],
['class' => 'some-class'],
]
];
}
}