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 the possibility to register classes/interface as being safe #3025

Merged
merged 2 commits into from May 25, 2019

Conversation

fabpot
Copy link
Contributor

@fabpot fabpot commented May 20, 2019

closes #2548

To avoid a too big performance impact on the escaper, we aggressively cache the safe classes, which means that changing the. configuration at runtime is not possible (and having different ones on 2 Twig instances is not possible either, this is really globally configured).

@fabpot fabpot mentioned this pull request May 20, 2019
@fabpot fabpot force-pushed the safe-strategies branch 2 times, most recently from 5b563ef to 6b06269 Compare May 20, 2019 12:03
@lstrojny
Copy link
Contributor

Looks promising! If you want to avoid having quasi singleton semantics in EscaperExtension and CoreExtension the static cache variables could be indexed by spl_object_hash() in order to allow two Twig instances with different extension configurations in the same system

@stof
Copy link
Member

stof commented May 20, 2019

@lstrojny that would not work, as the cached compiled template would have to be invalidated when that change, so using the spl object hash would force to always invalidate the cache for each HTTP request.

Copy link
Member

@stof stof left a comment

Choose a reason for hiding this comment

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

btw, changing safe classes does not invalidate the cache AFAICT.

src/Extension/EscaperExtension.php Outdated Show resolved Hide resolved
@stof
Copy link
Member

stof commented May 20, 2019

@fabpot maybe twig_escape_filter should become a method of the extension, to be able to store some private state for the instance.

@stof
Copy link
Member

stof commented May 20, 2019

btw, changing safe classes does not invalidate the cache AFAICT.

nvm, this is runtime escaping, not compile-time one.

@fabpot
Copy link
Contributor Author

fabpot commented May 21, 2019

Before going further here, I propose to move everything related to the escaping implementation to EscaperExtension. See #3026. One consequence of these changes will be that this feature will only be available as of Twig 2.x (as in Twig 1.x, we cannot be sure that both CoreExtension and EscaperExtension are available).

@lstrojny
Copy link
Contributor

@lstrojny that would not work, as the cached compiled template would have to be invalidated when that change, so using the spl object hash would force to always invalidate the cache for each HTTP request.

Not sure I get that: what’s the relationship of this topic and compiled templates, aren’t all escaping checks runtime only?

fabpot added a commit that referenced this pull request May 21, 2019
…::getEscaper() in favor of the same methods on EscaperExtension (fabpot)

This PR was merged into the 2.x branch.

Discussion
----------

Deprecate CoreExtension::setEscaper() and CoreExtension::getEscaper() in favor of the same methods on EscaperExtension

This is some preliminary work to ease #3025. Everything related to escaping is now part of the `EscaperExtension` instead of `CoreExtension`. This PR is submitted on 2.x because both extensions are always available in 2.x (which is not the case on 1.x).

Commits
-------

59d1d5d deprecated CoreExtension::setEscaper() and CoreExtension::getEscaper() in favor of the same methods on EscaperExtension
@fabpot fabpot changed the base branch from 1.x to 2.x May 21, 2019 13:08
@fabpot fabpot force-pushed the safe-strategies branch 2 times, most recently from 2ed0029 to 936f46f Compare May 21, 2019 14:09
@fabpot
Copy link
Contributor Author

fabpot commented May 21, 2019

I think it's ready for another round of reviews.

@fabpot fabpot merged commit fe6503f into twigphp:2.x May 25, 2019
fabpot added a commit that referenced this pull request May 25, 2019
…ing safe (fabpot)

This PR was squashed before being merged into the 2.x branch (closes #3025).

Discussion
----------

Add the possibility to register classes/interface as being safe

closes #2548

To avoid a too big performance impact on the escaper, we aggressively cache the safe classes, which means that changing the. configuration at runtime is not possible (and having different ones on 2 Twig instances is not possible either, this is really *globally* configured).

Commits
-------

fe6503f -
b18733b added the possibility to register classes/interfaces as being safe for the escaper
@fabpot fabpot deleted the safe-strategies branch June 3, 2019 09:25
@mpdude
Copy link
Contributor

mpdude commented May 23, 2022

What is the suggested/easiest way to configure this in Symfony?

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.

Mark types as "safe"
4 participants