Add spatie media library to demo #456
Conversation
|
Note: we should review the final syntax so that I can update this branch with the correct definitions to work in demo. |
|
Let's finish this PR to use the latest media syntax 💪 |
|
@tabacitu testing notes. This requires: Laravel-Backpack/CRUD#4921 Everything should be configured in this PR, just don't forget to This should work out-of-the-box! 🙏 |
Bumps [backpack/backupmanager](https://github.com/Laravel-Backpack/BackupManager) from 3.0.9 to 4.0.2. - [Release notes](https://github.com/Laravel-Backpack/BackupManager/releases) - [Changelog](https://github.com/Laravel-Backpack/BackupManager/blob/master/CHANGELOG.md) - [Commits](Laravel-Backpack/BackupManager@v3.0.9...v4.0.2) --- updated-dependencies: - dependency-name: backpack/backupmanager dependency-type: direct:production update-type: version-update:semver-major ... Signed-off-by: dependabot[bot] <[email protected]>
…kpack/backupmanager-4.0.2 Bump backpack/backupmanager from 3.0.9 to 4.0.2
|
I've just gone through this PR and "refreshed" it:
And I can confirm it is working WONDERFULLY: There's only one thing I didn't do, that's preventing me from merging this is. We don't have a stopping mechanism for the online demo 👀 @pxpm could you please prevent people from actually uploading, when this is in production? We can probably bind/intercept all uploaders in production, to prevent the uploads... right?! 🤷♂️ |
|
Thanks for moving this foward, was my next on line 🙏
Will investigate the best course of action here. |
# Conflicts: # composer.lock
|
This is pending Laravel-Backpack/medialibrary-uploaders#17 As soon as that gets merged I think we are good to go here. @tabacitu can you give this a double check in the production constrains to don't upload files ? Cheers |
There was a problem hiding this comment.
For UX and another layer of protection, I've made the upload fields disabled in production. For the repeatable I've chosen to hide it entirely.
I only want one extra thing out of this - for the mutator overrides to actually work, to prevent uploads instead of triggering an error.
I've added other ideas here, but those aren't urgent.
| public function specifications(): Attribute | ||
| { | ||
| return Attribute::make( | ||
| set: function ($item) { | ||
| if (app('env') === 'production') { | ||
| return null; | ||
| } | ||
|
|
||
| return json_encode($item); | ||
| }, | ||
| ); | ||
| } |
There was a problem hiding this comment.
I'm wondering... if the actual upload now... is being done with Uploaders, not Mutators... why are we using Mutators to catch and stop the upload process?
Why don't we bind to all our uploaders... and prevent the upload process from happening... for ALL uploaders, if the env is production? Wouldn't that be cleaner, and more secure? That way nobody can introduce a vulnerability in our demo, all Uploaders are prevented from working in production.
There was a problem hiding this comment.
Or maybe we can catch the ->withMedia() and ->withFiles() helper and do that there?
There was a problem hiding this comment.
This isn't urgent though. Just a thought. What I want right now is to get this merged and out of the way, ASAP.
There was a problem hiding this comment.
We can catch the withMedia and withFiles, with that we prevent the uploader events from beeing registered, so we wouldn't even need to bind into the uploader classes as they wouldn't be registered/called.
That wouldn't prevent much, just the upload from happening, but anything you submit in the input would be saved to the database, except it didn't go through the upload process.
At the moment we can't bind to our uploaders as they aren't resolved from the container, they are statically initialized.
We can work that out, or use the mutator as we are doing now. Everything is "working", except we always send empty values to the uploaders 🤷
Probably it's like you said, it's more future proof, but at the moment it's not a requirement.
| public function dropzoneUpload() | ||
| { | ||
| if (app('env') === 'production') { | ||
| return response()->json([]); | ||
| } | ||
|
|
||
| return $this->traitDropzoneUpload(); | ||
| } |
There was a problem hiding this comment.
Same with dropzone. Can't we bind on the DropzoneOperation trait, and prevent all dropzone uploads in production? It's too broad in a normal app, but in our case that's what we want, to prevent all uploads.
There was a problem hiding this comment.
Not urgent - just a thought. Right now I just want to merge this ASAP.
There was a problem hiding this comment.
Are you thinking something like this:
<?php
namespace App\Http\Controllers\Operations;
if (! env('APP_ENV') === 'production') {
trait DropzoneOperation
{
public function dropzoneUpload()
{
return response()->json([]);
}
}
} else {
trait DropzoneOperation
{
use \Backpack\Pro\Http\Controllers\Operations\DropzoneOperation;
}
}And then in demo we use App\Http\Controllers\Operations\DropzoneOperation instead of the one in the package ?
If you agree I can push this change.
Cheers
# Conflicts: # composer.json # composer.lock
|
Tested, works, merging 🎉 Let's postpone the other 2 suggestions, they're COULDs/SHOULDs. Added issues for them for later in the year: |
WHY
BEFORE - What was wrong? What was happening before this PR?
No working examples on how to use Spatie Media Library with our fields.
Installation steps:
composer updatefirst to get media library installed, I already added it tocomposer.jsonfilesystems.phpalong with links. If you are caching your configs, make sure tophp artisan config:clear.php artisan migrate:fresh --seedphp artisan storage:linkYou should be good to go. Navigate to Products Crud, you should see something like:
AFTER - What is happening after this PR?
We should now evaluate the steps taken to use Spatie Media Library and brainstorm a way to make developers life easier. I see there is a lot of "repetitive" work that we can automate.