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 support for PHP 8.0 #580

Merged
merged 54 commits into from
Dec 18, 2023
Merged

Add support for PHP 8.0 #580

merged 54 commits into from
Dec 18, 2023

Conversation

xvilo
Copy link
Contributor

@xvilo xvilo commented Apr 7, 2022

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 xvilo changed the title Feat/php 8.0 Add support for PHP 8.0 Apr 7, 2022
@xvilo
Copy link
Contributor Author

xvilo commented Apr 7, 2022

@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
Copy link
Member

akondas commented Apr 7, 2022

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
Copy link
Member

akondas commented Apr 7, 2022

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
Copy link
Member

akondas commented Apr 7, 2022

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 😉

@xvilo
Copy link
Contributor Author

xvilo commented Apr 7, 2022

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
Copy link
Member

akondas commented Apr 7, 2022

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

@xvilo
Copy link
Contributor Author

xvilo commented Apr 27, 2022

@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
Copy link
Contributor Author

xvilo commented Apr 27, 2022

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

composer.json Outdated Show resolved Hide resolved
@xvilo
Copy link
Contributor Author

xvilo commented May 5, 2022

@akondas can you do another check for this PR?

@akondas akondas mentioned this pull request May 5, 2022
composer.json Show resolved Hide resolved
@xvilo
Copy link
Contributor Author

xvilo commented May 7, 2022

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

@xvilo xvilo requested a review from akondas May 7, 2022 10:59
Copy link

@94noni 94noni left a comment

Choose a reason for hiding this comment

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

👍🏻

Dockerfile Outdated Show resolved Hide resolved
@xvilo
Copy link
Contributor Author

xvilo commented May 19, 2022

@akondas lets finish this?

Copy link
Contributor

@marmichalski marmichalski left a comment

Choose a reason for hiding this comment

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

👍

Dockerfile Outdated Show resolved Hide resolved
buddy.yml Outdated Show resolved Hide resolved
buddy.yml Outdated Show resolved Hide resolved
composer.json Outdated Show resolved Hide resolved
@akondas
Copy link
Member

akondas commented May 22, 2022

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
Copy link

Jeroeny commented Jun 15, 2022

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

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

@xvilo
Copy link
Contributor Author

xvilo commented Jun 15, 2022

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

@codecov
Copy link

codecov bot commented Jun 15, 2022

Codecov Report

All modified and coverable lines are covered by tests ✅

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

❗ 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
@@              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     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@xvilo
Copy link
Contributor Author

xvilo commented Jun 15, 2022

@akondas everything's green, with support for 8.0

@xvilo
Copy link
Contributor Author

xvilo commented Sep 3, 2022

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

@akondas
Copy link
Member

akondas commented Sep 10, 2022

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

@xvilo xvilo force-pushed the feat/php-8.0 branch 2 times, most recently from 6e559b9 to db701c4 Compare December 18, 2023 12:30
@xvilo
Copy link
Contributor Author

xvilo commented Dec 18, 2023

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
Copy link
Contributor Author

xvilo commented Dec 18, 2023

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

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? 👍
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
Copy link
Contributor Author

xvilo commented Dec 18, 2023

@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 xvilo force-pushed the feat/php-8.0 branch 4 times, most recently from d54aa4c to 2872a1a Compare December 18, 2023 13:03
@xvilo
Copy link
Contributor Author

xvilo commented Dec 18, 2023

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

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 🕵️

@xvilo
Copy link
Contributor Author

xvilo commented Dec 18, 2023

@marmichalski do you have write access to this repository?

Co-authored-by: Marcin Michalski <evulmastah@gmail.com>
@xvilo
Copy link
Contributor Author

xvilo commented Dec 18, 2023

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 marmichalski merged commit 6819f07 into repman-io:master Dec 18, 2023
2 checks passed
@marmichalski
Copy link
Contributor

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 🤞

@xvilo
Copy link
Contributor Author

xvilo commented Dec 18, 2023

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 (#660), but with some failing tests that I didn't know enough about

@marmichalski
Copy link
Contributor

Do you want me to find some time to work on php 8.2 compatibility, or will you handle this? PR was already open (#660), but with some failing tests that I didn't know enough about

I can take over that PR in my spare time :)

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.

None yet

8 participants