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

Deprecated loadFromArray for loadFromStdClass and fixed parsing of contacts in Groups::getContacts #182

Merged
merged 9 commits into from
Mar 3, 2022

Conversation

MelvinLoos
Copy link
Contributor

I fixed the Group::loadFromArray method and added return type to other objects.

@MelvinLoos MelvinLoos changed the title Fixes Group::loadFromArray method and adds return type to other objects. Multiple fixes Nov 15, 2021
@CoolGoose CoolGoose self-assigned this Dec 21, 2021
@CoolGoose
Copy link
Contributor

Hi @MelvinLoos thank you for the PR this looks neat. There are some small things that I would like to go over before merging this. (eg contacts being an stdClass but they're still initialized as array.

@MelvinLoos
Copy link
Contributor Author

Ah yes well spotted! If I can find the time I will modify the contacts property tonight. Anything else that I missed?

@CoolGoose
Copy link
Contributor

One small thing extra :)

@MelvinLoos
Copy link
Contributor Author

Also one more inconsistency which I wanted to point out and I think we should fix/correct. Since it caused a bit of confusing for me when I started working with this library and also when working on it again today. The fact that the models/objects use a method called loadFromArray but this method actually does not work with arrays it works with stdClass.
That it works with stdClass is fine in my opinion because that is what the PHP json_decode function returns by default and is what I personally prefer over working with arrays. So my only suggestion would be to rename all loadFromArray methods to loadFromStdclass. What do you think @CoolGoose ?

@CoolGoose
Copy link
Contributor

@MelvinLoos I agree, it makes sense. Let's just not rename it, but keep the current one, mark it as deprecated, and we can remove it in a future major version (3.0 isn't released, but I'd still want to have an easier upgrade path).

@MelvinLoos MelvinLoos changed the title Multiple fixes Deprecated loadFromArray for loadFromStdClass and fixed parsing of contacts in Groups::getContacts Jan 28, 2022
@MelvinLoos
Copy link
Contributor Author

MelvinLoos commented Jan 28, 2022

Took me awhile to find the time for this, sorry about that. I have renamed this issue to better reflect the changes. The initial fix revealed alot of inconsistencies in the implementation of the response parsing.

As requested I have deprecated the loadFromArray methods (instead of deleting them) so it should all be backwards-compatible. All internal methods have been refactor to use the new loadFromStdClass method but the public API should still be intact. Please check if I missed anything. (I have ran phpunit this time so if everything is tested it should be good)

@CoolGoose is the PR like this ok or would you like me to split the deprecation and the bug fix into two separate pull requests?

@CoolGoose
Copy link
Contributor

@MelvinLoos I apologise for the late reply. Can you please check psalm ? If not I'll try to make time next week to do it, thank you for the updates they're nice to cleanup the codebase.

@MelvinLoos
Copy link
Contributor Author

Will try to make time for it this weekend or else I will reserve some time in my work schedule on monday

@MelvinLoos
Copy link
Contributor Author

@CoolGoose ok I have corrected the code based on the php 7.3 phpunit tests. I had to downgrade some code to make it php 7.3 compatible but nothing major and I implemented docblocks annotations to compensate.

@CoolGoose CoolGoose merged commit e6d28a8 into messagebird:master Mar 3, 2022
@CoolGoose
Copy link
Contributor

Thank you @MelvinLoos for the patience. Will be in the 3.0 release this week.

@gronostajo
Copy link
Contributor

All internal methods have been refactor to use the new loadFromStdClass method

Unfortunately that's not the case - loadFromArray is still used by \MessageBird\Resources\Base::getList(…).

@MelvinLoos
Copy link
Contributor Author

Yes you seem to be right. I somehow missed that instance....
I don't have any time in the short term to look into it so feel free to create another pull request. If not I will follow it up at some later time.

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

Successfully merging this pull request may close these issues.

3 participants