-
Notifications
You must be signed in to change notification settings - Fork 9.8k
Fixes error handling in firebase_auth on Android and iOS #775
Conversation
@kroikie this is now ready for review and merge. |
Another big change in this code is that now all the dart code of @kroikie I just noticed my PR overlaps several others noted here. I just fixed things as I saw they were missing when adding the error logging.
Would be nice to see #718 merged in the same version release as this. |
d26dd78
to
f227aca
Compare
@slightfoot I don't mind you taking the changes from #768! :) |
@eseidelGoogle could you check on this please? It's really a needed fix. |
@bparrishMines may also be able to help with reviews. |
HI @slightfoot, Thanks for your contribution! Could you resolve the recent conflicts? I'll finish reviewing it as soon as you do. |
f227aca
to
4d3ecab
Compare
@bparrishMines think the analyze queue broken. Its been waiting a few hours. I'll double check all the formatting is good tomorrow when the results come in. Other than that, this is ready to go. |
@kroikie looks like the build server is borked. perhaps you can kick it. 😉 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Most of it looks great! I only have a few small comments. I'll see what I can do about the analyzer.
} else if (exception instanceof FirebaseApiNotAvailableException) { | ||
result.error( | ||
"ERROR_API_NOT_AVAILABLE", | ||
"The API that you are calling is not available on devices without Google Play Services.", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do you not use exception.getMessage()
for this and the other exceptions below?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changed.
/// Completes with an error if the user is signed out. | ||
/// |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
An extra space after documentation is unnecessary if we go by effective dart style guide. Same for comments below.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, changed these also.
packages/firebase_auth/CHANGELOG.md
Outdated
* `PlatformExceptions` now report error codes as stated in docs. | ||
* Added support for `linkWithTwitterCredential` in `FirebaseAuth`. | ||
* Added support for GitHub to `FirebaseAuth`. | ||
* **Breaking Change** Renamed `fetchProvidersForEmail` to its new name `fetchSignInMethodsForEmail`. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since there is only deprecation, you don't have to put **Breaking Change**
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated.
cc @fkorotkov @gspencergoog Do you know why the analyze check is stuck here? |
When will this change be released? |
I support getting this merged too. Let's get it done please. |
@slightfoot Sorry about the delay on this PR. Would you be able to rebase this on master then I'd be happy to merge. |
765178e
to
562f424
Compare
@bparrishMines @kroikie It's ready to merge. |
@slightfoot sorry for the churn here, could you rebase then should be good to merge. |
562f424
to
49366cf
Compare
Rebased. |
b6801bb
to
68bfa42
Compare
Updated FireAuth docs. Updated deprecated API "fetchProvidersForEmail" to "fetchSignInMethodsForEmail". Added "unlinkCredential". Updated changelog and brought wrapper inline with (flutter#915).
68bfa42
to
8da93c4
Compare
@kroikie I've rebased again, and merged whilst I can, but leave the release to you. |
@slightfoot Thanks for the long-awaited and hugely useful commit. An official release would be appreciated as this fixes some critical bugs wrt error handling. Renaming |
@xqwzts I was asked to remove my "breaking change" mention from the CHANGELOG and the @deprecated function I had because we have not yet reached stable API version, or something. |
How is the status? When will this be in beta? |
@slightfoot Do you know when this will be available? |
If you want these changes before an official release but without the stuff in the proposed 0.7.0 in the change log
The commit is of the stuff in this PR which in the changelog is 0.6.7 - just get the most recent commit if you want the stuff in 0.7.0 but that is nothing to do with this PR |
@kroikie is it possible to push this out. A lot of people are still waiting for this. |
Complete? |
Updated FireAuth docs. Updated deprecated API "fetchProvidersForEmail" to "fetchSignInMethodsForEmail". Added "unlinkCredential". Updated changelog and brought wrapper inline with (flutter#915).
This reverts commit f55c672.
Updated FireAuth docs. Updated deprecated API "fetchProvidersForEmail" to "fetchSignInMethodsForEmail". Added "unlinkCredential". Updated changelog and brought wrapper inline with (flutter#915).
Hey i'm also facing google signin issue in ios #2 MethodChannel.invokeMapMethod (package:flutter/src/services/platform_channel.dart:349:48) |
This standardises the error responses from the platform implementations. All functions in
FirebaseAuth
andFirebaseUser
are now documented with error codes.Fixes: flutter/flutter#18312 and Fixes: flutter/flutter#20223
This PR is based on #678 so includes the changes there.
Example:
Other changes include:
linkWithTwitterCredential
method.fetchProvidersForEmail
tofetchSignInMethodsForEmail
.unlinkCredential
.Full error code list can be found here:
https://docs.google.com/spreadsheets/d/1FDn0rRRYjgXMwc_FIAxO7_cQh69q5VXgQt7Hmvyb1Sg/edit?usp=sharing
Closes: #768 and Closes: #697 - Merged into this one.
Closes: #703 and #814