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

Php 32bit - Zip-stream with version 2.1 instead of 3.1 #2934

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

ildyria
Copy link
Member

@ildyria ildyria commented Jan 19, 2025

Fixes #2933

We leverage 2 traits one for each implementation of ZipStream.
The code is moved to a BaseArchive class and the installation decides which class to use.

@ildyria ildyria requested a review from a team as a code owner January 19, 2025 20:17
@ildyria ildyria marked this pull request as draft January 19, 2025 20:19
@ildyria
Copy link
Member Author

ildyria commented Jan 19, 2025

@RustyPotato could you try this branch ?

Copy link

codecov bot commented Jan 19, 2025

Codecov Report

Attention: Patch coverage is 43.85965% with 32 lines in your changes missing coverage. Please review.

Project coverage is 90.57%. Comparing base (2834e86) to head (f722a2d).

Additional details and impacted files

@ildyria
Copy link
Member Author

ildyria commented Jan 19, 2025

@RustyPotato you may need to remove the composer.lock before doing your install, but in theory it should now support your 32 bit install.

@tinohager
Copy link
Contributor

Couldn't the zip creation also be outsourced to the client?
https://github.com/gildas-lormeau/zip.js

the latest 2 version would be 2.4.0
https://github.com/maennchen/ZipStream-PHP/releases/tag/2.4.0

@ildyria
Copy link
Member Author

ildyria commented Jan 20, 2025

Couldn't the zip creation also be outsourced to the client? gildas-lormeau/zip.js

The problem is that the response is in streaming mode, In version 3 of Lychee, we were creating the zip file serverside before sending it. This has changed by the use of ZipStream, but I am not knowledgeable enough of how streaming files works with http, not how it would be possible for the client to know, that is one file, that is the next file.
The advantage of streaming the zip file directly is that the client no longer need to keep the page open and can just have it run in a background tab.

@ildyria
Copy link
Member Author

ildyria commented Jan 24, 2025

@RustyPotato please confirm otherwise I won't be able to merge this. :)

@RustyPotato
Copy link
Contributor

My apologies for the long delay, this took longer than expected to get back to. I have gotten it to work, and it's running successfully. (Link). Now just for me to set all the configs properly.

As we were messing with the zip library, had to double check what that'd look like. It gives off an empty zip file. So if I want that fixed, I'll probably need to do so myself eventually, but it's not an essential feature for me.

Having said that, it seems the demo site is also having difficulties with it too. (Link)

@ildyria
Copy link
Member Author

ildyria commented Feb 1, 2025

As we were messing with the zip library, had to double check what that'd look like. It gives off an empty zip file. So if I want that fixed, I'll probably need to do so myself eventually, but it's not an essential feature for me.

I checked on 64bit install it is working as expected. I will try to see with the 32bit version...

I can confirm the empty zip behaviour on the 32 bit.

@ildyria
Copy link
Member Author

ildyria commented Feb 1, 2025

@RustyPotato this should fix the empty zip issue you were observing.

@ildyria ildyria added the Review: easy Easy review expected: probably just need a quick to go through. label Feb 1, 2025
@ildyria ildyria marked this pull request as ready for review February 1, 2025 14:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Review: easy Easy review expected: probably just need a quick to go through.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

32 bit no longer supported
3 participants