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

Adding foundation for the direct download url #12854

Merged
merged 2 commits into from
Dec 29, 2014

Conversation

DeepDiver1975
Copy link
Member

refs #12286

@dragotin @danimo @guruz @ogoffart @karlitschek @icewind1991 THX

  • rename dav property to avoid unexpected behavior of client 1.7.x series
  • add implementation for S3
  • add implementation for SWIFT

@karlitschek
Copy link
Contributor

looks good 👍

* For now the returned array can hold the parameter url - in future more attributes might follow.
*
* @param string $path
* @return []|null
Copy link
Contributor

Choose a reason for hiding this comment

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

For return type you still need array instead of [] afaik

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe return an empty array instead of null when not supported?

* @return array
*/
public function getDirectDownload($path) {
// FIXME: we need a global mount config which enables/disables direct download links
Copy link
Member Author

Choose a reason for hiding this comment

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

@icewind1991 yet another mount config option - what is the plan to incorporate this? THX

Copy link
Contributor

Choose a reason for hiding this comment

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

Why would there be a config for this? shouldn't the backend have enough info to determine if it can be used?

Copy link
Member Author

Choose a reason for hiding this comment

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

Why would there be a config for this? shouldn't the backend have enough info to determine if it can be used?

we want to give the admin to explicitly enable/disable direct download on mount level.

@DeepDiver1975
Copy link
Member Author

@owncloud-bot retest this please

@ghost
Copy link

ghost commented Dec 22, 2014

Refer to this link for build results (access rights to CI server needed):
https://ci.owncloud.org//job/pull-request-analyser-ng-simple/5410/
👍 Test PASSed. 👍

@karlitschek
Copy link
Contributor

ping. can anyone have a look?
@dragotin @danimo @guruz @ogoffart @karlitschek @icewind1991

@schiessle
Copy link
Contributor

Code looks good, but currently it doesn't do anything useful, or do I miss something? The only implementation of getDirectDownload() returns a empty array.

What about the open point "rename dav property to avoid unexpected behavior of client 1.7.x series"?

@DeepDiver1975 Is this PR complete? Any steps to try it?

@karlitschek
Copy link
Contributor

I guess this requires an implementation in a storage backend.
@DeepDiver1975

@danimo danimo changed the title Adding basement for the direct download url Adding foundation for the direct download url Dec 23, 2014
@DeepDiver1975
Copy link
Member Author

@schiesbn besides the fact that the dav property has to be renamed - this PR is ready

hopefully I find the time to submit pull requests to implement S3 and/or swift

@ogoffart @guruz make me an X-Mas present and propse a cool new name 😉

@ogoffart
Copy link

Be explicit? "downloadURL"

@ghost
Copy link

ghost commented Dec 24, 2014

Refer to this link for build results (access rights to CI server needed):
https://ci.owncloud.org//job/pull-request-analyser-ng-simple/5556/
👍 Test PASSed. 👍

@scrutinizer-notifier
Copy link

The inspection completed: 3 new issues, 5 updated code elements

@ghost
Copy link

ghost commented Dec 28, 2014

Refer to this link for build results (access rights to CI server needed):
https://ci.owncloud.org//job/pull-request-analyser-ng-simple/5600/
👍 Test PASSed. 👍

@icewind1991
Copy link
Contributor

👍

@karlitschek
Copy link
Contributor

cool. Let´s merge.

karlitschek added a commit that referenced this pull request Dec 29, 2014
Adding foundation for the direct download url
@karlitschek karlitschek merged commit cd53da4 into master Dec 29, 2014
@karlitschek karlitschek deleted the add-direct-download-link branch December 29, 2014 17:47
@lock lock bot locked as resolved and limited conversation to collaborators Aug 14, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants