Pr/attachments fclose

[DJTommek]

If some file is added using unreal4u\TelegramAPI\Telegram\Types\Custom\InputFile(), file is locked using fopen() and read via stream_get_contents() but it is never closed. Usually it doesn't matter, because lock is released once PHP script is finished but in specific situations, it is causing troubles. Steps to reproduce:

1. create directory some-dir
2. create file some-dir/some.file
3. upload it using InputFile() and SendDocument()
4. delete file some-dir/some.file (file is really deleted)
5. delete directory some-dir (warning will occure, that Directory is not empty and directory is not deleted)

In this PR I also added real example with these steps, see examples/send-document-custom-name.php.

Disclaimer

It seems, that warning is occuring only on Windows not on Linux, can someone run more tests? Tested on:

• Windows 10, PHP 7.3.21, 7.4.9 and 8.0.9
• Linux Ubuntu 20.04.3, PHP 7.4.3

[NanoSector]

While I think this is an okay option for the short term, I don't think it should be the post option builder's responsibility to close file handles.

Instead, I'd rather have methods for clearing file handles on InputFile, like a close() method or similar.

After taking a look, InputFile is inherently broken because it calls setStream() on every invocation of getStream(), which in turn does a fopen call every time without checking if the resource is already open.

Adding this to InputFile and fixing up that mess (by adding file handle availability checks?) puts the user in charge of the file handle lifecycle. While this does add an extra necessary step ($inputFile->close();), the initial opening can be implied by the constructor.

Anyway, that's my two cents for this mess. Regarding the PR, again, I think it's an OK solution until the rewrite, but it might cause unexpected bugs down the line. I'm not sure if this can be put in a minor patch release; OTOH, setStream should reopen the stream if it was ever closed.

[DJTommek]

Thanks for your input!

I agree it might not be job for post builder, but it should be closed once it is uploaded, that is for sure.

I still don't have necessary knowledge of this library so I don't have strong opinion who's job closing is, so you can update it as you want. First though was it could be in InputFile()->__destruct() but it is too late so as you wrote - user would need to call it manually. Or it could be destroyed somewhere in library automatically.

[NanoSector]

Object destructuring in PHP unfortunately is unreliable as not enough care is generally taken for object life cycles, this could leave the file handle open for much longer than intended as you mentioned. Forcing object management on users is another can of worms I wouldn't put in this library.

Maybe a more elegant solution would be to expose a method which expects a callback, where only in that callback the resource is available through its parameters. This would provide automatic file handle lifecycle management by the InputFile object and also in the best case prevent accidental leaking of the handle elsewhere (unfortunately we can't battle stubborn developers who steal the handle from said closure).

I think @unreal4u is planning to do a major rewrite of the library soon(Tm) so depending on that we could fix up InputFile as-is, or implement this patch. Might be worth waiting for his input on this.

If the rewrite will be much later, I'd be in favour of a proper fix now which could perhaps be ported to the new version.

Thanks for bringing this up anyway, it's food for thought and it does impact the architecture of the rewrite, so it's worth thinking about now.

[unreal4u]

Difficult one: InputFile comes from the days where this library wasn't async yet, and I had to pass on a stream to curl.

As for responsibilities, InputFile should do this, however you would need to do this manually: I don't like that. The user doesn't open the file manually, so neither should he/she close it manually.

I was thinking that getStream() will know and can close the file appropiately: however since it is a stream, we can't close yet until we have appended it.

I will merge this PR for now, but I'll need to check out this properly. The idea of opening a file in the constructor was a very bad one, but fo fix it we'll have to rewrite InputFile entirely. This is something we can do for the big rewrite.

[unreal4u]

made a new card for this: https://github.com/unreal4u/telegram-api/projects/5#card-69531588