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

Response: support range requests for files #5153

Closed
wants to merge 1 commit into from

Conversation

distantnative
Copy link
Member

It's a start... but I think I am at the point where I need input from others

@distantnative distantnative added the type: enhancement ✨ Suggests an enhancement; improves Kirby label Apr 19, 2023
@distantnative distantnative self-assigned this Apr 19, 2023
@distantnative distantnative added needs: help 🙏 Really needs some help on this needs: tests 🧪 Requires missing tests labels Apr 19, 2023
Copy link
Member

@lukasbestle lukasbestle left a comment

Choose a reason for hiding this comment

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

I haven't yet tested it or checked for edge cases, but in general (code structure) it looks really good already.

$size = static::size($file);
$end ??= $size;

if ($end === $size) {
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't this also check for start being 0? Otherwise the full file is returned even when the beginning wasn't requested.

): void {
$size = static::size($file);
$end ??= $size;

Copy link
Member

Choose a reason for hiding this comment

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

I think there should be checks for start < 0, start >= end and end > size here as well, throwing an exception to provide extra safety and for those who use this method directly.

@bastianallgeier
Copy link
Member

What's the state of this? Should we keep this in the lab and remove the PR?

@distantnative
Copy link
Member Author

It's out here because I need help from the rest of the team. I won't get much further on my own. If this happens now or when, to be decided.

@distantnative
Copy link
Member Author

Closing this for now. PR is referenced in corresponding issue and can be revisited when tackling this again.

@distantnative distantnative deleted the feature/http-range-response branch June 27, 2023 20:26
@distantnative distantnative restored the feature/http-range-response branch April 28, 2024 15:38
@distantnative distantnative deleted the feature/http-range-response branch April 28, 2024 15:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs: help 🙏 Really needs some help on this needs: tests 🧪 Requires missing tests type: enhancement ✨ Suggests an enhancement; improves Kirby
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants