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

Videos don't have derivatives, serve them straight away. #9

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Videos don't have derivatives, serve them straight away. #9

wants to merge 1 commit into from

Conversation

ghost
Copy link

@ghost ghost commented May 16, 2017

This PR allows to serve directly the videos instead of trying to create derivates. They would fail to serve videos, since yo don't need either the full resolution permission or a derivative.

@yonjah
Copy link
Owner

yonjah commented May 17, 2017

I can see your point.
The thing is I took the authorization code from piwigo core action.php.
Maybe I missed something, but it doesn't seem like the core code skip this check for any file type.
So I don't think it wise to skip it by default.
Also your solution in theory will only work with 'mp4' files explicitly (not even MP4) which as common as it might be won't cover all use cases where skipping the derivative process might be necessary.
If there is no existing piwigo setting to solve this issue, I would suggest adding a setting -
piwigo_privacy_no_derivative which will be array of extensions which does not have derivative and original can be served.
The check should verify the extension of the file is exactly as the extension in the list (if user wants both upper case and lower case extension it should add both of them).

Please not that you have a bug in the test which will actually cause it to skip any file unless it starts with '.mp4' which is definitely not what we want.

@@ -210,6 +210,11 @@ function pwg_privacy_verify_access ($img_id, $req_path) {

//file is original uploaded image
if ( strpos($req_path, $path) === 0 ) {
//file is a video, serve it.
if ( strpos($path, '.mp4') !== 0 ) {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

strpos returns false if it can't find the needle in the haystack.
0 is returned only if it is the first match like '.mp4file'

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ack. Let me rework the condition (this would be like the second time in my life I do PHP ;-) ).

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Notice that piwigo provides a get_extension method (used couple of times in this file).
So you can probably just do get_extension($path) === 'mp4'.
With multiple valid extensions you'll probably need to use in_array to see if the extension is a valid one

@ghost
Copy link
Author

ghost commented May 17, 2017

Thanks for the comments. I like the idea of having that setting to control the extensions that have no derivatives generated. I will rework this code then.

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.

2 participants