BingAds / BingAds-PHP-SDK

Other
56 stars 46 forks source link

Wrong definition of response/API object classes #34

Closed sjoorm closed 6 years ago

sjoorm commented 7 years ago

Hello,

I've noticed that some response classes have wrong declaration which differs from what I do receive from the API. Let's take two examples:

  1. Customer Management Service ->GetCustomerPilotFeatures()

    final class GetCustomerPilotFeaturesResponse
    {
        /**
         * A list of integers that identifies the pilot programs in which the customer participates.
         * @var integer[]
         */
        public $FeaturePilotFlags;
    }

    what I get as an actual response from the API:

    screen shot 2017-10-12 at 15 40 58

    according to GetCustomerPilotFeaturesResponse declaration, there should not be an internal "int" array while it is right there.

  2. Campaign Management Service ->GetAccountMigrationStatuses()

    final class GetAccountMigrationStatusesResponse
    {
        /**
         * An array of account migration statuses.
         * @var AccountMigrationStatusesInfo[]
         */
        public $MigrationStatuses;
    }

    actual response object:

    screen shot 2017-10-12 at 15 45 40

    as you see, GetAccountMigrationStatusesResponse defines MigrationStatuses property as an array of AccountMigrationStatusesInfo objects while it is actually an object containing the array field "AccountMigrationStatusesInfo" instead of that.

Going further, same with AccountMigrationStatusesInfo declared as:

final class AccountMigrationStatusesInfo
    {
        /**
         * The Bing Ads identifier of the account.
         * @var integer
         */
        public $AccountId;

        /**
         * The list of migration status info for the corresponding account.
         * @var MigrationStatusInfo[]
         */
        public $MigrationStatusInfo;
    }

turns out to have different structure in an API response:

screen shot 2017-10-12 at 15 51 55

So definitely response classes do describe something completely different from what I receive from the API.

Can you check what exactly went wrong here?

Thanks

eric-urban commented 6 years ago

@sjoorm I'm not quite following the ask here. The PHP classes appear to match the service request/response. Also I've tested these with a previous version of the ad extensions sample (for sitelink migration). It might help if you show an example of how you envision the PHP classes should be defined.

J7mbo commented 6 years ago

I believe this is referring to the docblock. A lot of us (everyone on my team, 99% of people in our company, and also clearly @sjoorm) use PHPStorm as our IDE and it points out to us if there is a clear DocBlock error.

According to the MigrationStatuses example, I would expect a numerically indexed array of MigrationStatusInfo objects that looks like this:

[
    object\\MigrationStatusInfo,
    object\\MigrationStatusInfo
]

Instead, according to OP's post, the PHPDoc is lying about what is coming back. What actually comes back is an \StdClass containing another \StdClass, then containing an array with AccountMigrationStatuses as the key and a numerically indexed array containing a grouping of values at each index. This is not what the API suggests via DocBlock (and we all rely heavily on these in PHP for proper in-code documentation).

If it indeed did return an array of MigrationStatusInfo[] then I would be able to do the following:

// Assume we got the statuses back from here
$statuses = $accountMigrationStatusesInfo->MigrationStatusInfo;

foreach ($statuses as $statusInfo) {
    assert($statusInfo instanceof MigrationStatusInfo);
}

Will the above pass? Nope. Therefore the DocBlocks we all rely on are incorrect.

@eric-urban The problem is the SDK is returning generic \StdClass objects instead of the objects of the correct type that it should be.

In our experience, proving stuff works as intended can only be provided by unit tests. In open-source, especially with SDKs, there should be CI tests proving that the correct values are returned. I can provide links to some free open-source tools that can assist with this if required, but there's also the problem that you're just using a tool to generate this stuff. The tests would have to be manually created at a guess.

sjoorm commented 6 years ago

Hi, @J7mbo @eric-urban

eric-urban The problem is the SDK is returning generic \StdClass objects instead of the objects of the correct type that it should be.

stdClass'es would be ok, but I've mentioned that sometimes instead of direct class object property values we have additional array wrappers containing these values in the end. For me it looks like there is a problem with deserialisation of SOAP response XMLs (we suppose that the PHP documentation is correct, cause it looks similar to what we have in BingAds API reference)

lushc commented 6 years ago

The SoapClient has a classmap option for mapping WSDL types to PHP classes, and I believe this is what's required to get the expected behaviour of XML responses deserializing into the actual classes provided in this SDK, rather than stdClass.

Looking through the Bulk data objects most things will map properly, however, I'm not sure how you'd handle types such as ArrayOfBatchError found in ApiFaultDetail. In these cases, I think this is what's causing the extra array wrapping, so the typemap option would be used to implement custom conversions from XML.

rmarlow commented 6 years ago

Bump. I have a situation where I want to retrieve the objects from the API, make one minor edit to them, and immediately send them back in an update, but instead I get "Cannot create an abstract class" errors when trying to pass stdClass objects to an Update operation.

eric-urban commented 6 years ago

@rmarlow long term we are considering something like classmap as @lushc has suggested. In the meantime the samples use encoded SoapVar e.g., here.

@sjoorm I will go ahead and close this issue as the SoapVar question is tracked here. Please reach out if you have additional questions, suggestions, or concerns.