zombor / KOstache

Logic-less View/Mustache Module for Kohana v3
MIT License
138 stars 43 forks source link

$this->_template not considered when using factory() #22

Closed nanodocumet closed 13 years ago

nanodocumet commented 13 years ago

Not sure if this is a bug or desired behavior. Example

View:

class View_Homepage extends Kostache {
    $this->_template = 'home';
    ...
}

When using (@ controller)

// __construct called directly, $template is NULL
// $this->template->_template will be point to home.mustache
$this->template = new View_Homepage;

// factory will call __construct with $template set to 'Homepage'
// $this->template->_template will be point to homepage.mustache
$this->template = Kostache::factory('Homepage');

Either __construct should check if $this->_template is set like this

public function __construct($template = NULL, array $partials = NULL)
{
    if ($this->_template)
    {
        // Load the template defined in the view
        $template = $this->_template;
    }
    elseif ( ! $template)
    {
        // Detect the template for this class
        $template = $this->_detect_template();
    }
    ...
}

Or that check can be done at the factory, I think the __construct way looks cleaner.

shadowhand commented 13 years ago

Pretty sure this is a duplicate of #19.

nanodocumet commented 13 years ago

You are totally correct.

It is a duplicate, however, the patch just partially solved the issue. The first if just checks for $template, it should be for $this->_template (line 61). My patch is to redo that sequence of if and else statements with the code above (lines 61 - 73).

Sorry, was late at night and forgot to mention #19 (even though I looked at it). I should of added a comment to the other issue instead of creating a new one, sorry.

shadowhand commented 13 years ago

I'm actually not sure this behavior would preferred... what if you want to overload the template that is defined in the view? Your patch would make it impossible to overload.

nanodocumet commented 13 years ago

How are you thinking of overloading the template defined in the view? Right now using the New View constructor and the factory() load different templates if $this->_template is defined (take a look of my example in original issue). So, which one should take precedence?

1) The name of the view to load an equivalent template? or 2) A defined $this->_template?

Either way, I think constructor and factory should return the same template.

For example, in Controller_Template, $template = 'template' by default, if I extend Controller_Template and don't overwrite $template, it doesn't mean the the new Controller name will be used for the template. Controller_Website extends Controller_Template does not use $this->template set to Website unless I specify explicitly. Same here I guess.

Am I missing something? thanks.

shadowhand commented 13 years ago

I think I see what you mean now, and I think the correct solution is to patch Kostache::factory. This would ensure that the template is always detected by Kostache::__construct, not before.

nanodocumet commented 13 years ago

Yes. thanks.

shadowhand commented 13 years ago

Do not include the template name in Kostache::factory, do all template detection in __construct, closed by d2eadd380c0c72e1c2dc3135b4de6c7fbe3124ad