Add support for PHP 8.0

[xvilo]

This does upgrade the code to allow for PHP 8.0, which can be merged as-is if #579 is something we do not want to add. However, if we want to add #579, then I can update this PR with the accompanying Rector changes :)

[xvilo]

@akondas can you tell me how buddy works, it seems it's not pickup the runner config change. According to the logs, the used CI image is still 7.4

[akondas]

I need to approve these changes, but there is also a problem with the public instance, it needs to be prepared for php 8

[akondas]

I have to think about how to do it so that it would not be downtime, I would have to install php 8 there beforehand, which is actually to be done

[akondas]

I still have something to finish this week, but from next week I will sit down to the repman and I will try to catch up with all PR (not only this one). We will catch up with this - I promise :wink:

[xvilo]

I have to think about how to do it so that it would not be downtime

The best would then be to "PHP": "7.4 || 8.0", merge this, update your vhost with the right PHP(-fpm) instance, and do a reload. Then we can fully move the codebase to 8.0, do you want me to update this one?

For compatibility we could already move CI to 8, so it does the checking and stuffs

[akondas]

yes, good approach, that should allow the update to be done without downtime, thanks for help :+1:

[xvilo]

@akondas I've pushed the suggested update, just have to wait for the CI. If this is green, can you review and merge this PR?

[xvilo]

I'm unsure why the CI fails, it should be fine. It appears it might install the dependencies from some sort of cached vendor dir. I could use some help with this :)

[xvilo]

@akondas can you do another check for this PR?

[xvilo]

@marmichalski / @akondas everything has been addressed, how is this PR looking now?

[xvilo]

@akondas lets finish this?

[akondas]

So from my point of view, plan looks like this:

• allow to use php 8 in composer.json
• switch pipeline to php 8
• upgrade public instance to php 8
• upgrade code (composer.lock) to php 8
• drop support for php 7.4

We are on first point here, so please do not run composer update on php 8.

[Jeroeny]

You can do this in composer.json to keep composer.lock depdencies on 7.4 for now:

"config":
        "platform": {
            "php": "7.4"
        },

[xvilo]

I have applied @Jeroeny's suggestion and installed a 7.4 compatible set of dependencies.

[codecov[bot]]

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Comparison is base (423985c) 0.00% compared to head (ca923ae) 99.02%.

:exclamation: Current head ca923ae differs from pull request most recent head 8b24958. Consider uploading reports for the commit 8b24958 to get more accurate results

Additional details and impacted files

