kreait / laravel-firebase

A Laravel package for the Firebase PHP Admin SDK
https://github.com/kreait/firebase-php
MIT License
994 stars 163 forks source link

Support configuration of credentials with a config array #202

Closed clemblanco closed 8 months ago

clemblanco commented 9 months ago

Description

On our applications, in order to have more granularity in the configuration of the credentials, we had to extend the package to be able to optionally receive the configuration of the credentials via an array instead of a JSON file.

This makes it easier to integrate with our CI/CD pipelines and also gives us the ability to cache the config via Laravel.

This is a change we think could be valuable to others too, especially considering that firebase-php already supports arrays,

Note: we slightly tweaked the expected configuration array for credentials but don't worry this is a non-breaking change.

- 'credentials' => [
-     'file' => 'some_path.json',
- ],
+ 'credentials' => 'some_path.json',

Checklist:

clemblanco commented 8 months ago

Hi @jeromegamez,

I think it's ready for a second look.

I would move the checks into the resolveCredentials() method.

I renamed this into resolveJsonCredentials() as it's only useful for JSON credentials. If it's an array, there is simply no credentials to resolve, they are already there, ready to be used.

Out of curiosity: do you see a way to let users of the package now that using file should be considered deprecated other than adding a comment? Either way, I'm going to just™ remove it in the next major version.

Either log something with logger()->warn() for example while both are still supported... or don't bother and make it a breaking change in the next major release. Should be fine as long as things are documented properly and the release notes make it clear enough.

:v:

jeromegamez commented 8 months ago

Thanks for bearing with me (honestly), even though some requests were inconsistent 😅

There was a problem with the codecov action (https://github.com/codecov/codecov-action/issues/1089) which I fixed in the main branch, could you rebase the PR so that the workflows can run again?

jeromegamez commented 8 months ago

Uh, nice, I just discovered the Rebase button in the web view of a PR! If this works, I'll merge!

codecov[bot] commented 8 months ago

Codecov Report

Merging #202 (6ae431e) into main (10cc86b) will not change coverage. The diff coverage is 100.00%.

@@            Coverage Diff            @@
##               main     #202   +/-   ##
=========================================
  Coverage     98.41%   98.41%           
- Complexity       47       48    +1     
=========================================
  Files             3        3           
  Lines           126      126           
=========================================
  Hits            124      124           
  Misses            2        2           
Files Coverage Δ
src/FirebaseProjectManager.php 96.87% <100.00%> (ø)

:mega: We’re building smart automated test selection to slash your CI/CD build times. Learn more

jeromegamez commented 8 months ago

Thanks again! 🙏🏻

jeromegamez commented 8 months ago

I applied your config file blank line changes in and added you as a Co-Author to https://github.com/kreait/laravel-firebase/commit/5b682a00c5c82ade0fb8085432d103279a58ad4b - I hope I got them all 🤞🏻

I also changed the Code Style Fixer to Laravel Pint in https://github.com/kreait/laravel-firebase/commit/8772cc4742552e343e4d5c49f980e2dba1f12a4b

clemblanco commented 8 months ago

Nice one thanks.

Would you be able to publish a new release please?

jeromegamez commented 8 months ago

Done ✅