picqer / moneybird-php-client

PHP Client for Moneybird V2
MIT License
82 stars 77 forks source link

Improve datatypes of unserialized model #64

Closed holtkamp closed 7 years ago

holtkamp commented 7 years ago

When working on https://github.com/picqer/moneybird-php-client/issues/63 I noticed (again) that:

I think the integers are incorrectly returned as a string by the API, I will contact Moneybird about that.

About the dates, should we consider detecting 'date' strings and assemble a DateTimeImmutable for that?

stephangroen commented 7 years ago

Identifiers are given as strings on purpose by Moneybird. This is stated in their changelog:

Many JSON parsing tools out of the box don’t work well with our long integer IDs. Therefore we’ve updated the API to always return these IDs as strings instead.

Automatically creating DateTimeImmutable objects would require a new major version as this breaks backwards compatibility. I do like it, but we might consider some other changes if we're going to get to a new version.

holtkamp commented 7 years ago

Identifiers are given as strings on purpose by Moneybird.

aah, well, ok 😄 missed that one. What is your stance on using integer for them in this library? I would say PHP can deal with it properly...

would require a new major version as this breaks backwards compatibility. ... we might consider some other changes... Indeed, so maybe collect these things in a separate branch. I think I will collect some of these things in my fork, then you can cherry-pick the stuff you like / agree with.

holtkamp commented 7 years ago

@stephangroen I though a bit on how improved support for datatypes could be realized. Unfortunately I do not see an 'easy' way: automatic detection. For example: users might use string that appear an integer or DateTime as descriptions. So we need to keep track of what field has/should have what kind of datatype. With other words: metadata.

Did you ever consider using a library for the serialization (PHP => JSON) and deserialization (JSON => PHP) process? For example: http://jmsyst.com/libs/serializer

stephangroen commented 7 years ago

As long as we do not automatically form any string to a specific datatype, I don't see the usecase for adding complexity or the need to include extra libraries. I'd like to try and keep the library as clean as possible so it's easy to understand for all developers.

stephangroen commented 7 years ago

For dates their could be a setting/switch on the library to either get DateTimeImmutable objects or strings, depending on the preference. For example https://github.com/box/spout#datetime-formatting handles it this way.