implement default behavior for the partials_loader #134

Closed
wants to merge 5 commits into
from

Conversation

Projects
None yet
2 participants
Contributor

mattdeclaire commented Feb 15, 2013

Implement the default behavior for the option partials_loader as outlined here.

Owner

bobthecow commented Feb 15, 2013

Wow. Thanks for catching this.

Unfortunately now this will cause problems with the partials option (i.e. passing a partials array directly to the engine, or calling ->setPartials after instantiating.

I think the fix is probably best done in getPartialsLoader, something like:

return isset($this->partialsLoader) ? $this->partialsLoader : $this->loader;

... instead of passing back an ArrayLoader instance. Then setPartials would have to access the partialsLoader property directly before adding the partials, and set it to a new ArrayLoader instance if it's not set.

Contributor

mattdeclaire commented Feb 15, 2013

I suppose the issue is that the two options compete. partials says, "Use an ArrayLoader, (and use this array while you're at it)." partials_loader says, "Just get the partials from disk." The documentation seems to say that partials wins, so the fix could be as easy as:

if (isset($options['partials_loader'])) {
   $this->setPartialsLoader($options['partials_loader']);
} else if (!isset($options['partials']) && isset($options['loader'])) {
   $this->setPartialsLoader($options['loader']);
}
Owner

bobthecow commented Feb 15, 2013

True, but if neither option is set, the user should be able to call $mustache->setPartials(...) later. Setting the partials loader in the constructor would prevent that.

Contributor

mattdeclaire commented Feb 15, 2013

It seems that when partialLoader is an instance of Mustache_Loader_StringLoader, rendering {{> bar}} yeids bar. Is this the expected behavior?

Owner

bobthecow commented Feb 16, 2013

Yeah, that's the expected behavior. The StringLoader shouldn't really be used as a partials loader... it's really only for the passthrough style $mustache->render('{{ foo }}', $data) type calls.

Falling back to the StringLoader if no partials loader is defined definitely seems weird though.

Contributor

mattdeclaire commented Mar 12, 2013

So, what is to become of this?

Owner

bobthecow commented Mar 12, 2013

Sorry I've let this sit so long. I'll try to get it merged today.

Owner

bobthecow commented Mar 12, 2013

Thanks for your work on this. It has been merged into dev.

I adjusted it a bit so that the default StringLoader would never be used for partial loading fallback, since that's weird and unintuitive, in addition to being a backwards compatibility break. I also added test coverage for the partials loading fallback:

https://github.com/bobthecow/mustache.php/blob/dev/test/Mustache/Test/EngineTest.php#L243-L264

This will be in the next release. Thanks again!

bobthecow closed this Mar 12, 2013

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