fix(RemoteService): remove redundant stream cleanup in convertFileTo()

[nicfab]

Summary

Fixes #5743

convertFileTo() opens a raw stream and passes it to convertTo(), which hands it to the Guzzle-based HTTP client as the multipart request body. Guzzle wraps it in a PSR-7 stream and closes the underlying resource once the request has been sent. By the time the finally block runs, the resource is no longer valid, so fclose() logs a warning on every conversion — one per generated preview/thumbnail.

Since Guzzle takes ownership of the stream and convertTo() already handles request errors, this PR drops the redundant try/finally block entirely and returns the conversion result directly, as discussed in the review below.

Testing

The removal of the explicit cleanup has been discussed in the review; the original symptom and its diagnosis were validated independently by multiple users in #5743 on different stacks (Nextcloud 32/33, richdocuments 9.1/10.2, MariaDB/PostgreSQL, Ubuntu/Debian, Collabora 25.x/26.x): conversions and previews keep working as expected and the warning is no longer logged.

[elzody]

Thanks a lot for creating this pull request! Your contribution means a lot. :)

That said, the psalm failure brings up a good point:

Error: lib/Service/RemoteService.php:74:8: RedundantCondition: Type resource for $stream is always resource (see https://psalm.dev/122)

Even though is_resource() is a seemingly logical way to check if the stream is still there, from psalm's perspective it will always be a stream and there is no need to check the type. This got me thinking about a larger issue, though. I am not so sure we even need the try/finally block in the code to begin with. You are right that Guzzle closes the stream itself, but convertTo() already catches any errors thrown when sending the request. Even if an error is thrown, the stream should still get closed and there is not a need to check if the stream still exists (it will already have been closed).

TL;DR: I am wondering if we can just remove the try/finally altogether and just do

return $this->convertTo($file->getName(), $stream, $format, [], $timeout);

What do you think @nicfab and @juliusknorr?

[nicfab]

Thanks a lot for creating this pull request! Your contribution means a lot. :)

That said, the psalm failure brings up a good point:

Error: lib/Service/RemoteService.php:74:8: RedundantCondition: Type resource for $stream is always resource (see https://psalm.dev/122)

Even though is_resource() is a seemingly logical way to check if the stream is still there, from psalm's perspective it will always be a stream and there is no need to check the type. This got me thinking about a larger issue, though. I am not so sure we even need the try/finally block in the code to begin with. You are right that Guzzle closes the stream itself, but convertTo() already catches any errors thrown when sending the request. Even if an error is thrown, the stream should still get closed and there is not a need to check if the stream still exists (it will already have been closed).

TL;DR: I am wondering if we can just remove the try/finally altogether and just do

return $this->convertTo($file->getName(), $stream, $format, [], $timeout);

What do you think @nicfab and @juliusknorr?

Thanks a lot for the review and the kind words! You're right on both counts: psalm's complaint is legitimate given the type narrowing (the false case is already excluded above), and since Guzzle takes ownership of the stream and closes it once the request has been sent — with convertTo() already handling request errors — the try/finally is indeed redundant. The only theoretical leftover would be a stream not being explicitly closed if something throws before the request body is consumed, but PHP releases the resource as soon as it goes out of scope, so it's a non-issue in practice.

I've updated the PR accordingly: dropped the try/finally and the method now returns the conversion result directly.

[elzody]

@nicfab The failures now are not related to your changes -- PHP 8.2 support was dropped so it caused a lot of failures. Luckily, it has been fixed with #5759, so you should just need to rebase your changes on top of the latest changes from the main branch, and they will pass.

[nicfab]

@nicfab The failures now are not related to your changes -- PHP 8.2 support was dropped so it caused a lot of failures. Luckily, it has been fixed with #5759, so you should just need to rebase your changes on top of the latest changes from the main branch, and they will pass.

Thanks for the heads-up! Rebased on top of the latest main.