Skip to content

Conversation

maennchen
Copy link
Owner

@maennchen maennchen commented Nov 25, 2022

Motivation

Refactor Library to make full use of new PHP features and remove outdated code like DeflateStream.

TODO

Open Questions

Stream handling

Right now every file is converted into a StreamInterface. resources are converted into a StreamInterface via ResourceStream. To improve performance this could also be inverted so that StreamInterface is converted into a resource via stream_wrapper_register().

This would improve performance for native resources.

Inspiration: https://github.com/guzzle/psr7/blob/master/src/StreamWrapper.php

Steps TODO
  • Create & Register StreamWrapper for StreamInterface
  • Cast outputStream into custom stream wrapper
  • Cast stream of addFileFromPsr7Stream into custom stream wrapper
  • Read data of addFile into php://memory stream
  • Pass fopen of path in addFileFromPath into addFileFromStream
  • Pass resource directly into File in addFileFromStream
  • Change File to use resources exclusively

Related Issues

Closes #194
Closes #135
Closes #161
Closes #175
Closes #78

@maennchen maennchen self-assigned this Nov 25, 2022
@maennchen maennchen force-pushed the next branch 15 times, most recently from 36c864c to 395ece8 Compare November 25, 2022 02:06
@coveralls
Copy link

coveralls commented Nov 25, 2022

Pull Request Test Coverage Report for Build 3648952678

  • 310 of 310 (100.0%) changed or added relevant lines in 17 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+17.9%) to 100.0%

Totals Coverage Status
Change from base Build 3648269894: 17.9%
Covered Lines: 642
Relevant Lines: 642

💛 - Coveralls

@maennchen
Copy link
Owner Author

@donatj I would also really like your input if you have some time :)

I'm especially interested in your opinion on the stream handling question from the PR description.

@donatj
Copy link
Collaborator

donatj commented Dec 4, 2022

I opened a little PR against this, #232 - removes the need for deflate state from File - but I'm not entirely sure how to fix the phan complaint/php-cs-fixer complaint, I don't have PHP 8.2 to run against

Copy link
Collaborator

@donatj donatj left a comment

Choose a reason for hiding this comment

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

I think it's kinda silly to release a library targeting a version of PHP that isn't even out yet.

image

There's still over 30% of users using 7.4 - I can't personally imagine targeting a library 8.2 that isn't even out yet.

https://packagist.org/packages/maennchen/zipstream-php/php-stats

I'm personally stuck professionally on 7.4 for at least a few months and then I suspect we're probably just going to kick up to 8.0 so it'll be like 4-5 years before I could even use this.

@maennchen
Copy link
Owner Author

@donatj The idea was to rewrite it in a way that is more understandable for other people to contribute.

This is not a call to abandon v2 which is compatible with PHP 8.0.

I would fix bugs in v2 until 8.2 is the minimum supported version of PHP.

The maintenance effort of the library is quite minimal, so I don’t see an issue in temporarily adressing bugs in two versions.

PHP 8.1 support runs out in two years.

@maennchen
Copy link
Owner Author

@donatj Actually, the new features used were mostly PHP 8.0 / 8.1. With a tiny change 8.1 is supported as well.

PHP 8.0 doesn't work because there are no enums. Given that active support for PHP 8.0 has already ended, that's not that big an issue for me for an upcoming release as long as there's an alternative still available.

Therefore bugfixes need to be provided for v2 until Nov 23.

@maennchen
Copy link
Owner Author

I've added a commit to use a stream wrapper for the StreamInterface instead of converting resource to StreamInterface.

This seems to work fine and removes all required (non-dev) dependencies.

I've added the psr dependencies to suggested and added some docs on how to install it as well.

@maennchen maennchen marked this pull request as ready for review December 8, 2022 12:34
@maennchen maennchen merged commit 502efd3 into main Dec 14, 2022
@maennchen maennchen deleted the next branch December 14, 2022 13:05
@maennchen
Copy link
Owner Author

released as 3.0.0-beta.2 (after a tiny bugfix for the CI)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants