Add check for SecurityBundle in createAccessDeniedException #25604
Conversation
fgm
commented
Dec 26, 2017
•
fgm
commented
| Q | A |
|---|---|
| Branch? | 4.0 |
| Bug fix? | yes |
| New feature? | no |
| BC breaks? | no |
| Deprecations? | no |
| Tests pass? | yes |
| Fixed tickets | #25595 |
| License | MIT |
| Doc PR | n.a. |
| * @final since version 3.4 | ||
| */ | ||
| protected function createAccessDeniedException(string $message = 'Access Denied.', \Exception $previous = null): AccessDeniedException | ||
| { | ||
| if (!class_exists(AccessDeniedException::class)) { | ||
| throw new \LogicException('You can not use the "createAccessDeniedException" method if the SecurityBundle is not registered in your application.'); |
There was a problem hiding this comment.
To be consistent with the error messages displayed in other parts of the framework, we should change this message to:
throw new \LogicException('The SecurityBundle is not registered in your application. Try running "composer require security"');There was a problem hiding this comment.
@javiereguiluz : the message you suggest is only used in three methods in the file: getDoctrine(), getUser(), isGranted(), while the suggested choice is used in four methods: addFlash(), renderView(), render(), and stream(), hence the current wording choice, favoring the longer format for internal consistency of this file.
Is there a trend either way in recent Symfony changes, to ensure the consistency goes the proper way at a larger scale ?
There was a problem hiding this comment.
You are right!! However, I was mostly referring to the "try running composer require ..." part of the error message. It's an absolute must for newer Symfony apps using Flex. We do that in other error messages:
throw new LogicException('CSRF support cannot be enabled as the Security CSRF component is not installed. Try running "composer require security-csrf".');
throw new \LogicException(sprintf('The type="expression" attribute cannot be used without the ExpressionLanguage component. Try running "composer require symfony/expression-language".'));And that trend will continue because of PRs like this: #25601
There was a problem hiding this comment.
Oh, I see. There is not such wording in the current version of this file, but it does fit with the push for Flex. Done.
| * @final since version 3.4 | ||
| */ | ||
| protected function createAccessDeniedException(string $message = 'Access Denied.', \Exception $previous = null): AccessDeniedException | ||
| { | ||
| if (!class_exists(AccessDeniedException::class)) { | ||
| throw new \LogicException('You can not use the "createAccessDeniedException" method if the SecurityBundle is not registered in your application. Try running "composer require security".'); |
There was a problem hiding this comment.
Only the security-core component is required in fact, should it be more precise?
There was a problem hiding this comment.
I wondered about that too. On the one hand, previously, "security-core" and "security-csrf" were required, not the complete "security". But on the other hand two current situations where a requirement is suggested suggest the complete "security" (but one suggests "security-csrf"): https://github.com/symfony/framework-bundle/search?utf8=%E2%9C%93&q=composer+require+security&type=
Overall, since we're suggesting things, this is to make it easy on the kind of users who need the handholding, and including the whole package is likely to make it easier overall, hence that choice.
There was a problem hiding this comment.
we're moving to as-specific-as-possible, so we should update the places where symfony/security is referenced
There was a problem hiding this comment.
other errors are about missing services so suggesting security is correct (alias for security-bundle, not symfony/security). We only need to change for security-core on this one
|
OK, added the change just to that method. The AppVeyor fail is caused by a composer download failure, which seems unrelated. |
|
Thank you for this first contrib @fgm, welcome! |
…n (FGM) This PR was submitted for the 4.0 branch but it was squashed and merged into the 3.4 branch instead (closes #25604). Discussion ---------- Add check for SecurityBundle in createAccessDeniedException | Q | A | ------------- | --- | Branch? | 4.0 | Bug fix? | yes | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | #25595 | License | MIT | Doc PR | n.a. Commits ------- a013c01 Add check for SecurityBundle in createAccessDeniedException
|
Merged in 3.4. |
|
You're welcome. I'm wondering, though: the issue is also present in 4.0 but the merge was on 3.4: will the patch be forward-ported in some way ? |
|
Correct. Lower branches are regularly merged into their upper branch. So everything merged from 2.7 to 3.4 end up in master (4.1 right now) |
|
@sroze Thanks for this information. |
