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

Add framework target #97

Closed danielphillips closed 9 years ago

danielphillips commented 9 years ago

Please consider this pull request which adds a framework target in order for me to use Carthage to clone and build your project into a framework.

groue commented 9 years ago

Hello Daniel!

Thanks a lot, that's a very welcome pull request. Carthage integration was also a request from our fellow Marc Palmer, so let me invite him here: hello, @marcpalmer!

I have two immediate questions, before I dive deeper in your PR:

  1. I find no reference of JRSwizzle in your commit's diff.

    JRSwizzle is necessary for +[GRMustache preventNSUndefinedKeyExceptionAttack] (https://github.com/groue/GRMustache/blob/master/Guides/runtime.md#nsundefinedkeyexception-prevention).

    Now let's be serious. Pragmatically speaking, this method could now be removed. GRMustache has a default security policy that prevents any method from being invoked from Mustache templates, and this practically removes the need for NSUndefinedKeyException prevention.

    So I suggest that we remove all support for this feature (leaving no dead code of course), update the docs accordingly, and bump the major version of GRMustache to 8.0.0.

  2. IPHONEOS_DEPLOYMENT_TARGET = 9.0

    Doesn't Carthage supports iOS8? Is there a reason for your choice?

danielphillips commented 9 years ago

Carthage only requires the availability of dynamic frameworks. iOS 9.0 reference as a deployment target was my mistake, I didn't pay attention last night and was on Xcode 7.0.

I will remove JRSwizzle submodule and associated code around NSUndefinedKeyException prevention.

groue commented 9 years ago

That's pretty cool, thank you Daniel :-)

danielphillips commented 9 years ago

By leaving no dead code, do you still want me to deprecate the methods in question or simply remove them?

Probably deprecating is preferred although I will end up with some empty no-op methods. In such empty methods it would make sense to raise an exception or have an assert, the former would of course be hilariously ironic. :)

groue commented 9 years ago

If we were to deprecate preventNSUndefinedKeyExceptionAttack, we would need to keep JRSwizzle, so that it would still work.

Yet if we remove this method, and JRSwizzle with it, GRMustache-enabled applications won't lose anything at all:

PreventNSUndefinedKeyExceptionAttack used to be useful a few years ago, when GRMustache had no security and would call valueForKey: for any key it found. Debugging a template was a pain because the debugger would trap on all undefined keys, even though GRMustache would catch these, until the object that would provide a key was found in the context stack. preventNSUndefinedKeyExceptionAttack was a debugging convenience, a way to prevent those exceptions while debugging templates.

Since v7.0.0, GRMustache does not blindly call valueForKey:. It first checks if the key is safe, and by default safe keys are properties and managed Core Data attributes. In those default safety settings, no NSUndefinedKeyException is ever thrown, so preventing them is pointless. Use can still override default safety settings, and in this case, GRMustache may still have to catch NSUndefinedKeyException. We need to keep on catching exceptions. Yet the prevention of those exceptions is no longer a common use case: not many library users override the default safery settings. So we do not need to keep on preventing exceptions.

So the gain in not worth the pain. Deprecating preventNSUndefinedKeyExceptionAttack, and finding a way to integrate JRSwizzle in a way that does not break applications that would link to both the future GRMustache framework and JRSwizzle without any conflict is just too much work for a feature which is no longer that useful. Let's push GRMustache forward!

groue commented 9 years ago

So yes, I really meant "remove".

Don't spend too much time on that, though: I can handle my part of the cleanup job, even if I do appreciate clean PRs. I'll have to add a Carthage-enabled OSX target, anyway : this PR implies some work for me as well :-)

groue commented 9 years ago

BTW: let me bump the library version. I'll take the opportunity of a major version bump to provide a better handling of https://github.com/groue/GRMustache/issues/66.

danielphillips commented 9 years ago

I've removed my git tags for the version number. And I've reverted the availabilities macro header to no longer reference a version 8.0, I'll leave the versioning to you.

I think that means for iOS this PR is done. OS X Needs to be done still, I'll see if I can do it today.

groue commented 9 years ago

OK Daniel, thank you very much. I'll review your full PR very shortly. Thanks a lot for your dedication! About the OSX target, that would be very nice of you, but I already appreciate a lot your assistance: do not feel compelled to do do it!

marcpalmer commented 9 years ago

It would be great to get this landed. Much appreciate your efforts.

groue commented 9 years ago

Yes @marcpalmer, I understand. I won't have much time if the next few days, so I don't expect it to make much progress before next week. Maybe Daniels's branch could help you?

marcpalmer commented 9 years ago

Yeah, I am going to try with this in my Cartfile:

github "danielphillips/GRMustache" "f1f9970ad18caebf963c43d9d73f4ae6a511a4d9"
danielphillips commented 9 years ago

I'm not sure I can be of any more help here, @groue you got this but you just need a bit more time to wrap it up. @marcpalmer my repo isn't going anywhere so that's fine to use until this PR is merged.

groue commented 9 years ago

OK I'm happy you both are not blocked in a dead-end.

groue commented 9 years ago

Back to work. And for a start, let's accept your pull request!

groue commented 9 years ago

Folks, we are close: https://github.com/groue/GRMustache/issues/100