-
Notifications
You must be signed in to change notification settings - Fork 39
Implement new PHP features up to 8.0 #119
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
Conversation
What's the idea of these changes? |
Stricter typing I imagine, I'll review it. |
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.
In depth review as requested!
Mostly nitpicks except the ones where you change calls to methods where we originally passed null
. PHPStan does not agree but the calls are actually correct.
src/Codeception/Module/Yii2.php
Outdated
'cookie' => $_COOKIE, | ||
'session' => $_SESSION, |
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.
Are we sure these global variables are set?
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.
if I go for:
'cookie' => $_COOKIE ?? [],
I get this warning:
phpstan: Variable $_COOKIE on left side of ?? always exists and is not nullable
In the case of $_SESSION
I don't get this error, it appears that there are cases in which it may be undefined.
so I guess it should look like:
'cookie' => $_COOKIE,
'session' => $_SESSION ?? [],
shouldn't it?
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.
I think it's a bug in phpstan. I'd just ignore the phpstan message tbh.
src/Codeception/Module/Yii2.php
Outdated
'cookie' => $_COOKIE, | ||
'session' => $_SESSION, |
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.
'cookie' => $_COOKIE, | |
'session' => $_SESSION, | |
'cookie' => $_COOKIE ?? [], | |
'session' => $_SESSION ?? [], |
🎉 This PR is included in version 2.0.0 🎉 The release is available on GitHub release Your semantic-release bot 📦🚀 |
I am not a regular user of Yii, please review critically.