braintree / braintree_php

Braintree PHP library
https://developer.paypal.com/braintree/docs/start/overview
MIT License
547 stars 224 forks source link

No easy way to extract the all attributes out of Base or Instance #289

Closed valorin closed 3 years ago

valorin commented 3 years ago

General information

Issue description

There is no easy way to extract all attributes out of the Base or Instance classes, which makes it difficult to dump the complete event payload into a database or log.

The Base class includes jsonSerialize(), which can be used to extract the attributes, however any nested classes remain as classes and can be lost when the value is stored. The Instance class lacks any option to extract all attributes.

Implementing a toArray() function similar to this on each class would facilitate pulling the attributes out nicely.

public function toArray()
{
    return array_map(function ($value) {
        return method_exists($value, 'toArray') ? $value->toArray() : $value;
    }, $this->_attributes);
}

I haven't created a PR yet as I noticed similar PRs for adding JsonSerialize were rejected, so thought opening the discussion first would be more productive. I am happy to create a PR if you're interested.

hollabaq86 commented 3 years ago

šŸ‘‹ @valorin thanks for reaching out. I can totally see the value of being able to get all attributes in a response- my initial thoughts are that adding a toArray() function to every class seems a little repetitive... I'd like to think on this use case for a bit and come back to you.

hollabaq86 commented 3 years ago

hey @valorin I've been looking into this:

The Base class includes jsonSerialize(), which can be used to extract the attributes, however any nested classes remain as classes and can be lost when the value is stored.

Can you elaborate on scenarios/use cases where nested class values get lost? That might help us figure out how to get one function for all attributes.

valorin commented 3 years ago

@hollabaq86 It happens any time I want to convert a response into a format where it can be safely stored for later inspection. This is useful for error handling, if you want to log the errors on failed transactions, or store webhook payloads for asynchronous handling. The framework we use (Laravel) attempts to convert the response object into JSON to store as a string in the database. This then means you can access the data as an array on the record when it's retrieved at a later point.

The structure of the response objects is a bunch of nested objects, so although the top level may include the jsonSerialize(), this only applies to the top level. Some of the nested objects also include jsonSerialize(), which allows them to be converted too, but not all of them. The ones without jsonSerialize() return null/empty arrays.

The simplest example I can come up with is with a Dispute Webhook, as generated by the test framework:

$hook = $braintree->webhookTesting()->sampleNotification(WebhookNotification::DISPUTE_OPENED, $id);

I want to store the payload received from the webhook in the database, so it can be processed asynchronously. Part of storing it in the database involves pulling out the dispute transaction details and throwing it into the database:

$model->dispute_transaction_details = $payload->dispute->transactionDetails

The framework attempts to store the value by using json_encode/decode, which has no nice way to extract the data and results in an empty/null value:

array:1 [
  "dispute_transaction_details" => []
]

When inspecting the value directly, you can see it is an instance of TransactionDetails:

Braintree\Dispute\TransactionDetails^ {#919
    #_attributes: array:2 [
        "id" => "fake499TWgzwBlXLpNxP"
        "amount" => "250.00"
    ]
}

This extends Braintree/Instance.php, which has no way to pull the raw data out of nicely so it can be stored in the database for later use.

If this doesn't make sense, I'll make a more complete code example you can run to see the problem in action.

hollabaq86 commented 3 years ago

Super helpful! Thanks šŸ˜„

hollabaq86 commented 3 years ago

šŸ‘‹ @valorin since both the Base and Instance classes have __toString() functions, what if we added a toArray function to both of these classes that looks like:

public function toArray()
{
    return array_map(array($this, '__toString'), $this->_attributes);
}

To my knowledge, all of our classes inherit from either Base or Instance, so this should allow you to json encode the contents of a result object.

Let me know your thoughts!

hollabaq86 commented 3 years ago

Now that I'm adding unit tests your suggested code for a toArray function is the better way to go (and not just returning an array of everything whoops šŸ™ƒ ), I think we just add it to the Base and Instance classes and all other classes should inherit these functions šŸ¤”

valorin commented 3 years ago

Ah awesome, adding toArray() onto everything sounds good to me. Thanks! šŸ˜

hollabaq86 commented 3 years ago

Version 6.0.0 has been released with these functions šŸŽ‰ Feel free to open a new issue or submit a PR if there's any tweaks that could be made, and thanks for all of your input on this work!