use file_put_contents instead

fuel / core

Fuel PHP Framework - The core of the Fuel v1 framework

http://fuelphp.com

802 stars 335 forks source link

use file_put_contents instead #2187

Closed reillo closed 1 year ago

reillo commented 1 year ago

Touch has a bug sometimes with permission. Why not use file_put_contents function instead?

To replicate:

use s3StreamWrapper

WanWizard commented 1 year ago

I'm not sure what that touch() is supposed to do there, as the driver isn't used on its own, but as an abstract base class for the others. The save() method in those classes do a file_put_contents() to write the image, which would update the timestamp too.

If it is only a clumsy check to see if the file is writeable, why not simply use is_writable(dirname($filename))?

reillo commented 1 year ago

Hi @WanWizard, yes you're right. Perhaps we can make it muted using @ in front of the method touch? I think we need the @ in front of touch because of some stream wrapper (eg. s3) which don't support touch().

see: https://github.com/aws/aws-sdk-php/issues/1019

for now, i'll override this class in my app so the generation of thumbnails works.

Thanks,

WanWizard commented 1 year ago

Yes, I found that issue looking into this one.

I have no issue with simply removing the touch(), I'm just wondering if it needs to be replaced by a different check that is compatible, or that we don't replace it, but catch a possible file_put_contents() in case of for example missing path or write permissions.

reillo commented 1 year ago

we can probably use a muted (@) and exception catch with this one? I've used the muted and exception/catch on my core override. It seems working just fine both the s3 steam and default stream.

WanWizard commented 1 year ago

Yeah, probably. Not happy with it, but it will do the trick I guess.

Can you update your PR, so you get the credit?

reillo commented 1 year ago

@WanWizard sure. I adjusted the PR, let me know if that is okay.

WanWizard commented 1 year ago

Coding standards ! :grin: