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

[FTP-1778][PHP81] Fix PHP 8.1 types #2

Merged
merged 2 commits into from
Jan 23, 2023
Merged

[FTP-1778][PHP81] Fix PHP 8.1 types #2

merged 2 commits into from
Jan 23, 2023

Conversation

costash
Copy link
Member

@costash costash commented Jan 23, 2023

@costash costash requested a review from a team as a code owner January 23, 2023 16:39
return $this;
}

protected function hacklib_isImmutable() {
protected function hacklib_isImmutable()
Copy link

Choose a reason for hiding this comment

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

No return type on this and immutable()? Same on the other hacklib_imm ones.

Copy link
Member Author

Choose a reason for hiding this comment

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

While we can add it, it's not strictly required for PHP 8.1. I wanna change as little as it's strictly required here. We're slowly deprecating thrift, so don't wanna spend too much effort here.

@@ -24,23 +24,27 @@ trait HACKLIB_ImmVectorLike {
/**
* identical to at, implemented for ArrayAccess
*/
public function offsetGet($offset) {
public function offsetGet($offset)
Copy link

Choose a reason for hiding this comment

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

Missing return type?

Copy link
Member Author

Choose a reason for hiding this comment

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

This is mixed, which is a keyword added only in php 8. We can add attribute #ReturnTypeWillChange actually.

$this->remove($offset);
}

/**
* Adds an element to this Set and returns itself. "$c->add($v)" is
* equivalent to "$c[] = $v" (except that add() returns the Set).
*/
public function add($v) {
public function add($v)
Copy link

Choose a reason for hiding this comment

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

Missing return type?

Copy link
Member Author

Choose a reason for hiding this comment

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

It's not defined in the interface. I won't add what's not strictly necessary for PHP 8.1

@@ -24,26 +24,29 @@ trait HACKLIB_VectorLike {
/**
* identical to at, implemented for ArrayAccess
*/
public function &offsetGet($offset) {
public function &offsetGet($offset)
Copy link

Choose a reason for hiding this comment

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

Missing return type?

Copy link
Member Author

Choose a reason for hiding this comment

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

Added the ReturnTypeWillChange attribute.

Copy link

@pboos pboos left a comment

Choose a reason for hiding this comment

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

Besides a few missing types (not sure if necessary), all is good.

@costash costash merged commit 7ff9ae3 into master Jan 23, 2023
@costash costash deleted the FTP-1778-php81 branch January 23, 2023 22:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants