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.

Sign up for GitHub

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

Jump to bottom

Prepare v2.0.x development branch for php 8 #232

Merged
merged 8 commits into from
Dec 23, 2020
Merged

Prepare v2.0.x development branch for php 8 #232

merged 8 commits into from
Dec 23, 2020

Conversation

jaapio
Copy link
Contributor

@jaapio jaapio commented Jul 3, 2020

No description provided.

@jaapio jaapio changed the title Enable php8 in matrix build Prepare for php 8 Jul 3, 2020
@VincentLanglet
Copy link

Hi @jaapio.

Log have expired, what is needed to add php8 support ? :)

@brainexe
Copy link

at least the php requirement in the composer.json file needs to be updated.
I made a small check of the code and it looks still compatible to php8 🤞

And I'm looking forward to this change in this lib, as it's somehow blocking a full PHP8 update in other projects when it's punned to php7 only

@jaapio
Copy link
Contributor Author

jaapio commented Dec 16, 2020

Retriggered the CI pipeline, logs should be available soon. I don't remember where I got stuck. But I think it was because of missing dependencies.

@allejo
Copy link
Member

allejo commented Dec 17, 2020 •
edited
Loading

Triggered the CI and updated our composer.json to allow for PHP 8. Looks like a lot of dependencies need to be updated to newer versions or to support PHP 8

@jaapio
Copy link
Contributor Author

jaapio commented Dec 18, 2020

Moved a few steps forward. But maybe we need to split this or to make it easier for review.

deprecations in php8 unittests are fixed in bovigo/callmap#22 so we need to wait for that as well.

@jaapio jaapio requested review from bizurkur, allejo, mikey179 and a team December 18, 2020 14:35
@jaapio jaapio marked this pull request as ready for review December 18, 2020 14:38
@jaapio
Copy link
Contributor Author

jaapio commented Dec 22, 2020

Build should be green by now. I would like to get some reviews and feedback.

@VincentLanglet
Copy link

It's not fully related to this PR but the composer.lock should be in the gitignore IMHO and not commited.

VincentLanglet
VincentLanglet reviewed Dec 22, 2020
View reviewed changes
.github/workflows/runTests.yml
@@ -10,10 +10,11 @@ jobs:
tests:
name: PHP ${{ matrix.php-versions }} on ${{ matrix.os }} w/ ${{ matrix.dependencies }}
runs-on: ${{ matrix.os }}
continue-on-error: ${{ matrix.php-versions == '8.0' }}
Copy link

Choose a reason for hiding this comment

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

You can remove this.

bizurkur
bizurkur reviewed Dec 22, 2020
View reviewed changes
Copy link
Contributor

@bizurkur bizurkur left a comment

Choose a reason for hiding this comment

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

Looks great. Nice work. Looks like there's one small change that shouldn't have happened, but everything else looks good to me.

tests/phpunit/FilenameTestCase.php Show resolved Hide resolved
@mikey179
Copy link
Member

It's not fully related to this PR but the composer.lock should be in the gitignore IMHO and not commited.

I disagree. To quote one of the authors of Composer: The lock file exists to be commited!

@VincentLanglet
Copy link

It's not fully related to this PR but the composer.lock should be in the gitignore IMHO and not commited.

I disagree. To quote one of the authors of Composer: The lock file exists to be commited!

composer/composer#2461 (comment)

When you're working on a team project it should be committed I agree. But when it's a library you release, not commiting the composer.lock ensure the Ci to always work with the latest version and detect possible regressions.

Here, it's not the case since the composer.lock is commited and the ci runs "composer install"

@jaapio
Copy link
Contributor Author

jaapio commented Dec 23, 2020

We do have a work around please check the ci pipeline. You will see that the dependencies are updated in some steps

@VincentLanglet
Copy link

We do have a work around please check the ci pipeline. You will see that the dependencies are updated in some steps

Indeed, my bad.

@jaapio jaapio merged commit 08a7985 into master Dec 23, 2020
@jaapio jaapio deleted the php-8-support branch December 23, 2020 19:28
@VincentLanglet
Copy link

Thanks for this work. Does a release is planned ? :)

@jaapio
Copy link
Contributor Author

jaapio commented Dec 23, 2020

I just opened a discussion to check with the other maintainers if we are waiting for something. But I don't expect anything urgent. Hopefully, we can release this year :-)

@mathieupetrini mathieupetrini mentioned this pull request Dec 26, 2020
Merged
@VincentLanglet
Copy link

Any news @jaapio ? :)

@allejo
Copy link
Member

allejo commented Jan 6, 2021

Fellow maintainer for vfsStream here 👋

Am I missing something? The latest release for vfsStream is 1.6.8 which has a PHP requirement of >=5.3.0. This PR targets the master branch which is the development branch for vfsStream 2.x. vfsStream 2.x shouldn't really be used quite yet since things can still change/break. What are we missing in 1.x to support PHP 8?

@allejo allejo changed the title Prepare for php 8 Prepare v2.0.x development branch for php 8 Jan 6, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@VincentLanglet VincentLanglet VincentLanglet left review comments

@bizurkur bizurkur bizurkur left review comments

@allejo allejo Awaiting requested review from allejo

@mikey179 mikey179 Awaiting requested review from mikey179

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@jaapio @VincentLanglet @brainexe @allejo @mikey179 @bizurkur