[webview_flutter] Add getters and setters for cookies. #1880
Conversation
|
Thanks for the contribution! I'm following the initial PR review policy, this PR isn't trivial to review so I'm labeling it with "backlog" and we will prioritize according to the priority of the associated issues. |
|
Status on this PR? |
|
@joelbrostrom see the comment above - the issues will be prioritized as described in the wiki. |
|
How to use it |
|
现在更新版本里面没setcookie 呀，什么时候发布新版本 |
|
@joelbrostrom @18316603765 webview_flutter:
git:
url: git://github.com/devangels/plugins.git
ref: cookies
path: packages/webview_flutterThis does mean that any updates for |
|
Any news on it being added soon ? I can't seem to use the git path. |
|
I managed to add the package to my app, I needed to replace the 'git://github.com/devangels/plugins.git' to 'https://github.com/devangels/plugins.git' for it to work.
|
|
@jeroen-meijer can you merge this please. |
|
most of flutter webview, their all not method for the setcookies ,but communicate with js by it ,so can you add the api directly！ |
2 similar comments
|
most of flutter webview, their all not method for the setcookies ,but communicate with js by it ,so can you add the api directly！ |
|
most of flutter webview, their all not method for the setcookies ,but communicate with js by it ,so can you add the api directly！ |
|
Need it |
There was a problem hiding this comment.
Only tested for iOS. I quickly looked at the Android code and also found that you were only setting name and value as well. So I guess other properties must be supported there as well.
Thank you very much for this PR. I hope this get's merged soon. In the meantime I will make my custom Cookie Getter/Setter based on this :)
| - (void)setCookies:(FlutterMethodCall *)call result:(FlutterResult)result API_AVAILABLE(ios(11.0)) { | ||
| NSArray<CookieDto *> *cookieDtos = [CookieDto manyFromDictionaries:[call arguments]]; | ||
| for (CookieDto *cookieDto in cookieDtos) { | ||
| [cookieStore setCookie:[cookieDto toNSHTTPCookie] | ||
| completionHandler:^(){ | ||
| }]; |
There was a problem hiding this comment.
I tested this out and found several issues:
- the
resulthandler will never be called => your async dart code is stuck, same forclearCookies - the cookie data only considers
nameandvalue. So this is not going to work at all. In order for cookies to work we also need to consider other properties like thedomain. But after adding the rest of the fields, and implementing some stupid counter code, I made it work.
Stupid counter code that calls result in the end example:
- (void)setCookies:(FlutterMethodCall *)call result:(FlutterResult)result API_AVAILABLE(ios(11.0)) {
NSArray<CookieDto *> *cookieDtos = [CookieDto manyFromDictionaries:[call arguments]];
__block int counter = 0;
for (CookieDto *cookieDto in cookieDtos) {
[cookieStore setCookie:[cookieDto toNSHTTPCookie]
completionHandler:^(){
counter++;
if (counter == cookieDtos.count) {
result(nil);
}
}];
}
}There was a problem hiding this comment.
Thanks. I'll use the code you provided until I or someone else comes up with a better solution.
| - (NSHTTPCookie *)toNSHTTPCookie { | ||
| return [NSHTTPCookie cookieWithProperties:@{NSHTTPCookieName : name, NSHTTPCookieValue : value}]; | ||
| } | ||
|
|
There was a problem hiding this comment.
Other properties like
NSHTTPCookieDomainNSHTTPCookieSecureNSHTTPCookiePath
are missing. After adding them, it worked for me
There was a problem hiding this comment.
Got it. Do you propose those fields should be added to the CookieDto (in Dart, Java and ObjC) as well then?
There was a problem hiding this comment.
Yes, I would support all the fields that the Cookie class in Dart has.
|
Thanks, @wwwdata! I did the best I could at the time, but I really appreciate you stepping in and taking a look. I'll take a look at the changes you requested. Edit: Just to confirm, you propose to
Is that correct? |
|
All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter. We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the ℹ️ Googlers: Go here for more info. |
|
CLAs look good, thanks! ℹ️ Googlers: Go here for more info. |
|
All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter. We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the ℹ️ Googlers: Go here for more info. |
|
Good changes, I think sending it as a string makes the most sense for Android. I now also found out that the I have to check out your iOS part as well. I noticed that using this process pool was necessary to cookies sync across different web views. If you would also use that and everything works, I think we should push this PR through and close the other one from me again. You can check the code on my PR for how to do the process pool in iOS. |
|
@googlebot I consent. |
|
Error output from CocoaPods: Error running pod install |
|
Is this still in progress? |
what would be different in your PR? Did you also have a look at my open PR? It is a bid sad that this PR is now open for so long since cookie syncing is, in my opinion, a very important topic. |
Nothing special. |
|
This is something we could really use, and in fact its a requirement. How can we make google aware that this is something needed @jeroen-meijer and @wwwdata |
|
Could this PR be available during my rest of my life ? |
|
@Justwen maybe, maybe not, who knows! |
|
This is important feature for webview_flutter plugin as the existing solution which uses Javascript eval document.cookie cannot retrieve secured/HttpOnly cookies, which is important for certain authentication solution. |
|
Any news about it? |
|
If someone needed a solution |
Confirming this package works. |
|
Though I also believe that cookie handling is important in Flutter, I currently don't have more time to spend on this PR. 😔 The changes are still available on the source branch, though, so if anybody wants to fork it, pick this up and continue working on it, be my guest. In the meantime, the package proposed by @fryette seems to work, so I recommend people to use that instead. Closing this. Keeping the branch open. |
Description
This PR adds better support for cookies by adding
getCookiesandsetCookiesmethods to theCookieManager.Instead of a
List<String>, these methods will use aList<Cookie>where theCookieclass comes fromdart:io. This provides the user with easier cookie handling because the cookies from theWebViewinstance no longer have to be manually deserialised fromStrings. Also, this provides better interop with Dart's nativeHttpClient.Related Issues
This PR may help solve the following issues:
flutter/flutter#27795
flutter/flutter#27624
flutter/flutter#27597
Checklist
Before you create this PR confirm that it meets all requirements listed below by checking the relevant checkboxes (
[x]). This will ensure a smooth and quick review process.///).flutter analyze) does not report any problems on my PR.Breaking Change
Does your PR require plugin users to manually update their apps to accommodate your change?