Cleaner templates via new compiling stage about plates HOT 50 CLOSED

Just a random thought I had today in regards to auto escaping. When you store a variable onto a template you could put it in a wrapper class.

<?php

class Escaper implements IteratorAggregate
{
    private $variable;

    private $iteratorArray;

    public function __construct($variable)
    {
        $this->variable = $variable;

        $this->iteratorArray = is_array($this->variable) ? $this->variable : [];
    }

     public function getIterator()
    {
        return new ArrayIterator($this->iteratorArray);
    }

    public function __call($method, $arguments)
    {
        return $this->variable->$method($arguments, ...$method);
    }

    public function __toString()
    {
        return echo htmlentities($this->variable, ENT_QUOTES, "UTF-8");
    }

    public function raw()
    {
        return $this->variable;
    }
}

You should be able to allow most actions against the variable through magic methods. I've implemented a function call, echo, and foreach.

So the way it would work, is when you did $template->assign('title', 'A Fabulous Blog Post'); The string would be converted into the Escaper object. In your template when you do echo $title;, the __toString method would kick in and you'd have the escaped version of the string.

from plates.

One thing to note, the one-off functions would have no access to the current $template like real extensions do.

from plates.

@conorsmith Smart, I agree, thanks for sharing.

from plates.

I want to also be open about the possible negative side affects of this proposed change. I'll maintain a list here, which can be added to as needed.

1. Different escaping strategies are required for different contexts (HTML, HTML attributes, JavaScript and CSS. This technique only offers one strategy: HTML (using htmlspecialchars()). All others types would still have be done manually. Ex: <? echo escape()->js($name) ?>.
2. Perceived performance loss (although I believe with proper caching this really isn't an issue)
3. Introduces the need to define a caching folder, which must be ignored in your code repository (not a big deal, just an extra couple steps)
4. Template warnings/notices/errors will reference the compiled template, not the original template

from plates.

I use Plates for small-ish projects and I think this is a great direction to head. I would definitely enjoy auto-escaping and being able to remove "$this->".

Template warnings/notices/errors will reference the compiled template, not the original template

You are right, this can be annoying. I also use Laravel and the worst part about this in blade is that the cached compiled template file name doesn't reference the original template name. So to find the actual source of the error, I have to go into the compiled file, look at the code, then remember which template that belongs too, then fix the error.

So what I'd request, if it's possible, is to have the original template name (and parent dirs if possible) contained in the compiled template filename. Something like: "user-create-k4j35h3k5h43k5h35" instead of just "k4j35h3k5h43k5h35".

Introduces the need to define a caching folder, which must be ignored in your code repository

Wouldn't this be kind of an optional optimization for most people? If the compiled templates get committed, will they do anything other than just take up space? I don't actually know how that works so I definitely may be wrong. But if I'm right - I don't think experienced users should have much problem adding a directory to an ignore list. And I don't think it raises the barrier to entry for people who are new to Plates.

from plates.

So what I'd request, if it's possible, is to have the original template name (and parent dirs if possible) contained in the compiled template filename. Something like: "user-create-k4j35h3k5h43k5h35" instead of just "k4j35h3k5h43k5h35".

Agreed, I don't even think we need the random filenames. I'm hoping for something like /cache/default/home.php for home, and /cache/folders/emails/welcome.php for emails::welcome.

One thought I had was to just place compiled templates in the same folder as the templates, removing the need to even define a cache folder (good for beginners). For example, templates/home.php would compile to templates/.cache/home.php. I think this might actually be good default behaviour. If you prefer to define your own cache folder, you can do that via the engine, like so:

$engine->setCacheDirectory('your-cache-path');

Wouldn't this be kind of an optional optimization for most people? If the compiled templates get committed, will they do anything other than just take up space? I don't actually know how that works so I definitely may be wrong. But if I'm right - I don't think experienced users should have much problem adding a directory to an ignore list. And I don't think it raises the barrier to entry for people who are new to Plates.

Correct, committing compiled templates won't cause any issues, it just adds some extra noise to your repo.

from plates.

Template warnings/notices/errors will reference the compiled template, not the original template

I'm not really sure why this isn't done with Laravel or Twig, but in my Laravel Twig bridge, I just catch the exceptions and throw a new Exception with the 'real' template, so Whoops displays the original file, not the compiled file. As long as you don't change the line numbers (or by a set amount of lines) and have a reference to the original file, it shouldn't be that hard..

from plates.

I'm not really sure why this isn't done with Laravel or Twig, but in my Laravel Twig bridge, I just catch the exceptions and throw a new Exception with the 'real' template, so Whoops displays the original file, not the compiled file.

Smart. I might just try that. My proposed compiling stage doesn't change the amount of lines, so I don't see any issue there. Thanks for sharing!

from plates.

I don't find echo really explicit for new user, why <?=$name?> and <?php echo $name ?> are different will not be clear, and that doesn't smell pure PHP.
Maybe we can add a new function that outputs raw value ?

An other point, for example I've a plugin to manage i18n, I use it like that :
<?= $this->trans('Hello %user%', ['%user%' => $name]) ?>
That should be compiled as <?= $this->trans('Hello %user%', ['%user%' => $this->e($name)]) ?> or <?= $this->e($this->trans('Hello %user%', ['%user%' => $name])) ?> ?

from plates.

Thanks to @pmjones who helped identify another issue with this approach:

There is no single, magic-bullet, escaping technique. How you escape really depends on the context. There are four common escaping strategies:

• HTML
• HTML attributes
• JavaScript
• CSS

This proposed auto escaping technique would only offer one strategy: HTML (using htmlspecialchars()). All others will have to be done manually. Ex: <? echo escape()->js($name) ?>.

(I've added it the list above.)

from plates.

Based on regexp we can find the context I think

from plates.

Based on regexp we can find the context I think

I actually just learned that the Hoa Project does this, but it isn't fool proof. While a neat idea, it sounds like something that's impossible to get right. Not sure that's a nut I'm interested in trying to crack.

from plates.

To clarify, the solution to auto-escape is to compile the template from PHP to "secured" PHP view.
The perfect implementation should convert

<p id="<?= $id ?>">
    <?= $text ?>
    <?= raw($secure_value) ?>
</p>
<style>p{color: <?= $text_color ?>}</style>
<script>var foo = <?= $foo ?></script>

into

<p id="<?= attr_escape($id) ?>">
    <?= html_escape($text) ?>
    <?= raw($secure_value) ?>
</p>
<style>p{color: <?= css_escape($text_color) ?>}</style>
<script>var foo = <?= js_escape($foo) ?></script>

Is that ?
(the function's name are arbitrary )

from plates.

@lalop Thanks for clarifying, that makes sense. Although I wonder how I'd actually going about doing that properly. Consider this:

<?=$foo . ' ' . raw($bar)?>

How does that get handled? :)

from plates.

I think <?= $text ?> should be convert into <?php html_escape($text) ?> and <?= raw($secure_value) ?> should be write <?php raw($secure_value) ?> this two functions echo to the standard output.
So The answer is simple <?=$foo . ' ' . raw($bar)?> will be <?= html_escape($foo . ' ' . raw($bar)) ?> what is clearly not what the user wants but since raw echos the result is understandable.

from plates.

@lalop I agree with that. How do we handle echo within <?php?. I'm nervous of trying to parse blocks of PHP. Consider:

<h1>Hello, <?=$name?></h1>

<ul>
    <?php
        foreach ($friends as $friend) {
            echo '<li>' . $friend->name . '</li>';
        }
    ?>
</ul>

from plates.

As I get more feedback on this idea, I realize there are, to no surprise, two camps. Some people love this idea, and some hate it. Would it be terrible if Plates offered both options? By default Plates would NOT do any compiling. However, if you wanted to enable auto-escaping, short functions or short open tags, you could do so by enabling this via the engine. Something like this:

<?php

use \League\Plates\Engine;

$engine = new \League\Plates\Engine('templates');
$engine->enableCompiler(Engine::AUTO_ESCAPE|Engine::SHORT_FUNCTIONS|Engine::SHORT_OPEN_TAGS);

from plates.

@reinink Thanks for cc'ing me into the conversation. I disagree with the necessity of a "compiler" stage, and would prefer that auto-escaping be done via a "read only" trigger that goes into effect when the view rendering process is started. I have an example of this that I will include tomorrow.

from plates.

@reinink, to follow the @shadowhand point, I think

<ul>
    <?php
        foreach ($friends as $friend) {
            echo '<li>' . $friend->name . '</li>';
        }
    ?>
</ul>

Should be complied into

<ul>
    <?php
        foreach ($friends as $friend) {
            html_escape('<li>' . $friend->name . '</li>');
        }
    ?>
</ul>

to be consistent.
Or maybe that should be writen

<ul>
    <?php foreach ($friends as $friend) : ?>
            <li><?= $friend->name ?></li>
    <?php endforeach ?>
</ul>

Where becomes a real template language on top of PHP.

would prefer that auto-escaping be done via a "read only" trigger

So I'm agree with @shadowhand, the only issue is the escape context.

from plates.

@lalop but html_escape('<li>' . $friend->name . '</li>'); would make the <li> elements escaped, which is wrong.

from plates.

Yes but plates can't be magic
Le 8 juil. 2014 05:11, "Woody Gilk" [email protected] a écrit :

@lalop https://github.com/lalop but html_escape('

' . $friend->name
. '

'); would make the

elements escaped, which is wrong.

—
Reply to this email directly or view it on GitHub
#23 (comment).

from plates.

@baileylo and then how do you differentiate between JS escaping and HTML escaping?

from plates.

@shadowhand Good point. Since my idea would skip compiling, it would be impossible to add context sensitive escaping. That having been said, to allow multiple escaping options to my suggested escaper wouldn't be overly complex. I'd imagine there would be 2 different ways a user could specify escaping, 1 at assignment, 2 at render.

Assignment

When you assign the variable to the template object, you could have an optional third parameter that specifies the type of escaper to use. EG: $template->assign('someJs', "var i = 123;", Escaper::JAVASCRIPT); This would make the templates look like they have magic escaping.

Render Time

In the template itself you could call the specific type of escaping method you wanted. Similar to my "raw" method in my earlier example that provided the unescaped object. $someJs->javascript();.

Misc

The other short fall of this code is that it wouldn't auto escape variables generated in the template.

My Template:

<h1><?= "Page Title" ?></h1>
<?php $subhead = 'A brief talk about escaping';?>
<h2><?= $subhead ?></h2>

This seems a rather trite example, but it demonstrates another short falling, some echo's are escaped while some aren't. The bigger issue here is that it could possibly lead to double escaping.

<h1><?= html_escape("Page Title") ?></h1>
<?php $subhead = 'A brief talk about escaping';?>
<h2><?= html_escape($subhead) ?></h2>
<h2><?= html_escape($escaperVariable) ?></h2>

Also would create issues with instanceof comparisons:

<nav>
    <?php if($user instanceof User): ?>
        Hello, <?= $user->getName() ?>
    <?php else:?>
        Login!
    <?php endif;?>
</nav>

Would have to be written:

<nav>
    <?php if($user->raw() instanceof User): ?>
        Hello, <?= $user->getName() ?>
    <?php else:?>
        Login!
    <?php endif;?>
</nav>

This was just a random idea I had, but I don't think it will end up working.

from plates.

@baileylo I actually already experimented with this very idea, except that every variables would be "proxied" into a League\Plates\Variable object. This would allow some neat things:

Hello, <?=$name?> // automatically escaped using HTML strategy
Hello, <?=$name->raw()?>
Hello, <?=$name->css()?>
Hello, <?=$name->js()?>

However, as cool as this is, it doesn't solve the big issue I've been trying to solve, and that's how to handle objects and arrays. As @shadowhand can tell you, I don't like the idea of an escaping approach that only works on strings and not objects or arrays. I believe that a large majority of assigned template variables are actually objects. For example:

Hello, <?=$user->name?>

At that point, the object parameter name would not be escaped.

from plates.

That could be mitigated if you updated the __call method to always return an object of Escaper.

public function __call($method, $parameters)
{
    return new self($this->variable->$method(...$parameters));
}

The same thing could be applied to offsetGet for your ArrayAccess interface.

from plates.

@baileylo Right, but that would have to be applied to the Model object at that point, not the template object, which just seems way to extravagant for this problem.

<?php
class User extends \League\Plates\TemplateVariable

Unless I'm missing something?

from plates.

@reinink It wouldn't need to be applied to the model. You can run the following code and see what I mean:

<?php

class Template
{
    protected $variables;

    public function assign($name, $value)
    {
        $this->variables[$name] = new Escaper($value);
    }

    public function get($var)
    {
        return $this->variables[$var];
    }
}

class Escaper
{
    protected $variable;

    public function __construct($variable)
    {
        $this->variable = $variable;
    }

    public function __get($attribute)
    {
        return new Escaper($this->variable->$attribute);
    }
}

class User {
    public $username;
}

// Create object
$user = new User;
$user->username = 'baileylo';

// assign object
$template = new Template;

$template->assign('user', $user);

echo '$user is of type: ' . get_class($template->get('user')) . PHP_EOL;
echo '$user->username is of type: ' . get_class($template->get('user')->username) . PHP_EOL;

But I could be missing something.

from plates.

@baileylo That is VERY interesting. This has the potential of actually working.

Edit: I moved this idea to a new issue (#24) since it no longer pertains to the compiler.

from plates.

https://gist.github.com/shadowhand/039d72433aa262f10b91

This is (roughly) an implementation of automatic escaping in views. It's incomplete, but should be enough to show what I mean about a read-only mode that triggers automatic escaping.

from plates.

@shadowhand Thanks, do you mind maybe moving that to issue #24? I want to keep this issue focused on the compiler.

from plates.

@reinink done.

from plates.

@lalop I've been thinking about you're approach to escaping in the compiler, and I think we could make this this work:

• Anything echoed using <?= or echo is escaped. This way there is consistency between these two forms of output. This even includes echo content inside multiple line blocks of PHP.
• Adding a raw() template function around echoed content will tell the compiler NOT to escape it.
• Template functions (including extensions) be defined as "do not escape", shortening the template syntax for functions that will never need escaping (ie. get() or section()).

Before and after examples:

<h1>Hello, <?=$name?>!</h1>
<h1>Hello, <?php echo $this->escape($name)?>!</h1>

<h1>Hello, <?php echo $name ?>!</h1>
<h1>Hello, <?php echo $this->escape($name)?>!</h1>

<h1><?php echo 'Hello, ' . $name . '!' ?></h1>
<h1><?php echo $this->escape('Hello, ' . $name . '!' )?></h1>

// Note how the raw() function does nothing here
<h1><?php echo 'Hello, ' . raw($name) . '!' ?></h1>
<h1><?php echo $this->escape('Hello, ' . $name . '!' )?></h1>

<h1>Hello, <?=raw($name)?>!</h1>
<h1>Hello, <?php echo $name?>!</h1>

<h1>Hello, <?=$this->batch($name, 'strtoupper')?>!</h1>
<h1>Hello, <?php echo $this->escape($this->batch($name, 'strtoupper'))?>!</h1>

// get() is a "do not escape" function
<?=get('sidebar')?>
<?php echo $this->get('sidebar')?>

// Automatically prevent double escapes
<h1>Hello, <?=escape($name)?>!</h1>
<h1>Hello, <?php echo $this->escape($name)?>!</h1>

from plates.

If we go in this way we have to list some unescapable functions like get() , section() ... but user defined functions too.

from plates.

Agreed, that list would be placed on the escaping page. Alternatively, we don't have functions that are not escaped, but that would get really annoying:

<?=raw(get('sidebar'))?>

from plates.

yes or we can do it in the other way and assuming that each function
manage its escaping policy and don't escape the function result untill
it's not explicitly asked.

<?= get('sidebar') ?> => <?php echo get('sidebar') ?>
<?= escape(get('sidebar')) ?> => <?php echo escape(get('sidebar')) ?>

from plates.

@lalop Errr...then we're right back to manually escaping. 😕

from plates.

for functions yes

from plates.

or just let the few template engine's function unescaped than escape all the rest

from plates.

I think it's helpful to have functions escaped by default, since the big goal here is to make templates safer by auto-escaping. This forces developers to intentionally output raw variables, making them much more aware of the risks in doing so.

Generally all functions would be escaped, except in situations where the function offers assistance with HTML, such as the built-in get() method. I think it's wise to also give extension developers the freedom to make this decision for their template functions. For example:

// A function that should be escaped
<?=uppercase($name)?>

// A function that shouldn't be escaped
<?=html()->css_tag('styles.css')?>

If an extension function is not auto-escaped, it would be the extension developer's responsibility to handle the escaping within the extension.

from plates.

I have a working copy of the new compiler (locally) and hope to commit it soon so others can try it. Last night I switched from manually parsing the templates using regular expressions to @nikic's PHP-Parser. This works much nicer, as the hard regex work is already done in that library. A little snippet:

// Insert escape method
$node->exprs = array(
    new \PhpParser\Node\Expr\MethodCall(
        new \PhpParser\Node\Expr\Variable('this'),
        new \PhpParser\Node\Name('escape'),
        $node->exprs
    )
);

I also want to improve how the compiler handles temporary files when caching is disabled. Right now I'm just creating temporary files (tempnam(sys_get_temp_dir(), 'plates_')), but I wonder if streams would be more performant. I got this idea from Zend View, which also has a compiling stage that converts short open tags (<?) to regular open tags.

from plates.

As noted earlier, we need a way to define extensions functions as either "raw" or "escaped". Clearly this setting will only have an impact when the compiler is enabled. Here are three options. My pick is number three. Yes, it breaks backwards compatibility, but since Plates 3.0 will have so many breaking changes, having another one doesn't really matter.

<?php

// Option #1
// Underscore indicates a raw function
// All other functions will be escaped
// No backwards compatibility breaks with existing extensions
// Underscore could conflict with function names, if they started with an underscore

public function getFunctions()
{
    return array(
        '_some_html_function' => 'someHtmlFunction',
        'uppercase' => 'convertStringToUppercase'
    );
}

<?php

// Option #2
// Method returns an array with two functions sets: raw and escaped
// Breaks backwards compatibility with existing extensions
// Array structure is a little cumbersome

public function getFunctions()
{
    return array(
        'raw' => array(
            'some_html_function' => 'someHtmlFunction'
        ),
        'escaped' => array(
            'uppercase' => 'convertStringToUppercase'
        )
    );
}

<?php

// Option #3
// Two separate methods for each function type
// Breaks backwards compatibility with existing extensions
// It's very clear what's going on
// Both functions would be required by the interface
// If no functions exist, simply return an empty array()

public function getRawFunctions()
{
    return array(
        'some_html_function' => 'someHtmlFunction'
    );
}

public function getEscapedFunctions()
{
    return array(
        'uppercase' => 'convertStringToUppercase'
    );
}

from plates.

Love the number 3 too but maybe there is a 4th one:

// Option #4
// Consider extension contructor as a bootstrap 
// and let the extension register function
public function __construct()
{
$this->registerRawFunction('some_html_function', 'someHtmlFunction');
$this->registerRawFunctions([
    'some_html_function' =>  'someHtmlFunction'
]);
}
$this->registerEscapedFunction('uppercase', 'convertStringToUppercase');

Maybe more than a string we can define the callback via a callable element

public function __construct()
{
$this->registerRawFunction('some_html_function',[$this, 'someHtmlFunction']);
}

from plates.

Honestly, I think this entire concept of "compiling" PHP to PHP is completely the wrong approach. Bowing out of the conversation before I start saying things I regret.

from plates.

@shadowhand Fair enough. Thanks for participating until this point!

from plates.

@lalop Using the constructor as a bootstrap is actually a cool idea, except the function registering has to happen at the engine level, and the engine isn't available when an extension is first instantiated.

You could, however, have a register() method that does the same thing. This function would automatically be called when you call $engine->loadExtension(new \Your\Extension). I also really like the idea of simply making the second parameter a callable element. A complete example:

<?php

namespace League\Plates\Extension;

class Asset implements ExtensionInterface
{
    public $engine;
    public $template;

    public function register()
    {
        $this->engine->registerRawFunction('some_html_function', [$this, 'someHtmlFunction']);
        $this->engine->registerEscapedFunction('uppercase', 'strtoupper');
    }

    public function someHtmlFunction($text)
    {
        return '<html>' . $text . '</html>';
    }
}

from plates.

This actually also has the added benefit of adding one-off template functions if you wanted. Example:

<?php

// Create engine
$engine = new \League\Plates\Engine('templates', 'tpl');

// Register one off function
$engine->registerEscapedFunction('uppercase', function ($string) {
    return strtoupper($string);
});

// Display view
echo $engine->render('home');

from plates.

Actually, we can simplify the extensions even further, by simply adding a third parameter to registerFunction($name, $callback, $raw = false) method . This cleans things up for those who are not using the compiler. Also, since we only really need the $engine object to register the functions, it can just be passed to the register() method. If an extension registers the engine later on, it can save it as an object variable.

<?php

namespace League\Plates\Extension;

class Asset implements ExtensionInterface
{
    public $template;

    public function register($engine)
    {
        $engine->registerFunction('some_html_function', [$this, 'someHtmlFunction'], true);
        $engine->registerFunction('uppercase', 'strtoupper');
    }

    public function someHtmlFunction($text)
    {
        return '<html>' . $text . '</html>';
    }
}

And to register a one off function:

$engine->registerFunction('some_html_function', function ($string) {
        return '<html>' . $text . '</html>';
}, true);

from plates.

I disagree with adding a flag parameter to the method. I think it's a cleaner read with registerRawFunction and registerEscapedFunction as separate methods. Martin Fowler has some good rationale for not using flag parameters.

Then, to keep things clean for people not using the compiler, registerFunction could be added to the engine as an alias for registerRawFunction, since all functions will be raw functions without the compiler.

from plates.

The compiler has bee added (8a51dcc) and will be tagged with the Plates 3.0.0 release. Still have a bunch of tests to create and documentation to write before this will happen though. Just wanted to get the code up on Github in case anyone wanted to try this using dev-master. Since no documentation exists yet, here is a simple example:

Controller

<?php

// Create new Plates engine
$templates = new \League\Plates\Engine('templates', 'tpl');

// Enable compiler
$templates->enableCompiler('.cache');

// Render template
echo $templates->render('profile', ['name' => 'Jonathan']);

Template

// You can use short open tags, even if they're not enabled
// All echoed variables will be automatically escaped
// To prevent auto escaping, wrap the output with the raw() function
// Template functions do not require $this

<? layout('template', ['title' => 'User Profile']) ?>

<h1>Hello, <?=$name?></h1>

<h1>Hello, <?=raw($name)?></h1>

<h1>Hello, <?=batch($name, 'strtoupper')?></h1>

from plates.

By the way, if anyone has been watching the 3.0 release, you'll notice that I actually removed all the compiling features prior to this launch. This is something I pushed really hard for, and I actually had it working very well. And yet, based on all the feedback I received, only a very small portion thought it was a good idea.

In short, people like Plates because it's extremely simple, and extremely fast. Adding a compiler, despite it's obvious benefits, added a layer of unwanted complexity.

Sorry to anyone who was holding out for this feature.

from plates.