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

big file support #51

Merged
merged 8 commits into from
Nov 24, 2024
Merged

big file support #51

merged 8 commits into from
Nov 24, 2024

Conversation

mcdruid
Copy link
Contributor

@mcdruid mcdruid commented Sep 19, 2024

Working on a fix for #50

@mcdruid mcdruid requested a review from ashnazg September 19, 2024 17:22
@mcdruid
Copy link
Contributor Author

mcdruid commented Sep 20, 2024

FWIW I tried setting the unpack _fmt to use a for everything but it made a handful of tests fail; mostly because of this:

+'Fatal error: Uncaught ValueError: mkdir(): Argument #1 ($directory) must not contain any null bytes in /path/to/Archive_Tar/Archive/Tar.php:2452\n

So I think we should stick to only changing the format for the one field for now.

@ashnazg ashnazg self-assigned this Nov 24, 2024
Copy link
Member

@ashnazg ashnazg left a comment

Choose a reason for hiding this comment

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

What do you think about adding a version check that still uses Z for 5.4 ? Maybe that would resolve the build test failure, and allow us to not yet be forced to drop older support.

@mcdruid
Copy link
Contributor Author

mcdruid commented Nov 24, 2024

Can give it a try, but it looks like 5.4 might still be failing with the same problem we're trying to fix.. so I don't know that special-casing it will help.

Personally I'd be happy saying that PHP 5.6 is the oldest we'll try to support.

@mcdruid
Copy link
Contributor Author

mcdruid commented Nov 24, 2024

Hi, sorry I think I misunderstood the suggestion / had forgotten that there was already a version check in the code.

Let's try just using the same format for all versions and see if it works on PHP 5.4.


Okay, looks like that didn't work.

Did you want to try a Z for just the problem field for PHP 5.4?

Looks like the old change records suggest that Z is only a thing after PHP 5.5

https://web.archive.org/web/20201127071942/https://www.php.net/manual/en/migration55.incompatible.php#migration55.incompatible.pack

#50 (comment)

@mcdruid
Copy link
Contributor Author

mcdruid commented Nov 24, 2024

@ashnazg I think we've tried the a and Z for the problem field with PHP 5.4 and doesn't look like either work to extract the large file.

We could perhaps put some logic in the test to skip PHP 5.4.

I'm more inclined to drop explicit support for PHP 5.4 though if we can't make this work.

I don't think many linux distros - if any - still support it.

@ashnazg
Copy link
Member

ashnazg commented Nov 24, 2024

My fault, I was assuming the 5.4 test failure was related to the a/Z change, but looking more closely now, I do not think that's the case. Let's merge this, and I'll look at that separately.

@ashnazg ashnazg merged commit 3677851 into pear:master Nov 24, 2024
9 of 10 checks passed
@ashnazg
Copy link
Member

ashnazg commented Nov 24, 2024

Trying to address the 5.4 Uninitialized string offset the way I typically would just resulted in worse errors on 5.4, so now I think I also vote to drop 5.4 and just move on.

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.

2 participants