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 v2 #300

Closed
wants to merge 6 commits into from
Closed

Flysystem v2 #300

wants to merge 6 commits into from

Conversation

Lustmored
Copy link
Contributor

This is alternative approach to Flysystem v2 integration based on discussion in #291.

For now it's just a draft as none of the tests are refactored to handle new interfaces. Before touching them I'd love to hear opinions about code changes. I had no idea for better ResponseFactoryInterface, open to suggestions.

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.

And question about tests - how to proceed? Shall I detect available Flysystem version and check against available interfaces or write new tests for Flysystem v2 and skip those unavailable? Or maybe some other way? I think checking available interfaces is the way to go as we can then set up CI jobs to test against different Flysystem versions, but I might be wrong.

src/Server.php Outdated Show resolved Hide resolved
src/Server.php Outdated Show resolved Hide resolved
src/Server.php Outdated Show resolved Hide resolved
src/Server.php Outdated Show resolved Hide resolved
src/Server.php Outdated Show resolved Hide resolved
src/Server.php Outdated Show resolved Hide resolved
src/ServerFactory.php Outdated Show resolved Hide resolved
*/
public function setCache(FilesystemInterface $cache)
public function setCache($cache)
Copy link
Collaborator

@ADmad ADmad Jan 19, 2021

Choose a reason for hiding this comment

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

While the chances of someone extending the Server class are quite low, changing the signature of a public method is a BC break as per semver.

@ADmad ADmad mentioned this pull request Jan 19, 2021
@tgalopin tgalopin closed this Jan 24, 2021
@tgalopin
Copy link
Member

Closing as as discussed, we'll create a release focused on Flysystem 2 support (#301)

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.

4 participants