DigitalOceanPHP / Client

DigitalOcean API v2 client for PHP
MIT License
710 stars 205 forks source link

`Entity\Droplet::tags` incorrectly type-hinted as `Tag[]` #296

Closed webignition closed 2 years ago

webignition commented 2 years ago

Entity\Droplet::tags is type-hinted as Tag[], indicating an array of Entity\Tag.

When using Api\Droplet to retrieve an entity, I can see that the tags property contains an array of string not an array of Entity\Tag.

// $dropletApi is an instance of `Api\Droplet`
$droplet = $dropletApi->getById(274260949);
var_dump($droplet->tags);

// array(2) {
//   [0] =>
//   string(3) "one"
//   [1] =>
//   string(3) "two"
// }

I'm perfectly happy with Entity\Droplet::tags being an array of strings, it's nice and simple and works exactly how I need.

In cases were I need to pass $droplet->tags as an argument to a method, phpstan raises type-related errors when I attempt to treat the array items as being strings instead of Tag instances. Not a big deal but something that needs to be addressed.

I'm not sure what the type of items in the Entity\Droplet::tags array is intended to be so I can't be sure what the correct fix here would be.

Here are three possible fixes:

  1. Change the type hint on Entity\Droplet::tags from @var Tag[] to @var string[] to match the actual implementation
  2. Change Entity\Droplet::build to create an array of tags from the supplied parameters
  3. Change the type hint on Entity\Droplet::tags from @var Tag[] to @var Tag[]|string[] to match the the intended or actual implementation

Fix number 1 changes the type-hint to match what is actually happening. I prefer this. I can see the potential for causing problems with code expecting Entity\Dropet::tags to provide an array of Entity\Tag, but since this is not the case I can't imagine any such code existing.

Fix number 2 changes the implementation to match the type hint. This feels problematic to me as I'd expect it to break any existing code that treats Entity\Dropet::tag items as strings.

Fix number 3 is a sort-of middle ground. This allows Entity\Droplet::tags items to be operated on as if they were either Entity\Tag instance or strings. This may prove useful if the intent all along was for an array of Entity\Tag to be the case.

I'm happy to create a PR to fix once I know what the preferred fix is.

In the meantime, feel free to file this at the lowest possible priority there is!

GrahamCampbell commented 2 years ago

Thanks. I've fixed the typo in the phpdoc.

webignition commented 2 years ago

:+1: thanks!