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
Upgrade to PHP 7.2+ #10343
Upgrade to PHP 7.2+ #10343
Conversation
composer.lockPackage changes
Dev Package changes
Settings · Docs · Powered by Private Packagist |
The composer.lock diff comment has been updated to reflect new changes in this PR. |
You need to replace |
556046b
to
ec72386
Compare
The composer.lock diff comment has been updated to reflect new changes in this PR. |
832fb3e
to
ca446b5
Compare
The composer.lock diff comment has been updated to reflect new changes in this PR. |
you might do the phpunit upgrade using https://github.com/rectorphp/rector-phpunit |
The composer.lock diff comment has been updated to reflect new changes in this PR. |
9b99079
to
282d2fb
Compare
fd50cbf
to
bf23b35
Compare
The composer.lock diff comment has been updated to reflect new changes in this PR. |
f341ab4
to
30eb6d4
Compare
OK I think it's usable now, still some PHPStan stuff that could be looked at in the 8.1 build, but otherwise I'll add them to the baseline before merging. |
// @phpstan-ignore-next-line | ||
$proc = new Process($cmd, $this->testDir, $env, null, 300); | ||
} | ||
$proc = Process::fromShellCommandline(escapeshellcmd(PHP_BINARY).' '.escapeshellarg(self::$pharPath).' --no-ansi '.$testData['RUN'], $this->testDir, $env, null, 300); |
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.
why not using the array (as done before, and as described in the TODO), which avoids having to manually escape things ?
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.
Because the run command from test data is a command that can contain multiple args/params etc so that needs to be parsed as a command-line and not escaped as a single arg
5926dfb
to
965a427
Compare
@@ -226,7 +227,7 @@ private static function findClasses($path) | |||
$message = 'File at "%s" does not exist, check your classmap definitions'; | |||
} elseif (!Filesystem::isReadable($path)) { | |||
$message = 'File at "%s" is not readable, check its permissions'; | |||
} elseif ('' === trim(file_get_contents($path))) { | |||
} elseif ('' === trim((string) file_get_contents($path))) { | |||
// The input file was really empty and thus contains no classes | |||
return array(); |
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.
Accidental leftover array() syntax?
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 did not and will not upgrade all array() into []. That's pointless IMO as long as we support 2.2 as LTS. I don't intend on writing any new array()
though.
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 Love this choice, because it keeps the line-edit count lower. There is nothing to be debated about that which has not changed.
Slightly naughty IMO to give feedback on a line not changed where there is no syntax error or behavior change.
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.
Was not meant to be "naughty" :)
I saw their tweet about the update and how they won't miss the old "array()" syntax, so I was only trying to help
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 was linked the tweet too. They may not have meant this, but I thought I'd give an outsider perspective on why I Love that they left some cases like empty case; things that don't need to change as part of this already huge PR.
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.
@Seldaek but would you accept a PR for array()
=> []
?
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.
Asi wrote above already..
I did not and will not upgrade all array() into []. That's pointless IMO as long as we support 2.2 as LTS.
So no please don't waste your time on a PR, I have no interest in creating tons of merge conflicts for my future self for such little syntax gains.
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.
Okay, i thought YOU don't want to waste time for that.
So i asked to help you, i don't wanted to bother you :-)
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.
the wasted time is not about doing the change (that's automated with php-cs-fixer). It is about resolving conflicts when merging 2.2.x into master each time the diff between the branches involve either a line containing an array or one of its sibling lines.
This gets the whole build system and dependencies up to date for the new PHP 7.2+ requirement
If you're relying on snapshots and using some legacy PHP version (<7.2.5), please see #10341
Fixes #10322
Fixes #10376
Fixes #10014