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.

Sign up for GitHub

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

chore: [PHP] fix phpdoc for MapField keys #9536

Merged
merged 1 commit into from
May 6, 2022
Merged

chore: [PHP] fix phpdoc for MapField keys #9536

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 4 additions & 4 deletions php/src/Google/Protobuf/Internal/MapField.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -131,7 +131,7 @@ public function getLegacyValueClass()
*
* This will also be called for: $ele = $arr[$key]
*
* @param int|bool|string $key The key of the element to be fetched.
* @param int|string $key The key of the element to be fetched.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

@bshaffer bshaffer Apr 19, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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!

* @return object The stored element at given key.
* @throws \ErrorException Invalid type for index.
* @throws \ErrorException Non-existing index.
Expand All @@ -148,7 +148,7 @@ public function offsetGet($key)
*
* This will also be called for: $arr[$key] = $value
*
* @param object $key The key of the element to be fetched.
* @param int|string $key The key of the element to be fetched.
* @param object $value The element to be assigned.
* @return void
* @throws \ErrorException Invalid type for key.
Expand Down Expand Up @@ -211,7 +211,7 @@ public function offsetSet($key, $value)
*
* This will also be called for: unset($arr)
*
* @param object $key The key of the element to be removed.
* @param int|string $key The key of the element to be removed.
* @return void
* @throws \ErrorException Invalid type for key.
* @todo need to add return type void (require update php version to 7.1)
Expand All @@ -228,7 +228,7 @@ public function offsetUnset($key)
*
* This will also be called for: isset($arr)
*
* @param object $key The key of the element to be removed.
* @param int|string $key The key of the element to be removed.
* @return bool True if the element at the given key exists.
* @throws \ErrorException Invalid type for key.
*/
Expand Down