```diff @@ Coverage Diff @@ ## master #580 +/- ## ============================================= + Coverage 0 99.02% +99.02% - Complexity 0 1907 +1907 ============================================= Files 0 301 +301 Lines 0 5763 +5763 ============================================= + Hits 0 5707 +5707 - Misses 0 56 +56 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

[xvilo]

@akondas everything's green, with support for 8.0

[xvilo]

@akondas do you have some time to take a look at it?

[akondas]

Ok, let's go with this. Please resolve conflicts and we can merge this.

[xvilo]

@akondas I've resolved the merge conflicts. Unfortunately the test pipeline is not passing due to some weird behaviour regarding PHPStan. I suspect it caches some files that has been corrupt, locally it does all work fine

[paulwehage]

Any news here? Would love to use repman with PHP 8.0.

[xvilo]

In the past 24 days there were new merge conflicts. From my side it's good to go, but @akondas needs to give final approval + merge. I've resolved the new conflicts, but I will not be actively watching the status anymore for now (it's taking long enough) and we're running these changes already in prod with our internal fork.

[xvilo]

@akondas I have, again, resolved the merge conflicts. Could we please merge this PR? We've been running this code on our on-premise install for the past 2-3 months and it runs fine :)

[94noni]

@xvilo what about upgrading to php8.1?

[xvilo]

Good sugesstion, however, the upgrade path to 8.1 is a bit harder. I'd like to get this merged first. And honestly I'm wondering why this takes so long 😟

Also, an upgrade to Symfony 6.2 might be warranted. As this is also a fresh and hot release!

[xvilo]

@akondas We're closing in on a year, and have passed the EOL date of PHP 7.4. We've been running this code for almost a year with our private instance and haven't noticed any issues.

Can we get this reviewed and merged?

[xvilo]

@akondas Whats your idea? Do you still want this or not?

[xvilo]

It seems the tests fails due to a small timing issue, I tried to quickly require symfony/clock, but I can't due to https://github.com/repman-io/repman/issues/653

[ymorocutti]

I was able to reproduce multiple times, but I'm not sure it's related to PHP 8. My guess, you're having 2 DateTimes created within an interval of several milliseconds but are not on the same seconds. ie : 10.980 vs 11.002.

@akondas Might be a good idea to merge this one asap, as PHP 7.4 has reach end of life and should no longer be used :)

[xvilo]

Yeah I expected so, but since repman was broken I kept it as is. Again, we’re using this as a fork on our kubernetes environment and all works fine as is!

I would also like to work on PHP 8.1/2 compatibility and an upgrade to Symfony 6.3, but I will wait for this until we’re good with this one

[xvilo]

I thought we had this running internally already, but apparently we didn't yet. I've just switched our internal hosted version to this branch correctly and found one bug which is fixed with https://github.com/repman-io/repman/pull/580/commits/3f6bc36dcd9753d945e402b69e7ad4d668220f2b

For now I'll keep an eye on how well it works through Sentry

[xvilo]

@akondas in the past commits I've done the following:

• Add GitHub actions support for PHPUnit (just that for now, as Buddy is broken)
  • This currently runs tests both on PHP 7.4 and 8.0
• Update coduo/php-matcher as it indeed has incompatibilities when running PHPUnit tests
• Update a test to account for different wording between PHP 7.4 and PHP 8.0

As of know, both jobs report a succesfull status;

• 7.4: https://github.com/xvilo/repman/actions/runs/5501289892/jobs/10024774435
• 8.0: https://github.com/xvilo/repman/actions/runs/5501289892/jobs/10024774499

P.s. I think the status does not get reflected in this PR as GitHub actions have been disabled for the project. If you enable it, we might see the status here too

[xvilo]

So I made small booboo and accidentally pushed custom patches this way, cleaned and cherry-picked it.

Also thanks to @marmichalski for the review, answered and/or fixed the comments you left

PHP Unit tests statusses all in the green:

• Postgres 11.20:
  • PHP 7.4: https://github.com/xvilo/repman/actions/runs/5506320670/jobs/10034839156
  • PHP 8.0: https://github.com/xvilo/repman/actions/runs/5506320670/jobs/10034839692
• Postgres 13.11:
  • PHP 7.4: https://github.com/xvilo/repman/actions/runs/5506320670/jobs/10034839366
  • PHP 8.0: https://github.com/xvilo/repman/actions/runs/5506320670/jobs/10034839838
• Postgres 15.03:
  • PHP 7.4: https://github.com/xvilo/repman/actions/runs/5506320670/jobs/10034839517
  • PHP 8.0: https://github.com/xvilo/repman/actions/runs/5506320670/jobs/10034840032

Edit: Current state is also running smoothly on our Kubernetes deployment:

[xvilo]

@akondas ?

[xvilo]

@akondas do you have some time to look into this MR? PHP 8.0 is EOL this week, I would like to get this merged and add some effort in upgrading it to newer php versions. Also resolving some bugs we're hitting, but I will not invest more time until I get some response on here 👍

[xvilo]

Thanks for picking this up @marmichalski, let me know if there is anything I can do to help out 👍

[marmichalski]

Thanks for picking this up @marmichalski, let me know if there is anything I can do to help out 👍

Yea, I have no idea why some tests under ComposerPackageSynchronizer are failing in PHP 8.0 CI, considering they pass in 7.4 CI and locally for me 🤷

And debug output was not really helpful: https://github.com/repman-io/repman/actions/runs/7240781103/job/19724146992#step:18:19

[xvilo]

That’s weird, in one of my previous comments I had both 7.4 and 8.0 passing through GitHub actions just fine. I’ll take a look tomorrow or later this week if you haven’t solved it yet.

We could also opt to merge it as is, and see if we can resolve the final testing issues in another PR as you’ve validated them locally to run fine. Maybe some environment issue for example

[marmichalski]

That’s weird, in one of my previous comments I had both 7.4 and 8.0 passing through GitHub actions just fine. I’ll take a look tomorrow or later this week if you haven’t solved it yet.

I've brought your original workflow to life and even 7.4 failed on those tests 🙈

[xvilo]

Locall on macos I have the following 2 failing:

PHPUnit 9.5.25 #StandWithUkraine

...............................................................  63 / 500 ( 12%)
............................................................... 126 / 500 ( 25%)
........................E.E.................................... 189 / 500 ( 37%)
............................................................... 252 / 500 ( 50%)
............................................................... 315 / 500 ( 63%)
............................................................... 378 / 500 ( 75%)
............................................................... 441 / 500 ( 88%)
...........................................................     500 / 500 (100%)

You should really speed up these slow tests (>500ms)...
 1. 514ms to run Buddy\\Repman\\Tests\\Functional\\Controller\\Admin\\ConfigControllerTest::testToggleAuthenticationOptions

Time: 00:10.786, Memory: 141.00 MB

There were 2 errors:

1) Buddy\Repman\Tests\Unit\Service\Security\SensioLabsSecurityCheckerTest::testUpdateWhenRepoDontExist
Symfony\Component\Process\Exception\ProcessFailedException: The command "'git' 'clone' '--depth' '1' '--branch' 'master' 'file:///var/folders/zj/l6h4qqtn7hx6hcz2v4l9wl4m0000gn/T/repman/security-advisories-repo' '.'" failed.

Exit Code: 128(Invalid exit argument)

Working directory: /var/folders/zj/l6h4qqtn7hx6hcz2v4l9wl4m0000gn/T/repman/security-advisories

Output:
================

Error Output:
================
Cloning into '.'...
warning: Could not find remote branch master to clone.
fatal: Remote branch master not found in upstream origin

/Users/xvilo/projects/repman/src/Service/Security/SecurityChecker/SensioLabsSecurityChecker.php:218
/Users/xvilo/projects/repman/src/Service/Security/SecurityChecker/SensioLabsSecurityChecker.php:196
/Users/xvilo/projects/repman/src/Service/Security/SecurityChecker/SensioLabsSecurityChecker.php:34
/Users/xvilo/projects/repman/tests/Unit/Service/Security/SensioLabsSecurityCheckerTest.php:119

2) Buddy\Repman\Tests\Unit\Service\Security\SensioLabsSecurityCheckerTest::testUpdateWhenRepoExist
Symfony\Component\Process\Exception\ProcessFailedException: The command "'git' 'clone' '--depth' '1' '--branch' 'master' 'file:///var/folders/zj/l6h4qqtn7hx6hcz2v4l9wl4m0000gn/T/repman/security-advisories-repo' '.'" failed.

Exit Code: 128(Invalid exit argument)

Working directory: /var/folders/zj/l6h4qqtn7hx6hcz2v4l9wl4m0000gn/T/repman/security-advisories

Output:
================

Error Output:
================
Cloning into '.'...
warning: Could not find remote branch master to clone.
fatal: Remote branch master not found in upstream origin

/Users/xvilo/projects/repman/src/Service/Security/SecurityChecker/SensioLabsSecurityChecker.php:218
/Users/xvilo/projects/repman/src/Service/Security/SecurityChecker/SensioLabsSecurityChecker.php:196
/Users/xvilo/projects/repman/src/Service/Security/SecurityChecker/SensioLabsSecurityChecker.php:34
/Users/xvilo/projects/repman/tests/Unit/Service/Security/SensioLabsSecurityCheckerTest.php:134

ERRORS!
Tests: 500, Assertions: 1201, Errors: 2.

[marmichalski]

Yea, those are the reason of macOS git config (not the git version, as newer git versions are still using master as default branch). Assuming that would work for you if you'd init the repo with --initial-branch master in that test, so everything would pass on your local, too :sob:

I am still considering how to properly go around this; I think we don't have to specify branch at all in SensioLabsSecurityChecker as git clone with --depth 1 will fetch single (default) branch anyway (IIRC).

[xvilo]

From what I have found so far it that it appears to read the versions of the project itself in the test? The buffered IO contains the following logs:

Reading composer.json of repman-io/repman (0.1.0)\n
Importing tag 0.1.0 (0.1.0.0)\n
Reading composer.json of repman-io/repman (0.1.1)\n
Importing tag 0.1.1 (0.1.1.0)\n
Reading composer.json of repman-io/repman (0.1.2)\n
Importing tag 0.1.2 (0.1.2.0)\n
Reading composer.json of repman-io/repman (0.2.0)\n
Importing tag 0.2.0 (0.2.0.0)\n
Reading composer.json of repman-io/repman (0.2.1)\n
Importing tag 0.2.1 (0.2.1.0)\n
Reading composer.json of repman-io/repman (0.3.0)\n
Importing tag 0.3.0 (0.3.0.0)\n
Reading composer.json of repman-io/repman (0.4.0)\n
Importing tag 0.4.0 (0.4.0.0)\n
Reading composer.json of repman-io/repman (0.4.1)\n
Importing tag 0.4.1 (0.4.1.0)\n
Reading composer.json of repman-io/repman (0.5.0)\n
Importing tag 0.5.0 (0.5.0.0)\n
Reading composer.json of repman-io/repman (0.6.0)\n
Importing tag 0.6.0 (0.6.0.0)\n
Reading composer.json of repman-io/repman (1.0.0)\n
Importing tag 1.0.0 (1.0.0.0)\n
Reading composer.json of repman-io/repman (1.1.0)\n
Importing tag 1.1.0 (1.1.0.0)\n
Reading composer.json of repman-io/repman (1.1.1)\n
Importing tag 1.1.1 (1.1.1.0)\n
Reading composer.json of repman-io/repman (1.2.0)\n
Importing tag 1.2.0 (1.2.0.0)\n
Reading composer.json of repman-io/repman (1.2.1)\n
Importing tag 1.2.1 (1.2.1.0)\n
Reading composer.json of repman-io/repman (1.2.2)\n
Importing tag 1.2.2 (1.2.2.0)\n
Reading composer.json of repman-io/repman (1.3.0)\n
Importing tag 1.3.0 (1.3.0.0)\n
Reading composer.json of repman-io/repman (1.3.1)\n
Importing tag 1.3.1 (1.3.1.0)\n
Reading composer.json of repman-io/repman (1.3.2)\n
Importing tag 1.3.2 (1.3.2.0)\n
Reading composer.json of repman-io/repman (1.3.3)\n
Importing tag 1.3.3 (1.3.3.0)\n
Reading composer.json of repman-io/repman (1.3.4)\n
Importing tag 1.3.4 (1.3.4.0)\n
Reading composer.json of repman-io/repman (1.3.5)\n
Importing tag 1.3.5 (1.3.5.0)\n
Reading composer.json of repman-io/repman (1.3.6)\n
Importing tag 1.3.6 (1.3.6.0)\n
Reading composer.json of repman-io/repman (feat/php-8.0)\n
Importing branch feat/php-8.0 (dev-feat/php-8.0)\n
Reading composer.json of repman-io/repman (master)\n
Importing branch master (dev-master)\n

Those are clearly the branches and tags of this project, and not, as I expected, the versions of the dists zips in the tests folder. I'll check if the checkout is shallow or something like that

[xvilo]

That was it! I've added the fetch-depth and fetch-tags to the actions/checkout@v3 action. This makes it so the copy isn't (too) shallow. I think it shouldn't fetch itself, so the test might be incorrect. However, I believe this was already the case so I wont address that in this PR.

@marmichalski how you like?

[marmichalski]

That was it! I've added the fetch-depth and fetch-tags to the actions/checkout@v3 action. This makes it so the copy isn't (too) shallow. I think it shouldn't fetch itself, so the test might be incorrect. However, I believe this was already the case so I wont address that in this PR.

@marmichalski how you like?

Can you make the other workflow pass and drop this one, though? :+1: I don't think we should be fetching full history at all times, maybe fetch-tags: true would be enough? Also this does not really explain why the workflow (the one I'd like to keep) is passing on 7.4 and not 8.0

[xvilo]

@marmichalski ah, I've been debugging the wrong file in that case, as that is a different error. I'll restart my investigation with that one

[xvilo]

It might be an issue with git safe directories. I would assume that the 7.4 image might be a bit more outdated then the 8.0 image

[marmichalski]

It might be an issue with git safe directories. I would assume that the 7.4 image might be a bit more outdated then the 8.0 image

Yea, looks to be the case looking at the dubious ownership. Good find :detective:

[xvilo]

@marmichalski do you have write access to this repository?

[xvilo]

I think it can be merged in to the main branch as is, I don't see any objections right now. We've been using this in prod for a while now through a custom fork

[marmichalski]

I think it can be merged in to the main branch as is, I don't see any objections right now. We've been using this in prod for a while now through a custom fork

Thank you, let's see what happens next :crossed_fingers:

[xvilo]

I think it can be merged in to the main branch as is, I don't see any objections right now. We've been using this in prod for a while now through a custom fork

Thank you, let's see what happens next 🤞

Do you want me to find some time to work on php 8.2 compatibility, or will you handle this? PR was already open (https://github.com/repman-io/repman/pull/660), but with some failing tests that I didn't know enough about