Twig functions becoming internal

[cs278]

Since 3.9 it appears all of Twigs filter/functions underlying PHP implementations are marked as internal, could this be reconsidered in some cases? I'm assuming internal means the signature might change in non backwards compatible ways, if it doesn't I guess this isn't really a problem.

The two cases that affect myself are twig_escape_filter and twig_date_converter.

For example I have a filter which generates some HTML:

new TwigFilter('phone_number_link', function (Environment $env, $number) {
    $anchor = $this->libphonenumber->format($number, PhoneNumberFormat::RFC3966);

    return sprintf(
        '<a href="%s">%s</a>',
        \twig_escape_filter($env, $anchor, 'html_attr'),
        \twig_escape_filter($env, $this->formatDisplay($number), 'html'),
    );
}, ['needs_environment' => true, 'is_safe' => ['html']]),

I've consulted the documentation but it doesn't seem to offer any examples on how to deal with more complicated situations such as this, so perhaps this is the wrong way of doing it but it's worked since Twig 1.x.

Likewise with dates I have some custom filters to limit developers to a limited set of predefined formats, the filters look a bit like this:

new TwigFilter('date_formal_format', function (Environment $env, $date, ?string $locale = null) {
    if (null === $date) {
        return null;
    }

    return DateFormat::formalDate(twig_date_converter($env, $date), $locale ?? $this->locale);
}, ['needs_environment' => true]),

Being able to use twig_date_converter means the Twig timezone setting works as expected.

I did some searching on GitHub and found numerous other cases.

https://github.com/theoboldt/juvem/blob/98af699c95c46e919dc1690ab1730754ebd65141/app/src/AppBundle/Twig/Extension/CustomFieldValue.php#L244
https://github.com/njh/twig-html-helpers/blob/d3dc45f6a9ac67512126c02a8d11c7a09889d942/lib/Twig/Extension/HTMLHelpers.php#L53
https://github.com/symfony/ux/blob/d4df61465571381ffa8692268fca6acd09b32feb/src/StimulusBundle/src/Dto/StimulusAttributes.php#L223
https://github.com/symfony/ux/blob/d4df61465571381ffa8692268fca6acd09b32feb/src/LiveComponent/src/Util/LiveAttributesCollection.php#L121
https://github.com/studiometa/twig-toolkit/blob/9e73108d84c11171ef69fd34dd8946afee8e9a5c/src/Helpers/Html.php#L209
https://github.com/mautic/mautic/blob/175ceac9098f0b9515f34820a0eafd924877864b/plugins/MauticFocusBundle/Model/FocusModel.php#L183
https://github.com/symfony/symfony/blob/a2d03c548c2e897be78504fd4a389bb7eb41e756/src/Symfony/Bundle/WebProfilerBundle/Twig/WebProfilerExtension.php#L116
https://github.com/drupal/drupal/blob/449f6fbf7c058988ba6f18a8e70cba4bf6639941/core/lib/Drupal/Core/Template/TwigExtension.php#L464
https://github.com/phpbb/phpbb/blob/4c721de243967ed4cf372caef4e54142ee6bcf6c/phpBB/phpbb/template/twig/extension.php#L215
https://github.com/IISHF/web2019/blob/0ca849abf7f247abfcf01521b6cdeee22e1d637b/src/Infrastructure/Twig/DateExtension.php#L108-L115
https://github.com/imarc/cms/blob/ef349e03202628638247a71158ff04c49da2068a/src/web/twig/Extension.php#L482
https://github.com/twigphp/intl-extra/blob/00cd46d7860cf7e814f73b290edf32801a47cafc/IntlExtension.php#L282

[ericmorand]

Just my two cents here: that those functions became internal is perfectly legit; that they became internal in the context of a minor version is very controversial and requires some explanation.

[fabpot]

They are deprecated, but still here, that's the way we work for Symfony and Twig. We first deprecate so that you've time to upgrade to the new way, and we then remove it in the next major. twig_date_converter was missing but it was fixed, so the latest version of 3.9 should work fine now.

[ericmorand]

Thanks for the clarification. I stand corrected. 🙏

[cs278]

