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

Upgrade to PHP 7.2+ #10343

Merged
merged 17 commits into from Jan 3, 2022
Merged

Upgrade to PHP 7.2+ #10343

merged 17 commits into from Jan 3, 2022

Conversation

Seldaek
Copy link
Member

@Seldaek Seldaek commented Dec 8, 2021

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

@Seldaek Seldaek added this to the 2.3 milestone Dec 8, 2021
@private-packagist
Copy link

@private-packagist private-packagist bot commented Dec 8, 2021

composer.lock

Package changes

Package Operation From To Changes
psr/container add - 1.1.1 view code
symfony/deprecation-contracts add - v2.5.0 view code
symfony/polyfill-intl-grapheme add - v1.23.1 view code
symfony/polyfill-intl-normalizer add - v1.23.0 view code
symfony/polyfill-php73 add - v1.23.0 view code
symfony/polyfill-php80 add - v1.23.1 view code
symfony/service-contracts add - v2.5.0 view code
symfony/string add - v5.4.0 view code
react/promise upgrade v1.2.1 v2.8.0 diff
seld/phar-utils upgrade 1.1.2 1.2.0 diff
symfony/console upgrade v2.8.52 v5.4.1 diff
symfony/filesystem upgrade v2.8.52 v5.4.0 diff
symfony/finder upgrade v2.8.52 v5.4.0 diff
symfony/polyfill-ctype upgrade v1.19.0 v1.23.0 diff
symfony/polyfill-mbstring upgrade v1.19.0 v1.23.1 diff
symfony/process upgrade v2.8.52 v5.4.0 diff
symfony/debug remove v2.8.52 - -

Dev Package changes

Package Operation From To Changes
phpstan/phpstan add - 1.2.0 view code
phpstan/phpstan-deprecation-rules add - 1.0.0 view code
phpstan/phpstan-phpunit add - 1.0.0 view code
phpstan/phpstan-strict-rules add - 1.1.0 view code
symfony/phpunit-bridge upgrade v4.2.12 v6.0.0 diff
doctrine/instantiator remove 1.0.5 - -
phpdocumentor/reflection-docblock remove 2.0.5 - -
phpspec/prophecy remove v1.10.3 - -
sebastian/comparator remove 1.2.4 - -
sebastian/diff remove 1.4.3 - -
sebastian/exporter remove 2.0.0 - -
sebastian/recursion-context remove 2.0.0 - -

Settings · Docs · Powered by Private Packagist

@private-packagist
Copy link

@private-packagist private-packagist bot commented Dec 8, 2021

The composer.lock diff comment has been updated to reflect new changes in this PR.

@stof
Copy link
Contributor

@stof stof commented Dec 8, 2021

You need to replace PHPUnit_Framework_MockObject_MockObject with PHPUnit\Framework\MockObject\MockObject in all the phpdoc of the testsuite

@Seldaek Seldaek force-pushed the drop_old_php branch 2 times, most recently from 556046b to ec72386 Compare Dec 9, 2021
@private-packagist
Copy link

@private-packagist private-packagist bot commented Dec 9, 2021

The composer.lock diff comment has been updated to reflect new changes in this PR.

@Seldaek Seldaek force-pushed the drop_old_php branch 2 times, most recently from 832fb3e to ca446b5 Compare Dec 9, 2021
@private-packagist
Copy link

@private-packagist private-packagist bot commented Dec 9, 2021

The composer.lock diff comment has been updated to reflect new changes in this PR.

@staabm
Copy link
Contributor

@staabm staabm commented Dec 9, 2021

You need to replace PHPUnit_Framework_MockObject_MockObject with PHPUnit\Framework\MockObject\MockObject in all the phpdoc of the testsuite

you might do the phpunit upgrade using https://github.com/rectorphp/rector-phpunit

@private-packagist
Copy link

@private-packagist private-packagist bot commented Dec 9, 2021

The composer.lock diff comment has been updated to reflect new changes in this PR.

@private-packagist
Copy link

@private-packagist private-packagist bot commented Dec 10, 2021

The composer.lock diff comment has been updated to reflect new changes in this PR.

@Seldaek Seldaek force-pushed the drop_old_php branch 4 times, most recently from f341ab4 to 30eb6d4 Compare Dec 10, 2021
@Seldaek
Copy link
Member Author

@Seldaek Seldaek commented Dec 10, 2021

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);
Copy link
Contributor

@stof stof Dec 21, 2021

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 ?

Copy link
Member Author

@Seldaek Seldaek Dec 21, 2021

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

@Seldaek Seldaek force-pushed the drop_old_php branch 2 times, most recently from 5926dfb to 965a427 Compare Dec 31, 2021
@Seldaek Seldaek merged commit efa08e7 into composer:main Jan 3, 2022
17 checks passed
@Seldaek Seldaek deleted the drop_old_php branch Jan 3, 2022
@@ -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();

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Accidental leftover array() syntax?

Copy link
Member Author

@Seldaek Seldaek Jan 3, 2022

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.

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.

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

Copy link

@Lewiscowles1986 Lewiscowles1986 Jan 3, 2022

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.

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() => [] ?

Copy link
Member Author

@Seldaek Seldaek Jan 4, 2022

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.

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 :-)

Copy link
Contributor

@stof stof Jan 5, 2022

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment