groue / GRMustache

Flexible and production-ready Mustache templates for MacOS Cocoa and iOS
http://mustache.github.com/
MIT License
1.44k stars 190 forks source link

Cannot resolve partials relative to localized templates in app bundle #87

Closed marcpalmer closed 9 years ago

marcpalmer commented 9 years ago

I found a surprising problem with the latest GRMustache released to cocoapods (7.3.0) that I don't believe existed in the past i.e. it feels like a regression somewhere, either in the iOS 8 NSBundle APIs or GRMustache.

Here's the scenario:

See attached debugger screenshot showing the state of the stack and lldb output where I show that po [_bundle pathForResource:@"EmailShareFooter" ofType:@"mustache"] succeeds but [_bundle pathForResource:@"EmailShareFooter" ofType:@"mustache" inDirectory:relativePath]

It is obviously choking on the relativePath as the relativePath should be blank in this case, but it is '/en.lproj' so iOS is trying to look for <appfolder>/en.lproj/EmailShareFooter.mustache" and while this is correct, seems to fail, I'm guessing its actually looking for/en.lproj/en.lproj/EmailShareFooter.mustache` as the resource name.

screen shot 2014-12-05 at 11 42 58

groue commented 9 years ago

Hi @marcpalmer,

This regression is a side effect of #78 which has been shipped in v7.1.0.

OK, this deserves a fix.

marcpalmer commented 9 years ago

Yes, I just found the commit https://github.com/groue/GRMustache/commit/f598e62961bd7f27f3d21da353dda0f2915ac794 that seems to be responsible for this.

It seems quite tricky to solve... any thoughts? We can't know safely which part of the URL NSBundle is the localisation part can we? Isn't the "real" solution to keep the NSURL source of the file around and use that as baseTemplateURL instead of having a baseTemplateID?

marcpalmer commented 9 years ago

By this I mean, create a new NSURL relative the base NSURL that NSBundle originally returned, instead of doing string manipulations and asking NSBundle for a new URL all over again.

groue commented 9 years ago

It seems quite tricky to solve [...] We can't know safely which part of the URL NSBundle is the localisation part can we?

I had the same trend of thoughts :-) Yes it's tricky!

We should list all the use cases, given a sample hierarchy of resources:

o nonLocalized.mustache
o nonLocalizedSibling.mustache
o nonLocalizedTemplates
|--o a.mustache
|--o b.mustache
o en.lproj
|--o localized.mustache
|--o localizedSibling.mustache
|--o localizedTemplates
|  |--o a.mustache
|  |--o b.mustache
o fr.lproj
|--o localized.mustache
|--o localizedSibling.mustache
|--o localizedTemplates
|  |--o a.mustache
|  |--o b.mustache

MUST HAVE: Siblings - in the same realm

[GRMustacheTemplate templateFromResource:@"nonLocalized"] should be able to load:

- `{{> nonLocalizedSibling }}` /nonLocalizedSibling.mustache

[GRMustacheTemplate templateFromResource:@"nonLocalizedTemplates/a"] should be able to load:

- `{{> b }}` /nonLocalizedTemplates/b.mustache

[GRMustacheTemplate templateFromResource:@"localized"] should be able to load:

- `{{> localizedSibling }}` /en.lproj/localizedSibling.mustache

[GRMustacheTemplate templateFromResource:@"localizedTemplates/a"] should be able to load:

- `{{> b }}` /en.lproj/localizedTemplates/b.mustache

SHOULD HAVE: Siblings - accross realms (localized vs. non localized)

[GRMustacheTemplate templateFromResource:@"nonLocalized"] should be able to load:

- `{{> localized }}` /en.lproj/localized.mustache

[GRMustacheTemplate templateFromResource:@"localized"] should be able to load:

- `{{> nonLocalized }}` /nonLocalized.mustache

MUST HAVE: Absolute paths - in the same realm (localized vs. non localized)

[GRMustacheTemplate templateFromResource:@"nonLocalized"] should be able to load:

- `{{> /nonLocalizedSibling }}` /nonLocalizedSibling.mustache
- `{{> /nonLocalizedTemplates/b }}` /nonLocalizedTemplates/b.mustache

[GRMustacheTemplate templateFromResource:@"nonLocalizedTemplates/a"] should be able to load:

- `{{> /nonLocalized }}` /nonLocalized.mustache
- `{{> /nonLocalizedTemplates/b }}` /nonLocalizedTemplates/b.mustache

[GRMustacheTemplate templateFromResource:@"localized"] should be able to load:

- `{{> /localizedSibling }}` /en.lproj/localizedSibling.mustache
- `{{> /localizedTemplates/a }}` /en.lproj/localizedTemplates/a.mustache

[GRMustacheTemplate templateFromResource:@"localizedTemplates/a"] should be able to load:

- `{{> /localized }}` /en.lproj/localized.mustache
- `{{> /localizedTemplates/b }}` /en.lproj/localizedTemplates/b.mustache

MUST HAVE: Relative navigation - in the same realm (localized vs. non localized)

[GRMustacheTemplate templateFromResource:@"nonLocalized"] should be able to load:

- `{{> nonLocalizedTemplates/b }}` /nonLocalizedTemplates/b.mustache

