-
Notifications
You must be signed in to change notification settings - Fork 240
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 deprecated nullable areguments #624 #625
base: master
Are you sure you want to change the base?
Conversation
156b48c
to
af0c870
Compare
$php = $this->generateTypes($argument->getTypeNode()); | ||
$types = $argument->getTypeNode()->getTypes(); | ||
$null = $argument->isOptional() && $argument->getDefault() === NULL && \count($types) === 1 && $types[0] !== 'mixed'; | ||
$php = $this->generateTypes($argument->getTypeNode(), $null); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it needs polishing
af0c870
to
0f0f809
Compare
Would be lovely if this PR could get finished off and merged (and released!) as anyone who is currently using PHPUnit 9 to assess their own project's PHP 8.4 readiness is faced with a wall of deprecation notices coming from Prophecy. @andypost Anything you need help with ? Want me to review something ? |
@jrfnl thank, please help me to improve the code as I don't like amount of |
Personally I do use it as a part of patch for Drupal compatibility issue |
To make things easier to review, I'd like to see this PR split in 2:
|
I did split code-style related fixes into #630 |
657ae94
to
210c459
Compare
Please rebase this branch now that the other PR doing the CS changes on the codebase is merged. |
@andypost what is the case which requires the change in this PR ? Is it for any nullable optional argument, or only for cases where the original code is relying on an implicit nullable type ? |
210c459
to
6a64111
Compare
@stof example failures are The test case code public function testUnknownExtension(): void {
$module_extension_list = $this->prophesize(ModuleExtensionList::class);
$profile_extension_list = $this->prophesize(ProfileExtensionList::class);
$theme_extension_list = $this->prophesize(ThemeExtensionList::class);
$theme_engine_extension_list = $this->prophesize(ThemeEngineExtensionList::class);
$resolver = new ExtensionPathResolver(
$module_extension_list->reveal(),
$profile_extension_list->reveal(),
$theme_extension_list->reveal(),
$theme_engine_extension_list->reveal()
); The mocked class
produce following output
|
As I get it... when class with required typed constructor argument is mocked, prophecy making argument nullable but somehow without leading |
@andypost can you show the class definition of |
Sure, here's a link https://git.drupalcode.org/project/drupal/-/blob/e5653c11cd20e66831aca7e7a7568717baff5d9d/core/lib/Drupal/Core/Extension/ModuleExtensionList.php#L20 and I updated comment above |
6a64111
to
07be4a2
Compare
Fixed nullable types in spec and now it pass |
@andypost if you update the |
@stof The |
The error is the same
|
{ | ||
if (!$typeNode->getTypes()) { | ||
return ''; | ||
} | ||
|
||
// When we require PHP 8 we can stop generating ?foo nullables and remove this first block | ||
if ($typeNode->canUseNullShorthand()) { | ||
if ($typeNode->canUseNullShorthand() || $nullable) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this place exactly is not working, it just making all arguments nullable (optional) but if the type defined for one then no ?
is added
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Specifically for case when type is mixed
and no null
(PHP 8.2) defined in intersection (PHP 8.1)
$nullable = FALSE; | ||
} | ||
} | ||
$php = $this->generateTypes($argument->getTypeNode(), $nullable); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
new argument required because $argument->getTypeNode()
has no access to default value and optional property
OK, apparently, those errors come from the DisableConstructorPatch which aims at making all constructor arguments optional. the right fix is to update the implementation of that class patch so that it makes all arguments nullable as well (by updating the argument type) instead of adding a |
07be4a2
to
a3f0761
Compare
@stof I can't make such big refactoring and need help to cover all cases |
a3f0761
to
ad41b07
Compare
ad41b07
to
d0aa1d9
Compare
Related to #624