CRUD icon indicating copy to clipboard operation
CRUD copied to clipboard

Add a filename param to uploadFileToDisk

Open KevinCassier opened this issue 3 years ago • 1 comments

Add an optional destination filename for the uploadFileToDisk method

WHY

Sometimes we need/want to define a filename for uploaded files.

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

Impossible de define a filename for the uploaded files. It's only a random filename.

AFTER - What is happening after this PR?

You can easily pass an optional filename to your uploaded files

HOW

How did you achieve that, in technical terms?

I added an optional param "destination_filename" to uploadFileToDisk method to define a filename to the uploaded files if we need it.

Is it a breaking change?

No

How can we test the before & after?

Add a destination_filename param in the uploadFileToDisk method

KevinCassier avatar May 08 '22 15:05 KevinCassier

BOOM! Your first PR with us, thank you so much! Someone will take a look at it shortly.

Please keep in mind that:

  • if this constitutes a breaking change, it might take quite a while for this to get merged; we try to emulate the Laravel release cycle as much as possible, so developers can upgrade both software once; this means a new big release every ~6 months;
  • even if it's a non-breaking change, it might take a few days/weeks for the PR to get merged; unless it's a no-brainer, we like to have some community feedback on new features, before we merge them; this leads to higher-quality code, in the end; we learnt this the hard way :-)
  • not all PRs get merged; sometimes we just have to hold out new features, to keep the packages lean; sometimes we don't include features that only apply to niche use cases;
  • we're not perfect; if you think we're wrong, call us out on it; but in a kind way :-) we all make mistakes, best we learn from them and build better software together;

Thank you!

-- Justin Case The Backpack Robot

welcome[bot] avatar May 08 '22 15:05 welcome[bot]

@KevinCassier sorry - looks like this slipped through the cracks, just seeing it now. Good idea! @pxpm could you please test & merge this if it's ok?

tabacitu avatar Oct 24 '22 16:10 tabacitu

Thanks @KevinCassier it was a no brainer to add, sorry it took so much time to get into it.

I've just gave this a review and ended up creating a new branch to refactor some stuff. I started from your commits so that you show up as a contributor too in that branch https://github.com/Laravel-Backpack/CRUD/pull/4742

I will be closing this in favor of that other I created, thanks for the PR 🙏

pxpm avatar Oct 26 '22 10:10 pxpm

Thanks guys @pxpm @tabacitu for the merge! Happy to contribute a little bit to this cool project! :)

KevinCassier avatar Nov 04 '22 10:11 KevinCassier