Skip to content

[TwigBridge] Make AppVariable check if security.context exists #14816

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.

Sign up for GitHub

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

Merged
merged 1 commit into from
Jun 11, 2015
Merged

[TwigBridge] Make AppVariable check if security.context exists #14816

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 5 additions & 1 deletion src/Symfony/Bridge/Twig/AppVariable.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -74,7 +74,9 @@ public function getSecurity()
throw new \RuntimeException('The "app.security" variable is not available.');
}

return $this->container->get('security.context');
if ($this->container->has('security.context')) {
return $this->container->get('security.context');
}
}

/**
Expand All @@ -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;
Copy link
Member

Choose a reason for hiding this comment

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

an exception saying it is not available (as done in the previous case) may make more sense

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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. :/

Copy link
Member

Choose a reason for hiding this comment

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

then we should indeed return null

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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...

Copy link
Member

Choose a reason for hiding this comment

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

What about deprecating that behaviour with Symfony 2.8 and throw the exception starting with 3.0?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

}

$this->tokenStorage = $this->container->get('security.context');
Expand Down