Don't lie about the preview mimetype #7692
Conversation
rullzer
added
2. developing
rullzer
force-pushed
the
dont_lie_preview_mimetype
branch
from
a50a99c to
6241374
Compare
Codecov Report
@@ Coverage Diff @@
## master #7692 +/- ##
============================================
- Coverage 51.17% 51.17% -0.01%
- Complexity 24948 24959 +11
============================================
Files 1605 1605
Lines 94923 94951 +28
Branches 1376 1376
============================================
+ Hits 48579 48589 +10
- Misses 46344 46362 +18
|
|
How to reproduce this? I guess it is about to upload a JPG and a PNG and then look into the activity stream. In Firefox/Chrome it showed the previews and in IE it did not show the previews? |
|
@MorrisJobke yep. IE11 I tested it. Basically FF and Chrome say well you told me it was a PNG but I see it is a JPEG so I just display it as a jpeg. While IE says nope not a PNG so I won't show you anything. |
rullzer
added
3. to review
lib/private/Preview/Generator.php
Outdated
| case 'image/gif': | ||
| return 'gif'; | ||
| default: | ||
| return null; |
There was a problem hiding this comment.
Why not return ''? Then we don't have this nasty null?
| @@ -102,6 +102,12 @@ public function save($filePath = null, $mimeType = null); | |||
| */ | |||
| public function resource(); | |||
|
|
|||
| /** | |||
| * @return string Returns the raw data mimetype | |||
There was a problem hiding this comment.
Could also return null. Either return empty string in error case or point out the null here as well. I would vote for one type of return value.
| $path .= '.png'; | ||
|
|
||
| $ext = $this->getExtention($mimeType); | ||
| $path .= '.' . $ext; |
There was a problem hiding this comment.
if ext is null or '' (after morris change), the path is weird. so without any extension.
|
Thumbnails for png and jpg work fine 👍 |
|
Okay - let's wait with this after beta 4 and fix it until then. |
MorrisJobke
added
2. developing
For legacy reasons we stored all the previews with a png extention. However we did not put png data in them all the time. This caused the preview endpoints to always report that a preview is a png file. Which was a lie. Since we abstract away from the storage etc in the previewmanager. There is no need anymore to store them as .png files and instead we can use the actual file extention. Signed-off-by: Roeland Jago Douma <roeland@famdouma.nl>
rullzer
force-pushed
the
dont_lie_preview_mimetype
branch
from
6241374 to
048c1fd
Compare
|
Now just 1 return type. Also in the preview generation an exception will be thrown. Which will get converted into a NotFoundException. Thus showing a proper 404 to the user. |
rullzer
added
3. to review
| case 'image/gif': | ||
| return 'gif'; | ||
| default: | ||
| throw new \InvalidArgumentException('Not a valid mimetype'); |
There was a problem hiding this comment.
Add this to the PHPDoc please
Throw proper exception if we can't get the mimetype for a preview. Catch it later on so we can just return a not found for the preview. Signed-off-by: Roeland Jago Douma <roeland@famdouma.nl>
rullzer
force-pushed
the
dont_lie_preview_mimetype
branch
from
048c1fd to
faa68b2
Compare
|
@nickvergessen is this ok for you now? |
For legacy reasons we stored all the previews with a png extention.
However we did not put png data in them all the time.
This caused the preview endpoints to always report that a preview is a
png file. Which was a lie.
Since we abstract away from the storage etc in the previewmanager. There
is no need anymore to store them as .png files and instead we can use
the actual file extention.