googleapis / google-api-php-client-services

http://googleapis.github.io/google-api-php-client-services/
Apache License 2.0
1.23k stars 313 forks source link

Proper PHPDoc for nullable class properties and method returns #604

Open holtkamp opened 2 years ago

holtkamp commented 2 years ago

Related / follow-up of https://github.com/googleapis/google-api-php-client-services/issues/172 Is your feature request related to a problem? Please describe. Currently some classes have properties that

For example: https://github.com/googleapis/google-api-php-client-services/blob/dbd60410b289374a44fe91f3556d463b55c10246/src/Calendar/EventDateTime.php#L23-L25

And https://github.com/googleapis/google-api-php-client-services/blob/dbd60410b289374a44fe91f3556d463b55c10246/src/Calendar/EventDateTime.php#L42-L48

When using static analysis using (for example) PHPStan, on code like this:

//All-day event has the date property set
if (is_string( $calendarEvent->getStart()->date)) {
   //Process all-day event
}
} elseif (is_string( $calendarEvent->getStart()->dateTime)) {
   //event is not all-day-even
}

This will result in an error like:

Elseif branch is unreachable because previous condition is always true.  

Describe the solution you'd like It would be better to use PHPDoc to indicate that the property can be null, for example

  /**
   * @var string|null
   */
  public $date;

  /**
   * @return string|null
   */
  public function getDate()
  {
    return $this->date;
  }

Describe alternatives you've considered For PHP versions that support typed properties and return types, the PHPDoc can be omitted:

  public ?string $date = null;
  public function getDate() : ?string
  {
    return $this->date;
  }

But this library supports PHP >=5.6, so this is probably not an option yet: https://github.com/googleapis/google-api-php-client-services/blob/main/composer.json#L9

bshaffer commented 2 years ago

This is related to https://github.com/googleapis/google-api-php-client-services/pull/620

I am not sure how this will work across all APIs - what in the discovery doc decides if a field is nullable? There is a "required" keyword, but it's for the Resource classes making the requests, and not for the objects which are sent in the request.

So I believe the answer is that simply adding |null would be sufficient.

razvanphp commented 8 months ago

This library now has minimum PHP version support for PHP 7.4 (#2854), so nullable return types can be added. Can we contribute those kind of changes @bshaffer, or is the repo completely auto generated?