Flysystem v2

[ADmad]

This PR contains:

• Code formatting updated to PSR12 standard
• Added strict typing (and static analysis check using Psalm)
• Upgraded Glide to v2
• Added support for running testsuite on PHP 8 New Github Actions worfklow file (as TravisCI builds are extremely backlogged)

[Nitpick-CI]

Expected 0 spaces after opening bracket; newline found

[tgalopin]

Hello @ADmad! Thanks for your PR!

I have reserves concerning some aspects of strict typing and void return types. However I'd be happy to merge quickly support for flysystem 2. Would you be able to open a PR containing only flysystem 2 support so that we can discuss types independently?

[ADmad]

I had opened a PR for only strict typing changes a long time back, but haven't got any feedback/comment for it #284.

I made the flysystem v2 related changes on top of that as the explicit type errors made the task at hand a lot easier.

[ADmad]

@tgalopin I just noticed this comment #288 (comment). That's great news. It's good to finally have someone who can devote time towards maintaining this project 🙂.

[tgalopin]

Hi @ADmad !

I see that you're still working on this PR: thanks a lot! Given the recent release of Flysystem 2, that's indeed important :) .

However, I would really prefer to merge a PR focusing on Flysystem 2, as the other changes in this PR are unrelated. If that's something you would prefer not to do, I'd completely understand, feel free to tell me and I'll do it in a different PR as soon as I have time.

[ADmad]

@tgalopin Can you share what exactly are your reservations regarding strict typing? I stated earlier why I chose to do flysystem v2 related changes on top of the strict typing changes. I would prefer not undoing good work without understanding why they are undesirable.

[tgalopin]

My opinion on the subject is the following:

Typing in PHP is great way to describe a library API: it enforces the following of this API when implementing interfaces / inheriting classes, it helps IDEs understand how the code should be used and it helps developers get started quicker. I like typing a lot as an extension of phpdoc with several nice features added and a more native feel.

However, strict typing introduces more problems than it solves in my opinion. It creates the need for a check to ensure the strict typing declaration is there (thus creating an obstacle to contribution), it enforces a strict usage of casting that PHP users are not used to and it creates maintenance burdens in some specific cases where it's more difficult to fix with strict typing while keeping BC.

These problems would be fine if the gains were significant. I don't see these gains in any application or library I work on. A testing suite covering well the library is enough IMO. To my knowledge, most of the biggest open-source PHP projects in the world do not use strict typing, but they do have a reliable test suite. That's where we should invest time, not strict typing IMO.

And finally, I also have concerns about specific things like:

• Adding : void is unnecessary and forces a BC break if the interface changes. Void is not a normal type, it indicates no type. Adding it as a return typehint only forces child interfaces/classes to not return something, which is not something Liskov substitution principle care about. We don't need to add any : void IMO, people should be able to add a return value in a child if necessary (the @return void is OK though as it tells IDEs that it's the case, which is nice).

• I'm not a fan of psalm and tools which are very strict like this for open-source. Two reasons: 1/ it discourages contribution, because it shows a red cross (or several if there are many checks like this) for something that is not as important as tests and 2/ it incentivizes contributors to follow the tool blindly in order to have a green check, even if the tool may be wrong. It's important people stay aware of their code, and I tend to think that these tools incentivize people to think less and follow the tool more.

So to summarize, I think the best plan of action for your work would be:

1/ to create a PR dedicated to adding typehints, without the strict type declaration: I'm definitely in favor of adding them ;
2/ to base this Flysystem 2 PR on that new PR, and implement Flysystem 2 migration (which you already did if I'm not mistaken?)

Would that sound good to you :) ?

[ADmad]

I don't agree with lot of points you have made (expect for perhaps the one regarding void return type) but I am not interested in having a debate over them 🙂.

The most surprising thing to me is that you don't see value in using static analytics tools like psalm. While they can generate false positives at times, they all allow configuring the strictness level, so one can adjust them as per their needs/sensibilities.

Anyway, I have removed the strict type declarations and the void return types. I have kept the use of psalm. It's configured to a reasonably low level 4, which should avoid any deterrent to contribution or having people blindly chase the green check mark. If even that is unacceptable to you, you can easily remove it.

You requested making a separate PR with typing changes first but I am being lazy here and haven't done so. The changes related to typing and those related to flysystem v2 upgrades have been done in separate commits so it should still be easy enough to review them separately.

P.S. You might probably already know that TravisCI has effectively ended support for free usage even for OSS. The builds are severely backed logged there and the free credits/minutes they have provided will get exhausted soon.

Hence I have added a workflow file to run the testsuite using Github Actions. You need to check why Github Actions aren't running for this repo. They run fine on my fork https://github.com/ADmad/glide/actions?query=workflow%3Aci (The build failure is just because I don't have Scrutninizer setup, the test suite runs successfully).

[piotr-cz]

@ADmad
It's hard to review changes as PR contains all sorts of other things besides Flysystem 2 (editorconfig, GitHub workflows, strict types).
That said I'm not opposed to these

[ADmad]

@piotr-cz Did you read my previous comments and/or see the individual commits?

The changes related to typing and those related to flysystem v2 upgrades have been done in separate commits so it should still be easy enough to review them separately.

This commit only has changes related to type hints 96513dd
This commit only has changes related to flysystem v2 c15f0f5

These are my previous PRs related to specific tasks #279, #284 and this comment #291 (comment).

[ADmad]

Given the fact the the original author could no longer devote time to the project and previous contributions never got any feedback, the existing maintainer(s)/reviewer(s) should give more leeway to someone who has actually put time and effort into this.

Anyone who wants separate PRs can do so by splitting my commits. Unless there are actual code adjustments required I can no longer spend more time on this.

[piotr-cz]

@ADmad OK, thanks

[tgalopin]

@ADmad don't worry, having multiple commits is definitely fine for me.

I'll review this asap, thanks a lot for your work and understanding!

[Cruiser13]

Would be great to see flysystem 2 compatibility for this. Thanks for all the work!

[Lustmored]

Is there anything we can do to help getting this merged and released?

[tgalopin]

@Lustmored If you can review the code and test it locally, that would be super useful!

I'll have a look myself but more eyes are always better

[Lustmored]

@tgalopin I am already using it in production without any issues, so can confirm it works. Will review PR today 👍

[tgalopin]

Haha fair enough

[tgalopin]

A question I'm looking to answer (not sure yet) is should we release a new major of Glide for Flysystem 2 or should we try to accept both as inputs?

Depending on the answer to this question, this PR may need to be splitted in different PRs.

My (not strong) opinion is to try to see if we can support both Flysystem 1 & 2 in the current Glide version. It would ease adoption and would allow people to migrate smoothly. Then after than, we could release Glide 2 with types, and perhaps additional things (I have some in mind as well).

WDYT?

[Lustmored]

I think supporting both versions should be simple enough with a single wrapper class or some kind of backwards layer in case Flysystem 1 objects are used in Server constructor. I am unsure if it's worth the effort though.

But that would also mean dropping most of this PR (most of the changes are adding types which I understand you'd prefer to postpone to Glide 2?). I can provide simple implementation idea in a separate PR this week if you'd like to explore options.

[tgalopin]

I agree, it does feel like supporting both Flysystem 1 and 2 is feasible. That's what I wanted to investigate. If you want to have a look, go for it, that would be super useful!

most of the changes are adding types which I understand you'd prefer to postpone to Glide 2

It's not even a question of preference: if we choose not to upgrade to Glide 2, we can't add typehints as they break BC. Integrating types at the same time as Flysystem 2 support will depend on the difficulty of implementing Flysystem 1 + 2 support in Glide 1.

[Lustmored]

I agree, it does feel like supporting both Flysystem 1 and 2 is feasible. That's what I wanted to investigate. If you want to have a look, go for it, that would be super useful!

All right. I need a few days and will get back with PR 👍

[ADmad]

There are interfaces/classes which already use interfaces/classes of flysystem v1 for typing in method signatures. For e.g. ResponseFactoryInterface::create().

glide/src/Responses/ResponseFactoryInterface.php

Line 15 in ae5e267

public function create(FilesystemInterface $cache, $path);

So in order to support both flysystem v1 and v2 you would have to remove the type for the $cache argument which would be a backwards compatibility break and hence warrant a major version bump for Glide anyway.

[Lustmored]

I have just skimmed over code base and @ADmad is right about signature changes.

I see only one way to preserve signatures and be v1 and v2 compliant and this is instead of creating backwards compatibility wrapper, create forwards compatibility one - class decorating v2 FilesystemOperator with signature compatible with v1 FilesystemInterface. Let's call it FilesystemOperatorDriver for now (I know, name is bad).

That would enforce v2 users to wrap initialization code with this class, for example:

$server = ServerFactory::create(
    [
        'cache' => new FilesystemOperatorDriver($cacheStorage),
        'source' => new FilesystemOperatorDriver($publicStorage),
    ]
);

We could also write FilesystemDriver for v1 that'd take FilesystemOperator as only argument and work around that (implement all AdapterInterface method). That'd be more logical way of doing it, but probably require a bit more work and probably end user code would get uglier.

Anyways - it shouldn't be difficult to implement either way (after all v1 and v2 are doing the same things with different philosophies under the hood and different interfaces). But I'd love to hear opinions or other ideas :)

[tgalopin]

TBH honest I don't think ResponseFactories are much of a problem for many users. What matters to most users is that ServerFactory can accept both Flysystem 1 or 2 natively. We don't need anything there I think, as we can detect which object is provided. We can even detect the Flysystem version to instanciate when a string is passed.

Regarding the interfaces themselves, I would be completely fine to introduce new interfaces to tackle this problem. For instance:

interface Flysystem2ResponseFactoryInterface
{
    public function createFlysystem2(FilesystemOperator $cache, $path);
}

(naming can change)

And have response factories could implement both interfaces:

class PsrResponseFactory implements ResponseFactoryInterface, Flysystem2ResponseFactoryInterface

This is Backward Compatible and allow users of response factories to have essentially nothing to do to migrate to Flysystem 2. It allows for a much smoother transition between Flysystem 1 and 2 for Glide users, as they don't have to upgrade both Flysystem and Glide at the same time (prone to errors).

[Lustmored]

I like the way you think about it. As promised I will pursue that goal and come back with a PR 👍

[ADmad]

It allows for a much smoother transition between Flysystem 1 and 2 for Glide users, as they don't have to upgrade both Flysystem and Glide at the same time (prone to errors).

@tgalopin I fail to see the potential problem(s). Anyone using Flysystem 1 would continue using Glide 1 and anyone who wants to use Flysystem 2 would upgrade to Glide 2. As @Lustmored stated he's already using my branch without issues.

Trying to support both Flysystem 1 and 2 together will in fact cause problems like the one mentioned by @Lustmored in #300.

One important thing I might have missed some place is exception handling - Flysystem v2 changed a lot and now throws exceptions instead of returning false. I've tried to adapt to it, but probably missed a place or two.

Bumping the major version of a lib doesn't necessarily mean it has to include new features too. Version numbers are free, you can bump the version to 3.0 whenever you have new features ready, which you had planned for v2.

[tgalopin]

Anyone using Flysystem 1 would continue using Glide 1 and anyone who wants to use Flysystem 2 would upgrade to Glide 2

That's kind of my point :) . Introducing multiple BC breaks at the same time (for instance types + Flysystem 2) would force users to migrate both Glide and Flysystem at the same time, which is always more difficult than upgrading package by package. I'm not saying we shouldn't do it, I'm just saying I much prefer to spend a bit more time now trying to think about the upgrade experience than delegate the potential struggle to users.

I'll have to look for myself how difficult it is to actually make Glide 1 compatible with Flysystem 2. Perhaps we should create a very simple 2.0 with only Flysystem 2.0 support (to ease upgrade) and introduce other BC breaks in a 3.0.

[ADmad]

Given your comments and how long this PR has been open for, it seems to me that you have already made up your mind about not merging this PR in its current form.

So I'll just close it and stop bugging you. Thank you for your time.

@Lustmored I won't be deleting my PR branch on my fork and will keep it alive for a sufficient amount of time even after there's an official Flysystem v2 compatible release, so that you can transition without major problems.

[Lustmored]

@ADmad Thanks 👍

[tgalopin]

@ADmad I understand. I hope it doesn't frustrate you too much. This PR contained indeed too many subjective decisions which have an impact on the project for me to merge it quickly. Generally speaking, I think it's much easier if we focus PRs on specific topics. I'd be happy to discuss them beforehand though, to avoid spending time on it.

Thanks a lot for your help anyway, and please don't hesitate to propose other ideas if you have some for Glide 2/3 !