Emit warning when ftp_connect() fails to connect

[mubby3000]

Description

ftp_connect() should return fatal error or warning in case Ftp is turned off on Server. it took me days to figure out that FTP was turned off on the server since the function doesn't return any error/warning/notice.

[yani]

https://www.php.net/manual/en/function.ftp-connect.php#refsect1-function.ftp-connect-returnvalues

Returns an FTP\Connection instance on success, or false on failure.

[cmb69]

Right, you should check the return value of ftp_connect(). However, a warning would make sense in my opinion (but certainly not a fatal error). Something like the following might do:

 ext/ftp/ftp.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/ext/ftp/ftp.c b/ext/ftp/ftp.c
index 1af4f13c65..28a39e4dbb 100644
--- a/ext/ftp/ftp.c
+++ b/ext/ftp/ftp.c
@@ -119,6 +119,7 @@ ftp_open(const char *host, short port, zend_long timeout_sec)
 	ftpbuf_t		*ftp;
 	socklen_t		 size;
 	struct timeval tv;
+	zend_string *error_string = zend_empty_string;
 
 
 	/* alloc the ftp structure */
@@ -129,8 +130,10 @@ ftp_open(const char *host, short port, zend_long timeout_sec)
 
 	ftp->fd = php_network_connect_socket_to_host(host,
 			(unsigned short) (port ? port : 21), SOCK_STREAM,
-			0, &tv, NULL, NULL, NULL, 0, STREAM_SOCKOP_NONE);
+			0, &tv, &error_string, NULL, NULL, 0, STREAM_SOCKOP_NONE);
 	if (ftp->fd == -1) {
+		php_error_docref(NULL, E_WARNING, ZSTR_VAL(error_string));
+		zend_string_release(error_string);
 		goto bail;
 	}

[KapitanOczywisty]

Maybe option to throw on ftp error? Warning message is not easy to parse cleanly, and will only make people use @ftp_connect, but Exception can be handled in the proper way.

[cmb69]

While a warning message would be a minor BC break, an exception would be a major break. I don't think we could do that prior to PHP 9.0, and even then it would be hard to justify if there was no warning before.

And more generally, I don't think it is a good idea to raise exceptions for non-exceptional conditions (after all, not being able to connect to a network service is not exceptional per se).

[yani]

And more generally, I don't think it is a good idea to raise exceptions for non-exceptional conditions (after all, not being able to connect to a network service is not exceptional per se).

This should be true for so many libraries. It seems like the actual meaning of the word "exception" is being forgotten.

[KapitanOczywisty]

While a warning message would be a minor BC break, an exception would be a major break.

I'm talking about an option/parameter to throw on error, like json or mysqli.

And more generally, I don't think it is a good idea to raise exceptions for non-exceptional conditions (after all, not being able to connect to a network service is not exceptional per se).

SQLite3 throws an exception for failed connection, and PDO, and mysqli (with flag), and probably more. I don't care if this is fulfilling definition, it's just better way to handle network problems.

If you need to keep track of multiple ftp connection, warnings in logs are not sustainable. I'm using mainly curl for ftp, partially because I can call curl_error and write history of error messages for that server to db.

[cmb69]

I'm talking about an option/parameter to throw on error, like json or mysqli.

Okay.

I'm using mainly curl for ftp, partially because I can call curl_error and write history of error messages for that server to db.

You could use error_get_last() instead of curl_error().

[KapitanOczywisty]

You could use error_get_last() instead of curl_error().

Will not work with custom set_error_handler (which returns true), although I'd appreciate ftp_last_error.