telegram-bot-api icon indicating copy to clipboard operation
telegram-bot-api copied to clipboard

The `TransportInterface` may have incorrect capabilities.

Open kafkiansky opened this issue 9 months ago • 3 comments

Hi.

Currently, TransportInterface includes two additional methods: downloadFile and downloadFileTo. In my opinion, the transport would only need the downloadFile method. It's not correct to ask the transport to save to a file: first, saving isn't limited to files — it could also be to S3, where we could implement zero-copy if TransportInterface::downloadFile returned a StreamInterface that could be directly sent to S3; second, many transports, including non-blocking ones like amphp/http-client, simply don't support saving to a file, so they would have to duplicate file-saving logic among themselves — logic that could otherwise be abstracted away.

I propose the following interface:

/**
 * @api
 */
interface TransportInterface
{
    /**
     * @psalm-param array<string, mixed> $data
     */
    public function send(
        string $urlPath,
        array $data = [],
        HttpMethod $httpMethod = HttpMethod::POST,
    ): ApiResponse;

    /**
     * Downloads a file by URL.
     *
     * @param string $url The URL of the file to download.
     *
     * @throws DownloadFileException If an error occurred while downloading the file.
     */
    public function downloadFile(string $url): StreamInterface;
}

The file-saving logic doesn’t seem necessary to implement in this library at all.

kafkiansky avatar Apr 14 '25 06:04 kafkiansky

Overall, I like the idea.

But I have problem with cURL that can save a file immediately to a file, see: https://github.com/vjik/telegram-bot-api/blob/f22eec9d4ae0633c3f45566f5c8a0635a5ccb9d8/src/Transport/CurlTransport.php#L103-L107

Do you have idea how wrap it to stream?

The file-saving logic doesn’t seem necessary to implement in this library at all.

This is often convenient, so I want to keep it. But after suggested refactoring I can move this from transport to TelegramBotApi method

vjik avatar Apr 14 '25 08:04 vjik

Do you have idea how wrap it to stream?

As far as I can see, the only workaround with curl is using php://memory or php://temp.

kafkiansky avatar Apr 14 '25 08:04 kafkiansky

Do you have idea how wrap it to stream?

As far as I can see, the only workaround with curl is using php://memory or php://temp.

This way download and temporary save file before create stream. I would like to avoid pre-downloading...

vjik avatar Apr 16 '25 07:04 vjik