Find Firebase user(s) by federated user id

[mkilmanas]

This is a port of a method that is available in the official node.js sdk, implemented in the analogous way.

[jeromegamez]

The thousands PR, congrats! 😅🎊

Thank you for the contribution! I will have to find out how I can add this without breaking BC due to the changed interface, but that's a general problem 😕

[codecov]

Codecov Report

Attention: Patch coverage is 0% with 11 lines in your changes missing coverage. Please review.

Project coverage is 50.04%. Comparing base (651e64c) to head (3e5b538).
Report is 4 commits behind head on 7.x.

❗ There is a different number of reports uploaded between BASE (651e64c) and HEAD (3e5b538). Click for more details.

HEAD has 5 uploads less than BASE

Flag	BASE (651e64c)	HEAD (3e5b538)
unit	7	2

Additional details and impacted files

@@              Coverage Diff              @@
##                7.x    #1000       +/-   ##
=============================================
- Coverage     90.00%   50.04%   -39.97%     
- Complexity     1444     1447        +3     
=============================================
  Files           140      140               
  Lines          4184     4202       +18     
=============================================
- Hits           3766     2103     -1663     
- Misses          418     2099     +1681     

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[jeromegamez]

Okay, there are two ways I currently see without breaking BC or creating a new major release:

1. Add a new interface just for this method, implement it in Auth and require SDK users to do

if ($auth instanceOf ProvidesUsersByProviderUid) {
    $user = $auth->getUserByProviderUid($providerId, $providerUid);
}

2. Add the new method to Auth and require users to do

if (method_exists($auth, 'getUserByProviderUid')) {
    $user = $auth->getUserByProviderUid($providerId, $providerUid);
}

I'm leaning towards (1), but the next added method then would need a new interface as well. So, I'm leaning towards (2), but asking users to method_exists() feels icky!

What do you think?

I need a solution for this in general. At the moment, each added method/feature is a breaking change and I'm just lucky this doesn't happen too often.

[mkilmanas]

I have to admit, until now I have never realised that adding new methods to an interface is actually a BC-break (but now I see why).

I would strongly prefer the first option as it is way cleaner and intentions are pretty clear.

I suppose the process for all those additional single-purpose-micro-interfaces could be like this:

• in any minor release one can add additional interfaces, usage involves instanceof <thatInterface>
• in the next major release, make the main interface extend all existing micro-interfaces (usage permits instanceof but also the main interface would already contain/guarantee it); deprecate included micro-interfaces;
• in the next next major release integrate methods into main interface and remove micro-interfaces

A bit of maintenance, but for now I don't see an easier way out. Holding off any new methods until next major release seems like an overkill.

So would you like me to split this of into a micro-interface? Any ideas for the naming? Would UserByProviderUidProvider fit? or FederatedUserProvider? The reason why I'm not entirely happy with ProvidesUsersByProviderUid is that in my opinion interface name should be more of a noun rather than verb. But that might be a matter of taste / schooling 😄

[jeromegamez]

Thank you so much for the thought you've put into the problem, I agree with you 100%. Maintenance work is not a problem for me - I'd just have to remember, but perhaps that's a good opportunity to start using GitHub projects/milestones 😅

As for the naming, I agree as well, I've just been working with Laravel too much lately 🙈. Naming is hard - what if there's another Federated ID related methods coming up before the next major release 🤔. How would you feel about FederatedUserFetcher?

[mkilmanas]

Yeah, that sounds great 👍 I'll do the update a little later