Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
[FrameworkBundle][TwigBridge] make csrf_token() usable without forms#25197
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
| ); | ||
| if (null !==$this->csrfTokenManager) { | ||
| $functions[] =newTwigFunction('csrf_token',array($this,'getCsrfToken')); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Regarding DX, wouldn't it be better to always add the method but throw if nocsrfTokenManager injected?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I would say it's better this way because you will notice the error when templates are parsed. Having the function fail at runtime can just go without notice.
| private$csrfTokenManager; | ||
| publicfunction__construct(AuthorizationCheckerInterface$securityChecker =null) | ||
| publicfunction__construct(AuthorizationCheckerInterface$securityChecker =null,CsrfTokenManagerInterface$csrfTokenManager =null) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Adding the CSRF manager adds more deps, would it be better to create a Twig runtime class at this point?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
#24692 does it also
stof commentedNov 29, 2017
Rather than coupling it to the The authorization layer and the CSRF layer are totally independant components (the CSRF one is not even configured by SecurityBundle but by FrameworkBundle) |
The Twig function `csrf_token()` is currently only registered when theForm component is installed. However, this function is also useful, forexample, when creating simple login forms for which you do not need thefull Form component.
2abd80e to709efa3Comparexabbuh commentedDec 1, 2017 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
I moved the new function into a new extension class inside FrameworkBundle. Do we nonetheless want to use a runtime class here? |
javiereguiluz commentedDec 4, 2017
@xabbuh just asking: is there any reason yo create so many small Twig extensions inhttps://github.com/xabbuh/symfony/tree/master/src/Symfony/Bridge/Twig/Extension? Could we add this tiny new function in the existing SecurityExtension instead? Thanks! |
xabbuh commentedDec 4, 2017
Tobion left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Isn't this missing the deprecation of the csrf_token in the FormExtension and FormRenderer? Otherwise we have the same thing in two different ways.
xabbuh commentedMar 12, 2018
Maybe I am missing something, but I don't see how to deprecate registering a single function from a Twig extension. But as far as I can see that's not a big deal as the extension being registered later will just replace the existing definition which does no harm as both implementations internally work the same. |
fabpot commentedMar 13, 2018
xabbuh commentedMar 13, 2018
@fabpot in our case both filters have the same name. We could make registering the additional filters an opt-out feature. But I am not sure if it's worth it. |
fabpot commentedMar 21, 2018
Thank you@xabbuh. |
… without forms (xabbuh)This PR was merged into the 4.1-dev branch.Discussion----------[FrameworkBundle][TwigBridge] make csrf_token() usable without forms| Q | A| ------------- | ---| Branch? | master| Bug fix? | no| New feature? | yes| BC breaks? | no| Deprecations? | no| Tests pass? | yes| Fixed tickets || License | MIT| Doc PR |The Twig function `csrf_token()` is currently only registered when theForm component is installed. However, this function is also useful, forexample, when creating simple login forms for which you do not need thefull Form component.Commits-------709efa3 make csrf_token() usable without forms
rejinka commentedMay 24, 2018
@xabbuh Thank you! 👍 |
…endency on CSRF token storage (tgalopin)This PR was merged into the 4.1 branch.Discussion----------[FrameworkBundle][TwigBridge] Fix BC break from strong dependency on CSRF token storage| Q | A| ------------- | ---| Branch? | 4.1| Bug fix? | yes| New feature? | no| BC breaks? | no| Deprecations? | no| Tests pass? | yes| Fixed tickets | -| License | MIT| Doc PR | -The PR#25197 introduced the `csrf_token` function in Twig. This extension relies on `CsrfTokenManagerInterface`, which itself relies on the session. In some contexts such as when sessions are stored in Redis and we try to warmup the cache in CLI without Redis available, this makes the process fails.This PR fixes this by using a Twig runtime instead of a direct extension to avoid a strong dependency on `CsrfTokenManagerInterface`.Commits-------68994a6 [FrameworkBundle][TwigBridge] Fix BC break from strong dependency on CSRF token storage
The Twig function
csrf_token()is currently only registered when theForm component is installed. However, this function is also useful, for
example, when creating simple login forms for which you do not need the
full Form component.