search

messagebird / php-rest-api

This repository contains the open source PHP client for MessageBird's REST API.
https://developers.messagebird.com/
BSD 2-Clause "Simplified" License
158 stars 95 forks source link

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

Closed MelvinLoos closed 2 years ago

MelvinLoos commented 3 years ago

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

CoolGoose commented 2 years ago

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 commented 2 years ago

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

CoolGoose commented 2 years ago

One small thing extra :)

MelvinLoos commented 2 years ago

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 commented 2 years ago

@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 commented 2 years ago

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 commented 2 years ago

@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 commented 2 years ago

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

MelvinLoos commented 2 years ago

@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 commented 2 years ago

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

gronostajo commented 2 years ago

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 commented 2 years ago

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.