Repository: twigphp/Twig (php)

Issue:
Trim filter marks string as unsafe; it is not HTML-aware
I was expecting to be able to pass a safe string of HTML through `trim` and have it still marked as safe. However, as far as I can tell, the `trim` filter is returning a plain string, and Twig no longer treats it as safe HTML.

This means I cannot use the same code/macro to trim an input string which could be either safe `\Twig_Markup` or an unsafe string, and have a predictable output.

Consider this. It's at https://twigfiddle.com/hpnf8v

```twig
{% set myhtml %}
  Here is<br>my HTML  
{% endset %}

{% set myunsafestring = " i <3 u " %}

Plain output (fine, but I want it trimmed):
{{ myhtml }}
{{ myunsafestring }}

Raw (bad):
{{ myhtml | raw }}
{{ myunsafestring | raw }}

Trimmed (bad):
{{ myhtml | trim }}
{{ myunsafestring | trim }}

Trimmed and raw (bad):
{{ myhtml | trim | raw }}
{{ myunsafestring | trim | raw }}
```

- When passed through `raw`, the HTML is fine but not trimmed and the unsafe string breaks my markup.
- When passed through `trim`, the HTML is now trimmed but escaped (it should not be escaped), and the unsafe string is fine (escaped and trimmed)
- When passed through `trim` then `raw`, the HTML is trimmed and fine but the unsafe string breaks my markup.

I see this as a bug, if `\Twig_Markup` is meant to be a first-class data type. The built-in filters should be aware of it.

Pull request:
Improve how trim behaves
Closes #3651

Patch:
diff --git a/doc/filters/escape.rst b/doc/filters/escape.rst
index cf1ad61e..574724e3 100644
--- a/doc/filters/escape.rst
+++ b/doc/filters/escape.rst
@@ -39,8 +39,7 @@ And here is how to escape variables included in JavaScript code:
 The ``escape`` filter supports the following escaping strategies for HTML
 documents:
 
-* ``html``: escapes a string for the **HTML body** context, 
-  or for HTML attributes values **inside quotes**.
+* ``html``: escapes a string for the **HTML body** context.
 
 * ``js``: escapes a string for the **JavaScript** context.
 
@@ -51,8 +50,7 @@ documents:
 * ``url``: escapes a string for the **URI or parameter** contexts. This should
   not be used to escape an entire URI; only a subcomponent being inserted.
 
-* ``html_attr``: escapes a string for the **HTML attribute** context, 
-  **without quotes** around HTML attribute values.
+* ``html_attr``: escapes a string for the **HTML attribute** context.
 
 Note that doing contextual escaping in HTML documents is hard and choosing the
 right escaping strategy depends on a lot of factors. Please, read related
