[TwigBridge] Deprecate passing $field argument to is_granted()
#59282
Conversation
This PR deprecates the `$field` argument of the `is_granted` Twig function. https://github.com/symfony/symfony/blob/78f4d9a1059725384ce563c4e1696b8e66c6983a/src/Symfony/Bridge/Twig/Extension/SecurityExtension.php#L37 Passing a third argument "field" is a feature only made/kept for the AclBundle. https://github.com/symfony/acl-bundle The symfony documentation (https://symfony.com/doc/current/reference/twig_reference.html#is-granted) states: > Optionally, an object can be passed to be used by the voter. More information can be found in Security. But in the linked security page, no mention is made about it: https://symfony.com/doc/current/security.html#security-template The last version of the documentation about ACL in Symfony (https://symfony.com/doc/3.x/security/acl_advanced.html) > ACL support was deprecated in Symfony 3.4 and will be removed in 4.0. Install the Symfony ACL bundle if you want to keep using ACL. This feature is, **afaik**, not available in the rest of the similar security tools/helpers (IsGranted attribute, is_granted function in expression language, ...) And passing an argument will throw an error as the FieldVote class cannot be used without installing the AclBundle (something not documented that can lead to very frustating DX). ... I may not have all context/history here, so please don't bite :)
Done! |
|
But AclBundle is still maintained, so it's going to break it, right? Or maybe I'm missing something. |
is_granted()$field argument to is_granted()
I didn’t know that, to be honest. Not having new features doesn’t mean "not maintained", and I’m sorry for disregarding this aspect. I based this PR on what I read in the current Symfony documentation ("support will be removed in 4.0"). Since AclBundle is not compatible with Symfony 8.0 (obviously), I didn’t see a BC issue here. I’ve always considered BC from a "past" perspective (stating this genuinely — still learning here). However, I don’t understand why we added compatibility in the new I may have been too "solution-oriented" instead of describing the problem(s) I tried to solve here. Current problemThe ACL bundle is not mentioned on any of the pages related to the "is granted" toolkit: But... https://symfony.com/doc/current/reference/twig_reference.html#is-granted 
(Twig functions reference also redirects to this page) The documentation of the This may result in users installing a bundle they neither intended to nor needed to install. If we need to maintain both present and future compatibility, I suggest the following: 0) Fix
|
|
Could ACLBundle override these functions? I think it'd be worth it for the sake of not polluting the core API with something we took care of extracting from core. |
There’s no mention of Twig in general—or So, I’ll politely decline the invitation… that wasn’t even one, haha 😘 As I said, if you think it's good as it is, let's close here :) |
I agree with all items then. Can you please update the PR accordingly? |
|
Actually, let's do the follow up in another PR to keep the discussion relevant. |
The third argument of is_granted / is_granted_for_user is undocumented (except on this page) (neither on Symfony docs, code, nor on... Symfony ACL Bundle docs). It must be kept and maintained for BC reasons. But it can be missleading/frustrating to show this argument to users, when they cannot use it with a standard/recommended Symfony installation. And if they just test it, the error could lead them to believe the AclBundle needs to be installed to use the `is_granted` function(s). As suggested in symfony/symfony#59282 (comment) I believe "not showing it" is the "best" solution here.
…nt (smnandre) This PR was merged into the 7.3 branch. Discussion ---------- [Security] Remove mention of is_granted_ `$field` argument The third argument of is_granted / is_granted_for_user is undocumented (except on this page) (neither on Symfony docs, code, nor on... Symfony ACL Bundle docs). It must be kept and maintained for BC reasons. But it can be missleading/frustrating to show this argument to users, when they cannot use it with a standard/recommended Symfony installation. And if they just test it, the error could lead them to believe the AclBundle needs to be installed to use the `is_granted` function(s). As suggested in symfony/symfony#59282 (comment) I believe "not showing it" is the "best" solution here. Commits ------- 693b758 [Security] Remove mention of is_granted_ `$field` argument
…lsy $field (smnandre) This PR was squashed before being merged into the 7.3 branch. Discussion ---------- [TwigBridge] Align isGrantedForUser on isGranted with falsy $field | Q | A | ------------- | --- | Branch? | 7.3 | Bug fix? | no | New feature? | no <!-- please update src/**/CHANGELOG.md files --> | Deprecations? | no <!-- please update UPGRADE-*.md and src/**/CHANGELOG.md files --> | Issues | Fix #... <!-- prefix each issue number with "Fix #", no need to create an issue if none exists, explain below instead --> | License | MIT Align `is_granted_for_user` on `is_granted` behaviour when passing a falsy $field argument. (Follows #59282 (comment)) Commits ------- cc92b65 [TwigBridge] Align isGrantedForUser on isGranted with falsy $field
This PR deprecates the
$fieldargument of theis_grantedTwig function, and removes it from the just-added is_user_granted function.It seems that passing a third argument "field" is a feature made/kept for the AclBundle.
The Symfony documentation states:
But in the linked security page, no mention is made about it.
The last version of the documentation about ACL writes
This feature is, afaik, not available in the rest of the similar security tools/helpers (IsGranted attribute, is_granted function in expression language, ...)
Finally, passing an value here will throw an error as the FieldVote class cannot be used without installing the AclBundle (something not documented that can lead to very frustating DX).
...
I may not have all context/history here, so please don't bite :)