ext/ftp/tests: add with_data_connection to mock server

[Sjord]

FTP uses separate control and data connections. Extract setting up the data connection to a separate function. This was duplicated many times throughout the test server.

Also use fgets instead of fread, because that matches FTP's line-based protocol better.

[Sjord]

@NickSdot could you take a look at this?

[NickSdot]

@NickSdot could you take a look at this?

Looked at it and generally like it. But I am not actually confident enough to say whether it's fine or not. Some thoughts:

• early returns do not have pasv/pasv_listener cleanup; has to be fixed
• response codes change, what are the implications for existing core and extension tests? CI passes, but is this a sign that it is actually fine or are we just having bad tests because these response code changes where not caught at all?
• the catch could hide actual errors; should it fail loudly instead of silently fclose/fputs?
• should warnings for from stream_socket_client() actually be supressed?

I brainstormed those with Codex and we landed on the below; that fixes some of it and Codex says it's fine. But this obviously still changes some old response codes, hence I myself am not sure it's fine or not -- maybe someone else can answer this more competently.

Brainstorming Result

function with_data_connection(callable $callback): bool
{
    global $s, $pasv, $pasv_listener, $host, $port, $ssl, $bug73457;

    $passive = !empty($pasv);
    $fs = null;

    if (!$passive) {
        $fs = stream_socket_client("tcp://$host:$port");
    } elseif (empty($bug73457)) {
        $fs = stream_socket_accept($pasv_listener, 10);
        fclose($pasv_listener);
    }

    $pasv = false;
    $pasv_listener = null;

    fputs(
        $s,
        $passive
            ? "125 Data connection already open; transfer starting.\r\n"
            : "150 File status okay; about to open data connection\r\n"
    );

    if (!$fs) {
        fputs($s, "425 Can't open data connection\r\n");
        return false;
    }

    if (!empty($ssl) && !stream_socket_enable_crypto(
        $fs,
        true,
        STREAM_CRYPTO_METHOD_TLS_SERVER
    )) {
        fclose($fs);
        fputs($s, "425 TLS negotiation failed\r\n");
        return false;
    }

    try {
        $callback($fs);
    } finally {
        fclose($fs); // maybe plus defensive check in case callback closed it?
    }

    fputs($s, "226 Closing data Connection.\r\n");

    return true;
}

[Sjord]

Thank you.

• early returns do not have pasv/pasv_listener cleanup; has to be fixed

Yes, I changed this.

• response codes change, what are the implications for existing core and extension tests? CI passes, but is this a sign that it is actually fine or are we just having bad tests because these response code changes where not caught at all?

This is intentional. The stream_socket_accept was first done on PASV, but is now done in with_data_connection. So now we never have a "Data connection already open" and always use "about to open data connection". I think this is more correct, or more according to a real FTP server, and the reason that it makes no difference is because the FTP details are abstracted away.

• the catch could hide actual errors; should it fail loudly instead of silently fclose/fputs?

This was intentional, but I changed it. It was meant to keep the same functionality in STOR. But that functionality only triggers when the test fails, so throwing the exception is better.

• should warnings for from stream_socket_client() actually be supressed?

This was intentional, so that when a test wants to test a failing stream_socket_client it is not interrupted by warnings. However, since such tests do not (yet) exist it may indeed be better to remove the error supression.