@@ -92,27 +90,6 @@ to learn more about this topic.
             {{ var|escape(strategy)|raw }} {# won't be double-escaped #}
         {% endautoescape %}
 
-.. tip::
-
-    The ``html_attr`` escaping strategy can be useful when you need to 
-    escape a **dynamic HTML attribute name**:
-
-    .. code-block:: html+twig
-
-        <p {{ your_html_attr|e('html_attr') }}="attribute value">
-    
-    It can also be used for escaping a **dynamic HTML attribute value** 
-    if it is not quoted, but this is **less performant**. 
-    Instead, it is recommended to quote the HTML attribute value and use 
-    the ``html`` escaping strategy:
-
-    .. code-block:: html+twig
-
-        <p data-content="{{ content|e('html') }}">
-
-        {# is equivalent to, but is less performant #}
-        <p data-content={{ content|e('html_attr') }}>
-
 Custom Escapers
 ---------------
 
diff --git a/src/AbstractTwigCallable.php b/src/AbstractTwigCallable.php
index d85f0f86..2e9b34d1 100644
--- a/src/AbstractTwigCallable.php
+++ b/src/AbstractTwigCallable.php
@@ -104,9 +104,6 @@ abstract class AbstractTwigCallable implements TwigCallableInterface
         return $this->options['needs_context'];
     }
 
-    /**
-     * @return static
-     */
     public function withDynamicArguments(string $name, string $dynamicName, array $arguments): self
     {
         $new = clone $this;
diff --git a/src/ExpressionParser.php b/src/ExpressionParser.php
index d09cfbb4..07786eea 100644
--- a/src/ExpressionParser.php
+++ b/src/ExpressionParser.php
@@ -319,7 +319,7 @@ class ExpressionParser
     }
 
     /**
-     * @deprecated since Twig 3.11, use parseSequenceExpression() instead
+     * @deprecated since 3.11, use parseSequenceExpression() instead
      */
     public function parseArrayExpression()
     {
@@ -360,7 +360,7 @@ class ExpressionParser
     }
 
     /**
-     * @deprecated since Twig 3.11, use parseMappingExpression() instead
+     * @deprecated since 3.11, use parseMappingExpression() instead
      */
     public function parseHashExpression()
     {
diff --git a/src/Extension/CoreExtension.php b/src/Extension/CoreExtension.php
index 59937aec..f1488b59 100644
--- a/src/Extension/CoreExtension.php
+++ b/src/Extension/CoreExtension.php
@@ -93,6 +93,8 @@ use Twig\Util\CallableArgumentsExtractor;
 
 final class CoreExtension extends AbstractExtension
 {
+    private const DEFAULT_TRIM_CHARS = " \t\n\r\0\x0B";
+
     private $dateFormats = ['F j, Y H:i', '%d days'];
     private $numberFormat = [0, '.', ','];
     private $timezone = null;
@@ -443,9 +445,9 @@ final class CoreExtension extends AbstractExtension
      *
      *   {{ post.published_at|date("m/d/Y") }}
      *
-     * @param \DateTimeInterface|\DateInterval|string|int|null $date     A date, a timestamp or null to use the current time
-     * @param string|null                                      $format   The target format, null to use the default
-     * @param \DateTimeZone|string|false|null                  $timezone The target timezone, null to use the default, false to leave unchanged
+     * @param \DateTimeInterface|\DateInterval|string $date     A date
+     * @param string|null                             $format   The target format, null to use the default
+     * @param \DateTimeZone|string|false|null         $timezone The target timezone, null to use the default, false to leave unchanged
      */
     public function formatDate($date, $format = null, $timezone = null): string
     {
@@ -466,8 +468,8 @@ final class CoreExtension extends AbstractExtension
      *
      *   {{ post.published_at|date_modify("-1day")|date("m/d/Y") }}
      *
-     * @param \DateTimeInterface|string|int|null $date     A date, a timestamp or null to use the current time
-     * @param string                             $modifier A modifier string
+     * @param \DateTimeInterface|string $date     A date
+     * @param string                    $modifier A modifier string
      *
      * @return \DateTime|\DateTimeImmutable
      *
@@ -506,8 +508,8 @@ final class CoreExtension extends AbstractExtension
      *      {# do something #}
      *    {% endif %}
      *
-     * @param \DateTimeInterface|string|int|null  $date     A date, a timestamp or null to use the current time
-     * @param \DateTimeZone|string|false|null     $timezone The target timezone, null to use the default, false to leave unchanged
+     * @param \DateTimeInterface|string|null  $date     A date or null to use the current time
+     * @param \DateTimeZone|string|false|null $timezone The target timezone, null to use the default, false to leave unchanged
      *
      * @return \DateTime|\DateTimeImmutable
      */
@@ -1116,30 +1118,29 @@ final class CoreExtension extends AbstractExtension
     /**
      * Returns a trimmed string.
      *
-     * @param string|null $string
-     * @param string|null $characterMask
-     * @param string      $side
+     * @param string|\Stringable|null $string
+     * @param string|null             $characterMask
+     * @param string                  $side          left, right, or both
      *
-     * @throws RuntimeError When an invalid trimming side is used (not a string or not 'left', 'right', or 'both')
+     * @throws RuntimeError When an invalid trimming side is used
      *
      * @internal
      */
-    public static function trim($string, $characterMask = null, $side = 'both'): string
+    public static function trim($string, $characterMask = null, $side = 'both'): string|\Stringable
     {
         if (null === $characterMask) {
-            $characterMask = " \t\n\r\0\x0B";
+            $characterMask = self::DEFAULT_TRIM_CHARS;
         }
 
-        switch ($side) {
-            case 'both':
-                return trim($string ?? '', $characterMask);
-            case 'left':
-                return ltrim($string ?? '', $characterMask);
-            case 'right':
-                return rtrim($string ?? '', $characterMask);
-            default:
-                throw new RuntimeError('Trimming side must be "left", "right" or "both".');
-        }
+        $trimmed = match ($side) {
+            'both' => trim($string ?? '', $characterMask),
+            'left' => ltrim($string ?? '', $characterMask),
+            'right' => rtrim($string ?? '', $characterMask),
+            default => throw new RuntimeError('Trimming side must be "left", "right" or "both".'),
+        };
+
+        // trimming a safe string with the default character mask always returns a safe string (independently of the context)
+        return $string instanceof Markup && self::DEFAULT_TRIM_CHARS === $characterMask ? new Markup($trimmed, $string->getCharset()) : $trimmed;
     }
 
     /**
diff --git a/src/Loader/FilesystemLoader.php b/src/Loader/FilesystemLoader.php
index cad163ea..c60964f5 100644
--- a/src/Loader/FilesystemLoader.php
+++ b/src/Loader/FilesystemLoader.php
@@ -24,9 +24,6 @@ class FilesystemLoader implements LoaderInterface
     /** Identifier of the main namespace. */
     public const MAIN_NAMESPACE = '__main__';
 
-    /**
-     * @var array<string, list<string>>
-     */
     protected $paths = [];
     protected $cache = [];
     protected $errorCache = [];
@@ -34,7 +31,7 @@ class FilesystemLoader implements LoaderInterface
     private $rootPath;
 
     /**
-     * @param string|string[] $paths    A path or an array of paths where to look for templates
+     * @param string|array $paths    A path or an array of paths where to look for templates
      * @param string|null  $rootPath The root path common to all relative paths (null for getcwd())
      */
     public function __construct($paths = [], ?string $rootPath = null)
@@ -51,8 +48,6 @@ class FilesystemLoader implements LoaderInterface
 
     /**
      * Returns the paths to the templates.
-     *
-     * @return list<string>
      */
     public function getPaths(string $namespace = self::MAIN_NAMESPACE): array
     {
@@ -63,8 +58,6 @@ class FilesystemLoader implements LoaderInterface
      * Returns the path namespaces.
      *
      * The main namespace is always defined.
-     *
-     * @return list<string>
      */
     public function getNamespaces(): array
     {
@@ -72,7 +65,7 @@ class FilesystemLoader implements LoaderInterface
     }
 
     /**
-     * @param string|string[] $paths A path or an array of paths where to look for templates
+     * @param string|array $paths A path or an array of paths where to look for templates
      */
     public function setPaths($paths, string $namespace = self::MAIN_NAMESPACE): void
     {
diff --git a/src/Markup.php b/src/Markup.php
index 4fae779e..c7aa65bd 100644
--- a/src/Markup.php
+++ b/src/Markup.php
@@ -32,6 +32,11 @@ class Markup implements \Countable, \JsonSerializable, \Stringable
         return $this->content;
     }
 
+    public function getCharset(): string
+    {
+        return $this->charset;
+    }
+
     /**
      * @return int
      */
diff --git a/src/Node/Expression/Binary/MatchesBinary.php b/src/Node/Expression/Binary/MatchesBinary.php
index ba25313c..4669044e 100644
--- a/src/Node/Expression/Binary/MatchesBinary.php
+++ b/src/Node/Expression/Binary/MatchesBinary.php
@@ -12,27 +12,9 @@
 namespace Twig\Node\Expression\Binary;
 
 use Twig\Compiler;
-use Twig\Error\SyntaxError;
-use Twig\Node\Node;
-use Twig\Node\Expression\ConstantExpression;
 
 class MatchesBinary extends AbstractBinary
 {
-    public function __construct(Node $left, Node $right, int $lineno)
-    {
-        if ($right instanceof ConstantExpression) {
-            $regexp = $right->getAttribute('value');
-            set_error_handler(static fn ($t, $m) => throw new SyntaxError(\sprintf('Regexp "%s" passed to "matches" is not valid: %s.', $regexp, substr($m, 14)), $lineno));
-            try {
-                preg_match($regexp, '');
-            } finally {
-                restore_error_handler();
-            }
-        }
-
-        parent::__construct($left, $right, $lineno);
-    }
-
     public function compile(Compiler $compiler): void
     {
         $compiler
diff --git a/src/Node/Expression/CallExpression.php b/src/Node/Expression/CallExpression.php
index 8e999c7e..920f65ca 100644
--- a/src/Node/Expression/CallExpression.php
+++ b/src/Node/Expression/CallExpression.php
@@ -128,7 +128,7 @@ abstract class CallExpression extends AbstractExpression
     }
 
     /**
-     * @deprecated since Twig 3.12, use Twig\Util\CallableArgumentsExtractor::getArguments() instead
+     * @deprecated since 3.12, use Twig\Util\CallableArgumentsExtractor::getArguments() instead
      */
     protected function getArguments($callable, $arguments)
     {
@@ -258,7 +258,7 @@ abstract class CallExpression extends AbstractExpression
     }
 
     /**
-     * @deprecated since Twig 3.12
+     * @deprecated since 3.12
      */
     protected function normalizeName(string $name): string
     {
@@ -334,7 +334,7 @@ abstract class CallExpression extends AbstractExpression
                 [
                     'is_variadic' => $this->hasAttribute('is_variadic') ? $this->getAttribute('is_variadic') : $current->isVariadic(),
                 ],
-            ))->withDynamicArguments($this->getAttribute('name'), $this->hasAttribute('dynamic_name') ? $this->getAttribute('dynamic_name') : $current->getDynamicName(), $this->hasAttribute('arguments') ? $this->getAttribute('arguments') : $current->getArguments()),
+            ))->withDynamicArguments($this->getAttribute('name'), $this->hasAttribute('dynamic_name') ? $this->getAttribute('dynamic_name') : $current->getDynamicName(), $this->hasAttribute('arguments') ?: $current->getArguments()),
             'function' => (new TwigFunction(
                 $this->hasAttribute('name') ? $this->getAttribute('name') : $current->getName(),
                 $this->hasAttribute('callable') ? $this->getAttribute('callable') : $current->getCallable(),
@@ -344,7 +344,7 @@ abstract class CallExpression extends AbstractExpression
                     'needs_charset' => $this->hasAttribute('needs_charset') ? $this->getAttribute('needs_charset') : $current->needsCharset(),
                     'is_variadic' => $this->hasAttribute('is_variadic') ? $this->getAttribute('is_variadic') : $current->isVariadic(),
                 ],
-            ))->withDynamicArguments($this->getAttribute('name'), $this->hasAttribute('dynamic_name') ? $this->getAttribute('dynamic_name') : $current->getDynamicName(), $this->hasAttribute('arguments') ? $this->getAttribute('arguments') : $current->getArguments()),
+            ))->withDynamicArguments($this->getAttribute('name'), $this->hasAttribute('dynamic_name') ? $this->getAttribute('dynamic_name') : $current->getDynamicName(), $this->hasAttribute('arguments') ?: $current->getArguments()),
             'filter' => (new TwigFilter(
                 $this->getAttribute('name'),
                 $this->hasAttribute('callable') ? $this->getAttribute('callable') : $current->getCallable(),
@@ -354,7 +354,7 @@ abstract class CallExpression extends AbstractExpression
                     'needs_charset' => $this->hasAttribute('needs_charset') ? $this->getAttribute('needs_charset') : $current->needsCharset(),
                     'is_variadic' => $this->hasAttribute('is_variadic') ? $this->getAttribute('is_variadic') : $current->isVariadic(),
                 ],
-            ))->withDynamicArguments($this->getAttribute('name'), $this->hasAttribute('dynamic_name') ? $this->getAttribute('dynamic_name') : $current->getDynamicName(), $this->hasAttribute('arguments') ? $this->getAttribute('arguments') : $current->getArguments()),
+            ))->withDynamicArguments($this->getAttribute('name'), $this->hasAttribute('dynamic_name') ? $this->getAttribute('dynamic_name') : $current->getDynamicName(), $this->hasAttribute('arguments') ?: $current->getArguments()),
         });
 
         return $this->getAttribute('twig_callable');


