defstudio / telegraph

Telegraph is a Laravel package for fluently interacting with Telegram Bots
MIT License
697 stars 118 forks source link

Telegraph->files not cleaned between 2 telegram requests #432

Closed varemel closed 10 months ago

varemel commented 1 year ago

Hi,

Thanks for your library.

I found interesting bug when try to edit message after I sent message with attached audio file. Long story short.

Telegraph facade object vendor/defstudio/telegraph/src/Telegraph.php When we clone telegraph object, I believe, we need to clear files Collection, because it is an object and it will point to the same object in original Telegraph object.

005744

I propose to add something like this to vendor/defstudio/telegraph/src/Telegraph.php

public function __clone(): void
{
    $this->files = Collection::empty();
}
varemel commented 1 year ago

@fabio-ivona Is it worth it to create a PR for such a small fix in future?

fabio-ivona commented 1 year ago

Hi! sure! feel free to open a PR for this ❤️

varemel commented 1 year ago

Hi! sure! feel free to open a PR for this ❤️

Okay. I will add some tests first. Because solution is not so straightforward, as I thought.

We clone Telegraph object on every method call. If we clear files on every clone call, we loose attached file on following call. For example,

Telegraph::photo(Storage::path('photo.jpg'))->message('test')->send();
varemel commented 1 year ago

Update:

I wrote test and discovered that problem is with TelegraphFake actually and not with Telegraph. Reason is that TelegraphFacade is bind to factory, not instance of Telegraph object. So every time Facade creates new object:

class TelegraphServiceProvider extends PackageServiceProvider
{
    public function configurePackage(Package $package): void
    {
        $package
            ->name('telegraph')
            ->hasConfigFile()
            ->hasRoute('api')
            ->hasMigration('create_telegraph_bots_table')
            ->hasMigration('create_telegraph_chats_table')
            ->hasCommand(CreateNewBotCommand::class)
            ->hasCommand(CreateNewChatCommand::class)
            ->hasCommand(SetTelegramWebhookCommand::class)
            ->hasCommand(UnsetTelegramWebhookCommand::class)
            ->hasCommand(GetTelegramWebhookDebugInfoCommand::class)
            ->hasTranslations();

===>        $this->app->bind('telegraph', fn () => new Telegraph());
    }
}

But TelegraphFake is binded to instance to preserve sent messages between fake requests:

class Telegraph extends Facade
{
    protected static $cached = false;

    /**
     * @param array<string, array<mixed>> $replies
     */
    public static function fake(array $replies = []): TelegraphFake
    {
        TelegraphFake::reset();
===>        static::swap($fake = new TelegraphFake($replies));

        return $fake;
    }
....

I fix that and make PR https://github.com/defstudio/telegraph/pull/433. Please check and merge if it's ok. Thanks.

fabio-ivona commented 10 months ago

solved by @varemel PR, thanks !