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

Deprecate the procedural API of ext/zip #5746

Closed
wants to merge 1 commit into from

Conversation

kocsismate
Copy link
Member

@kocsismate kocsismate commented Jun 20, 2020

As discussed in #5601 (comment) this seems to be the most sensible approach to get rid of resources.

@Girgias
Copy link
Member

Girgias commented Jun 20, 2020

I'm not sure if this wouldn't need an RFC? Except if maintainers of the extension can call this sort of thing without going through that process?

@kocsismate
Copy link
Member Author

I'm not exactly sure, too, so it perfectly fine for me if this needs an RFC. But I can also imagine that Remi has the final word on this. :)

Copy link
Member

@remicollet remicollet left a comment

Choose a reason for hiding this comment

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

LGTM

@kocsismate
Copy link
Member Author

@nikic We don't need an RFC in this case, right?

@nikic
Copy link
Member

nikic commented Jun 23, 2020

@kocsismate Don't think we need an RFC in this case.

Please add some UPGRADING notes when merging though :)

@php-pulls php-pulls closed this in f3b1f34 Jun 23, 2020
@kocsismate kocsismate deleted the zip-resource2 branch June 23, 2020 15:40
@carusogabriel carusogabriel added this to the PHP 8.0 milestone Jun 23, 2020
@cmb69
Copy link
Member

cmb69 commented Jul 19, 2020

Please see https://bugs.php.net/bug.php?id=79874. In my opinion, we had to make ZipArchive traversable, before we deprecated the procedural API. The implementation is not hard, but probably we'll want to bikeshed the details (name of the ZipEntry class, names of its methods, maybe even use (typed) properties instead of methods, making the properties readonly etc.)

Would it be better to actually postpone the deprecation to 8.1, so we have sufficient time to figure these details out?

@thelounge-zz
Copy link

Would it be better to actually postpone the deprecation to 8.1

in the real world you need that missing stuff in the eversion before the deprecations, i really started to migrate my code ZipArchive which looked good for open and replace the is_resource by a boolean check - anything else was "and how do that funny guys imagine that?"

and no it is not a solution unpack the whole archive and make the foldername/filename sanitize and filters for allowed extensions after that becaue that would mean a from scratch implementation of sometzhing wroking for many years because someone was funny deprecating something for no benefit

@kocsismate
Copy link
Member Author

@thelounge-zz It's not about being funny... -.- You can disagree with a decision, but please use a respectful tone. We can still revert the change easily.

@nikic
Copy link
Member

nikic commented Jul 20, 2020

@thelounge-zz (aka rhsoft) is blocked from participation on all php.net properties. I apologize that this was not enforced on GitHub previously.

@remicollet
Copy link
Member

The procedural API is deprecated as it is mostly unmaintained and haven't receive any update for lot ofr new feature (new compression, encryption...)

IMHO ZipArchive is the way to go.
Iterating is not so hard...
for ($idx=0 ; $s = $zip->statIndex($idx) ; $idx++) {

@nikic
Copy link
Member

nikic commented Jul 20, 2020

@remicollet I think there may still be some holes in the ZipArchive API, for example, I don't immediately see how one would replace zip_entry_filesize.

@remicollet
Copy link
Member

zip_entry_filesize => use statIndex

@nikic
Copy link
Member

nikic commented Jul 20, 2020

@remicollet I see, thanks.

Maybe we can update the zip_entry docs to point to the corresponding ZipArchive methods, as it is not always completely obvious.

@remicollet
Copy link
Member

@nikic see php/doc-en@69795fb

@nikic
Copy link
Member

nikic commented Jul 20, 2020

@remicollet Thank you!

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.

None yet

7 participants