From 8d7dcdf6ebc0775cf84b3f61df6f12640d0233d0 Mon Sep 17 00:00:00 2001 From: Paulo Castellano Date: Sun, 28 Jun 2026 20:37:04 -0300 Subject: [PATCH] refactor(social): trim verbose comments + harden X chunked upload from review MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Cold-review follow-ups on the PR: - Trim the oversized docblocks/inline comments added across the API controller, MediaAttacher, Post, the publish job, and the X publisher to one line (keeping the @param/@return array-shape annotations). - XPublisher::chunkedUpload now accepts ?string $mediaCategory and only sends media_category when present — getMediaCategory() can return null, so the strict string param was a latent TypeError (unreachable on X today, removed anyway). - Fix MediaAttacher docblocks: the file imports Type as MediaType, so the @param array annotations didn't resolve — now array. - Tests: cover the failed() job hook genericizing a raw error, and X failing cleanly (XPublishException) when media can't be downloaded. --- app/Http/Controllers/Api/PostController.php | 6 +----- .../Requests/Api/Post/StorePostRequest.php | 3 --- app/Jobs/PublishToSocialPlatform.php | 7 ++----- app/Models/Post.php | 5 +---- app/Services/Post/MediaAttacher.php | 15 +++++--------- app/Services/Social/XPublisher.php | 20 ++++++++++--------- .../Jobs/PublishToSocialPlatformTest.php | 15 ++++++++++++-- .../Services/Social/XPublisherTest.php | 19 +++++++++++++----- 8 files changed, 47 insertions(+), 43 deletions(-) diff --git a/app/Http/Controllers/Api/PostController.php b/app/Http/Controllers/Api/PostController.php index 525dbd7c..3326c5fa 100644 --- a/app/Http/Controllers/Api/PostController.php +++ b/app/Http/Controllers/Api/PostController.php @@ -100,11 +100,7 @@ public function update(UpdatePostRequest $request, Post $post): PostResource|Jso } /** - * Download and host any external media URLs in an inline media array so the - * stored post never references a third-party URL (which can 404 or change at - * publish time). Items already on our storage pass through. Rejects the - * request when any URL can't be fetched, so a post is never persisted with - * broken media. + * Download and host external media URLs; reject (422) if one can't be fetched. * * @param array $allowedTypes * @param array> $media diff --git a/app/Http/Requests/Api/Post/StorePostRequest.php b/app/Http/Requests/Api/Post/StorePostRequest.php index 52972eae..10c35c39 100644 --- a/app/Http/Requests/Api/Post/StorePostRequest.php +++ b/app/Http/Requests/Api/Post/StorePostRequest.php @@ -69,9 +69,6 @@ public function rules(): array } /** - * The distinct platforms selected in this request, used to compute the - * media types acceptable for the post being created. - * * @return Collection */ public function selectedPlatforms(): Collection diff --git a/app/Jobs/PublishToSocialPlatform.php b/app/Jobs/PublishToSocialPlatform.php index eca432f0..090540f2 100644 --- a/app/Jobs/PublishToSocialPlatform.php +++ b/app/Jobs/PublishToSocialPlatform.php @@ -224,11 +224,8 @@ private function broadcastStatus(): void } /** - * A failure message safe to surface to the user — it ends up in the - * post-failure email. Only our own publish exceptions carry a vetted - * user-facing message; any other throwable (engine errors like TypeError, - * or library exceptions) can embed file paths and internals, so it's - * replaced with a generic line. The raw detail stays in the logs. + * A user-safe failure message: only our own publish exceptions are shown + * verbatim; anything else is genericized so internals never reach the email. */ private function safeFailureMessage(\Throwable $e): string { diff --git a/app/Models/Post.php b/app/Models/Post.php index 64150180..1fa41456 100644 --- a/app/Models/Post.php +++ b/app/Models/Post.php @@ -155,10 +155,7 @@ public function allowedMediaTypes(): array } /** - * The media types acceptable across a set of platforms: the intersection of - * what each platform allows. With no platform, accept anything. Lets callers - * that don't have a persisted Post yet (e.g. the API create flow) compute the - * same constraint from the platforms in the request. + * Media types acceptable across a set of platforms (intersection; empty = all). * * @param Collection $platforms * @return array diff --git a/app/Services/Post/MediaAttacher.php b/app/Services/Post/MediaAttacher.php index b4b7019b..a5320334 100644 --- a/app/Services/Post/MediaAttacher.php +++ b/app/Services/Post/MediaAttacher.php @@ -45,13 +45,10 @@ public function attachFromUrls(Post $post, array $urls): array } /** - * Resolve an inline media array (as accepted by the public API on post - * create/update) into fully hosted media items. Items already stored on our - * disk (they carry a `path`) pass through untouched; every other item is - * treated as an external `url` to download and host, so publishing never - * depends on a third-party URL staying alive. + * Resolve an inline media array into hosted items: items with a `path` pass + * through, external URLs are downloaded and hosted. * - * @param array $allowedTypes + * @param array $allowedTypes * @param array> $items * @return array{media: array>, failed: array} */ @@ -78,11 +75,9 @@ public function resolveInlineMedia(Workspace $workspace, array $allowedTypes, ar } /** - * Download a public URL, validate it against the accepted media types, and - * store it on the workspace. Returns the media item, or null on any failure - * (download error, disallowed type, oversized). + * Download a URL, validate its type, and store it on the workspace. * - * @param array $allowedTypes + * @param array $allowedTypes * @return array|null */ public function fetchToWorkspace(Workspace $workspace, array $allowedTypes, string $url): ?array diff --git a/app/Services/Social/XPublisher.php b/app/Services/Social/XPublisher.php index be844daa..5a535605 100644 --- a/app/Services/Social/XPublisher.php +++ b/app/Services/Social/XPublisher.php @@ -124,9 +124,6 @@ private function uploadMedia($mediaItem): ?array ); } - // Recover the MIME from the downloaded bytes when the item carries - // none (e.g. media attached by URL), and fail cleanly rather than - // with a TypeError when it still can't be determined. if (blank($mimeType)) { $mimeType = mime_content_type($tempFile) ?: null; } @@ -198,16 +195,21 @@ private function uploadMedia($mediaItem): ?array } } - private function chunkedUpload(string $tempFile, int $totalBytes, string $mimeType, string $mediaCategory): array + private function chunkedUpload(string $tempFile, int $totalBytes, string $mimeType, ?string $mediaCategory): array { + $initPayload = [ + 'media_type' => $mimeType, + 'total_bytes' => $totalBytes, + ]; + + if ($mediaCategory) { + $initPayload['media_category'] = $mediaCategory; + } + // INIT $initResponse = $this->socialHttp()->withToken($this->accessToken) ->timeout(60) - ->post("{$this->baseUrl}/media/upload/initialize", [ - 'media_type' => $mimeType, - 'media_category' => $mediaCategory, - 'total_bytes' => $totalBytes, - ]); + ->post("{$this->baseUrl}/media/upload/initialize", $initPayload); if ($initResponse->failed()) { Log::error('X chunked upload INIT error', [ diff --git a/tests/Feature/Jobs/PublishToSocialPlatformTest.php b/tests/Feature/Jobs/PublishToSocialPlatformTest.php index d3d955e7..1c7230e9 100644 --- a/tests/Feature/Jobs/PublishToSocialPlatformTest.php +++ b/tests/Feature/Jobs/PublishToSocialPlatformTest.php @@ -181,7 +181,6 @@ $this->postPlatform->refresh(); expect($this->postPlatform->status)->toBe(PlatformStatus::Failed); - // Untrusted exceptions are genericized — only vetted publish exceptions surface their message. expect($this->postPlatform->error_message)->toBe('An unexpected error occurred while publishing. Please try again.'); }); @@ -206,7 +205,6 @@ test('publish never leaks a raw internal error to the failure record (and the email)', function () { Event::fake(); - // A PHP TypeError embeds the server file path in its message; it must not reach the user. $publisher = Mockery::mock(LinkedInPublisher::class); $publisher->shouldReceive('publish')->andThrow(new TypeError( 'X::getMediaCategory(): Argument #1 ($mimeType) must be of type string, null given, called in /home/forge/app.trypost.it/releases/72198060/app/Services/Social/XPublisher.php on line 130' @@ -223,6 +221,19 @@ ->and($this->postPlatform->error_message)->not->toContain('getMediaCategory'); }); +test('the job-failed hook also genericizes a raw internal error', function () { + Event::fake(); + + (new PublishToSocialPlatform($this->postPlatform))->failed(new TypeError( + 'boom in /home/forge/app.trypost.it/releases/72198060/app/Services/Social/XPublisher.php on line 130' + )); + + $this->postPlatform->refresh(); + expect($this->postPlatform->status)->toBe(PlatformStatus::Failed) + ->and($this->postPlatform->error_message)->toBe('An unexpected error occurred while publishing. Please try again.') + ->and($this->postPlatform->error_message)->not->toContain('/home/forge'); +}); + test('publish to social platform marks account as token expired on auth failure', function () { Event::fake(); Mail::fake(); diff --git a/tests/Feature/Services/Social/XPublisherTest.php b/tests/Feature/Services/Social/XPublisherTest.php index b9383453..9847eaa5 100644 --- a/tests/Feature/Services/Social/XPublisherTest.php +++ b/tests/Feature/Services/Social/XPublisherTest.php @@ -4,6 +4,7 @@ use App\Enums\PostPlatform\ContentType; use App\Enums\SocialAccount\Platform; +use App\Exceptions\Social\XPublishException; use App\Exceptions\TokenExpiredException; use App\Models\Post; use App\Models\PostPlatform; @@ -254,16 +255,12 @@ }); test('x publisher recovers a missing mime type from the downloaded bytes', function () { - // Media attached by URL can arrive without a mime_type; X must still publish - // it instead of crashing with a TypeError in getMediaCategory(). $this->post->update([ 'media' => [ ['url' => 'https://cdn.example.com/listing'], ], ]); - // The resize itself is covered by MediaOptimizerTest; here we only need the - // MIME to be recovered so the upload doesn't crash on a null mime. $mockOptimizer = Mockery::mock(MediaOptimizer::class); $mockOptimizer->shouldReceive('optimizeImage')->andReturnUsing(function (string $tempFile) { $optimized = tempnam(sys_get_temp_dir(), 'x_opt_'); @@ -284,7 +281,6 @@ return Http::response(['data' => ['id' => '1212121212', 'text' => 'Hello from X!']], 200); } - // The media download — real image bytes so the MIME can be sniffed. return Http::response( file_get_contents(__DIR__.'/../../../fixtures/1x1.png'), 200, @@ -399,3 +395,16 @@ expect($appendCount)->toBeGreaterThan(1); }); + +test('x publisher fails cleanly when media cannot be downloaded', function () { + $this->post->update([ + 'media' => [ + ['url' => 'https://cdn.example.com/listing', 'mime_type' => 'image/jpeg'], + ], + ]); + + Http::fake(['cdn.example.com/listing' => Http::response(null, 404)]); + + expect(fn () => $this->publisher->publish($this->postPlatform)) + ->toThrow(XPublishException::class, 'Could not fetch the media to upload to X'); +});