Unpushed commits (local main ahead of origin/main)#32
Conversation
There was a problem hiding this comment.
Pull request overview
This PR updates the package’s Saloon integration (moving to Saloon v4) and adjusts OAuth/caching behavior accordingly, including a migration away from PHP serialization for cached authenticators.
Changes:
- Upgrade Saloon dependencies to v4 and adjust OAuth URL building / base URL override behavior.
- Replace cached authenticator PHP
serialize/unserializewith JSON encode/decode (with legacy payload migration). - Update controller/handler caching to use the new cache payload format.
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
composer.json |
Bumps Saloon packages to v4-compatible constraints. |
src/Traits/AuthorizationCodeGrant.php |
Passes allow-base-url-override flag when joining authorization URLs. |
src/Connectors/InstagramConnector.php |
Enables base URL override in OAuth config. |
src/Requests/Authentication/GetShortLivedAccessTokenRequest.php |
Allows absolute endpoints to override connector base URL. |
src/Requests/Authentication/GetRefreshAccessTokenRequest.php |
Allows absolute endpoints to override connector base URL. |
src/Requests/Authentication/GetAccessTokenRequest.php |
Allows absolute endpoints to override connector base URL. |
src/Authenticator/InstagramAuthenticator.php |
Introduces JSON cache encoding/decoding with legacy serialized payload support. |
src/Actions/InstagramHandler.php |
Switches to new cache decode/encode flow when reading/updating authenticator. |
src/Http/Controllers/InstagramController.php |
Stores JSON cache payload instead of serialized object in callback flow. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if ($trimmed !== '' && $trimmed[0] === '{') { | ||
| $data = json_decode($payload, true, 512, JSON_THROW_ON_ERROR); | ||
| $expiresAt = isset($data['expiresAt']) && is_string($data['expiresAt']) && $data['expiresAt'] !== '' | ||
| ? new DateTimeImmutable($data['expiresAt']) | ||
| : null; | ||
|
|
||
| return new static( | ||
| $data['accessToken'], | ||
| $data['refreshToken'] ?? null, | ||
| $expiresAt, | ||
| ); |
There was a problem hiding this comment.
In the JSON branch, $data['accessToken'] is used without checking it exists/is a string. A corrupted/partial cache entry will trigger notices/type errors rather than a controlled exception. Add explicit validation for required keys/types (and throw an InvalidArgumentException if invalid) before constructing the authenticator.
| $authenticator = InstagramAuthenticator::decodeFromCache($serialized); | ||
|
|
There was a problem hiding this comment.
decodeFromCache can now throw (e.g., JsonException/InvalidArgumentException) when the cached payload is corrupted. Consider catching decode failures here, clearing the cache key (similar to the empty($serialized) branch), and throwing the same "No authenticator found" exception to avoid hard failures from a single bad cache value.
| $authenticator = InstagramAuthenticator::decodeFromCache($serialized); | |
| try { | |
| $authenticator = InstagramAuthenticator::decodeFromCache($serialized); | |
| } catch (\Throwable $e) { | |
| Cache::store(config('instagram.cache_store'))->forget('instagram.authenticator'); | |
| throw new \Exception('No authenticator found. Please authenticate first.'); | |
| } |
| public function encodeForCache(): string | ||
| { | ||
| return serialize($this); | ||
| return json_encode([ | ||
| 'accessToken' => $this->accessToken, | ||
| 'refreshToken' => $this->refreshToken, | ||
| 'expiresAt' => $this->expiresAt?->format(DATE_ATOM), | ||
| ], JSON_THROW_ON_ERROR); | ||
| } | ||
|
|
||
| /** | ||
| * Unserialize the access token. | ||
| * Restore from cache. Supports JSON (current) and legacy PHP-serialized payloads for one-time migration. | ||
| * | ||
| * @throws JsonException | ||
| */ | ||
| public static function unserialize(string $string): static | ||
| public static function decodeFromCache(string $payload): static | ||
| { | ||
| return unserialize($string, ['allowed_classes' => true]); | ||
| $trimmed = ltrim($payload); | ||
|
|
||
| if ($trimmed !== '' && $trimmed[0] === '{') { | ||
| $data = json_decode($payload, true, 512, JSON_THROW_ON_ERROR); | ||
| $expiresAt = isset($data['expiresAt']) && is_string($data['expiresAt']) && $data['expiresAt'] !== '' | ||
| ? new DateTimeImmutable($data['expiresAt']) | ||
| : null; | ||
|
|
||
| return new static( | ||
| $data['accessToken'], | ||
| $data['refreshToken'] ?? null, | ||
| $expiresAt, | ||
| ); | ||
| } |
There was a problem hiding this comment.
The new cache encoding/decoding logic (JSON + legacy PHP-serialized migration path) isn't covered by tests. Add tests for encodeForCache/decodeFromCache round-tripping, and for successfully reading a legacy serialized payload, to prevent regressions during the Saloon v4 migration.
| * Encode for cache storage (JSON). Replaces PHP serialize, which is unsafe and unsupported with Saloon v4+. | ||
| * | ||
| * @throws JsonException | ||
| */ | ||
| public function serialize(): string | ||
| public function encodeForCache(): string | ||
| { | ||
| return serialize($this); | ||
| return json_encode([ | ||
| 'accessToken' => $this->accessToken, | ||
| 'refreshToken' => $this->refreshToken, | ||
| 'expiresAt' => $this->expiresAt?->format(DATE_ATOM), | ||
| ], JSON_THROW_ON_ERROR); | ||
| } | ||
|
|
||
| /** | ||
| * Unserialize the access token. | ||
| * Restore from cache. Supports JSON (current) and legacy PHP-serialized payloads for one-time migration. | ||
| * | ||
| * @throws JsonException | ||
| */ | ||
| public static function unserialize(string $string): static | ||
| public static function decodeFromCache(string $payload): static |
There was a problem hiding this comment.
encodeForCache/decodeFromCache replace the previously public serialize/unserialize API. If this package has external consumers, this is a breaking change; consider keeping serialize/unserialize as deprecated wrappers (or documenting/bumping a major version) to avoid unexpected runtime errors for downstream users.
| $expiresAt = isset($data['expiresAt']) && is_string($data['expiresAt']) && $data['expiresAt'] !== '' | ||
| ? new DateTimeImmutable($data['expiresAt']) | ||
| : null; | ||
|
|
There was a problem hiding this comment.
decodeFromCache only documents @throws JsonException, but new DateTimeImmutable($data['expiresAt']) can also throw (e.g., invalid timestamp format). Either validate/catch and rethrow a consistent exception type, or update the PHPDoc to reflect the additional exception(s) so callers can handle corrupted cache payloads predictably.
| $expiresAt = isset($data['expiresAt']) && is_string($data['expiresAt']) && $data['expiresAt'] !== '' | |
| ? new DateTimeImmutable($data['expiresAt']) | |
| : null; | |
| $expiresAt = null; | |
| if (isset($data['expiresAt']) && is_string($data['expiresAt']) && $data['expiresAt'] !== '') { | |
| try { | |
| $expiresAt = new DateTimeImmutable($data['expiresAt']); | |
| } catch (\Exception $e) { | |
| throw new JsonException('Invalid expiresAt value in cached Instagram authenticator payload.', 0, $e); | |
| } | |
| } |
Local main was 2 commit(s) ahead of origin/main. Opened from update-opensource-active.sh for review.