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 90.90909% with 1 line in your changes missing coverage. Please review.

Project coverage is 72.98%. Comparing base (a355739) to head (11f6c62).
Report is 15 commits behind head on 7.x.

❗ There is a different number of reports uploaded between BASE (a355739) and HEAD (11f6c62). Click for more details.

HEAD has 7 uploads less than BASE

Flag	BASE (a355739)	HEAD (11f6c62)
unit	7	0

Additional details and impacted files

@@              Coverage Diff              @@
##                7.x    #1000       +/-   ##
=============================================
- Coverage     90.02%   72.98%   -17.05%     
- Complexity     1444     1447        +3     
=============================================
  Files           140      140               
  Lines          4192     4201        +9     
=============================================
- Hits           3774     3066      -708     
- Misses          418     1135      +717     

🚀 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

[mkilmanas]

I've put the new interface in a 'Transitional' namespace and marked with a @TODO - I hope that is ok?

[jeromegamez]

Thanks for your patience with this! I'll try to get to it as soon as I have fixed the PHPStan errors which are not related to your changes. If possible, could you enable that I can rebase and push to the branch?

[mkilmanas]

@jeromegamez I don't really know how to enable the access to let you push to the branch. But I am happy to rebase once you are done with the changes/fixes on the target branch - just ping me when ready.

[jeromegamez]

You should now be able to rebase your PR and not get PHPStan errors anymore 🤞🏻

[jeromegamez]

Also, I made some changes to the workflows, including enabling integration and emulator tests in PRs, I'm really looking forward to seeing if it works 🤞🏻

[mkilmanas]

Fair point - I will add the test coverage for the new code

[mkilmanas]

I have added a negative case (UserNotFound) but cannot think of a way to have a realistic federated account available for fetching from the emulator. If you have any ideas for that - they're more than welcome.

[mkilmanas]

By the way - I had some troubles with the unit tests at first. One of them insisted on failing with a timestamp off by exactly 1 hours. So after some digging I found out that it works well if the default timezone is UTC, but with other timezones it fails.
Not sure if this is test flakiness or does this indicate a real problem with timezone handling. Nevertheless I have added explicit php -d date.timezone=UTC to phpunit calls from composer.json. Let me know if you think this should be solved differently. You can replace UTC with any other timezone (which is not +00:00) to see it fail.

[jeromegamez]

Thanks for taking the time to add tests! I believe the -d date.timezone=UTC can be avoided by adding this to phpunit.xml.dist:

<phpunit>
    <php>
        <ini name="date.timezone" value="UTC" />
    </php>
</phpunit>

[jeromegamez]

The tests fail again, but I think you've done enough good work already 😅. I will merge the PR manually and fix the errors while I'm at it!

[mkilmanas]

That was silly of me - i stepped onto the same rake that I have introduced with an extra interface 😄
PHPStan somehow did not like my type check (or rather inferred nothing from it) so we ended up with a phpstan-ignore annotation. 🤷‍♂️
Changed the timezone config the way you suggested - that's definitely cleaner 👍

[jeromegamez]

It's weird, PHPStan wasn't happy with me either - I worked around it with $auth = $this->auth;.

You've probably already received the notification about #1003 😅. Let me know what you think, I'll hold off merging it until then 🙏🏻

[jeromegamez]

Merged with #1003, thanks for the collaboration! 🌺