[GRMustacheTemplate templateFromResource:@"nonLocalizedTemplates/a"] should be able to load:

- `{{> ../nonLocalized }}` /nonLocalized.mustache
- `{{> /nonLocalized }}` /nonLocalized.mustache

[GRMustacheTemplate templateFromResource:@"localized"] should be able to load:

- `{{> localizedTemplates/a }}` /en.lproj/localizedTemplates/a.mustache

[GRMustacheTemplate templateFromResource:@"localizedTemplates/a"] should be able to load:

- `{{> ../localized }}` /en.lproj/localized.mustache
- `{{> /localized }}` /en.lproj/localized.mustache

SHOULD HAVE: Absolute paths - accross realms (localized vs. non localized)

[GRMustacheTemplate templateFromResource:@"nonLocalized"] should be able to load:

- `{{> /localized }}` /en.lproj/localized.mustache
- `{{> /localizedTemplates/a }}` /en.lproj/localizedTemplates/a.mustache

[GRMustacheTemplate templateFromResource:@"nonLocalizedTemplates/a"] should be able to load:

- `{{> /localized }}` /en.lproj/localized.mustache
- `{{> /localizedTemplates/a }}` /en.lproj/localizedTemplates/a.mustache

[GRMustacheTemplate templateFromResource:@"localized"] should be able to load:

- `{{> /nonLocalized }}` /nonLocalized.mustache
- `{{> /nonLocalizedTemplates/b }}` /nonLocalizedTemplates/b.mustache

[GRMustacheTemplate templateFromResource:@"localizedTemplates/a"] should be able to load:

- `{{> /nonLocalized }}` /nonLocalized.mustache
- `{{> /nonLocalizedTemplates/b }}` /nonLocalizedTemplates/b.mustache

NOT SURE: Relative navigation - accross realms (localized vs. non localized)

[GRMustacheTemplate templateFromResource:@"nonLocalized"] should be able to load:

- `{{> localizedTemplates/a }}` /en.lproj/localizedTemplates/a.mustache

[GRMustacheTemplate templateFromResource:@"nonLocalizedTemplates/a"] should be able to load:

- `{{> ../localized }}` /en.lproj/localized.mustache

[GRMustacheTemplate templateFromResource:@"localized"] should be able to load:

- `{{> nonLocalizedTemplates/b }}` /nonLocalizedTemplates/b.mustache

[GRMustacheTemplate templateFromResource:@"localizedTemplates/a"] should be able to load:

- `{{> ../nonLocalized }}` /nonLocalized.mustache
- `{{> ../nonLocalizedTemplates/b }}` /nonLocalizedTemplates/b.mustache
groue commented 9 years ago

I should have a thourough review of those cases, decide whether they are valid or not, and turn them into tests. I'm not sure yet of the "MUST HAVE", "SHOULD HAVE", "NOT SURE", mentions as well.

groue commented 9 years ago

@marcpalmer I can't write any regression test.

Given the bundle:

o /path/to/bundle
|--o en.lproj
|  |--o Main.mustache      "{{> Footer }}"
|  |--o Footer.mustache    "Footer"

The following code runs fine and renders the expected content.

NSBundle *bundle = [NSBundle bundleWithPath:@"/path/to/bundle"];
GRMustacheTemplate *template = [GRMustacheTemplate templateFromResource:@"Main" bundle:bundle error:NULL];
NSString *rendering = [template renderObject:nil error:NULL];
XCTAssertEqualObjects(rendering, @"Footer");

What do I miss?

marcpalmer commented 9 years ago

Hmmm... the only difference I think is the bundle mechanism, which is likely the problem?

Our real code uses

        template = [GRMustacheTemplate templateFromResource:name bundle:[NSBundle mainBundle] error:&error];
groue commented 9 years ago

Damn. How should I write the test, then :persevere: ?

marcpalmer commented 9 years ago

I don't know, but I'll try to build you a quick sample project to prove it

groue commented 9 years ago

Well, why not, thanks. I'm trying to fetch documentation about NSBundle.

marcpalmer commented 9 years ago

FYI I can't reproduce this in isolation either now. I'm trying to recreate the problem in my working app (after I flattened my templates dir to not use localisations to get past this)... I think I must have had a problem with resource paths. I will let you know - don't work on it any more for now.

marcpalmer commented 9 years ago

Gwendal - please accept my apologies. I have investigated further and it looks like it must be the case that my footer template was not in the /en.lproj/ folder in the bundle, but the main template was. I have re-added them all to the correct folders and it is working currently.

I am really sorry for wasting your time. I was tricked by the Xcode resource grouping not mapping to the folder location on disk for that file :(

marcpalmer commented 9 years ago

Although... I'm not sure this is 100% the case as I wonder if it will fall back from a footer in say "fr.lproj" to one in "Base.lproj".

groue commented 9 years ago

OK Marc, no problem. It happens all the time, and I'm the first to blame the third-party code when it happens :wink:.

Yet, I know that my tests are lacking on bundle-based templates. As you say, some nasty edge cases my appear between localized, base-localized, non-localized resources, and today I don't have the slightest idea of the exact behavior of the library beyond the most simple cases!