TelegramTypes ChatMember updates #154
Conversation
…ng latest api specifications (June 25, 2021 Bot API 5.3)
… and in GetChatMember
…edit_stories and $can_delete_stories
There was a problem hiding this comment.
It's great to see Telegram handling some deprecated use-cases!
However, I do see some inconsistencies which could benefit from another approach which is a bit less elegant, but will not introduce the chance for more BC in the future.
I know I am requesting a lot of changes here for which I'm sorry! But I can also offer you some help with it if needed :)
| private const CHILD_CLASS_REFERENCES = [ | ||
| ChatMemberOwner::class, | ||
| ChatMemberAdministrator::class, | ||
| ChatMemberMember::class, | ||
| ChatMemberRestricted::class, | ||
| ChatMemberLeft::class, | ||
| ChatMemberBanned::class, | ||
| ]; |
There was a problem hiding this comment.
I would like for the actual Telegram classes to remain as clean as possible from 'external' code for two reasons:
- If Telegram decides to create a field called
createdit won't clash with the defined function. - It will prevent our users from actually depending on this code, people do crazy stuff and any changes to this will be BC.
I'm thinking that a better option would be to perhaps create a TelegramAPI\Telegram\Custom\ChatMemberFactory factory and make the bindToObject in GetChatMember (and others that return a ChatMemer type) call that dedicated factory instead.
There was a problem hiding this comment.
Ok, I would need to take a closer look, but I believe the function mapSubObjects was created for this purpose.
| /** | ||
| * Create specific ChatMember class based on 'status'. If there is no match, | ||
| */ | ||
| public static function create(array $data, LoggerInterface $logger): ChatMember |
There was a problem hiding this comment.
This should go into a factory and not in the Telegram class type.
| */ | ||
| class ChatMemberBanned extends ChatMember | ||
| { | ||
| public const STATUS = 'kicked'; |
There was a problem hiding this comment.
In the same vain of keeping this as close to the source as possible, it would be better not to define constants here (or in any of the other ChatMember classes).
A simpler and more consistent way would be to define the status in the same way we do here:
| @@ -15,131 +22,27 @@ | |||
| */ | |||
| class ChatMember extends TelegramTypes | |||
There was a problem hiding this comment.
This should be defined as an abstract method since a ChatMember will always be of a certain type.
| 'data' => $data, | ||
| ]); | ||
|
|
||
| return new ChatMember($data, $logger); |
There was a problem hiding this comment.
It's ok to throw an exception here. I would not support creating an object that is invalid to begin with.
| foreach (self::CHILD_CLASS_REFERENCES as $childClassReference) { | ||
| if ($status === $childClassReference::STATUS) { | ||
| return new $childClassReference($data, $logger); | ||
| } | ||
| } |
There was a problem hiding this comment.
While this is pleasant to look at, I much prefer dropping the constant within each type in order to keep those objects as clean as possible, and make the factory call the child classes directly.
I'm not a big fan of this, but I believe that's the best way, very much as we do here: https://github.com/unreal4u/telegram-api/blob/master/src/Telegram/Types/Message.php#L501-L566 (Yes, I hate that part of the code but it was the only way I could think of at the time).
This updates is mostly related to users. Here is quick overview of changes:
Added public constants for
Chatrepresenting chat type (private, group, supergroup and channel)Added new Telegram type classes:
Few small improvements and refactoring
All touched classes were updated to match documentation (types and PHPDocs) as of September 22, 2023 Bot API 6.9.