Perhaps I made this unclear by using the old names, I understand they have been deprecated and there are replacements, my concern is those replacements are marked with @internal annotations. As an example twig_escape_filter() is replaced with:

Twig/src/Extension/EscaperExtension.php

Lines 170 to 182 in ad93431

	/**
	* Escapes a string.
	*
	* @param mixed $string The value to be escaped
	* @param string $strategy The escaping strategy
	* @param string $charset The charset
	* @param bool $autoescape Whether the function is called by the auto-escaping feature (true) or by the developer (false)
	*
	* @return string|Markup
	*
	* @internal
	*/
	public static function escape(Environment $env, $string, $strategy = 'html', $charset = null, $autoescape = false)

It seems crazy to me that I'm not supposed to use Twig to escape content I generate in a custom Twig extension.

[ericmorand]

@cs278 I totally agree. A method that was not marked as internal - and thus that was implicitly public - disappeared and it's replacement was marked internal, all of this during a minor version change.

[stof]

The non-internal functions did not disappear. They are marked as deprecated, which means they still work until Twig 4.0.

And the new internal methods are not meant to be replacements for public usages of those functions. The replacement is to stop using Twig internal APIs directly.

[cs278]

And the new internal methods are not meant to be replacements for public usages of those functions. The replacement is to stop using Twig internal APIs directly.

So what is the migration path then? Your reply seems to suggest there is a different way of calling these from an extension but I cannot find anything in the documentation or changelog, clearly I'm not alone in missing this knowledge:

https://github.com/symfony/ux/blob/d4df61465571381ffa8692268fca6acd09b32feb/src/LiveComponent/src/Util/LiveAttributesCollection.php#L115-L123

[diffy0712]

Hello,

Regarding the 3.9 changes I got a problem where a publicly available function twig_get_attribute was removed from the public api breaking the yiisoft/yii2-twig package when upgrading above 'twig:3.9'. There is an open issue in the yiisoft/yii2-twig repo for this. This function is called at this line. I found that the function was removed in this pr.

I think this breaking change should at least be mentioned in the changelog and being in a mayor release.
This might just be some unfortunate mistake, but should be addressed so it might not break others relying on it.

Am I missing something on this?

Thank you

[xabbuh]

I think this breaking change should at least be mentioned in the changelog and being in a mayor release.

twig_get_attribute() was an internal function which was never meant to be called from third-party packages. That's why it could be removed in a minor release and why the change was also not listed in the changelog.

[diffy0712]

I see, that is completely reasonable. Unfortunately the yiisoft/yii2-twig used it and by removing it, it broke that package, which is an issue, but it should be handled there and nothing to do with twig.
Thank you for your reply :)

[fabpot]

Thank you all for the feedback.

One option would be to review all the new internal methods and determine which ones should be part of the public API because they are useful when creating Twig extensions. For instance, the EscaperExtension::escape() method should probably be part of the public API.

[fabpot]

#4044 is a first step that would allow extracting a public API for some methods.

[cs278]

Here's a rough idea of the methods I think should at the very least become part of the public API.

