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

FYI, minor bug with streamWithHeaders() #573

Closed
starfishpatkhoo opened this issue Apr 11, 2024 · 11 comments
Closed

FYI, minor bug with streamWithHeaders() #573

starfishpatkhoo opened this issue Apr 11, 2024 · 11 comments
Labels

Comments

@starfishpatkhoo
Copy link

The new ->streamWithHeaders is good, but there is a bug... ?

If you do not have a "status" => 200 defined in the array, then there is an exception in Engine.php line 490: Undefined array key "status"

Documentation though says: optional status code, defaults to 200 .. Or maybe you meant the default HTTP header is 200, but that you must have a status?

It is not a big deal, just thought I would point it out.. ^_^

Flight version 3.8.1 ..

@fadrian06
Copy link
Contributor

Thanks for notifying, it is already corrected, the patch will be published in a couple of hours

@starfishpatkhoo
Copy link
Author

Thanks buddy! Like I said, not a big deal, just was confused myself because code and documentation didn't match.. LOL...

Now, it should be possible to do ->streamWithHeaders([]) and set all headers within your route function.. Feels neater that way..

@n0nag0n
Copy link
Collaborator

n0nag0n commented Apr 12, 2024

Now I'm just curious, what are you streaming? What's your use case?

@fadrian06
Copy link
Contributor

Thanks buddy! Like I said, not a big deal, just was confused myself because code and documentation didn't match.. LOL...

Feel free to comment on any questions about the documentation, especially any issues you may have with the examples.

@starfishpatkhoo
Copy link
Author

starfishpatkhoo commented Apr 12, 2024

Now I'm just curious, what are you streaming? What's your use case?

Oh, well it's a small thing basically, we need to restrict access to a PDF by authorisation, so we check first, and if authorised, then send the binary data. If not, just 404 it or something. Or it could be a .pptx or whatever..

Other small things like checking if the PDF exists, and setting content-length, content-disposition file name, force no caching, force ob_end_clean(), etc, etc, all these are best done inside the function before we set/send the headers. Plus when I have time, I wanna add ranged bytes too etc.

TL;DR, lots of stuff to do, and I didn't want to put headers in the array in ->streamWithHeaders .. some headers are there, some are in the function, it gets a bit confusing.. So all headers in one place feels neater to me... Easier to find.

As I think about it, a special function Flight::routeStream() or Flight::routeMustAddYourOwnHeaders() where you were responsible for ALL headers and such might be more obvious... ha ha ha!

@n0nag0n
Copy link
Collaborator

n0nag0n commented Apr 12, 2024

As I think about it, a special function Flight::routeStream() or Flight::routeMustAddYourOwnHeaders() where you were responsible for ALL headers and such might be more obvious... ha ha ha!

Yeah it was taking a stab at common use cases and seeing where it went. Seems like the use cases have pivoted a bit and it might make more sense to force you to declare all the headers in your methods first by hand. Something for the future though! Thanks for explaining it for me! Tell your friends about Flight!

@starfishpatkhoo
Copy link
Author

starfishpatkhoo commented Apr 12, 2024

As I think about it, a special function Flight::routeStream() or Flight::routeMustAddYourOwnHeaders() where you were responsible for ALL headers and such might be more obvious... ha ha ha!

Yeah it was taking a stab at common use cases and seeing where it went. Seems like the use cases have pivoted a bit and it might make more sense to force you to declare all the headers in your methods first by hand. Something for the future though! Thanks for explaining it for me! Tell your friends about Flight!

Yep! that's what I'm doing right now (declaring all headers in the method).. some things, content-length, file name etc etc, just needs to be done after the route is declared. I can set a route("/*.pdf") and it works for all PDFs.

Like I said, it is not a big deal because everything works right now, so just wanted to share is all.. ^_^

@n0nag0n
Copy link
Collaborator

n0nag0n commented Apr 12, 2024

You bring up a good point. I made another PR to help simplify this even further so you could just call Flight::route()->stream(); and it'll work. #575

@starfishpatkhoo
Copy link
Author

Awesome! Thanks!

@n0nag0n
Copy link
Collaborator

n0nag0n commented Apr 13, 2024

Clarified and added the documentation as well. https://docs.flightphp.com/learn/routing#streaming

@n0nag0n
Copy link
Collaborator

n0nag0n commented Apr 15, 2024

Merged into master. Should be released soon.

@n0nag0n n0nag0n closed this as completed Apr 15, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants