refactor(social): trim verbose comments + harden X chunked upload from review
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<Type> annotations didn't resolve — now array<MediaType>. - Tests: cover the failed() job hook genericizing a raw error, and X failing cleanly (XPublishException) when media can't be downloaded.
This commit is contained in:
parent
5b26747dde
commit
8d7dcdf6eb
8 changed files with 47 additions and 43 deletions
|
|
@ -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<MediaType> $allowedTypes
|
||||
* @param array<int, array<string, mixed>> $media
|
||||
|
|
|
|||
|
|
@ -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<int, Platform>
|
||||
*/
|
||||
public function selectedPlatforms(): Collection
|
||||
|
|
|
|||
|
|
@ -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
|
||||
{
|
||||
|
|
|
|||
|
|
@ -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<int, Platform> $platforms
|
||||
* @return array<Type>
|
||||
|
|
|
|||
|
|
@ -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<Type> $allowedTypes
|
||||
* @param array<MediaType> $allowedTypes
|
||||
* @param array<int, array<string, mixed>> $items
|
||||
* @return array{media: array<int, array<string, mixed>>, failed: array<int, string>}
|
||||
*/
|
||||
|
|
@ -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<Type> $allowedTypes
|
||||
* @param array<MediaType> $allowedTypes
|
||||
* @return array<string, mixed>|null
|
||||
*/
|
||||
public function fetchToWorkspace(Workspace $workspace, array $allowedTypes, string $url): ?array
|
||||
|
|
|
|||
|
|
@ -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', [
|
||||
|
|
|
|||
|
|
@ -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();
|
||||
|
|
|
|||
|
|
@ -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');
|
||||
});
|
||||
|
|
|
|||
Loading…
Reference in a new issue