backdrop-contrib / githubapi

GitHub API integration module
GNU General Public License v2.0
0 stars 3 forks source link

Use array for payload instead of an object? #1

Closed quicksketch closed 8 years ago

quicksketch commented 8 years ago

Over in the PR to Project module (https://github.com/backdrop-contrib/project/pull/13) it's noted that the $payload object switched from an array to an object. I know that is the default behavior when calling json_decode() on a JSON string. Could we consider using an array in GitHub API? Reasons why I find arrays to be better to work with:

Although the change to Project module is actually fairly minimal, going forward I believe the $payload variable will be an easier and more consistent to work with if it is an array rather than an object. What do you think?

Gormartsen commented 8 years ago

My first reaction was "Why not, it does not really matter".

But then I started to think about it and there is why I think we should keep it as object:

$response  =json_decode(json_encode($response), TRUE);

Instead of just converting object to array for payloads and answers from GitHub API server, I would create classes:

But I don't really want to do it at this stage of development. This stuff is necessary for really HUGE project where such level of abstraction is required.

Gormartsen commented 8 years ago

Ok, I made $payload an array for now. Updated githubapi is common ...

Gormartsen commented 8 years ago

backdrop-contrib/project#13

quicksketch commented 8 years ago

Great thanks! Per our Gitter discussion, the reason for this ultimately was because we didn't want to cast to arrays for things like $payload->commits, which would have required something like:

foreach ((array) $payload->commits as $commit) {

Using an array makes for easier manipulation. Long-term we may switch to a more structured (object-based) payload, but for now an array structure directly from the GitHub REST service is simple and effective.

Thanks Gor for making this change. I've merged in https://github.com/backdrop-contrib/project/pull/13 as well, fewer changes now required over there because of this switch.