New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Improving Keyboards #1297
Comments
Since we could add Proposal 6 without breaking anything that works now, that is my favorite for sure. |
I discussed this with @noplanman: We implement the following as described:
We create new Keyboard classes in a new Entities\Keyboard namespace:
We postpone the decision on Proposal 5 until we bump up the minimum PHP requirement. We create a new package as a plugin for Proposal 6. |
The new package (Proposal 6) can be found here: https://github.com/php-telegram-bot/fluent-keyboard/ |
I have no time to view all these, but i submitted a pull request may fix some of this. |
I'm thinking about improving keyboards since it happens a lot to me, that I'm a bit confused how to use keyboards. I always have to check the docs and I think we can make that a little bit more intuitive.
Status Quo
Currently
Keyboards
need to be defined either by mimicking the complete array structure of TelegramOr with a tiny amount of syntactic sugar with the
InlineKeyboard
class:I'm thinking about a few improvements.
Proposal 1
I already mentioned in #1239 that we should definitely use variable parameters with the
...
notation instead of usingfunc_get_args()
Impact: low
This is backwards compatible to the previous behaviour.
Proposal 2
Maybe we should split
Keyboard
andInlineKeyboard
into sibling classes instead of parent and child. (CurrentlyInlineKeyboard extends Keyboard
). Main reason for this is, becauseInlineKeyboard
has in fact nogetResizeKeyboard
,getOneTimeKeyboard
, ... methods but it gets inherited fromKeyboard
.Impact: low
This is backwards compatible to the previous behaviour.
Proposal 3
To be more consistent with current behaviour of the project we should create classes for
ReplyKeyboardRemove
andForceReply
. Currently you need to use the static methods onKeyboard
:Keyboard::forceReply()
andKeyboard::remove()
.Since
InlineKeyboard
extendsKeyboard
you can useInlineKeyboard::forceReply()
andInlineKeyboard::remove()
too, which makes no sense semantically. This get's fixed with Proposal 2.Impact: medium
We should mark the static methods as deprecated. But until it gets removed this is backwards compatible.
Proposal 4
We can change the
__construct()
methods ofKeyboard
,InlineKeyboard
,KeyboardButton
,InlineKeyboardButton
and maybe even more to allow passing the required parameters as the first parameters of the constructor and the rest as an array after that.Currently
Keyboard
andInlineKeyboard
accept Buttons/Rows.KeyboardButton
andInlineKeyboardButton
accept an array with all arguments OR you can pass a string that get's automatically set as text property in the data array. But you can't pass additional parameters.For
Keyboard
this means (as far as I know) that you need to instantiate aKeyboard
object with the buttons, and after that you can call the set* methods for stuff likeresize_keyboard
andone_time_keyboard
. I think you can't pass rows AND those properties.This means the Keyboard constructor would look like this:
It would not be possible to specify Keyboard rows like
new Keyboard($row1, $row2, $row3)
anymore.Impact: high
The changes to Keyboard are not possible with backwards compatibility in mind. InlineKeyboard has no additional parameters, but I recommend changing it in a way so both classes behave the same.
Since InlineKeyboardButton and KeyboardButton already allow
string $text
as first parameter instead ofarray $data
this stays backwards compatible. We would add an optional second parameter that is definitelyarray $data
, but if this is not passed and the first parameter is an array, we take this as $data instead.Partially backwards compatible
Proposal 5
Possible as soon as we bump the PHP dependency to 8.0
Like Proposal 4 but instead of a second
array $data
we make all those properties explicit arguments.Pro: You can use them with named parameters like
This would be even an awesome idea for other Entities/classes in the project. But this has one downside on the first look.
Currently most of the entities are perfectly usable when Telegram adds new fields to an object, but the library has not yet adapted it.
But I think we can still make this possible, if we always add a general
array $data
to the list of constructor parameters that can be used, if something is missing from the original parameter list. (In cases of Telegram Bot API Updates that are not yet reflected in the library.)Impact: high
I think this has the same backwards compatibility notes as the previous Proposal.
Proposal 6
Adapt a more Laravel-like syntax.
Pro: It's awesome
Contra: We don't have this anywhere else in this project... But we could add this style in a few other areas too.
Example:
or
The awesome thing here is: We could implement a magic method that catches every additional call (like
callbackData()
andloginUrl()
) and puts those (in snake_case) into the data array. And of course we would do that so we still have that awesome feature, that you can use new Telegram Bot API versions even if there is not yet a new release of this package.Since InlineKeyboardButtons have this special case that you MUST use exactly one of the optional fields, it may be good to streamline this a little bit more like this:
Impact: high
Backwards compatibility: We could double-track this, leave the previous implementation in but mark as deprecated and add the new features like described.
The text was updated successfully, but these errors were encountered: