v1.3 contains breaking changes

[nelsonpecora]

Calling include on an absolute path (e.g. /path/to/tpl.nunjucks) works in v1.2, but not in v1.3. In v1.2 it builds the path from the web root (or possibly cwd?), whereas in v1.3 (because of the relative paths) I think it's looking at the filesystem root. This is a good and reasonable change (and relative paths are incredibly useful), but it's a breaking change and probably shouldn't be a minor release.

[carljm]

I don't have strong feelings about the version numbering, but I'll note one other breaking change in 1.3 that would have been useful to list in the release notes: the shift from env.autoesc to env.opts.autoescape. Maybe that's supposed to be private api, but it's often necessary for custom filters to access that value in order to behave properly. The upgrade to 1.3 required updating a number of our custom filters.

[jlongster]

@yoshokatana it shouldn't be looking at the filesystem root, since that's dangerous. We try to only allow including templates from inside the base "view" path. I'll take a look and see if it's a bug or not.

@carljm oh, that's a good point. We probably need better docs about this, but that was considering an internal API. The intention was that if you accept a string and return a string, you use the copySafeness function in runtime.js. This makes sure that if the original string was a SafeString, you return a SafeString as well. Can you paste one of your filters and see if that works or not?

A minor bump can have breaking changes if they are small (most apps should keep working but a few might break), I thought. I'd say @yoshokatana's description might be a big change and did warrant a major change. The env.autoesc is a small change since you shouldn't be depending on that, but maybe more filters are than I think.

[nelsonpecora]

@jlongster thanks for taking a look! :-)

[boutell]

@jlongster if you're trying to follow the semver standard, all bc breaks require a new major version. Minor versions can introduce new features but can't change existing APIs (unless via optionally setting a flag to turn on the new behavior, etc).

This is a very strict standard which we are nowhere near ready to follow ourselves with Apostrophe, but FYI.

[boutell]

When I say "can't change existing APIs," that's slightly too strict - you could introduce new properties to an options object, or add an additional non-required parameter to the end of a method, etc. But changing the behavior of the "public API" in a way that causes existing code to fail isn't supposed to be cool under semver.

See the underscore kerfluffle:

jashkenas/underscore#1805

[jlongster]

@SamyPesse can you take a look at the first issue? I see why it happens in the code, and I think we should make it work for now. We can remove the hack later. I can't figure out where the best place is it make it work. We should be able to see if a template name starts with / and just strip it.

@yoshokatana any reason why you use it? that just happened to work before, but it should work exactly the same with the forward /.

[jlongster]

@boutell ah I may have misunderstand that a little bit. I'll keep that in mind for the future, thanks!

[carljm]

@jlongster I don't think copySafeness alone is sufficient to meet the use cases. Consider a simple linebreaks filter that translates newlines into HTML <br> tags (or any filter which may introduce known-safe HTML into a previously should-be-HTML-free string). If autoescaping is on, such a filter needs to forcibly escape the incoming string (unless it is a SafeString) before doing its own transformation, then unconditionally mark the outgoing string as safe. If autoescaping is off, the filter should not forcibly escape the incoming string. Correct handling for this type of filter requires checking the autoescape setting (unless there's some other utility I'm not aware of for handling this logic.)

[nelsonpecora]

@jlongster I think it was just a shorthand for [view path] + '/path/to/template'. We're changing those to be less ambiguous, but it sorta threw us for a loop when we npm install'd and all our templates broke.

In the new logic, [view path] + './path/to/template' would work, correct?

[jlongster]

@yoshokatana if you include a template with ./, it will load it relatively to the current one. Otherwise, it will load it from the view path. So if you want it to be [view path] + [template name], just don't prefix it with anything and say include "path/to/template.html"

[jlongster]

@carljm sorry for letting this go for a bit, do you think we should add a getter for env.autoesc so it's backwards-compatible?

[carljm]

@jlongster I dunno how much value there is in doing that now - it certainly wouldn't make any difference to me, since I've already updated the affected code. I think it's more relevant to agree that user filters do need to access that value, so its location should be documented and covered by backwards compatibility in the future.

[nelsonpecora]

Yup, I agree with @carljm

[jlongster]

ok I documented the autoescape property: 6469c4d

I think we're done here, as the breaking change was really just unspecified before