Method	API?
Twig\Extension\CoreExtension::cycle()	No
Twig\Extension\CoreExtension::random()	No
Twig\Extension\CoreExtension::dateFormatFilter()	Yes
Twig\Extension\CoreExtension::dateModifyFilter()	?
Twig\Extension\CoreExtension::sprintf()	No
Twig\Extension\CoreExtension::dateConverter()	Yes
Twig\Extension\CoreExtension::replaceFilter()	No
Twig\Extension\CoreExtension::round()	No
Twig\Extension\CoreExtension::numberFormatFilter()	Yes
Twig\Extension\CoreExtension::urlencodeFilter()	No
Twig\Extension\CoreExtension::arrayMerge()	No
Twig\Extension\CoreExtension::slice()	No
Twig\Extension\CoreExtension::first()	No
Twig\Extension\CoreExtension::last()	No
Twig\Extension\CoreExtension::joinFilter()	No
Twig\Extension\CoreExtension::splitFilter()	No
Twig\Extension\CoreExtension::defaultFilter()	No
Twig\Extension\CoreExtension::getArrayKeysFilter()	No
Twig\Extension\CoreExtension::reverseFilter()	No
Twig\Extension\CoreExtension::sortFilter()	No
Twig\Extension\CoreExtension::inFilter()	No
Twig\Extension\CoreExtension::compare()	No
Twig\Extension\CoreExtension::matches()	No
Twig\Extension\CoreExtension::trimFilter()	No
Twig\Extension\CoreExtension::nl2br()	No
Twig\Extension\CoreExtension::spaceless()	?
Twig\Extension\CoreExtension::convertEncoding()	?
Twig\Extension\CoreExtension::lengthFilter()	No
Twig\Extension\CoreExtension::upperFilter()	No
Twig\Extension\CoreExtension::lowerFilter()	No
Twig\Extension\CoreExtension::striptags()	No
Twig\Extension\CoreExtension::titleStringFilter()	No
Twig\Extension\CoreExtension::capitalizeStringFilter()	No
Twig\Extension\CoreExtension::callMacro()	No
Twig\Extension\CoreExtension::ensureTraversable()	No
Twig\Extension\CoreExtension::toArray()	No
Twig\Extension\CoreExtension::testEmpty()	No
Twig\Extension\CoreExtension::testIterable()	No
Twig\Extension\CoreExtension::include()	No
Twig\Extension\CoreExtension::source()	No
Twig\Extension\CoreExtension::constant()	No
Twig\Extension\CoreExtension::constantIsDefined()	No
Twig\Extension\CoreExtension::arrayBatch()	No
Twig\Extension\CoreExtension::getAttribute()	No
Twig\Extension\CoreExtension::arrayColumn()	No
Twig\Extension\CoreExtension::arrayFilter()	No
Twig\Extension\CoreExtension::arrayMap()	No
Twig\Extension\CoreExtension::arrayReduce()	No
Twig\Extension\CoreExtension::arraySome()	No
Twig\Extension\CoreExtension::arrayEvery()	No
Twig\Extension\CoreExtension::checkArrowInSandbox()	No
Twig\Extension\CoreExtension::captureOutput()	No
Twig\Extension\DebugExtension::dump()	No
Twig\Extension\EscaperExtension::raw()	No
Twig\Extension\EscaperExtension::escapeFilterIsSafe()	?
Twig\Extension\EscaperExtension::escape()	Yes
Twig\Extension\StringLoaderExtension::templateFromString()	No

I've mainly gone for the complicated ones like escape() that offer a lot of utility to extension authors or those that change their behaviour according to Twig configuration like numberFormatFilter().

I generated the list with:

collect.php

<?php

use Twig\Extension as Ext;

require 'vendor/autoload.php';

$ref = trim(shell_exec('git rev-parse HEAD'));
$exts = [Ext\CoreExtension::class, Ext\DebugExtension::class, Ext\EscaperExtension::class];
$exts = [...$exts, Ext\StagingExtension::class, Ext\StringLoaderExtension::class, Ext\YieldNotReadyExtension::class];

function isInternal(\ReflectionMethod $m): bool
{
    return \str_contains($m->getDocComment(), '@internal');
}

echo "| Method | API? |\n";
echo "| ------ | ---- |\n";

foreach ($exts as $ext) {
    $class = new \ReflectionClass($ext);
    foreach ($class->getMethods() as $method) {
        if ($method->isPublic() && $method->isStatic() && isInternal($method)) {
            echo sprintf("| [`%s::%s()`](https://github.com/twigphp/Twig/blob/%s/src/Extension/%s.php#L%u-L%u) |     |\n", $class->getName(), $method->getName(), $ref, $class->getShortName(), $method->getStartLine(), $method->getEndLine());
        }
    }
}

[stof]

There is no reason to make CoreExtension::convertEncoding public IMO. If you need to perform encoding conversion, use iconv directly.

dateModifyFilter does not deserve being public either. Once the date conversion is exposed, it is actually easier and more readable to use DatetimeImmutable::modify directly in your code.
For spaceless, I would also keep it internal. I don't see a need to call the implementation of that filter directly in PHP code.

[cs278]

