Closed J7mbo closed 4 years ago
@J7mbo I agree that would be a useful feature for the SDK. If I understand correctly you want to just write $locationCriterion = new LocationCriterion();
and have the SDK abstract the encoding details. If you have any other specific requirements or suggestions please let me know.
In the short term we will update documentation to clarify that to send a complex type that inherits from a base class, you must encode the object as a SoapVar.
Exactly, the user would just create a LocationCriterion
or any other object and the underlying implementation would know to use a \SoapVar
inside it. No worrying about encoding as a BiddableCampaignCriterion
, and the Criterion
within it etc.
Another suggestion for the long-term would be to code the business requirements that Bing Ads specifies in the documentation into the objects themselves. No more public properties and no more instantiating an object in an invalid state (basic OO); they must be passed in via constructor at the same time otherwise the object cannot be created.
For example, either throw an exception or ignore if we try and add more than one LocationIntent
criterion to a campaign. Developers must spend a long time on the developer documentation chasing required or optional words to figure out how to create their own objects that wrap the Bing SDK ones with these business requirements coded into them, rather than the object API leading the way. This is Microsoft's requirements and not the end user's - should the end users be able to create SDK objects in an invalid state?
Finally, this would be a breaking API change but the plural for Criterion
is Criteria
(I have had to add a new non-english word to the dictionary in my IDE 😄), not "Criterions".
These last points are of course individual issues as of themselves but I thought I'd put them here while they're fresh~ly painful~ in my mind in developing with the Bing SDK 😺
Thanks for adding it to the developer docs though, I'm sure it'll save a lot of people a lot of time 👍
@J7mbo yes there are some challenges as the business requirements vary for the same object across add/update/get operations. Ideally if we would be able to guarantee business requirements in the contract, they would be defined in the WSDL.
Regarding criterions vs criteria yes we are aware and have often second guessed the messaging. The intent was to refer to multiple Criterion objects. We don't have any 'Criteria' object so that might also be confusing. Will continue to assess feedback here.
I had the same problem and at least mitigated it, (I think), through the use of static initialisers for new campaigns and existing campaigns within my Campaign
object.
Thinking about it, they could be two separate objects, the constructors of which would require different things (for example, a NewCampaign
does not require an id in it's constructor and an UpdateCampaign
requires an id). It was a ballache for me too! If you ever want to brainstorm on this, because we're going to use this SDK a hell of a lot here at trivago, please let me know. Otherwise I eventually aim to share my 'wrapper' in open source as well.
Finally, \SoapFault
is also an internal detail to the SDK - I'm converting to SuccessResponse
and FailureResponse
objects depending on the operation, so this is another Soap thing that might not want to be made available to the end user.
Thanks for the responses eric! :)
@J7mbo as mentioned above for the short term we added some tips on SoapVar in the API docs.
For the longer term source change to make SoapVar internal I'll need to spend some time in the code for our proxy generator. All of the classes e.g. LocationCriterion.php are generated by a tool. Of course we know from the WSDL whether a type is inherited, etc, so I have some degree of confidence this should be feasible to implement. I don't have any ETA yet. Might not come this month, but I will keep track of the request.
So far I have encountered twice an error that is only solved after googling and finding out that we have to, for example, encode a
Criterion
object as a\SoapVar
.The transport for this API should be an internal implementation detail - the end user should not have to care about encoding ANYTHING as a \SoapVar.
I hope to open a dialogue on this topic because the requirements for exactly what objects need to be encoded in this way are not even documented anywhere and make for a very painful experience with the Bing Ads API and this SDK.