Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Unsetting properties? #426

Closed
YOzaz opened this issue Dec 15, 2017 · 8 comments
Closed

Unsetting properties? #426

YOzaz opened this issue Dec 15, 2017 · 8 comments
Assignees

Comments

@YOzaz
Copy link
Contributor

YOzaz commented Dec 15, 2017

Despite the fact that I hate your decision to close properties as protected - and leave communication through getters and setters - can you please shed the light for me, how can we unset certain properties?

E.g. I want to set advertiser ID for creative to null (as obviously, unset( $creative->advertiserId ) won't work).

$creative->setAdvertiserId(null);

But instead of unsetting or setting to null, it sets to zero - because:
https://github.com/googleads/googleads-php-lib/blob/master/src/Google/AdsApi/Dfp/v201705/Creative.php#L95

And then SOAP call fails.

And yes, PHP_INT_SIZE is 4 on my environment.

Help!

@fiboknacky fiboknacky self-assigned this Dec 15, 2017
@fiboknacky
Copy link
Member

Would not touching advertiser ID at all have the same effect?
In AdWords API, if you don't set attributes to any values, they will be set as null and ignored by the servers.
I expect this to be the same in DFP API.

Knack

@YOzaz
Copy link
Contributor Author

YOzaz commented Dec 15, 2017

@fiboknacky that is correct if you're creating objects from scratch. But in our case, we are duplicating (copying) various objects. Till this update, creative(s) duplicating was as easy as querying from API, unsetting ID, and calling createCreatives with same object; now, we'll either have to create lots of IF...ELSE checks and copy properties over, or call Reflection class and unmark ID property as protected (which is kind of hacky).

@YOzaz
Copy link
Contributor Author

YOzaz commented Dec 18, 2017

As a temporary workaround, we'll use this approach:

/** @var \Google\AdsApi\Dfp\v201705\Creative $creative */
$reflectionClass = new \ReflectionClass( get_class($creative) );
$idProperty = $reflectionClass->getProperty('id');
$idProperty->setAccessible( true );
$idProperty->setValue( $creative, null );
$idProperty->setAccessible( false );

But ideally, I'd suggest changing API and checking for value integrity:

    /**
     * @param int $id
     * @return \Google\AdsApi\Dfp\v201705\Creative
     */
    public function setId($id)
    {
      $this->id = ($id && PHP_INT_SIZE === 4)
          ? floatval($id) : $id;
      return $this;
    }

Or using your colleague's approach:
https://github.com/google/google-api-php-client#how-do-i-set-a-field-to-null

@fiboknacky fiboknacky assigned thangduo and unassigned fiboknacky Jan 9, 2018
@thangduo
Copy link
Contributor

thangduo commented Jan 9, 2018

This is a great feedback.

Unfortunately, this is not a high priority issue for us. And we will have to close it as Not Fixed for now.

Please reopen this issue if it becomes high priority for you. Otherwise, we will revisit it later this year.

@thangduo thangduo closed this as completed Jan 9, 2018
@thangduo
Copy link
Contributor

thangduo commented Jan 9, 2018

It looks like there are 2 aspects of this issue:

  1. You would like to set optional ID properties (setAdvertiserId) to null
  2. You would like to unset an arbitrary properties.

For (1), we can fix it as a bug in the next release.

For (2), we will continue to investigate the use case and work on it later.

@thangduo thangduo reopened this Jan 9, 2018
@YOzaz
Copy link
Contributor Author

YOzaz commented Jan 10, 2018

Thanks @thangduo for the feedback.
In general, setting values to null is pretty much the same like unsetting in this case, as your library filters them out from JSON request. So for my project, option (1) is fine 👍
It can be either value check in the function itself, or NULL_VALUE constant - we'll adopt as required.
Thank you!

@fiboknacky
Copy link
Member

Classes are now generated to support setting null in v33.0.0.

@YOzaz
Copy link
Contributor Author

YOzaz commented Feb 14, 2018

Thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants