Skip to content
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

Refactor: better handling of dynamic properties for Entities #1390

Merged
merged 1 commit into from May 7, 2023

Conversation

alesinicio
Copy link
Contributor

Introduce usage of magic __set and __get methods to handle dynamic properties in Entities class.

#1375

Introduce usage of magic __set and __get methods to handle dynamic properties in Entities class.
@alesinicio alesinicio closed this Apr 28, 2023
@alesinicio alesinicio reopened this Apr 28, 2023
@noplanman
Copy link
Member

@TiiFuchs This should be ok to merge, yes?

Or was there something else you wanted to have in here?

@TiiFuchs
Copy link
Member

I'm not that fond of the property name being "parameters".
Telegram calls them "fields" in their documentation, PHP usually calls them "properties". Parameters are the one on functions. That's misleading.

I vote renaming them to "fields".

Additionally: Should we throw an Exception when accessing a non-defined field?
That's not exactly the previous behavior, previously you would at least get a warning. Now nothing.

An Exception would help, since most of those Classes get their fields set from the incoming API Update. So if you access $update->mssage it was 100% an error, that get's undetected.

Let's merge it but rename the field, and add a throw there. What do you think, @noplanman ?

@noplanman
Copy link
Member

noplanman commented Apr 30, 2023

@TiiFuchs Go for it!

Thanks @alesinicio 🎉

@TiiFuchs TiiFuchs merged commit 37355b4 into php-telegram-bot:develop May 7, 2023
1 of 9 checks passed
@TiiFuchs
Copy link
Member

TiiFuchs commented May 7, 2023

I added the discussed refactoring and changed the jsonSerialize() method as well.
See #1397

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants