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

Flysystem v3 port #55

Merged
merged 2 commits into from
Sep 15, 2024
Merged

Flysystem v3 port #55

merged 2 commits into from
Sep 15, 2024

Conversation

rulatir
Copy link
Contributor

@rulatir rulatir commented Dec 27, 2022

This is a preliminary PR porting FlyFinder to Flysystem 3 and minimum PHP 8, motivated by our use of (and contributions to) FlyFinder, combined with our desire to free ourselves from the antedeluvial Flysystem 1 dependence.

Summary:

  • Flysystem 3 got rid of plugins, so control is now inverted: Finder constructor takes a FilesystemOperator object, handle() was renamed to find(), and you invoke it on the Finder object.
  • Methods that used to take an array with file/directory attributes will take array|StorageAttributes now. This is preliminary, and the array alternative is probably only used in tests that will need further tweaking to properly mock FileAttributes and DirectoryAttributes objects.
  • Implementation had to be tweaked to parse the path in various ways as necessary (with pathinfo(), regex, str_*()), because StorageAttributes only offer unparsed path - this is an optimization decision by Flysystem authors, to avoid parsing every path where it isn't known to be needed.

Things I got green:

  • tests (modulo StorageAttributes mocking)
  • phpcs
  • psalm (mostly)

Things I'd appreciate help with:

  • phpstan - the command in the Makefile uses an image that can't be easily bumped to PHP 8 by changing a tag.
  • Deciding what to do with @psalm-pure. Previously native array accesses to $value['path'] are now ProxyArrayAccessToProperties::offsetGet(), and Flysystem authors either neglected or couldn't mark it as pure. We can give up being pure ourselves, suppress the diagnostic (which is what I did for now), or plead with Flysystem authors.
  • No idea what phive is, make install-phive fails on gpg --recv-keys
  • How to properly tag and alias v1 and v3 in composer.json
  • Anything I missed

Szczepan Hołyszewski and others added 2 commits December 27, 2022 14:20
@jaapio
Copy link
Member

jaapio commented Dec 28, 2022

First, thank you so much for taking the time to rebuild this library! It was on my wish list for so long to do this!

CI & Tools
We might need to do some cleaning work to get this done. It has been a while since we created this repo. The other repos in our organization have moved forward regarding ci. Please check out this commit: phpDocumentor/ReflectionDocBlock@863ec23

We moved away from using phive, as it was not stable enough on windows, and most of the dependencies installed with phive are now shipped as phar binaries using composer. So it's safe to migrate them to composer. And clean up the makefile. This will also fix your issues with phpstan.

I would prefer a separate PR for this, as it is separate from the refactoring you are doing right now.

Phive

Most tool builders, like we (phpdoc), have issues supporting all framework versions. The main problem is, building an application that supports, for example, symfony 3, 4, 5 and 6 is nearly impossible. So we ship our applications as binaries. Phive is a tool that helps with this. It will download a file from a known repo, just like composer does. And checks the GPG key, as it is a binary that can be executed. But there is no way for you as a user to validate the contents. So all those phars are signed with a private key. To ensure the content is not modified. And the user is safe to execute them.

As I said, we dropped phive usage, which wasn't stable enough. So that would solve this part as well.

@psalm-pure

It's ok for me to keep it suppressed for now. I don't want to wait for other maintainers to move to this. Not everyone is using psalm, so they might even ignore this suggestion.

V1 & V3 support

Flysystem v1 and v3 are not compatible. So I think it will not be possible to support both versions without much hacking. So the easy approach would be to drop support for v1 and support v2 and v3 as they are compatible at the levels we need. if we would support v2 and v3, we could release a new major version of this library. So people are aware of the breaking changes. I can help you to write an upgrade document. v2 will help us to cover the older PHP versions 7.4 is still used a lot, so I don't want to drop support for it yet. We could do that in a future release.

I hope this answers your questions? Thanks again, if you have any questions feel free to drop them here.

I will review your pr later in depth.

@jaapio
Copy link
Member

jaapio commented Sep 15, 2024

We are years later since this pr was submitted. I think when looking at the code it it quite complete. Thanks a lot for your work. I will merge this pr and take over what is left to do the migration to flysystem 3.

@jaapio jaapio merged commit 2d18000 into phpDocumentor:master Sep 15, 2024
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