Skip to content

Add framework target #97

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

Merged
merged 7 commits into from
Sep 26, 2015
Merged

Add framework target #97

merged 7 commits into from
Sep 26, 2015

Conversation

danielphillips
Copy link

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

groue commented Sep 8, 2015

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

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

groue commented Sep 8, 2015

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

@danielphillips
Copy link
Author

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

groue commented Sep 8, 2015

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

groue commented Sep 8, 2015

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

groue commented Sep 8, 2015

BTW: let me bump the library version. I'll take the opportunity of a major version bump to provide a better handling of #66.

@danielphillips
Copy link
Author

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

groue commented Sep 8, 2015

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!

groue added a commit that referenced this pull request Sep 10, 2015
groue added a commit that referenced this pull request Sep 10, 2015
groue added a commit that referenced this pull request Sep 10, 2015
FIXME:
- @import GRMustache does not work in both test targets.
- tests for private APIs in iOS tests won't compile.
@marcpalmer
Copy link

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

@groue
Copy link
Owner

groue commented Sep 15, 2015

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

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

github "danielphillips/GRMustache" "f1f9970ad18caebf963c43d9d73f4ae6a511a4d9"

@danielphillips
Copy link
Author

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

groue commented Sep 16, 2015

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

groue added a commit that referenced this pull request Sep 26, 2015
@groue groue merged commit f7b7039 into groue:master Sep 26, 2015
@groue
Copy link
Owner

groue commented Sep 26, 2015

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

@groue groue mentioned this pull request Sep 26, 2015
27 tasks
@groue
Copy link
Owner

groue commented Sep 26, 2015

Folks, we are close: #100

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

Successfully merging this pull request may close these issues.

3 participants