-
Notifications
You must be signed in to change notification settings - Fork 295
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
fix: [BREAKING] remove legacy subscription; change subscription interface #394
base: master
Are you sure you want to change the base?
Conversation
Hey sorry I plan to review this this weekend, thanks! |
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! Sorry for the delay, I'm not sure about the change about null => ""
though
$associativeArray['publicKey'] ?? null, | ||
$associativeArray['authToken'] ?? null, | ||
$associativeArray['contentEncoding'] ?? "aesgcm" | ||
$associativeArray['endpoint'] ?? "", |
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.
endpoint shouldn't be empty, it should throw before, shouldn't it?
$associativeArray['authToken'] ?? null, | ||
$associativeArray['contentEncoding'] ?? "aesgcm" | ||
$associativeArray['endpoint'] ?? "", | ||
$associativeArray['keys']['p256dh'] ?? "", |
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.
I'm not sure this is an improvement, having an empty string instead of null ?
* @author Sergii Bondarenko <sb@firstvector.org> | ||
*/ | ||
interface SubscriptionInterface | ||
{ | ||
public function getEndpoint(): string; | ||
|
||
public function getPublicKey(): ?string; | ||
public function getPublicKey(): string; |
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.
A subscription can have no public key, auth token or content encoding (sent without any payload)
Remove legacy GCM Remove old Chrome subscription support
[BREAKING] change default encoding to aes128gcm
e632f72
to
59f2e6e
Compare
@@ -212,25 +221,25 @@ private static function createContext(string $clientPublicKey, string $serverPub | |||
* | |||
* @throws \ErrorException |
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.
Could add @throws \ValueError
here?
protected readonly string $endpoint, | ||
protected readonly string $publicKey, | ||
protected readonly string $authToken, | ||
ContentEncoding|string $contentEncoding = ContentEncoding::aes128gcm, |
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.
Extraneous space?
if(is_string($contentEncoding)) { | ||
try { | ||
if(empty($contentEncoding)) { | ||
$this->contentEncoding = ContentEncoding::aesgcm; // default |
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.
Shouldn't the default be aes128gcm
?
Hopefully this can be finished & merged soon :) |
@Minishlink Just wondering, what's currently missing in that PR? |
Yes that's right |
I might delay this to v10 and release v9 at the beginning of july. Let me know @Rotzbua if you have time in the near future for this PR, thank you |
Hi, Thanks for the amazing repo! |
Hello, this library is agnostic about which endpoint is sent by the browser, it only requires compliance with the Web Push standard. |
Converted to draft until I have time and back into the php topic. |
Thanks for the update! I have one more question: what's the pull request about? Are you planning on supporting the v1 HTTP API too, or is it focused on something else? Looking forward to your response! |
@cclegend90 It is just cleanup of obsolete code. The FCM v1 HTTP API (for native apps) is not the Push API (for browser) and not target of this library. |
Changes
Breaking:
Feature:
Reference
fixes #381
fixes #345
reference #388