Test patch:
diff --git a/src/Test/IntegrationTestCase.php b/src/Test/IntegrationTestCase.php
index b66c17fd..67c16809 100644
--- a/src/Test/IntegrationTestCase.php
+++ b/src/Test/IntegrationTestCase.php
@@ -245,7 +245,7 @@ abstract class IntegrationTestCase extends TestCase
                 $output = trim($template->render(eval($match[1].';')), "\n ");
             } catch (\Exception $e) {
                 if (false !== $exception) {
-                    $this->assertStringMatchesFormat(trim($exception), trim(\sprintf('%s: %s', \get_class($e), $e->getMessage())));
+                    $this->assertSame(trim($exception), trim(\sprintf('%s: %s', \get_class($e), $e->getMessage())));
 
                     return;
                 }
diff --git a/tests/Fixtures/expressions/matches.test b/tests/Fixtures/expressions/matches.test
index 843e5e89..8f5e3669 100644
--- a/tests/Fixtures/expressions/matches.test
+++ b/tests/Fixtures/expressions/matches.test
@@ -2,19 +2,13 @@
 Twig supports the "matches" operator
 --TEMPLATE--
 {{ 'foo' matches '/o/' ? 'OK' : 'KO' }}
-{{ 'foo' matches '/o/'|lower ? 'OK' : 'KO' }}
 {{ 'foo' matches '/^fo/' ? 'OK' : 'KO' }}
-{{ 'foo' matches '/^' ~ 'fo/' ? 'OK' : 'KO' }}
 {{ 'foo' matches '/O/i' ? 'OK' : 'KO' }}
 {{ null matches '/o/' }}
-{{ markup matches '/test/' ? 'OK': 'KO' }}
 --DATA--
-return ['markup' => new \Twig\Markup('test', 'UTF-8')]
+return []
 --EXPECT--
 OK
 OK
 OK
-OK
-OK
 0
-OK
diff --git a/tests/Fixtures/expressions/matches_error_compilation.test b/tests/Fixtures/expressions/matches_error.test
similarity index 53%
rename from tests/Fixtures/expressions/matches_error_compilation.test
rename to tests/Fixtures/expressions/matches_error.test
index c251be13..1220eb42 100644
--- a/tests/Fixtures/expressions/matches_error_compilation.test
+++ b/tests/Fixtures/expressions/matches_error.test
@@ -5,4 +5,4 @@ Twig supports the "matches" operator with a great error message
 --DATA--
 return []
 --EXCEPTION--
-Twig\Error\SyntaxError: Regexp "/o" passed to "matches" is not valid: No ending delimiter '/' found in "index.twig" at line 2.
+Twig\Error\RuntimeError: Regexp "/o" passed to "matches" is not valid: No ending delimiter '/' found in "index.twig" at line 2
diff --git a/tests/Fixtures/expressions/matches_error_runtime.test b/tests/Fixtures/expressions/matches_error_runtime.test
deleted file mode 100644
index 4a2bb594..00000000
--- a/tests/Fixtures/expressions/matches_error_runtime.test
+++ /dev/null
@@ -1,8 +0,0 @@
---TEST--
-Twig supports the "matches" operator with a great error message
---TEMPLATE--
-{{ 'foo' matches 1 + 2 }}
---DATA--
-return []
---EXCEPTION--
-Twig\Error\RuntimeError: Regexp "3" passed to "matches" is not valid: Delimiter must not be alphanumeric%sbackslash%sin "index.twig" at line 2
diff --git a/tests/Fixtures/filters/trim.test b/tests/Fixtures/filters/trim.test
index 141f8635..c8d10c52 100644
--- a/tests/Fixtures/filters/trim.test
+++ b/tests/Fixtures/filters/trim.test
@@ -4,11 +4,11 @@
 {{ "  I like Twig.  "|trim }}
 {{ text|trim }}
 {{ "  foo/"|trim("/") }}
-{{ "xxxI like Twig.xxx"|trim(character_mask="x", side="left") }}
-{{ "xxxI like Twig.xxx"|trim(side="right", character_mask="x") }}
+{{ "xxxI like Twig.xxx"|trim(character_mask: "x", side: "left") }}
+{{ "xxxI like Twig.xxx"|trim(side: "right", character_mask: "x") }}
 {{ "xxxI like Twig.xxx"|trim("x", "right") }}
 {{ "/  foo/"|trim("/", "left") }}
-{{ "/  foo/"|trim(character_mask="/", side="left") }}
+{{ "/  foo/"|trim(character_mask: "/", side: "left") }}
 {{ "  do nothing.  "|trim("", "right") }}
 *{{ ""|trim }}*
 *{{ ""|trim("", "left") }}*
@@ -16,6 +16,14 @@
 *{{ null|trim }}*
 *{{ null|trim("", "left") }}*
 *{{ null|trim("", "right") }}*
+
+{% set myhtml %}
+  Here is<br>my HTML  
+{% endset %}
+{% set myunsafestring = " I <3 u " %}
+{{ myhtml | trim }}
+{{ myunsafestring | trim }}
+{{ myhtml | trim(character_mask: "f") }}
 --DATA--
 return ['text' => "  If you have some <strong>HTML</strong> it will be escaped.  "]
 --EXPECT--
@@ -34,3 +42,7 @@ xxxI like Twig.
 **
 **
 **
+
+Here is<br>my HTML
+I &lt;3 u
+  Here is&lt;br&gt;my HTML