Skip to content
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

Use explicit userInfo keys for NSManagedObjectContextDidSaveNotification #2449

Merged
merged 7 commits into from Aug 9, 2016

Conversation

alexanderedge
Copy link
Contributor

In iOS 10 beta 4 there is a new key in the userInfo dictionary, managedObjectContext, so the existing collection operator causes a crash as it assumes all values are NSSets.

I have added explicit checking for inserted, deleted and updated objects.

In iOS 10 beta 4 there is another key in the `userInfo` dictionary, `managedObjectContext`, so the collection operator causes a crash as it assumes all values are `NSSet`s.

I have added explicit checking for inserted, deleted and updated objects.
@valeriomazzeo
Copy link
Member

it looks good to me 👍
@segiddins ?

@@ -24,6 +24,8 @@

@class RKManagedObjectStore;

extern NSSet *RKSetOfManagedObjectIDsFromManagedObjectContextDidSaveNotification(NSNotification *notification);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd just redeclare this in the test file, I don't think we should be exposing this as part of public API

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe useful, an alternative is to put it into a private extension not exported in the public headers but imported into the test file rather than redeclare it, at least it's always in one place

Adding the definition immediately before the function in RKManagedObjectStore.m removes a “No previous prototype for function…” warning.

Tests build by using `extern` with the same definition.
@alexanderedge
Copy link
Contributor Author

Thanks a lot for the comments. I'll add separate private {h,m} files for this function as @valeriomazzeo suggests, as that seems neater than what we currently have here.

unionObjectIDs(objectIDs,notification.userInfo[NSDeletedObjectsKey]);

return objectIDs;
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

my preference would be to leave the implementation inside RKManagedObjectStore.m

@alexanderedge
Copy link
Contributor Author

No problem – I've moved the impl into RKManagedObjectStore.m based on your comment.

@valeriomazzeo
Copy link
Member

I think the only thing missing is to specify that header is private both in the project file and in the podspec.

screen shot 2016-08-04 at 17 31 17

@alexanderedge
Copy link
Contributor Author

While I was there I noticed another header that should be private and added that (RKMappingOperation_Private.h)

@valeriomazzeo valeriomazzeo merged commit 9cbba9e into RestKit:development Aug 9, 2016
@alexanderedge alexanderedge mentioned this pull request Aug 9, 2016
@SandyChapman
Copy link

@segiddins @valeriomazzeo : Any idea when the next release is scheduled? I'd like to incorporate this fix into my project and would want to do it off a patch release rather than a specific commit.

@NelepovDmitry
Copy link

Hi every one, then it fix will be in cocoapods? Now there version RestKit 0.27.0 but still:

static NSSet *RKSetOfManagedObjectIDsFromManagedObjectContextDidSaveNotification(NSNotification *notification) { NSUInteger count = [[[notification.userInfo allValues] valueForKeyPath:@"@sum.@count"] unsignedIntegerValue]; NSMutableSet *objectIDs = [NSMutableSet setWithCapacity:count]; for (NSSet *objects in [notification.userInfo allValues]) { [objectIDs unionSet:[objects valueForKey:@"objectID"]]; } return objectIDs; }

@SandyChapman
Copy link

@NelepovDmitry : They haven't released a version with the fix. You can try this in your Podfile:

pod 'RestKit', :git => 'https://github.com/RestKit/RestKit.git', :commit => '13d98d5a6a5e06656ad040013dcae149b7cf8b99'

@Sajjon
Copy link

Sajjon commented Sep 21, 2016

I also see this warning in the debugger:

userInfo = {
   NSTargetObjectUserInfoKey = "<_PFWeakReference: 0x1704211c0>";
   NSUnknownUserInfoKey = "@count";
}

I dont really understand, is this just a warning? Or has something indeed failed? Everything seams to be working... :S

@RustamKhisamov
Copy link

I used this commit
pod 'RestKit', :git => 'https://github.com/RestKit/RestKit.git', :commit => '9cbba9eb1b490c3c5e2873c8fba8e9c0fec1bf07'

