Skip to content

Declare tentative return types for Zend #7251

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

Jump to bottom
Merged
kocsismate merged 5 commits into from
Jul 19, 2021
Merged

Declare tentative return types for Zend #7251

kocsismate merged 5 commits into from
Jul 19, 2021

Conversation

@kocsismate
Copy link
Member

@kocsismate kocsismate commented Jul 16, 2021

No description provided.

@kocsismate kocsismate force-pushed the tentative-return-type-24 branch from d42e27f to c693213 Compare July 16, 2021 13:38
@kocsismate kocsismate added the Tentative returns Temporary label for tentative return follow ups label Jul 16, 2021
@kocsismate kocsismate added this to the PHP 8.1 milestone Jul 16, 2021
kocsismate
kocsismate commented Jul 16, 2021
View reviewed changes
nikic
nikic reviewed Jul 16, 2021
View reviewed changes
nikic
nikic reviewed Jul 16, 2021
View reviewed changes
nikic
nikic reviewed Jul 16, 2021
View reviewed changes
@kocsismate kocsismate force-pushed the tentative-return-type-24 branch from c693213 to 54085eb Compare July 16, 2021 14:50
kocsismate
kocsismate commented Jul 16, 2021
View reviewed changes
kocsismate
kocsismate commented Jul 16, 2021
View reviewed changes
ext/phar/tests/bug45218_SLOWTEST.phpt Outdated
var $a;
var $count = 1;

#[ReturnTypeWillChange]
Copy link
Member Author

@kocsismate kocsismate Jul 16, 2021

Choose a reason for hiding this comment

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

Something weird is going on here, the test timed out if I made this method and rewind void

Copy link
Member

@nikic nikic Jul 19, 2021

Choose a reason for hiding this comment

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

Works fine for me.

@kocsismate kocsismate force-pushed the tentative-return-type-24 branch from deca466 to 3f5a536 Compare July 16, 2021 16:01
@kocsismate
Copy link
Member Author

kocsismate commented Jul 16, 2021 •
edited
Loading

Some tests fail when opcache is enabled: Zend/tests/return_types/generators003.phpt.

php: /home/vsts/work/1/s/ext/opcache/ZendAccelerator.c:2414: zend_accel_inheritance_cache_add: Assertion `zend_accel_in_shm(dep_name)' failed.
Aborted (core dumped)

Termsig=6

@codecov-commenter

This comment has been minimized.

Sign in to view
nikic added a commit that referenced this pull request Jul 19, 2021
@nikic
Don't track internal class dependencies
a837d35 
Subtyping relationships established on internal classes are always
going to hold (if we ignore Windows), so there is no need to
explicitly track them.

This fixes an assertion failure in GH-7251.
@nikic
Copy link
Member

nikic commented Jul 19, 2021

Assertion failure should be fixed by a837d35.

@nikic nikic force-pushed the tentative-return-type-24 branch from 9271ab2 to 22a8a08 Compare July 19, 2021 08:56
nikic
nikic approved these changes Jul 19, 2021
View reviewed changes
Copy link
Member

@nikic nikic left a comment

Choose a reason for hiding this comment

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

LG assuming CI happy

@nikic
Copy link
Member

nikic commented Jul 19, 2021

CI failure unrelated.

@kocsismate kocsismate merged commit 75a678a into php:master Jul 19, 2021
@kocsismate kocsismate deleted the tentative-return-type-24 branch July 19, 2021 11:44
@shqking
Copy link
Contributor

shqking commented Jul 20, 2021

Hi, I noticed that the community_job on CI failed after this commit.

As shown in https://dev.azure.com/phpazuredevops/PHP/_build/results?buildId=18723&view=logs&j=55323bc0-ded7-5cf3-3d38-be3edc0baeb7&t=639abae4-2042-5d50-76a0-d1b638be5f29&l=107, here is the error message:

Fatal error: During inheritance of ArrayAccess: Uncaught Return type of PHPUnit\TestFixture\SampleArrayAccess::offsetExists($offset) should either be compatible with ArrayAccess::offsetExists(mixed $offset): bool, or the #[ReturnTypeWillChange] attribute should be used to temporarily suppress the notice

As I'm not familiar with this feature, I was wondering how to bypass this failure?
Do we need make a patch to the PHPunit benchmark? Thanks.

@kocsismate
Copy link
Member Author

kocsismate commented Jul 20, 2021

Thanks for the report! I'm going to provide a fix for these errors today :)

@shqking
Copy link
Contributor

shqking commented Jul 21, 2021

I believe you have fixed the failure for PHPUnit. As I checked the latest test result of community job, PHPUnit can pass now.

But I found the similar failure occurred for "lavarel" project. See https://dev.azure.com/phpazuredevops/PHP/_build/results?buildId=18795&view=logs&j=55323bc0-ded7-5cf3-3d38-be3edc0baeb7&t=34be0e1e-9444-50b4-935f-27096e97282a&l=448

Could you help to check this? Thanks.

@kocsismate
Copy link
Member Author

kocsismate commented Jul 21, 2021

I believe you have fixed the failure for PHPUnit. As I checked the latest test result of community job, PHPUnit can pass now.

No, it was Sebastian himself :) I noticed that he added the necessary suppressions yesterday (e.g. https://github.com/sebastianbergmann/phpunit/blob/master/tests/_files/SampleArrayAccess.php#L33), so I went directly to the Laravel failures: #7290

@shqking
Copy link
Contributor

shqking commented Jul 21, 2021

Thanks for the information.

@thekid thekid mentioned this pull request Jul 25, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@nikic nikic nikic approved these changes

Assignees

No one assigned

Labels

Tentative returns Temporary label for tentative return follow ups

Projects

None yet

Milestone

PHP 8.1

Development

Successfully merging this pull request may close these issues.

4 participants

@kocsismate @codecov-commenter @nikic @shqking