czim / laravel-paperclip

Laravel file attachment solution for Eloquent
MIT License
78 stars 18 forks source link

Filename encoding inconsistent with stapler #41

Open iredmedia opened 5 years ago

iredmedia commented 5 years ago

I'm migrating from stapler and my last hurdle is that filenames are being encoded differently.

Stapler would save the filename without encoding.

As a work around i've extended the Interpolator to override the filename function, doing a urldecode on $attachment->originalFilename().

Would love some feedback, is this something you'd like to have configurable? Is there a better solution? Would be happy to contrib.

czim commented 5 years ago

I don't understand why it should be different. I've been looking at the code, and the logic is the same as far as I can see:

Stapler and Paperclip share exactly this same logic:

And both use :filename in the default setup (for S3 in the case of Stapler, anyway).

So the most help I need is with understanding this.

If a short-term workaround is needed, I'd prefer adding something like a :url_decoded_filename placeholder to the interpolator for this purpose.

iredmedia commented 5 years ago

Fascinating, I've logged out all the way back to symfony/http-foundation and I'm seeing that it's not encoding the filename.

laravel/framework @ v5.5.45 codesleeve/laravel-stapler @ v1.0.09
codesleeve/stapler @ v1.2.0 symfony/http-foundation @ v3.4.26

vendor/symfony/http-foundation/File/UploadedFile.php

    public function getClientOriginalName()
    {
        \Log::info('getClientOriginalName', [$this->originalName]); // My log
        return $this->originalName;
    }
czim commented 5 years ago

Perhaps it's something to do with the database the filename is stored to? This too should be 100% identical between Stapler and Paperclip, but it might be worth examining.

iredmedia commented 5 years ago

I'm loosing my mind over this issue. The problem has apparently disappeared (even though I tested multiple times). Closing issue.

czim commented 5 years ago

Alright, those are the fun ones! Let me know if it comes back to stay. 👍

iredmedia commented 5 years ago

Hey there!

Finally had time to take another look at this. It seems that the characters that cause paperclip to fail (Laravel 5.8 + Postgres + S3)

The characters that are causing us to fail (without thinking about stapler, just with a fresh paperclip install are: +, ?, and "

Upon uploading a file, say +.png, the result returned is somepath/+.png. This fails, however checking somepath/%2B.png works fine.

Any advice/insights on this? Stapler didn't suffer from this issue.

czim commented 5 years ago

What what settings do you use for S3 storage? I've been seeing some issues with S3 in some cases..

czim commented 5 years ago

Update: I've been experimenting with Minio, trying to get a setup that simulates S3 well enough for automated testing. Unfortunately it does not appear that the behavior is identical. If anyone gets this working (or rather: gets it to break in the same way), I'd be obliged to hear the details!

austenc commented 4 years ago

We just noticed this with filenames containing # too.

austenc commented 4 years ago

So it seems to be happening with characters that are otherwise valid in a URI. Particularly +, ? and #.

Other characters (such as a space) seem to be automatically encoded by modern browsers to things like %20 -- but since characters such as # could be part of a URL, they aren't encoded automatically. Seems like paperclip or czim/file-handling should account for some of these characters in the filename when generating the URL. Happy to submit a PR, but not sure yet where to make this change 🤔