Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

Higher-order sections #4

Closed
bobthecow opened this Issue Jul 5, 2010 · 17 comments

Comments

Projects
None yet
6 participants
Owner

bobthecow commented Jul 5, 2010

The current higher-order section implementation in Mustache.php is a bit dangerous (see feature/higher-order-sections or 60d593d).

"Higher-order sections" is a feature of Mustache that allows template manipulation or pre-processing. If the tag referenced by a section returns a function, this function will be called and passed the body of the section. Whatever the function returns will replace the body of the section. See the 'lambdas' section here for examples.

This works great in Ruby and Python, but can have unexpected side effects in PHP. The current implementation assumes that anything 'callable' should be treated as a higher-order function. Consider the following:

<?php

class Monster extends Mustache {
    public $_template = '{{#title}}{{title}} {{/title}}{{name}}';
    public $title;
    public $name;
}

$frank = new Monster();
$frank->title = 'Dr.';
$frank->name  = 'Frankenstein';
$frank->render();

$dracula = new Monster();
$dracula->title = 'Count';
$dracula->name  = 'Dracula';
$dracula->render();

?>

The first monster renders as Dr. Frankenstein, just as one would expect. But the second monster renders as 1Dracula since, according to PHP, the string 'Count' is a valid callback for count(). This is not desirable.

I see a couple of options:

  1. Only allow PHP 5.3+ anonymous functions for higher-order sections. This keeps us from executing unintentional callbacks, but leaves all the poor PHP 5.2 folks out of the fun.
  2. Only allow anonymous functions or class-based callbacks for higher-order sections. i.e. array($this, 'myCallback') is a valid class-based callback, as are array('SomeClass', 'myCallback') and 'SomeClass::myCallback' ... 'count', however, is not.
  3. Keep the current implementation and let the buyer beware. This seems like a bad idea, so I'd suggest not voting for number 3.

rafi commented Jul 20, 2010

Option 2 makes most sense to me

Option 1 is the correct answer, but I suppose if we want to support 5.2 then we are forced to do Option 2.

rafi commented Jul 21, 2010

exactly :) php 5.3 doesn't have a wide support with hosting companies yet

Zeelot commented Aug 6, 2010

option 2 sounds good, I would also be fine with option 1... note that 5.2 is already out of support from PHP so adoption will have to be quicker for 5.3 than it was for php 5

Owner

bobthecow commented Aug 9, 2010

I agree that option 2 is the best for now. It has been implemented in 18bffda

With this fix I feel like this feature is ready to release. If you've got a minute, please play with the higher-order sections feature branch a bit... Feel free to provide test cases as well :)

Zeelot commented Aug 18, 2010

could you merge dev or master into this branch? there are a few differences it seems that don't relate to the feature so I am a little reluctant to switch to this branch on my project... am willing to test this feature on a project but would like to make sure old bugs don't sneak in

Owner

bobthecow commented Aug 18, 2010

Dev is merged into feature/higher-order-sections now. Thanks for checking it out.

Zeelot commented Aug 18, 2010

I'm not too sure the array syntax should be supported. The only reason is the slight chance that I would enable the implicit iterator pragma and actually return an array values that I wanted displayed instead of called. Is this a valid concern or am I over thinking things?

Owner

bobthecow commented Aug 18, 2010

I think you might be over thinking things. The only time you'll have a collision is if your array has exactly two elements, and the first one is a valid class name or an object, and the second value is a valid function name on that class. This seems like a small enough chance to me....

Zeelot commented Aug 30, 2010

hmm... ran into a little problem here... how do you render the content inside of the lambda without leaving the context of the view?

public function full_date()
{
    return function($string)
    {
        return date_create($this->render($string))
            ->format(Kohana::config('dates')->output['default']);
    };
}

$this in the above example does not work '[ Fatal Error ]: Using $this when not in object context'

Is this because the function is not in the context of the view at the point of being called? Or am I doing something else wrong? This seems to be how it gets used in Ruby

It's because the view ($this) is not in the scope of the lambda function. You need to first create a closure on that variable. This is because PHP does not automatically close on variables from the parent scope like Ruby or JavaScript... or most other scripting languages.So, in order to use $this, you need to create a closure on $this... which PHP does not allow due to the special nature of the $this variable. So, to get around that, you will first need to assign $this to another variable, like $view in the following example:

public function full_date()
{
    $view = $this; // Assign $this to another variable
    return function($string) use ($view) // The "use" keyword creates a closure
    {
        return date_create($view->render($string))
            ->format(Kohana::config('dates')->output['default']);
    };
}

Keep in mind that you will only be able to use public methods on the view class.

I'm not aware of any other way to do this. If someone else has a different idea, I'd love to hear it.

Contributor

damz commented Dec 7, 2010

A mix of Option 1 and 2 sounds best for me, but requiring an array form for Option 2.

Ie:

(is_object($var) || is_array($var)) && is_callable($var);

That allow every possible cases of Option 2 available (including calling a static class method), while making sure that no string will possibly be confused for a lambda.

Owner

bobthecow commented Dec 7, 2010

The current implementation in the feature branch uses the following logic:

In Mustache.php, a variable is considered 'callable' if the variable is:

  1. an anonymous function.
  2. an object and the name of a public function, i.e. array($SomeObject, 'methodName')
  3. a class name and the name of a public static function,
    i.e. array('SomeClass', 'methodName')
  4. a static function name in the form 'SomeClass::methodName'

Does this seem reasonable?

Contributor

damz commented Dec 7, 2010

I'm unconfortable with 'SomeClass::methodName', because it has the same confusion factor as described in the original post.

I implemented the change in damz@fcffe3e

Contributor

scribu commented Aug 27, 2011

I agree with damz that 'SomeClass::methodName' is too risky.

(!is_string($var) && is_callable($var)) is the sweet spot, I think.

Contributor

damz commented Aug 27, 2011

I updated and rebased the change in damz@ac418ad

Owner

bobthecow commented Aug 28, 2011

Thank you. Merged. I think it's about time to close this issue :)

@bobthecow bobthecow closed this Aug 28, 2011

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