There is no reason to make CoreExtension::convertEncoding public IMO. If you need to perform encoding conversion, use iconv directly.

Yeah I was on the fence about that the only value is the exception it throws for you, so probably not worth it.

dateModifyFilter does not deserve being public either. Once the date conversion is exposed, it is actually easier and more readable to use DatetimeImmutable::modify directly in your code.

👍 Good point.

For spaceless, I would also keep it internal. I don't see a need to call the implementation of that filter directly in PHP code.

Indeed limited use, I was thinking if you wanted to remove white space from any generated markup.

[TLG-Gildas]

I'd like to have access to CoreExtension::toArray and CoreExtension::include.

• toArray: it's simply to allow to have the same rules as core twig engine when extension need to cast parameters to array. CoreExtension::testEmpty could be considered for the same reason
• include: I have encounter some times to have to include some partials in an extension function based on some business calculation. StringLoaderExtension::templateFromString could be considered for the same use case

[stof]

StringLoaderExtension::templateFromString is a wrapper around $env->createTemplate() so there is no reason to make it non-internal. If you need to create a template from a string, use the public API of Twig doing exactly that.

• include: I have encounter some times to have to include some partials in an extension function based on some business calculation.

why not using Environment::render for that use case ? It is the Twig public API for rendering a template.

[TLG-Gildas]

Indeed you are right about templateFromString, I had not explored this proposal in depth.

For the case of include, the main reason is to be able to provide a list of templates while render does not support it.

There is also the fact of being able to control the ignoreMissing or sandboxed parameters independently of the environment configuration.
And finally, from a logical point of view, as it is called during the rendering process, it seems more coherent to me to call include rather than render which corresponds to the entry point of a complete rendering process.

[stof]

Support for using a list of templates is also available in the public API by using Environment::resolveTemplate instead of Environment::load to load the TemplateWrapper (knowing that the implementation of Environment::render is really just a shortcut doing return $this->load($name)->render($context);).
And the equivalent of ignoreMissing is just catching the LoaderError exception when calling Environment::resolveTemplate (this is literally how this option is implemented in the include function).

If there is really a need for the equivalent of the sandboxed parameter, maybe we should rather expose a SandboxExtension::runSandboxed(callable) API allowing to turn on the sandboxed mode temporarily (and resetting it to its current state after that) while executing some logic instead of only support that for an include replacement (and the implementation of include might reuse it)

And finally, from a logical point of view, as it is called during the rendering process, it seems more coherent to me to call include rather than render which corresponds to the entry point of a complete rendering process.

This is not relevant. The implementation of include actually calls the TemplateWrapper::render() method internally to render the included template, doing exactly the same than the Environment::render shortcut. There is no difference between rendering an included template and rendering a template from your controller, except which code receives the output string.

[TLG-Gildas]

Support for using a list of templates is also available in the public API by using Environment::resolveTemplate instead of Environment::load to load the TemplateWrapper (knowing that the implementation of Environment::render is really just a shortcut doing return $this->load($name)->render($context);).

Ok good point, I agree, this point cover the need, thanks.

This is not relevant. The implementation of include actually calls the TemplateWrapper::render()

I agree, in point of view of implementation is not relevant. It was just a remark for an external point of view.

[lostfocus]

It also seems to break https://twig.symfony.com/doc/3.x/filters/format_datetime.html - I'm getting an "Call to undefined method Twig\Extension\CoreExtension::dateConverter()" in vendor/twig/intl-extra/IntlExtension.php:372

[xabbuh]

@lostfocus twig/intl-extra in 3.9+ requires Twig to be present in 3.9 too. This was not reflected in 3.9.0 but got fixed in 3.9.2 (see #4030 and #4027). You either need to update Twig to 3.9 or downgrade the bundle to 3.8.

[lostfocus]

Thanks!

[gharlan]

For instance, the EscaperExtension::escape() method should probably be part of the public API.

It might even deserve its own standalone symfony component. Because proper escaping is not easy and this function could also be useful outside of twig.

[fabpot]

For escaping, can you have a look at #4055 and give me your feedback?

[fabpot]

I think #4066 finally closes this issue.