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

Low memory file fetch #73

Merged
merged 5 commits into from Jul 1, 2015
Merged

Conversation

daniel-sc
Copy link
Member

Hi,

the commits add the functionality to fetch files from podio without loading them into memory - this is very useful when handling large files.

Could you please review (and merge)?

cheers, Daniel

this is usefull for minimal memory settings.
(This prevents explosion of open file handles on many requests.)
@daniel-sc
Copy link
Member Author

Hi there,

Is anyone watching this?
If there are questions concerning the code and/or the intend - I'm happy to answer!

Daniel

* @throws PodioServerError
* @throws PodioUnavailableError
*/
public static function request($method, $url, $attributes = array(), $options = array(), $returnRawAsRescourceOnly = false) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you get rid of the docstring as it's not related to the functionality you're adding. Also, use the $options arg for your new option instead of adding a new function argument

@haugstrup
Copy link
Contributor

Sorry about that! I've added some comments to get the code style aligned for starters

@daniel-sc
Copy link
Member Author

Hi @haugstrup ,

thanks for the feedback - I committed the changes you suggested.

@haugstrup
Copy link
Contributor

Thanks Daniel, I'll do a proper review sooner rather than later :)

@daniel-sc
Copy link
Member Author

Hi @haugstrup ,
could you please merge?

@daviferreira
Copy link
Contributor

Hey @daniel-sc, awesome work and sorry for the late response. Could you rebase against the latest master please so I can merge? :) Cheers!

Conflicts:
	models/PodioFile.php
@daniel-sc
Copy link
Member Author

Done. @haugstrup you can merge.

daviferreira added a commit that referenced this pull request Jul 1, 2015
@daviferreira daviferreira merged commit 0673364 into podio-community:master Jul 1, 2015
@daviferreira
Copy link
Contributor

Thanks!

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.

None yet

3 participants