chore: [PHP] fix phpdoc for MapField keys

[bshaffer]

object is not a valid offset type. Anything other than string|int is not a valid offset type. bool may be added, but it's not a true offset because it will be cast to an int if used:

$a = [false => '1' , true => '2'];
$a[0] = '3';
$a[1] = '4';
var_dump($a);
// array(2) {
//   [0]=>
//   string(1) "3"
//   [1]=>
//   string(1) "4"
// }

[haberman]

If we remove bool, will PHP users get an error if they write something like this?

protobuf/php/tests/MapFieldTest.php

Lines 301 to 304 in 020e4e3

	$arr = new MapField(GPBType::BOOL, GPBType::BOOL);
	// Test boolean.
	$arr[True] = True;

I don't believe we have PHP 8.1 testing set up in our CI yet, so I'm a little bit nervous merging anything that could throw errors on 8.1 only.

[bshaffer]

Hey @acozzette! This is a good question. The user would never receive an error from a PHPDoc change. The only thing that would be effected here is a linter.

As PHP arrays do accept bools as array keys, this parameter is okay to remain bool. Although it's worth mentioning that once set, the bools are casted to ints, so this class would never return a bool. This PR changes the params though, and not the returns, which I believe should actually be reversed, so I'll look into it.

[haberman]

We are removing bool here. Will this cause type checking errors when a user writes $arr[True]?

[bshaffer]

Circling back to this! Sorry for the delay.

The param type here should be the same as the other @param $key types (for instance, in offsetSet) as it's the same array operation for all of them (php primitive array access). So we can either (1) update the rest of them to be int|bool|string, or (2) update this one to be int|string.

Advantages of (1): It's technically correct (PHP allows it), and it's "safer" (opening up the types instead of restricting them), as restricting this type could lead to static analysis errors in existing code (Note: As this is static analysis, it isn't BC-breaking, as running code would remain unaffected).

Advantages of (2): PHP is moving towards stricter types, and when a bool is used as a PHP array key, it is casted into an int (0 for false and 1 for true). So while it's accepted, it isn't truly a valid array key type. If we document this as int|string, it's better practice, and also reflects what we would likely do once this library becomes PHP 8.0+ and we use union types.

Whichever we decide is okay with me! I am happy to change all of them to int|bool|string, as it would be the more conservative move!

[haberman]

As PHP arrays do accept bools as array keys, this parameter is okay to remain bool.

I am referring to the case where this PR removes bool (noted inline).

This PR changes the params though, and not the returns, which I believe should actually be reversed, so I'll look into it.

I took this to mean that the PR is in your court. Am I understanding correctly?

[bshaffer]

@haberman I believe this is ready for final review / merge, but I can update this if you'd feel more comfortable with the other approach (see comment reply above!)