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

CakeResponse::file() should accept an instance of SplFileObject #3515

Closed
lorenzo opened this issue May 17, 2014 · 21 comments
Closed

CakeResponse::file() should accept an instance of SplFileObject #3515

lorenzo opened this issue May 17, 2014 · 21 comments
Milestone

Comments

@lorenzo
Copy link
Member

lorenzo commented May 17, 2014

If an instance of a file is passed it should be able to stream the file. This is super useful for sending files that come from another stream, from a tmp file or just from memory.

@lorenzo lorenzo added this to the 3.0.0 milestone May 17, 2014
@jippi
Copy link
Contributor

jippi commented May 17, 2014

👍 <3

@markstory
Copy link
Member

This could be done in 2.6 and end up in 3.0 via merges.

@ADmad
Copy link
Member

ADmad commented Jun 7, 2014

CakeResponse::file() currently using the core File class internally. If SplFileObject is always available in php 5.2 the internal code can be changed to use SplFileObject instead, then it would just be a matter of checking method param type.

@lorenzo
Copy link
Member Author

lorenzo commented Jun 8, 2014

SplFileObject is always available

@ADmad ADmad self-assigned this Jun 8, 2014
@ADmad
Copy link
Member

ADmad commented Jun 8, 2014

Okay then, I will add this to my todo list.

@markstory
Copy link
Member

This could be done in 2.6, and then forward ported to 3.0.

@ADmad
Copy link
Member

ADmad commented Jun 8, 2014

That's what i plan to do.

@ADmad
Copy link
Member

ADmad commented Jun 8, 2014

So apparently SplFileObject doesn't have a method to read bytes making it unusable for non-text files.

@markstory
Copy link
Member

I guess we can't implement this then.

@ADmad ADmad removed their assignment Jun 10, 2014
@markstory
Copy link
Member

Closing as it doesn't sound like this is going to be possible. SplFileObject does not provide the necessary tools to work with binary files.

@lorenzo
Copy link
Member Author

lorenzo commented Jun 12, 2014

What is it lacking?

@ADmad
Copy link
Member

ADmad commented Jun 12, 2014

Function to read binary files. It only has functions to read text files, per character or per line.

@lorenzo
Copy link
Member Author

lorenzo commented Jun 12, 2014

It does not have to be line by line though. Just let php do the work for you using fpassthru

@ADmad
Copy link
Member

ADmad commented Jun 12, 2014

Yes but fpasthru doesn't take SplFileObject as param only resource handle. In that case don't see the point of accepting SplFileObject instance as param. Plus we lose the ability to send the output in chunks since fpassthru outputs till EOF.

@lorenzo
Copy link
Member Author

lorenzo commented Jun 12, 2014

http://dk1.php.net/manual/en/splfileobject.fpassthru.php

Do we need to send chunks?

@ADmad
Copy link
Member

ADmad commented Jun 12, 2014

Oh sorry was looking at the fpasthru function instead of method.

We currently do send in chunks which allows terminating the output if client closes connection (at least that's what i understand). Also we would lose the ability to send only specified chunk based on HTTP_RANGE which is currently supported.

@lorenzo
Copy link
Member Author

lorenzo commented Jun 12, 2014

@ADmad range can be done with fseek()

I think doing chunks in php was a bad idea, personally. Web servers are good at negotiating that on their own. Anyway, I would be willing to live we the downside of no chunk when a SplFileObject is passed to Response::file() only because of the benefit of being able to pass it :D

@ADmad
Copy link
Member

ADmad commented Jun 12, 2014

@ADmad range can be done with fseek()

With fseek() you would be just able to set the start not the end isn't it?

I think doing chunks in php was a bad idea, personally. Web servers are good at negotiating that on their own.

I don't have much idea about how webserver handle this so I won't comment :)

Anyway, I would be willing to live we the downside of no chunk when a SplFileObject is passed to Response::file() only because of the benefit of being able to pass it :D

In that case this would goto 3.0 since removing the chunking would be BC break.

@lorenzo
Copy link
Member Author

lorenzo commented Jun 12, 2014

@ADmad You mark the end with ftruncate()

I'm fine with it being in 3.0

@lorenzo
Copy link
Member Author

lorenzo commented Jun 12, 2014

Let me investigate bit more about that. I was under the impression that you needed to fwrite after ftruncate. Will find an alternative

@lorenzo
Copy link
Member Author

lorenzo commented Jun 12, 2014

bah, too many complications. Scrap this feature

hmic pushed a commit to hmic/cakephp that referenced this issue Aug 24, 2014
https://cakephp.lighthouseapp.com/projects/42648-cakephp/tickets/3515

I used (string) because it is faster than strval() and there are no real benefits in this case.
Also enabled assign(), append() and prepend() to take any values convertible to string.

Removed try/catch as discussed with ADmad.
Changed the three exception expecting tests to check for PHPUnit_Framework_Error now.
hmic pushed a commit to hmic/cakephp that referenced this issue Aug 24, 2014
Conflicts:
	lib/Cake/Test/Case/View/ViewTest.php

Fixes cakephp#3515
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants