Skip to content

Pass charset to escaper callback again#4088

Closed
fritzmg wants to merge 1 commit intotwigphp:3.xfrom
fritzmg:patch-1
Closed

Pass charset to escaper callback again#4088
fritzmg wants to merge 1 commit intotwigphp:3.xfrom
fritzmg:patch-1

Conversation

@fritzmg
Copy link
Copy Markdown

@fritzmg fritzmg commented May 12, 2024

Fixes #4087

This restores the behaviour of 3.9.x where the $charset was passed to the escaper callback.

$this->escapers[$strategy] = $callable;
$callable = function ($string, $charset) use ($callable) {
return $callable($this->environment, $string);
return $callable($this->environment, $string, $this->environment->getCharset());
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

It was indeed a mistake. I don't think we should restore this odd behavior as the callables never got the charset before (so without changing the escapers, the charset is not useful). And as adding the charset does not make migrating easier, I don't think we should support it.

Copy link
Copy Markdown
Author

@fritzmg fritzmg May 12, 2024

Choose a reason for hiding this comment

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

as the callables never got the charset before

Not sure what you mean? The callables did get the charset before:

return $escapers[$strategy]($env, $string, $charset);

That's the BC break.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Sorry, I'm stupid sometimes. See #4091 for the fix. Here, we need the charset passed to the escape filter, not the default charset.

@fritzmg fritzmg changed the title Parse charset to escaper callback again Pass charset to escaper callback again May 12, 2024
@fabpot fabpot closed this in f553f16 May 14, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

Possible BC break with EscaperExtension in 3.10.x

2 participants