danielaguta pushed a commit to Riidr/RestKit that referenced this pull request Sep 26, 2016
…cation` (RestKit#2449)

* Use specific `userInfo` keys

In iOS 10 beta 4 there is another key in the `userInfo` dictionary, `managedObjectContext`, so the collection operator causes a crash as it assumes all values are `NSSet`s.

I have added explicit checking for inserted, deleted and updated objects.

* Expose function for testing

* Add test case

* Remove function from public interface

Adding the definition immediately before the function in RKManagedObjectStore.m removes a “No previous prototype for function…” warning.

Tests build by using `extern` with the same definition.

* Add header for private function used in test

* Move implementation into RKManagedObjectStore.m

* Specify private headers in Podspec
@shial4
Copy link

shial4 commented Oct 5, 2016

in what version of restKit it will be included avaiable from cocoapods?

@devcarlos
Copy link

We need this fix as soon as possible ready on CocoaPods

@shial4
Copy link

shial4 commented Oct 5, 2016

This will come with https://github.com/RestKit/RestKit/issues/2459 correct?

@valeriomazzeo
Copy link
Member

valeriomazzeo commented Oct 7, 2016

If anyone can confirm
pod 'RestKit', :git => 'https://github.com/RestKit/RestKit.git', :commit => '13d98d5a6a5e06656ad040013dcae149b7cf8b99'
fixes the issue, either me or @segiddins can make an hotfix release (0.27.1).

I didn't really follow on #2459

Does this pull request fixes it?

@danielgronlund
Copy link

@valeriomazzeo Yep, it does seem to resolve the exception issues for me! Hotfix release would be appreciated!

@balazsnemeth
Copy link

👍 for the hotfix release!

@SriramKrishnan8
Copy link

@SandyChapman and @RustamKhisamov : I used the commits you had specified, but still the issue persists. Initially I tried updating the pod to the latest version, but that didn't work. I had to modify the code directly in RKManagedObjectStore.m and add the header file RKMappingOperation_Private.h. Only then it worked. Is there any other commit that works?

@valeriomazzeo
Copy link
Member

valeriomazzeo commented Oct 16, 2016

I have started some work on #2466 and #2467 in order to release 0.27.1, but there are issues with some of the dependencies.

Once those two PR will be passing on Travis then we can release.
Any help is appreciated.

@zsoltbalint
Copy link

This fixed for me:
pod 'RestKit', :git => 'https://github.com/RestKit/RestKit.git', :commit => '13d98d5a6a5e06656ad040013dcae149b7cf8b99'

But it would be really good to have a stable version.

@SandyChapman
Copy link

@zsoltbalint : If it makes you feel any better, that commit is identical to the 0.27.0 release with the addition of the single fix required.

@zsoltbalint
Copy link

Thanks @SandyChapman , for clarification! 👍

@valeriomazzeo valeriomazzeo mentioned this pull request Oct 22, 2016
falkobuttler pushed a commit to JIFFinc/RestKit that referenced this pull request Oct 25, 2016
* development:
  New changes to CocoaPods (RestKit#2443)
  Use explicit `userInfo` keys for `NSManagedObjectContextDidSaveNotification` (RestKit#2449)

# Conflicts:
#	Code/CoreData/RKManagedObjectStore.m
mmackowiak pushed a commit to sgrouples/RestKit that referenced this pull request Nov 7, 2016
…cation` (RestKit#2449)

* Use specific `userInfo` keys

In iOS 10 beta 4 there is another key in the `userInfo` dictionary, `managedObjectContext`, so the collection operator causes a crash as it assumes all values are `NSSet`s.

I have added explicit checking for inserted, deleted and updated objects.

* Expose function for testing

* Add test case

* Remove function from public interface

Adding the definition immediately before the function in RKManagedObjectStore.m removes a “No previous prototype for function…” warning.

Tests build by using `extern` with the same definition.

* Add header for private function used in test

* Move implementation into RKManagedObjectStore.m

* Specify private headers in Podspec
ivkosh added a commit to ivkosh/RestKitMapper that referenced this pull request Nov 15, 2016
@saiday
Copy link

saiday commented Nov 24, 2016

Can we have a CocoaPods release with this wonderful fixes?

murraycu added a commit to murraycu/ios-galaxyzoo that referenced this pull request Dec 17, 2016
To fix this problem:
RestKit/RestKit#2449 (comment)
Hopefully a real version will be released at some point.
This required me to update cocoapods like so before running
pod install:
https://stackoverflow.com/questions/39980096/xcode8-cocoapods-abort-trap6/40438232#40438232
@gvklooster
Copy link

Any news on when the new version with this fix will be available?

@honkmaster
Copy link

Seems like Rest.Kit is somewhat dead ...

@aron878
Copy link

aron878 commented Feb 1, 2017

pod 'RestKit', :git => 'https://github.com/RestKit/RestKit.git', :commit => '13d98d5a6a5e06656ad040013dcae149b7cf8b99'

It fixed my all issues with RestKit by upgrading to XCode 8 from XCode 7.

Thanks guys. 👍

@ecstasy2
Copy link

Such a critical fix should have already been released :-(

@honkmaster
Copy link

Again: looks like RestKit died the typical open source death. Rest In Peace. It's sad to see, but developers move on, especially when they earn no money.

@segiddins
Copy link
Member

see #2466

@honkmaster
Copy link

honkmaster commented Feb 14, 2017

@segiddins I see that with #2466, you are hinting to a 0.27.1 realease. However, this PR is long overdue and not merged into the master. There is maybe an error with Travic CI, but "some" maintainer should take it as a starting point and make the final step.

The next sentences may sound harsh and I feel sorry in advance (as I really understand those situations): If you (or someone else) does not have the time to maintain the component please state this publicly by making RestKit deprecated. Maybe this also helps bringing other people to the project.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet