Switch from #[AllowDynmicProperties] to __set method for data properties

[jyxjjj]

🐞 Bug Report

Required Information

?	!
Operating system	Fedora 37
PHP Telegram Bot version	0.80.0
PHP version	8.2.0
MySQL version	none
Update Method	Webhook
Self-signed certificate	yes
RAW update (if available)

Summary

Creation of dynamic property is deprecated

Current behaviour

$this->$key = $value;

[2022-12-07 10:53:35.971230]
channel=deprecations
level=WARNING
message=Creation of dynamic property Longman\TelegramBot\Entities\User::$id is deprecated in /www/wwwroot/TelegramBot/vendor/longman/telegram-bot/src/Entities/Entity.php on line 99
context=
extra=

How to reproduce

Use PHP 8.2.0

[jyxjjj]

There are two ways to fix this warning, one is switch to enum that i talked about it months ago, one is to implement ArrayAccess, Laravel's Models used ArrayAccess then I did not get this warning.

[TiiFuchs]

Currently the whole library works with dynamic properties. We didn't discuss how to handle this yet.

Implementing a public function __set($name, $value); and saving the properties in an array instead of a dynamic property would solve this problem too and might be the most elegant solution. We could cleanup those $raw properties in the same steps.

Or we could just add the #[AllowDynamicProperties] attribute to the class. (This is doesn't result in errors in earlier PHP versions!)

I don't see how Enums would help at all with the actual problem and dynamic properties.

Accessing every property via ArrayAccess would help too, but as I said earlier: The whole library works with dynamic properties, so this would be the most complex and bc-breaking solution of all. We should not do that, as long as there is another way.

I think the magic set function would be the best way to go for us.
@noplanman your thoughts on this?

[jyxjjj]

yeah, I am in favor of using this scheme magic __set, but i think it will always be a temp way, finally we should define all the properties.

[jyxjjj]

I read the wiki
https://wiki.php.net/rfc/deprecate_dynamic_properties

First of all, sorry for my English, it seems like
#[AllowDynamicProperties]
will be like a new keyword to allow using it and will not be deleted or mark as deprecated in the future?
Then it will be a good way to use it for old projects.

Btw, i don't think bc break should never happen, PHP is breaking us like this, it may be the time to get into 1.0.0

[TiiFuchs]

I do not think we should avoid bc breaking changes too, but if we do it should be thoughtful. And with this I think the cost/benefit ratio is very low.

The attribute could be a solution, but the docs say itself "As a last resort the class can be marked with the .. attribute". I don't think we should go that way, as long we could do it the "right" way with __set.

Happy to hear more thoughts on this.

[DrNixx]

I have been programming a port of this library for Yii2. You can see my way of getting properties through getters and setters with Yii Model based entities
Entity.php
Audio.php

[TiiFuchs]

@alesinicio added AllowDynamicProperties for now. I'd like to keep this issue open, so we can at some point make the jump to __set in the future, since it is cleaner IMHO.

[alesinicio]

@TiiFuchs I believe I can help with the change for __set / __get.
Do you think trying to retrieve a invalid parameter should cast the return value to null? I believe this is OK, but I don't know the codebase and cannot evaluate potential problems with this approach.

[TiiFuchs]

Ideally the same thing should happen that happens now, if you access an invalid property.
Maybe you can test that and report to us?

[alesinicio]

Today the engine returns null when accessing an undefined property (https://3v4l.org/fOKWN).

PHP7.4 and less issue a notice when this happens, and PHP8.0+ issue a warning.
Obviously there will be no notice/warning with the proposed patch, which I guess is fine for the use case.

I'll get to work on this very soon and generate a PR.