googleads / google-ads-php

Google Ads API Client Library for PHP
https://developers.google.com/google-ads/api/docs/client-libs/php
Apache License 2.0
295 stars 262 forks source link

Abstraction between GRPC types in the SDK #49

Closed leongersen closed 5 years ago

leongersen commented 5 years ago

Currently, all conversions to GRPC types have to be made in user code, resulting in rather unwieldy object creation.

Please consider the following example:

$keywordPlanCampaign = (new KeywordPlanCampaign())
    ->setCpcBidMicros(new Int64Value(['value' => 1000000]))
    ->setName(new StringValue(['value' => 'Keyword plan campaign']))
    ->setKeywordPlanNetwork(KeywordPlanNetwork::GOOGLE_SEARCH_AND_PARTNERS)
    ->setKeywordPlan(new StringValue(['value' => $keywordPlanResourceName]));

Arguably, this code could look like the following, with the SDK abstracting away the encapsulation in StringValue, Int64Value, etc.

$keywordPlanCampaign = (new KeywordPlanCampaign())
    ->setCpcBidMicros(1000000)
    ->setName('Keyword plan campaign')
    ->setKeywordPlanNetwork(KeywordPlanNetwork::GOOGLE_SEARCH_AND_PARTNERS)
    ->setKeywordPlan($keywordPlanResourceName);

The same issue applies when handling responses, where scalar values and Protobuf objects are mixed. This has some ergonomic issues, because the user has to check the return type for every method/property individually:

/** @var KeywordPlanKeywordHistoricalMetrics $metric */
foreach ($response->getMetrics() as $metric) {
    $keywordMetrics = $metric->getKeywordMetrics();

    // SearchQuery is a StringValue
    $metric->getSearchQuery()->getValue();

    // Competition is an int (not any of the Google\Protobuf\* types)
    $keywordMetrics->getCompetition();
}

Since the SDK has a minimum requirement of PHP > 7.1, typing could be enforced using PHP's native type hints, resulting in more accurate static analysis and generally leaner code.

In the AdWords SDK for the Soap based API, the protocol specifics are abstracted away. The user does need to know about the Soap envelope, namespacing or encoding specifics.

Was the choice to work directly with the Protobuf types in user code deliberate, or is this merely the result of generating the PHP classes from protocol buffers? Is the current SDK design "final", or could changes in this area be expected?

Raibaz commented 5 years ago

Hello and thanks for the detailed explanation.

We are aware of this issue and we are actually already working on an improvement, soon to be released, that will add getters setters with scalar types to resource classes.

Once this is released, hopefully in the next release of the client library, you will be able to keep calling, for instance, setName as you currently do, passing a StringValue object, but you will also have convenience functions like setNameValue, that will allow you to pass a string.

In the same way, you will have a function called getNameValue that will allow you to get the name as a string, while still maintaining the existing getName returning a StringValue.

Raibaz commented 5 years ago

As of V1_2, the convenience functions mentioned in my previous comments have been implemented for all resources.