Add API endpoint for uploading screenshots on lazer #12618
Conversation
LiquidPL
force-pushed
the
screenshot-upload
branch
from
642fcf6 to
c52a595
Compare
| $screenshot = new Screenshot(); | ||
| $screenshot->user_id = auth()->user()->getKey(); | ||
| $screenshot->save(); |
There was a problem hiding this comment.
can be done in one step with ::create. And use \Auth::user() instead of auth()->user()
| $screenshot->user_id = auth()->user()->getKey(); | ||
| $screenshot->save(); | ||
|
|
||
| $this->storage()->putFileAs('/', $validated['screenshot'], "{$screenshot->getKey()}.jpg"); |
There was a problem hiding this comment.
should be done in the model
| return response()->json([ | ||
| 'url' => route('screenshots.show', [ | ||
| 'screenshot' => $screenshot->getKey(), | ||
| 'hash' => substr(md5($screenshot->getKey().config('osu.screenshots.shared_secret')), 0, 4), |
There was a problem hiding this comment.
should be done in the model. And use $GLOBALS['cfg'][...] instead of config(...)
|
|
||
| public function definition(): array | ||
| { | ||
| return []; // nothing in this model to fake |
| 'screenshot' => $screenshot->getKey(), | ||
| 'hash' => substr(md5($screenshot->getKey().config('osu.screenshots.shared_secret')), 0, 4), | ||
| ]), | ||
| ], 201); |
There was a problem hiding this comment.
201 requires returning location: header when it's not the same url as the post request.
Or just return 200.
|
|
||
| // Copyright (c) ppy Pty Ltd <contact@ppy.sh>. Licensed under the GNU Affero General Public License v3.0. | ||
| // See the LICENCE file in the repository root for full licence text. | ||
|
|
There was a problem hiding this comment.
can use declare strict (also for all other files)
| $validated = $request->validate([ | ||
| 'screenshot' => [ | ||
| 'required', | ||
| File::types(['jpeg']), |
There was a problem hiding this comment.
need a size limit of maybe 10MB (or App\Libraries\Uploader::DEFAULT_MAX_FILESIZE)
config/filesystems.php
Outdated
| 'visibility' => 'public', | ||
| ], | ||
|
|
||
| 'local-screenshot' => [ |
There was a problem hiding this comment.
before solo-replay (also for the other one)
|
|
||
| 's3-screenshot' => [ | ||
| ...$s3Default, | ||
| 'bucket' => presence(env('S3_SCREENSHOT_BUCKET')) ?? 'screenshots', |
There was a problem hiding this comment.
add to .env.example (also for the other one)
| use Illuminate\Contracts\Filesystem\Filesystem; | ||
| use Illuminate\Http\Request; | ||
| use Illuminate\Validation\Rules\File; | ||
| use Storage; |
There was a problem hiding this comment.
just use directly with \Storage instead of adding more lines
2 participants
Web side PR for ppy/osu#10718.
This implementation matches the behaviour of the respective endpoint in legacy web, since the screenshot retrieval endpoint is currently being handled by that. Ability to retrieve said screenshots using osu-web will come in a separate PR.
Steps to deploy:
S3_SCREENSHOT_BUCKETenv varSCREENSHOTS_SHARED_SECRETenv var