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

Fix generating signatures with nullable unions #4451

Closed
wants to merge 1 commit into from

Conversation

nicolas-grekas
Copy link
Contributor

Found while debugging the failure on https://github.com/composer/composer/runs/1101133265

I'd happily accept help to finish the PR (add tests especially).

@codecov
Copy link

codecov bot commented Sep 11, 2020

Codecov Report

Merging #4451 into 9.3 will not change coverage.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff            @@
##                9.3    #4451   +/-   ##
=========================================
  Coverage     85.16%   85.16%           
- Complexity     4511     4512    +1     
=========================================
  Files           243      243           
  Lines         12363    12363           
=========================================
  Hits          10529    10529           
  Misses         1834     1834           
Impacted Files Coverage Δ Complexity Δ
src/Framework/MockObject/MockMethod.php 98.60% <100.00%> (ø) 50.00 <0.00> (+1.00)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update fa50b89...3101432. Read the comment docs.

@sebastianbergmann sebastianbergmann added feature/test-doubles Stubs and Mock Objects type/bug Something is broken labels Sep 11, 2020
@sebastianbergmann sebastianbergmann self-assigned this Sep 11, 2020
@sebastianbergmann
Copy link
Owner

Can you provide a minimal, self-contained, reproducing example that shows the problem you want to solve? Thanks! I will then use your example to create a test case for PHPUnit's test double code generator.

@Seldaek
Copy link
Contributor

Seldaek commented Sep 11, 2020

Essentially I think what triggers it, is mocking the ZipArchive class, namely the extractTo method.

I believe this is the code triggering the issue in our test suite (probably the second expects is enough..):

        $zipArchive = $this->getMockBuilder('ZipArchive')->getMock();
        $zipArchive->expects($this->any())
            ->method('open')
            ->will($this->returnValue(true));
        $zipArchive->expects($this->any())
            ->method('extractTo')
            ->will($this->returnValue(false));

@nicolas-grekas said it was generating a mock with signature public function extractTo(string $pathto, ?array|string|null $files = NULL), and the ? is superfluous here.

@sebastianbergmann
Copy link
Owner

Thanks, @Seldaek.

@sebastianbergmann
Copy link
Owner

Merged manually, thanks.

@Seldaek
Copy link
Contributor

Seldaek commented Sep 11, 2020

Thanks for the quick merge & release. I can confirm it's working well now. Down to 1 actual failure and then we're green on php 8 :)

@sebastianbergmann
Copy link
Owner

Glad I could help.

@nicolas-grekas nicolas-grekas deleted the patch-1 branch September 11, 2020 13:32
@ste93cry
Copy link
Contributor

ste93cry commented Dec 9, 2021

@sebastianbergmann would you be favourable to release this fix for version 8.5 as well? In getsentry/sentry-symfony we're still using that version in the build of the lowest deps due to the need of supporting PHP 7.2 and when installing it alongside with Symfony 6.x our tests fail 😢

@sebastianbergmann
Copy link
Owner

PHPUnit 8.5 must work for PHP code compatible with PHP 7.2 on PHP 8. Are you saying that this is not the case? I do have to say, though, that I am confused why "installing it alongside with Symfony" should have an effect on how PHPUnit behaves. PHPUnit does not depend on any Symfony component. Or is Symfony's PHPUnit bridge causing your problem?

@nicolas-grekas
Copy link
Contributor Author

nicolas-grekas commented Dec 10, 2021

When running "old" code base (like phpunit 8.5 in this case), one should expect that some deprecations are triggered. Either by the bridge, or the latest php, etc
Check the doc of the bridge to silence some deprecations @ste93cry

@nicolas-grekas
Copy link
Contributor Author

( if that is related to the bridge because I also don't get the link with Symfony yet )

@Seldaek
Copy link
Contributor

Seldaek commented Dec 10, 2021

I did experience the same issue (I think) trying to make Composer tests pass with PHPUnit 8.5 on PHP 7.2 to 8.1 in composer/composer#10343

The problem I saw was that some Symfony 6 code broke PHPUnit MockObject when running the PHP 8.x build (with PHPUnit 8.5). I did see a backport PR which was closed by @sebastianbergmann so I gave up on that project and went back to using symfony/phpunit-bridge to run multiple PHPUnit versions based on PHP version, which is working well now.

@ste93cry
Copy link
Contributor

When running "old" code base (like phpunit 8.5 in this case), one should expect that some deprecations are triggered. Either by the bridge, or the latest php, etc

Sorry, I think that my message has been misinterpreted. Sentry needs to support PHP 7.2 as minimum version, which forces me to use PHPUnit 8.5 for the build that tests against the lowest version of the deps. Since this version of PHPUnit can be installed on PHP 8 and Symfony 6 supports only PHP 8, this combination downloads the Symfony Console component where the InputInterface interface uses a nullable union type as argument of one of its methods. When trying to mock such interface, I get a fatal error because the typehint gets transformed to ?$default = false which is an invalid syntax

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature/test-doubles Stubs and Mock Objects type/bug Something is broken
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants