Send custom fields on Lead object in format custom.FIELD_NAME/ID

[danbarker]

Previously, custom fields on leads were sent as a dictionary called 'custom'.

According to the Close documentation here: https://developer.close.io/#leads , this format is deprecated and will be removed (although when you request a lead from the API, it's still given in this format!)

Additionally, when sending using this format, it's not possible to unset values.

This PR converts the 'custom' array into multiple properties called custom.FIELD_NAME or custom.FIELD_ID (whichever you use) just before sending to the API. Note that it's necessary to use the field IDs if you want to be able to unset values (by setting value as null)

[mickadoo]

Some suggestions, feel free to argue back!

[mickadoo]

s/alraedy/already/ and "Close" => "close.io"

[mickadoo]

It makes sense to have this to cover the custom fields case, but how about having it in the model instead? All model classes implement \JsonSerializable and use the JsonSerializableHelperTrait. How about for lead we keep \JsonSerializable but remove JsonSerializableHelperTrait and implement jsonSerialize inside LooplineSystems/CloseIoApiWrapper/Model/Lead.php instead? It would do the same as what your code below does.

By doing that we wouldn't have an exception - right now all the model classes are all responsible for serializing themselves. Some people might grumble that having a model that handles serializing itself is not following the single responsibility principle, but that's the way it is for now (although a PR to change that for all could be an idea).

It would also be nice to have some unit tests for this too.

[danbarker]

I thought that I had explained this in the PR but on second reading, apparently not. Maybe I did it in line 2 of one of the commits? Sorry.

The problem is that Close want us to give them an object that has keys in the form "custom.XXX". But we can't set properties on the PHP object that's going to be JSONSerialized in the format "custom.XXX", because you can't put a fullstop in a property name in PHP.

So unless we move away from the approach of taking the PHP object and then JSONSerializing it before sending it to Close, I couldn't think of a better solution than this. It stumped me for half a day so if you can think of a better solution then I'm open!

[mickadoo]

That's fine, maybe it's me misunderstanding the situation, but from the change it looks like you're replacing

$lead = json_encode($lead);

With

$lead = $this->encodeLead($lead);

What I'm saying is that what's already happening here with the implementation of JsonSerializable is that it's calling jsonSerialize() under the hood. This is because

Objects implementing JsonSerializable can customize their JSON representation when encoded with json_encode().

From PHP docs

Right now for Lead this means it's calling jsonSerialize which is defined in the trait that Lead uses.

My suggestion is to remove this trait from the Lead class, and instead implement jsonSerialize directly inside Lead. That means when you call:

json_encode($lead);

It will call your implementation of jsonSerialize to fetch the values to serialize. Does that make sense?

[danbarker]

Hi @mickadoo - apologies for the delay getting back. I've come back to this a couple of times and have been unsure whether I agree or not. Feeling less uncertain today so here goes:

I'm not sure if implementing a custom jsonSerialize inside Lead feels right to me, because:

• If we implement the logic above for jsonSerialize, then calling json_encode will not actually just encode the object to JSON, but will also change the data structure.
• Because of this, anyone coming to the code in future might end up being tripped up by this as I think most people would expect json_encode to do what it normally does, and what it is called (just encode to JSON and no other unexplained magic)

What do you think?

If you accept this, then I still have a couple of questions:

• Should I rename encodeLead to make it even more explicit, e.g. encodeAndReformatLead or something else?
• Should the method be on LeadApi or Lead? I still think LeadApi as it also has some other utility functions such as buildQueryString and validateLeadForPost, and what we are doing strictly relates to the API. But open to debate.

I've pushed the typo changes you requested, so this PR could be merged now. Or I'm happy to make the changes you suggested if you disagree, or have any other suggestions.

[danbarker]

To address your point about exceptional behaviour: Yes, this is exceptional in that it means that we use a different method to encode the Lead object compared to the other objects. But that is because the API behaves differently for this object and so is also exceptional. If we put the logic into a custom jsonSerialize, we would still end up having this difference in the way the object is encoded to JSON, but just in a place that is more difficult to find and therefore arguably more confusing.

[mickadoo]

most people would expect json_encode to do what it normally does

I'd say that it already doesn't really do what it "normally" does because it's behavior is altered by the trait it uses.

If we implement the logic above for jsonSerialize, then calling json_encode will not actually just encode the object to JSON, but will also change the data structure.

For me the purpose of implementing jsonSerialize is so we have a handy way to ensure an object is prepared for transmission over the network, and I'd say that your change from the encodeLead function are part of that.

In the future if someone sees everywhere in the code that we're always just calling json_encode on entities before sending them over the network they might be more likely to assume that's how it should work with lead too since it's the only exception.

I would also be a bit concerned that going forward we might have more exceptions and pollute the API classes with lots of functions to cater to specific entities.

I think in the long term it would be nicer to use something like the Symfony serializer to handle serialization and have a separate service that handles that. If you'd like to work on that it would be a welcome change, since the current implementation breaks the single responsibility principle (entities are responsible for serializing themselves too). The change in this PR goes some way towards changing that, but it's breaking convention too, and only for a single entity. If we kept the convention with this PR I can imagine it would be easier to make the changes for all entities in the future since we could say confidently that it would involve moving the logic from jsonSerialize for each entity to another service, with no exceptions.

That said, I'm not really involved in this project so much. If you feel really strongly that yours is the best way then I'm not going to get in your way in getting this merged since it is a positive change.

[ste93cry]

FYI: I didn't notice that this problem was already being addressed here, so I opened #97 to fix it while trying to maintain the usage of the JsonSerializableHelperTrait trait.