Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add withFiles() functionality to CRUD fields #4988

Merged
merged 73 commits into from
Apr 25, 2023
Merged

Add withFiles() functionality to CRUD fields #4988

merged 73 commits into from
Apr 25, 2023

Conversation

pxpm
Copy link
Contributor

@pxpm pxpm commented Mar 17, 2023

WHY

BEFORE - What was wrong? What was happening before this PR?

Managing uploads with backpack fields could be a challenge sometimes. Especially if we are talking about repeatable fields etc.

This PR aims to provide a tool to help developers setup their file uploads quicker ... well .. way way quicker 😄

AFTER - What is happening after this PR?

You should be able to get a fully working upload field in your crud with just:

CRUD::field('document')->type('upload')->withFiles();
CRUD::field('documentsss')->type('upload_multiple')->withFiles();
CRUD::field('passport_image')->type('image')->withFiles();

Is it a breaking change?

No it's not! Fantastic!

How can we test the before & after?

Will write some docs later.

@pxpm pxpm changed the base branch from main to poc-upload-non-breaking March 17, 2023 20:33
@pxpm pxpm self-assigned this Mar 17, 2023
pxpm and others added 6 commits April 5, 2023 20:34
… into crud-uploads

# Conflicts:
#	src/app/Library/Uploaders/Support/Interfaces/UploaderInterface.php
[ci skip] [skip ci]
[ci skip] [skip ci]
@scrutinizer-notifier
Copy link

The inspection completed: 36 new issues, 77 updated code elements

Copy link
Member

@tabacitu tabacitu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is much MUCH better Pedro! Only a few questions really, nothing important. This is GOOD TO GO! 🎉🎉🎉


class FileNameGenerator implements FileNameGeneratorInterface
{
public function getName(string|UploadedFile $file): string
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmmm could/should we make this for() to match the other one? And maybe it reads better? FileNameGenerator::for('somefile.txt') - what do you think @pxpm?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree with this, let's dor it 💪

@pxpm
Copy link
Contributor Author

pxpm commented Apr 14, 2023

image

Requires manual validation (no laravel rules supplied to handle base64). One can use a package like https://github.com/crazybooot/base64-validation

  • When nothing changes, the field send the db path in the request.
  • When file is deleted and a new one is NOT added, it will send an empty value (null) in request.

Even using the package mentioned it cannot be used with regular laravel validation (eg. form requests), requiring custom validation to properly work.
Possible solution: don't send nothing when nothing changes. allowing devs to supply two validators (create/update with required/sometimes respectively), see upload example bellow.

upload

Need two separate validators, one for create and another for update.

  • When file is deleted and a new one is NOT added, it will send an empty value (null) in request.

use case: required avatar field (can never be null needs to be a valid file)

// create validation:
'avatar' => [
                'required', // on create this field must be a valid type
                File::types(['pdf'])
                    ->min(1024)
                    ->max(10 * 1024),
            ],
// update validation:
'avatar' => [
                'sometimes', // if nothing change, nothing is validated. If file is cleared an empty value is sent and it will trigger the validation error.
                File::types(['pdf'])
                    ->min(1024)
                    ->max(10 * 1024),
            ],

upload_multiple

Requires custom validator to properly validate. No other way around, needs breaking change to fix.

There is one hidden input that when nothing changes on field breaks the sending values and sends [ [0=>null] ].

Assuming now we make it like upload so that when nothing changes the validation is not run.

When you delete a file it gets added to a clear_field hidden input used to clear the files. There is a major problem if we do it that way: if you clear ALL files, and your field is required, the validation wouldn't run and the same values would be kept on the db assuming nothing changed in the field.

We can do the same we do in upload, so when ALL files are cleared an hidden input is added with an empty value, making the validation work again.

This goes against #3278 that proposed we send the value instead. I don't think we should be sending the input, what are we validating in a string ? It indeed stores a string, but as far as "application validation" is concerned, what comes from the form should be a file, with type x, size y etc, otherwise don't touch that input.

@pxpm pxpm changed the base branch from main to v6 April 24, 2023 16:36
@pxpm pxpm changed the base branch from v6 to main April 24, 2023 16:42
@pxpm pxpm changed the title Add withUploads() functionality to CRUD fields and columns Add withFiles() functionality to CRUD fields Apr 25, 2023
Copy link
Member

@tabacitu tabacitu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code review ✅ I'm ok with merging this as-is. My only (small) mention is that we might reconsinder some parameter names for temporary URLs - what I said in https://github.com/Laravel-Backpack/docs/pull/421/files

@tabacitu tabacitu changed the base branch from main to v6 April 25, 2023 13:00
@tabacitu
Copy link
Member

tabacitu commented Apr 25, 2023

GOD FUCKING DAMN, THIS WOKS WELL! It works like freakin' magic! Well done Pedro 👏👏👏 Well freakin' done!

thumbs_up_proud_cap

@tabacitu tabacitu merged commit 2c91664 into v6 Apr 25, 2023
@tabacitu tabacitu deleted the crud-uploads branch April 25, 2023 13:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

4 participants