-
-
Notifications
You must be signed in to change notification settings - Fork 450
Replace Stringable|string argument types with string-only
#1042
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
6044673 to
6aa8d6c
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## 8.x #1042 +/- ##
============================================
- Coverage 88.30% 88.28% -0.02%
Complexity 1474 1474
============================================
Files 145 145
Lines 4164 4150 -14
============================================
- Hits 3677 3664 -13
+ Misses 487 486 -1 🚀 New features to boost your workflow:
|
Methods that previously accepted `Stringable|string` as argument types
now only support `string`.
`Stringable` was added for convenience so that someone could do,
for example
```php
$user = $auth->getUser('uid');
$auth->updateUser($user, [...]);
```
While convenient, this adds overhead when processing these arguments.
For example, if a method expects a non-empty string, the SDK would
have to do a `trim((string) $arg)` and check if it's empty.
With this change, we can rely only on a `@var non-empty-string $arg`
docblock annotation.
```php
$user = $auth->getUser('uid');
$auth->updateUser($user->uid, [...]);
```
6aa8d6c to
30aa7a1
Compare
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.
Pull request overview
This PR removes support for Stringable types from method signatures throughout the Firebase SDK, requiring all arguments to be explicit string types instead. This change reduces runtime overhead by eliminating type casting (e.g., (string) $arg, trim((string) $arg)) and allows the SDK to rely on PHPStan's non-empty-string annotations for validation. This is a breaking change for v8.0 that affects Auth, Messaging, and related value objects.
Key changes:
- Updated method signatures across Auth interfaces and implementations to accept only
stringinstead ofStringable|string - Removed type casting logic from value objects (Uid, Email, Url, ClearTextPassword) and other classes
- Removed test cases for non-string types that were previously coerced to strings
Reviewed changes
Copilot reviewed 20 out of 20 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
src/Firebase/Contract/Auth.php |
Updated all method signatures to remove Stringable union types |
src/Firebase/Contract/Transitional/FederatedUserFetcher.php |
Updated getUserByProviderUid() parameters to accept only strings |
src/Firebase/Auth.php |
Updated implementation methods to match interface changes, removed type casting logic |
src/Firebase/Auth/ApiClient.php |
Updated unlinkProvider() parameter type hints |
src/Firebase/Auth/DeleteUsersRequest.php |
Updated withUids() parameter type hints |
src/Firebase/Auth/CustomTokenViaGoogleCredentials.php |
Updated createCustomToken() signature to accept string uid and nullable claims array |
src/Firebase/Auth/CreateActionLink.php |
Updated new() method to accept string email parameter |
src/Firebase/Request/UpdateUser.php |
Refactored provider removal logic, removed Stringable support and empty string validation |
src/Firebase/Request/EditUserTrait.php |
Updated all trait methods to accept only string parameters |
src/Firebase/Value/Url.php |
Removed Stringable support and casting from fromString() |
src/Firebase/Value/Uid.php |
Removed Stringable support and casting from fromString() |
src/Firebase/Value/Email.php |
Removed Stringable support and casting from fromString() |
src/Firebase/Value/ClearTextPassword.php |
Removed Stringable support and casting from fromString() |
src/Firebase/Messaging/MessageData.php |
Updated to accept only string values, removed type casting |
src/Firebase/Messaging/Message.php |
Updated PHPStan type annotation for data field |
src/Firebase/Messaging/CloudMessage.php |
Updated PHPStan type annotation for data parameter |
tests/Unit/Value/UrlTest.php |
Updated test to accept only string parameters |
tests/Unit/Messaging/MessageDataTest.php |
Removed test cases for non-string types (integer, float, boolean, null, Stringable objects) |
UPGRADE-8.0.md |
Added documentation for the breaking change with migration examples |
CHANGELOG.md |
Added changelog entry for the breaking change |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| $value = [$value]; | ||
| } | ||
|
|
||
| foreach ($value as $provider) { |
Copilot
AI
Dec 5, 2025
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.
Missing validation for provider values before calling withRemovedProvider(). The method now expects a non-empty-string but doesn't validate it. The loop should validate that each $provider is a non-empty string before calling withRemovedProvider(), otherwise empty strings or non-string values could be added to providersToDelete without validation.
Consider adding validation:
foreach ($value as $provider) {
if (!is_string($provider) || $provider === '') {
continue;
}
$request = $request->withRemovedProvider($provider);
}| foreach ($value as $provider) { | |
| foreach ($value as $provider) { | |
| if (!is_string($provider) || $provider === '') { | |
| continue; | |
| } |
| static fn(string $value): bool => $value !== '', | ||
| ), | ||
| ); | ||
| $provider = array_map(strval(...), (array) $provider); |
Copilot
AI
Dec 5, 2025
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.
Missing validation to filter out empty strings from providers. The old implementation used array_filter() to remove empty strings before passing to the API client, which expects list<non-empty-string>. Now the code only converts to strings with array_map(strval(...), (array) $provider) but doesn't filter empty strings.
Consider restoring the filter:
$provider = array_values(
array_filter(
array_map(strval(...), (array) $provider),
static fn(string $value): bool => $value !== '',
),
);| $provider = array_map(strval(...), (array) $provider); | |
| $provider = array_values( | |
| array_filter( | |
| array_map(strval(...), (array) $provider), | |
| static fn(string $value): bool => $value !== '', | |
| ), | |
| ); |
| return $this->client->handleSignIn(SignInWithRefreshToken::fromValue($refreshToken)); | ||
| } | ||
|
|
||
| public function signInWithEmailAndPassword($email, $clearTextPassword): SignInResult |
Copilot
AI
Dec 5, 2025
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.
Missing string type hints for parameters. The interface Auth declares this method as signInWithEmailAndPassword(string $email, string $clearTextPassword) (line 424), but the implementation is missing type hints for both parameters. These should be explicitly typed as string to match the interface and align with the PR's goal of replacing Stringable|string with string.
| public function signInWithEmailAndPassword($email, $clearTextPassword): SignInResult | |
| public function signInWithEmailAndPassword(string $email, string $clearTextPassword): SignInResult |
| return $this->client->handleSignIn(SignInWithEmailAndPassword::fromValues($email, $clearTextPassword)); | ||
| } | ||
|
|
||
| public function signInWithEmailAndOobCode($email, string $oobCode): SignInResult |
Copilot
AI
Dec 5, 2025
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.
Missing string type hint for $email parameter. The interface Auth declares this method as signInWithEmailAndOobCode(string $email, string $oobCode) (line 432), but the implementation is missing the type hint for $email. It should be explicitly typed as string to match the interface and align with the PR's goal.
| public function signInWithEmailAndOobCode($email, string $oobCode): SignInResult | |
| public function signInWithEmailAndOobCode(string $email, string $oobCode): SignInResult |
| throw new FailedToSignIn('Failed to sign in anonymously: No ID token or UID available'); | ||
| } | ||
|
|
||
| public function signInWithIdpAccessToken($provider, string $accessToken, $redirectUrl = null, ?string $oauthTokenSecret = null, ?string $linkingIdToken = null, ?string $rawNonce = null): SignInResult |
Copilot
AI
Dec 5, 2025
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.
Missing string type hint for $provider parameter. The interface Auth declares this method with string $provider (line 451), but the implementation is missing the type hint. It should be explicitly typed as string to match the interface and align with the PR's goal of removing Stringable support.
| public function signInWithIdpAccessToken($provider, string $accessToken, $redirectUrl = null, ?string $oauthTokenSecret = null, ?string $linkingIdToken = null, ?string $rawNonce = null): SignInResult | |
| public function signInWithIdpAccessToken(string $provider, string $accessToken, $redirectUrl = null, ?string $oauthTokenSecret = null, ?string $linkingIdToken = null, ?string $rawNonce = null): SignInResult |
| return $this->client->handleSignIn($action); | ||
| } | ||
|
|
||
| public function signInWithIdpIdToken($provider, $idToken, $redirectUrl = null, ?string $linkingIdToken = null, ?string $rawNonce = null): SignInResult |
Copilot
AI
Dec 5, 2025
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.
Missing string type hint for $provider parameter. The interface Auth declares this method with string $provider (line 462), but the implementation is missing the type hint. It should be explicitly typed as string to match the interface and align with the PR's goal of removing Stringable support.
| public function signInWithIdpIdToken($provider, $idToken, $redirectUrl = null, ?string $linkingIdToken = null, ?string $rawNonce = null): SignInResult | |
| public function signInWithIdpIdToken(string $provider, $idToken, $redirectUrl = null, ?string $linkingIdToken = null, ?string $rawNonce = null): SignInResult |
Methods that previously accepted
Stringable|stringas argument types now only supportstring.Stringablewas added for convenience so that someone could do, for exampleWhile convenient, this adds overhead when processing these arguments. For example, if a method expects a non-empty string, the SDK would have to do a
trim((string) $arg)and check if it's empty.With this change, we can rely only on a
@var non-empty-string $argdocblock annotation.