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

[5.4] Add ability to keep old files when testing uploads [second attempt] #19859

Merged
merged 3 commits into from
Jul 6, 2017
Merged

Conversation

gocanto
Copy link
Contributor

@gocanto gocanto commented Jul 2, 2017

Hi guys,

This is my second attempt to get this behavior into the framework.

This PR had been closed in the past due to the uses of Boolean variables. It does not introduce breaking changes because it keeps the actual behavior by default.

They way how it can be used is like so:

Storage::withOldFiles()->fake($disk)

Whether users want to keep the uploaded files when testing and make assertions to them.

Storage::fake($disk)

Whether you want to keep the actual behavior.


Even though I will understand if this PR is not accepted, I would like to see something similar to this in the framework.

@taylorotwell I will appreciate your thoughts on this.

Thanks,

@gocanto gocanto changed the title Add ability to keep old files when testing uploads [second attempt] [5.4] Add ability to keep old files when testing uploads [second attempt] Jul 2, 2017
@deleugpn
Copy link
Contributor

deleugpn commented Jul 2, 2017

keepFiles would be a much better name, imo.

@gocanto
Copy link
Contributor Author

gocanto commented Jul 3, 2017

I really used that name knowing taylor with comes out woth soemthing better. I jist would like to have this behavior in the framework :) though I like that one also :)

@taylorotwell
Copy link
Member

taylorotwell commented Jul 5, 2017

What about just a whole new method persistentFake? I don't know why you need a property at all really, and it also creates this state that is left hanging around.

@gocanto
Copy link
Contributor Author

gocanto commented Jul 5, 2017

Ok, that sounds better then. I ll send the pr tonight. Thanks

@gocanto
Copy link
Contributor Author

gocanto commented Jul 5, 2017

Hey @taylorotwell

I made the required changes.

Thanks,

@taylorotwell taylorotwell merged commit ac4e043 into laravel:5.4 Jul 6, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants