Description
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 Telegram
Request::sendMessage([
'chat_id' => 12345,
'text' => 'Test',
'reply_markup' => [
'inline_keyboard' => [
[ // Row 1
[ // Button 1 in 1st row
'text' => 'A',
'callback_data' => 'A'
],
],
]
]
]);Or with a tiny amount of syntactic sugar with the InlineKeyboard class:
Request::sendMessage([
'chat_id' => 12345,
'text' => 'Test',
'reply_markup' => new InlineKeyboard(
[ // Row 1
[ // Button 1 in 1st row
'text' => 'A',
'callback_data' => 'A'
],
],
)
]);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 using func_get_args()
public function __construct(...$rows)Impact: low
This is backwards compatible to the previous behaviour.
Proposal 2
Maybe we should split Keyboard and InlineKeyboard into sibling classes instead of parent and child. (Currently InlineKeyboard extends Keyboard). Main reason for this is, because InlineKeyboard has in fact no getResizeKeyboard, getOneTimeKeyboard, ... methods but it gets inherited from Keyboard.
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 and ForceReply. Currently you need to use the static methods on Keyboard: Keyboard::forceReply() and Keyboard::remove().
Since InlineKeyboard extends Keyboard you can use InlineKeyboard::forceReply() and InlineKeyboard::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 of Keyboard, 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 and InlineKeyboard accept Buttons/Rows. KeyboardButton and InlineKeyboardButton 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 a Keyboard object with the buttons, and after that you can call the set* methods for stuff like resize_keyboard and one_time_keyboard. I think you can't pass rows AND those properties.
This means the Keyboard constructor would look like this:
public function __construct(
array $rows,
array $data = []
)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 of array $data this stays backwards compatible. We would add an optional second parameter that is definitely array $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
'reply_markup' => new Keyboard(
$rows,
selective: true,
input_field_placeholder: 'Type something...'
)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:
Keyboard::make()
->row([
KeyboardButton::make('Test')->requestLocation()
])
->row([
KeyboardButton::make('Second row')
])
->inputFieldPlaceholder('Type something...')
->selective();or
InlineKeyboard::make()
->row([
InlineKeyboardButton::make('Login')
->loginUrl('https://example.com/login-with-telegram'),
])
->row([
InlineKeyboardButton::make('Previous')
->callbackData('prev'),
InlineKeyboardButton::make('Next')
->callbackData('next'),
]);The awesome thing here is: We could implement a magic method that catches every additional call (like callbackData() and loginUrl()) 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:
InlineKeyboardButton::makeUrl($text, $url);
InlineKeyboardButton::makeLoginUrl($text, $loginUrl);
InlineKeyboardButton::makeCallbackData($text, $callbackData);
// ...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.