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

Add PHP 8 support #446

Merged
merged 21 commits into from
Dec 8, 2020
Merged

Add PHP 8 support #446

merged 21 commits into from
Dec 8, 2020

Conversation

smoench
Copy link
Contributor

@smoench smoench commented Nov 30, 2020

Waiting for:

@smoench
Copy link
Contributor Author

smoench commented Nov 30, 2020

@theofidry @villfa It seems e2e_020 fails with composer v2. With composer v1 it works but composer v1 is not compatible with PHP 8.0 yet. Can you have look?

composer --working-dir=fixtures/set020-infection install
 146/146 [▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓] 100%Installing dependencies from lock file (including require-dev)
Verifying lock file contents can be installed on current platform.
Package operations: 29 installs, 0 updates, 0 removals
  - Downloading infection/extension-installer (0.1.1)
  - Downloading thecodingmachine/safe (v1.3.3)
  - Downloading symfony/process (v5.2.0)
  - Downloading symfony/finder (v5.2.0)
  - Downloading symfony/filesystem (v5.2.0)
  - Downloading sanmai/pipeline (v5.1.0)
  - Downloading ondram/ci-detector (3.5.1)
  - Downloading infection/include-interceptor (0.2.4)
  - Downloading infection/abstract-testframework-adapter (0.3.1)
  - Downloading infection/infection (0.18.2)
  0/10 [>---------------------------]   0%
  1/10 [==>-------------------------]  10%
  2/10 [=====>----------------------]  20%
  3/10 [========>-------------------]  30%
  4/10 [===========>----------------]  40%
  6/10 [================>-----------]  60%
  7/10 [===================>--------]  70%
  8/10 [======================>-----]  80%
  9/10 [=========================>--]  90%
 10/10 [============================] 100%  - Installing composer/package-versions-deprecated (1.11.99.1): Extracting archive
  - Installing infection/extension-installer (0.1.1): Extracting archive
  - Installing symfony/polyfill-ctype (v1.20.0): Extracting archive
  - Installing webmozart/assert (1.9.1): Extracting archive
  - Installing webmozart/path-util (2.3.0): Extracting archive
  - Installing thecodingmachine/safe (v1.3.3): Extracting archive
  - Installing symfony/polyfill-php80 (v1.20.0): Extracting archive
  - Installing symfony/process (v5.2.0): Extracting archive
  - Installing symfony/finder (v5.2.0): Extracting archive
  - Installing symfony/filesystem (v5.2.0): Extracting archive
  - Installing symfony/polyfill-mbstring (v1.20.0): Extracting archive
  - Installing symfony/polyfill-intl-normalizer (v1.20.0): Extracting archive
  - Installing symfony/polyfill-intl-grapheme (v1.20.0): Extracting archive
  - Installing symfony/string (v5.2.0): Extracting archive
  - Installing psr/container (1.0.0): Extracting archive
  - Installing symfony/service-contracts (v2.2.0): Extracting archive
 1213/1270 [▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓░░]  95%

[OK] Successfully prefixed 1270 files.

// Memory usage: 21.40MB (peak: 64.68MB), time: 11.17s

composer --working-dir=build/set020-infection dump-autoload
Generating autoload files
Generated autoload files

We generate the expected output file: we test that the scoping process

does not alter it

php fixtures/set020-infection/vendor/infection/infection/bin/infection
--coverage=dist/infection-coverage
--skip-initial-tests
--only-covered
--no-progress
1270/1270 [▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓] 100%PHP Fatal error: Uncaught OutOfBoundsException: Package "infection/infection" is not installed in /home/runner/work/php-scoper/php-scoper/vendor/composer/InstalledVersions.php:928
Stack trace:
#0 /home/runner/work/php-scoper/php-scoper/vendor/composer/package-versions-deprecated/src/PackageVersions/Versions.php(155): Composer\InstalledVersions::getPrettyVersion()
#1 /home/runner/work/php-scoper/php-scoper/fixtures/set020-infection/vendor/infection/infection/src/Console/Application.php(75): PackageVersions\Versions::getVersion()
#2 /home/runner/work/php-scoper/php-scoper/fixtures/set020-infection/vendor/infection/infection/bin/infection(83): Infection\Console\Application->__construct()
#3 {main}
thrown in /home/runner/work/php-scoper/php-scoper/vendor/composer/InstalledVersions.php on line 928
make: *** [e2e_020] Error 255
Makefile:204: recipe for target 'e2e_020' failed

@theofidry
Copy link
Member

Hm given how easy to is to upgrade to Composer 1 I wonder if it doesn't make sense to drop it completely...

@villfa
Copy link
Contributor

villfa commented Nov 30, 2020

Look here:

#0 /home/runner/work/php-scoper/php-scoper/vendor/composer/package-versions-deprecated/src/PackageVersions/Versions.php(155): Composer\InstalledVersions::getPrettyVersion()

This is not the right path. Instead it should be (notice build/set020-infection/) :

/home/runner/work/php-scoper/php-scoper/build/set020-infection/vendor/composer/package-versions-deprecated/src/PackageVersions/Versions.php

I don't know more for now.

EDIT:
By replacing php fixtures/set020-infection/vendor/infection/infection/bin/infection by cd fixtures/set020-infection && php vendor/infection/infection/bin/infection it works better.

EDIT2:
I confirm that the working directory used to run Infection matters: https://github.com/infection/infection/blob/1368b12acf0e9679deb5d371edf2d5ebf3c98e34/bin/infection#L78

@smoench
Copy link
Contributor Author

smoench commented Dec 1, 2020

Thanks. Infection is running again but I'm not sure if it is correct.

@smoench
Copy link
Contributor Author

smoench commented Dec 1, 2020

I just parallelized the e2e tests in GitHub Actions 👀 (because composer v1 is not compatible with PHP 8.0)

@villfa
Copy link
Contributor

villfa commented Dec 1, 2020

(because composer v1 is not compatible with PHP 8.0)

FYI, it will be compatible from the next release: composer/composer#9523

.github/workflows/e2e.yml Outdated Show resolved Hide resolved
Co-authored-by: Fabien Villepinte <fabien.villepinte@gmail.com>
Makefile Outdated Show resolved Hide resolved
@smoench
Copy link
Contributor Author

smoench commented Dec 4, 2020

@villfa Can you have a look at e2e_027 again? Still failing. Thanks in advance

@villfa
Copy link
Contributor

villfa commented Dec 4, 2020

Can you have a look at e2e_027 again? Still failing. Thanks in advance

Yes, I will, but I can't tell you when exactly. Probably this weekend.

@villfa
Copy link
Contributor

villfa commented Dec 4, 2020

The remaining error is not directly related to PHP 8.

Here some scoped code (generated with PHP 7.3):

if (!\function_exists('_PhpScoperd4c93190f897\\str_before')) {
    /**
     * Get the portion of a string before a given value.
     *
     * @param  string  $subject
     * @param  string  $search
     * @return string
     */
    function str_before($subject, $search)
    {
        return \_PhpScoperd4c93190f897\Illuminate\Support\Str::before($subject, $search);
    }
}
if (!\function_exists('str_contains')) {
    /**
     * Determine if a given string contains a given substring.
     *
     * @param  string  $haystack
     * @param  string|array  $needles
     * @return bool
     */
    function str_contains($haystack, $needles)
    {
        return \_PhpScoperd4c93190f897\Illuminate\Support\Str::contains($haystack, $needles);
    }
}

We see that str_contains has been treated as an internal function while this function doesn't exist before PHP 8.

A possible solution would be to take into account the @since annotation provided by phpstorm-stubs.

I'm not sure we want to fix this issue with this PR.

@smoench
Copy link
Contributor Author

smoench commented Dec 4, 2020

Well, this is more or less related to #440

@villfa
Copy link
Contributor

villfa commented Dec 7, 2020

@smoench e2e_027 is fixed with PHP8, and disabled with PHP7: smoench#1

@theofidry
Copy link
Member

Regarding the Symfony & Laravel test suite: if those are the blockers in the CI I would ignore them. There is definitely issues with those two but they are more related to polyfill support and whether or not a symbol is considered as internal depending of the target PHP versions, which are two sets of problems to be addressed on their own

@smoench smoench marked this pull request as ready for review December 8, 2020 08:23
@smoench
Copy link
Contributor Author

smoench commented Dec 8, 2020

Ready for review 🚀

I also reworked the build workflow. It parallelized the e2e tests and is hopefully more readable.

@smoench
Copy link
Contributor Author

smoench commented Dec 8, 2020

A big thank you to @villfa. It was an honor to work with you on this!

Copy link
Member

@theofidry theofidry left a comment

Choose a reason for hiding this comment

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

LGTM. Really impressed by the workflow it's really nice!

What is the issue about the pending build? Is it due to them being required by the branch in settings but no longer available?

@smoench
Copy link
Contributor Author

smoench commented Dec 8, 2020

Yes, the required builds are removed due to the rework and the settings needs to be updated.

@theofidry
Copy link
Member

👌

@theofidry theofidry merged commit 6ff13aa into humbug:master Dec 8, 2020
@theofidry
Copy link
Member

Thank you again @smoench @villfa for the amazing work

@smoench smoench deleted the php8 branch December 8, 2020 10:02
@TomasVotruba
Copy link
Contributor

Hi, thank you for adding PHP 8!

It one of last blockers for Symplify packages, as we use scoper to release scoped PHP 8 packages.
Just curious, any ETA on release?

Thanks

@smoench
Copy link
Contributor Author

smoench commented Dec 21, 2020

@theofidry Can we have also a new release for php-scoper with PHP 8 support? Is there anything I can do to help you?

@theofidry
Copy link
Member

https://github.com/humbug/php-scoper/releases/tag/0.14.0 released, sorry forgot about it :)

@smoench
Copy link
Contributor Author

smoench commented Dec 21, 2020

Thank you @theofidry 🚀

@TomasVotruba
Copy link
Contributor

Thank you both!

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.

4 participants