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

Jump to bottom

[VarDumper] Fix tests for PHP 8.1 #41701

Merged
merged 1 commit into from Jun 22, 2021
Merged

[VarDumper] Fix tests for PHP 8.1 #41701

merged 1 commit into from Jun 22, 2021

Conversation

alexandre-daubois
Copy link
Contributor

Q A
Branch? 4.4
Bug fix? yes
New feature? no
Deprecations? no
Tickets Part of #41552
License MIT
Doc PR N/A

@carsonbot carsonbot added this to the 4.4 milestone Jun 14, 2021
@alexandre-daubois alexandre-daubois force-pushed the fix-vardumper-tests branch 2 times, most recently from 9f41eb0 to 5db7f8b Compare June 14, 2021 14:18
@derrabus
Copy link
Member

Can you explain why the dumper behaves differently on PHP 8.1?

@stof
Copy link
Member

stof commented Jun 15, 2021

@derrabus looks like the behavior of ReflectionGenerator::getTrace() changed in PHP 8.1, returning some info when there was none before.

@alexandre-daubois
Copy link
Contributor Author

alexandre-daubois commented Jun 15, 2021 •
edited

@derrabus also, some other functions seem to have changed their behaviour. For example, when null, ReflectionParameter::getDefaultValue() returns null on PHP 8.0, and ReflectionException: Internal error: Failed to retrieve the default value on PHP 8.1. Searched why in recent PRs of https://github.com/php/php-src/, but couldn't find which commit is responsible of it.

@derrabus
Copy link
Member

@derrabus For example, when null, ReflectionParameter::getDefaultValue() returns null on PHP 8.0, and ReflectionException: Internal error: Failed to retrieve the default value on PHP 8.1.

I'd like to clarify first, if that change was intentional: https://bugs.php.net/bug.php?id=81147

@derrabus
Copy link
Member

The other changes look reasonable to me.

@derrabus
Copy link
Member

Okay, that was fast and we've got the answer we were looking for.

Just to confirm, this is indeed an intentional change to ensure that optional-before-required is consistently treated as a required parameter. Previously it was treated as such in nearly all situations, but there were loopholes. One, as you have discovered, is reflection. The other is named arguments, where it was possible to not specify such a parameter if later required parameters were specified.

Can you have a look at the Travis failure? It looks related.

@alexandre-daubois
Copy link
Contributor Author

Thank you so much for your investigation, super good to know! I'll definitely take a look at Travis tomorrow to finish this PR. 👍

@alexandre-daubois
Copy link
Contributor Author

alexandre-daubois commented Jun 17, 2021 •
edited

@derrabus done 👌 This failing build for 7.4 seems unrelated to this PR.

I'm not sure I understand the difference between "Tests / Integration (x.x)" and Travis CI tests ?

@derrabus
Copy link
Member

Thank you @alexandre-daubois.

@derrabus derrabus merged commit 0c1343f into symfony:4.4 Jun 22, 2021
@derrabus derrabus mentioned this pull request Jun 22, 2021
Closed
This was referenced Jun 30, 2021
Merged
Merged
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@stof stof stof left review comments

@Nyholm Nyholm Nyholm left review comments

@nicolas-grekas nicolas-grekas nicolas-grekas approved these changes

@derrabus derrabus derrabus approved these changes

Assignees
No one assigned
Labels
Bug Status: Reviewed VarDumper
Projects
None yet
Milestone
4.4
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@alexandre-daubois @derrabus @stof @nicolas-grekas @Nyholm @carsonbot