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

Added forward compatibility for PHP 8.1+ #13

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

Bilge
Copy link

@Bilge Bilge commented Mar 30, 2024

When the shell_exec call fails, it returns null. Passing null to trim(), which expects a string, is deprecated since PHP 8.1. I understand we're bundling PHP 8.0 at present, but this is just a forward compatibility change for when it's upgraded later.

As a bonus, removed some trailing whitespace.

Removed trailing whitespace from some files.
@cmb69
Copy link
Member

cmb69 commented Jul 15, 2024

Good catch, thank you! Indeed we should update the bundled PHP version (and likely some other stuff, such as the MinGW tools), but only after having tagged a new release.

@cmb69
Copy link
Member

cmb69 commented Jul 23, 2024

Passing null to trim(), which expects a string, is deprecated since PHP 8.1.

Right. I've seen that same trim(shell_exec(…)) call a couple of lines below, and that would need to be fixed as well (to be on the safe side for PHP 9). And then I noticed that Config::guessCurrentBranchName() returns ?string, but the result is passed to Config::setCurrentBranchName() which expects string here:

$branch = self::guessCurrentBranchName();
self::setCurrentBranchName($branch);

That's probably even worse, since that fatals, if there is no git.exe in the PATH, and there would be something wrong with php_version.h.

I expect many more of such issues, and I don't think addressing this piecemeal makes much sense. Maybe we should employ some static analysis tool, but which one (PHPStan, Psalm, or something else)? Any preferences?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants