Skip to content
This repository has been archived by the owner on Mar 9, 2024. It is now read-only.

Facades do not behave same as underscored counterparts -- not marked safe when they should be #6

Open
mnpenner opened this issue Mar 9, 2014 · 5 comments

Comments

@mnpenner
Copy link

mnpenner commented Mar 9, 2014

form_open seems to be marked as is_safe whereas Form.open is not (once you enable it by adding Form to the facades config). Form.* functions should be marked as safe as well.

I'm not sure if there's a good way of dealing with this. We wouldn't want to make everything unsafe by default. Perhaps the best was to extend the facades configuration to allow the user to choose which methods to mark safe by default.

Perhaps something like:

'facades' => array(
    'Auth' => ['is_safe' => false],
    'Form' => ['is_safe'=>'html'],
    'URL' => ['is_safe'=>'html'],
)

Could work, but we might want even more fine-grained control to mark individual methods as safe or not?

@barryvdh
Copy link
Owner

barryvdh commented Mar 9, 2014

I have to see if that is possible, because the Facade calls aren't actually handled as twig functions, but as attributes on a variable.
That is why I created extensions for common functions, like the form helpers.
A work around would be adding an autoescape = false block around your form.
I will see if it is somehow possible to use more options, then I could use the same options as for the helpers (defaults and per facade)

@barryvdh
Copy link
Owner

barryvdh commented Mar 9, 2014

Hmm, it isn't possible to return a Twig_SimpleFunction, but I could return a Twig_Markup instance, which is never escaped. This does only work with strings, so would kill any chaining (when using Former for instance: Former.text().addClass().require() etc), unless I extend the Twig_Markup class and add a magic __call function there also, like this:

//StaticCaller.php
public function __call($method, $arguments){
    $output = call_user_func_array($this->className.'::'.$method, $arguments);
    if(is_string($output) or method_exists($output, '__toString')){
        return new \Twig_Markup($output, 'UTF-8');
    }else{
        return $output;
    }
}

//Twig_Markup
public function __call($method, $arguments){
    $output = call_user_func_array(array($this->object, $method), $arguments);
    if(is_string($output) or method_exists($output, '__toString')){
        return new static($output, 'UTF-8');
    }else{
        return $output;
    }
}

That way we can chain, and when the __toString is called after the last chain, it returns the safe string. But this doesn't feel very pretty..

@mnpenner
Copy link
Author

I don't think checking if the return value is a string is sufficient to mark it as safe; we don't know what these facades are returning, we can't assume it's safe just because it's a string.

I took a crack at it. Working great so far.

Also, we need a way to let users specify which extensions they want. I would have just created my own FacadeExtension locally and unregistered yours, but you seem to have hardcoded the list. rcrowe allows customization.

@barryvdh
Copy link
Owner

It is pretty easy to register your own extension, but it would be better if the default extensions where configurable. I'll see if I can change that soon, so you don't have to maintain that fork just for that.

@barryvdh
Copy link
Owner

The problem is that this kills chaining, and to make that work it would be ugly. But if we make it clear that facades using is_safe just return the string, instead of chaining, that wouldn't be a problem.
It wouldn't be a problem for most Facades anyway, mostly just for Former.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants