Removed the is_safe option on the Twig function #98

Merged
merged 1 commit into from Jan 22, 2013

Conversation

Projects
None yet
2 participants
Contributor

stof commented Jan 22, 2013

This function has no reason to be marked as safe, and marking
functions as safe should not be encouraged as being the common
practice for everything.

@stof stof Removed the is_safe option on the Twig function
This function has no reason to be marked as safe, and marking
functions as safe should not be encouraged as being the common
practice for everything.
7b90344
Owner

schmittjoh commented Jan 22, 2013

Is there a reason to not mark this particular function as safe apart from the general notion that you should not mark everything as safe?

Contributor

stof commented Jan 22, 2013

Well, as I don't see any reason to echo the return value, the option will probably never be used. But I already saw several times people copy-pasting a Twig extension to create their own, and sometimes ending with the is_safe option on functions that were far from being safe. So I think it is better to avoid the option when it is useless.

schmittjoh merged commit 08e0b46 into schmittjoh:master Jan 22, 2013

1 check failed

default Review: No Comments — Travis: Failed
Details
Contributor

stof commented Jan 23, 2013

btw, I found another reason why it was a bad idea: trying to echo the return value with the old code was simply breaking Twig, because the is_safe option expects an array, not a boolean. So previously, the following code was throwing a fatal error:

{{ dump(is_expr_granted('something')) }}

stof deleted the stof:patch-1 branch Jan 23, 2013

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment