search

defstudio / telegraph

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

Fix config breaking change in minor release #566

Closed newtonjob closed 5 months ago

newtonjob commented 5 months ago

This PR fixes a breaking change when people upgrade to 1.42.

The latest release changes the config key for the webhook handler from telegraph.webhook_handler to telegraph.webhook.handler. However, it doesn't handle backward compatibility correctly.

The following line in WebhookController attempts to use the new config value while falling back to the old one. https://github.com/defstudio/telegraph/blob/d5176bcebb41982a2a266eb54ff92298080f76c8/src/Controllers/WebhookController.php#L23

Since the package internal config file is merged with the app's published config file, the telegraph.webhook.handler key will always be available and the fallback is thus redundant.

This should however be the other way around, as we ideally want to first check with the old key and fallback to the new one since people will still have the old config published.

It should look like this;

$handler = config('telegraph.webhook_handler', config('telegraph.webhook.handler'));
newtonjob commented 5 months ago

@fabio-ivona your thoughts on this? Our app was broken after a seemingly harmless composer update and we couldn't understand, until we investigated and traced it to the package update.

We ended up upgrading to the new config structure to get it working again. Would that be expected from a minor update?

fabio-ivona commented 5 months ago

@newtonjob thanks for pointing this out, it was a sad mistake made by me, sorry

I'm reviewing all the places where the fallback has been added in order to avoid this to happen again

fabio-ivona commented 5 months ago

Fixed in #570 as this was a broader issue, added you as co-author, thanks @newtonjob

newtonjob commented 5 months ago

Thank you very much for your help @fabio-ivona