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

Improved #36

Merged
merged 14 commits into from
Dec 15, 2023
Merged

Improved #36

merged 14 commits into from
Dec 15, 2023

Conversation

abilogos
Copy link
Contributor

Hello @dilab .

I have made a couple of improvement in order to revive the library with new dependency versions.
fixed test cases depredations.
& a few bugfixes

@dilab dilab merged commit 27fa0c5 into dilab:master Dec 15, 2023
williamdes added a commit to code-lts/resumable.php that referenced this pull request Dec 16, 2023
@williamdes
Copy link
Contributor

williamdes commented Dec 16, 2023

That's funny, I did quite some similar changes in https://github.com/code-lts/resumable.php

And infact you closed a bug: #30 by 5172968
I fixed the tests this PR broke in #37

@abilogos
Copy link
Contributor Author

@williamdes yes that was a wired one since it was considering the chunk before the last one as the final chunk.

but after I think I had fixed that, I have found out it raised another issue.
then after investigating the resumablejs front-end library I have found out that some configuration there to make the the last chunk bigger than the other unless you make forceChunkSize option as true.

for example : if your uploading file is 3.5 MB and your chunk size is 1 MB => it chunks your file into 1MB 1MB 1.5MB
and if you set the forceChunkSize option it would chunk the file as 1MB 1MB 1MB 0.5MB

and in the resumable.php the assumption was forceChunkSize is always true (which was defaulted to false)
and it was considering the-one-before-last as the final chunk.

but hopefully the resuamblejs sends a header resumableTotalChunks so with use of that, we dont need to calculate the last chunk in the backend, and the bug which made the trouble would be fixed.
so I have fixed the new issue with this 72f438c and the problem get solved

@williamdes
Copy link
Contributor

Exactly, I came to the same conclusion and implemented total chunks 😄

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.

3 participants