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

[3.x] Breaking change on FtpAdapter #1414

Closed
nunomaduro opened this issue Jan 31, 2022 · 4 comments
Closed

[3.x] Breaking change on FtpAdapter #1414

nunomaduro opened this issue Jan 31, 2022 · 4 comments

Comments

@nunomaduro
Copy link

nunomaduro commented Jan 31, 2022

Bug Report

Q A
BC Break yes
Version 3.0.2
Package https://github.com/thephpleague/flysystem-ftp

Summary

The commit thephpleague/flysystem-ftp@813bc06 introduces a breaking change where the FtpAdapter::__construct on the line $this->rootDirectory = $this->resolveConnectionRoot($this->connection()); is trying to resolve the given connection.

Usually, I would expect the adapter only to try to resolve the connection on methods such as read, readStream, etc, and not while trying to instantiate the FtpAdapter class.

How to reproduce

Try to create new instance of FtpAdapter with an invalid connection.

@frankdejonge
Copy link
Member

@nunomaduro what is the BC break in your opinion?

@nunomaduro
Copy link
Author

In fact there are 2 breaking changes:

  • The first breaking change is related to the FtpAdapter::__construct that is now throwing a League\Flysystem\Ftp\UnableToConnectToFtpHost exception when a given connection is invalid. That is behavior was not there in the past, therefore is a breaking change. As example, it breaks the existing Laravel Test suite, as we give a dummy connection that __construct argument.

  • The second breaking change, is related to the fact that the connection is now eagerly resolved instead of lazy resolved. Meaning that places in my code, such as Laravel Service Providers that create FtpAdapter instances, are now actually resolving connections, even if I don't even use those connections later at HTTP level for example.

@frankdejonge
Copy link
Member

PR welcome 👍

@frankdejonge
Copy link
Member

Fixed in 3.0.3

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

No branches or pull requests

2 participants