googleads / googleads-php-lib

Google Ad Manager SOAP API Client Library for PHP
Apache License 2.0
656 stars 769 forks source link

Modifying Children in CriteriaSet array #428

Closed YOzaz closed 6 years ago

YOzaz commented 6 years ago

As properties have been closed to protected access, CriteriaSet array manipulation on LineItem becomes troublesome - especially, when it has nested levels (which is default). Consider this - we want to append new targeting criteria at the end of the last set:

/** @var \Google\AdsApi\Dfp\v201705\Targeting $targeting **/
$custom_targeting = $targeting->getCustomTargeting();   // object by reference: &CustomCriteriaSet
$first_level = $custom_targeting->getChildren();   // array: CustomCriteriaSet[]

// N.B. not the most effective way of doing this
$first_level_count = count( $first_level );
$second_level = $first_level[ $first_level_count - 1 ]->getChildren();    // array: CustomCriteriaSet[]
$second_level[] = new CustomCriteria( /** ... **/ );

// The Problem: We need to push targetings backwards, as 2 (!) arrays have changed
$first_level[ $first_level_count - 1 ]->setChildren( $second_level );
$custom_targeting->setChildren( $first_level );

Proposal: convert arrays to ArrayObjects: http://php.net/manual/en/class.arrayobject.php (or custom arrays with ArrayAccess) In that way, references will be carried over, and will allow direct manipulation. Note, this may be breaking change.

Theory tests for proof: http://sandbox.onlinephpfunctions.com/code/6b21b80d2a7f7340471377105b5420331b250074

thangduo commented 6 years ago

Hi Marijus,

Thanks for suggesting the object model to us. What is the impact of this issue to your project / business?

YOzaz commented 6 years ago

@thangduo we are running background checker on DFP API, which scans for incorrectly set up line items - and fixes them as required. Therefore manipulating with CriteriaSets is one of the common things. We'll get away by re-assigning arrays, but just I though the way it is possible to amend object properties - as PHP passes objects by reference by default (like InventoryTargting) - without re-setting them through setter, it should be possible with other properties as well. Not crucial, but would eliminate lots of unnecessary code imho.

thangduo commented 6 years ago

Do you expect to send back only the changes in the CriteriaSets? Or do you expect to send along with other corrections to the same line item?

YOzaz commented 6 years ago

To answer - we're doing more than that, e.g. pausing (or resuming) line item, changing target impressions goal, etc. So it's more like sending corrections to the same line item.

thangduo commented 6 years ago

Hi Marijus,

Thanks for your answer. We took your suggestion and will investigate different ways to improve the memory handling aspect of the CriteriaSet.

In the meantime, we will close this issue out for now as it doesn't have crucial impacts on your end. Please feel free to re-open this issue if it would become crucial for you.

Thanks, Thang Duong