[TwigBridge] Make AppVariable check if security.context exists #14816
Conversation
If security isn't configured in the application, the `security.context` service, nor the `security.token_storage` service exists. Therefore, if a third-party bundle relies on the app.user check in Twig templates, an exception was thrown about asking for an non-existing service. Instead, this change check if the `security.context` actually exists before trying to use it, and return null otherwise.
| @@ -89,6 +91,8 @@ public function getUser() | |||
| if (null === $this->tokenStorage) { | |||
| if (null === $this->container) { | |||
| throw new \RuntimeException('The "app.user" variable is not available.'); | |||
| } elseif (!$this->container->has('security.context')) { | |||
| return; | |||
There was a problem hiding this comment.
an exception saying it is not available (as done in the previous case) may make more sense
There was a problem hiding this comment.
Actually, I know, but it will make the check in templates unusable if third-party bundles relies on it to activate templates sections when the security is configured.
GlobalVariables class, previously used for twig app globals in previous versions did this very same check and returned null when the service didn't exist.
So, this is basically a BC break. :/
There was a problem hiding this comment.
But I'm indeed wondering why we should throw a RuntimeException in the above case where the container isn't defined. When does this particular case might happen ? Does it make sense to throw this exception instead of returning null?
Obviously, if this PR is merged as it is, we'll have some kind of inconsistency. It feels odd, and the same might be applicable to request and session globals...
There was a problem hiding this comment.
What about deprecating that behaviour with Symfony 2.8 and throw the exception starting with 3.0?
There was a problem hiding this comment.
I don't know. It was an easy way to enable or hide sections in templates, depending on the application configuration without much effort. Apps will require to check themselves if security is configured and inject the result in every template where they need it, or create their own global.
When does returning null in such case became wrong ? I can't find anything mentioning the reasons in #13354
|
👍 |
|
Thank you @ogizanagi. |
…ists (ogizanagi)
This PR was merged into the 2.7 branch.
Discussion
----------
[TwigBridge] Make AppVariable check if security.context exists
| Q | A
| ------------- | ---
| Bug fix? | yes
| New feature? | no
| BC breaks? | no
| Deprecations? | no
| Tests pass? | yes
| Fixed tickets | -
| License | MIT
| Doc PR | -
If security isn't configured in the application, neither the `security.context` service, nor the `security.token_storage` service exists.
Therefore, if a third-party bundle relies on the `app.user` or `app.security` (deprecated) check in Twig templates, an exception was thrown about asking for an non-existing service:
```yml
security: false
```
```twig
{% if app.user %}
<div>
...
</div>
{% endif %}
```
```
You have requested a non-existent service "security.context"
```
Instead, this patch checks if the `security.context` actually exists before trying to use it, and returns null otherwise.
Note that the **GlobalVariables** class, used for twig app globals in previous versions, and still used for PHP templating, behaves the same way.
So this is basically a BC break.
Commits
-------
ea71174 [TwigBridge] Make AppVariable check if security.context exists
If security isn't configured in the application, neither the
security.contextservice, nor thesecurity.token_storageservice exists.Therefore, if a third-party bundle relies on the
app.userorapp.security(deprecated) check in Twig templates, an exception was thrown about asking for an non-existing service:{% if app.user %} <div> ... </div> {% endif %}Instead, this patch checks if the
security.contextactually exists before trying to use it, and returns null otherwise.Note that the GlobalVariables class, used for twig app globals in previous versions, and still used for PHP templating, behaves the same way.
So this is basically a BC break.