Skip to content
New issue

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

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
nanodocumet opened this issue Apr 2, 2011 · 7 comments
Closed

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

nanodocumet opened this issue Apr 2, 2011 · 7 comments

Comments

@nanodocumet
Copy link

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
Copy link
Contributor

Pretty sure this is a duplicate of #19.

@nanodocumet
Copy link
Author

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
Copy link
Contributor

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
Copy link
Author

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
Copy link
Contributor

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
Copy link
Author

Yes. thanks.

@shadowhand
Copy link
Contributor

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

ernestas pushed a commit to ernestas/KOstache that referenced this issue Jun 21, 2011
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants