Skip to content

Conversation

@t-richard
Copy link
Member

As PHP 8.1 is coming and support is already considered in #980, I think its needed to run tests against 8.1 in the CI to ensure that the code doesn't break and eventually spot something, be it in Bref, php or a downstream dependency.

@t-richard
Copy link
Member Author

Darn, FPM 8.1 is not available via apt-get yet. I'll try to figure this out

@mnapoli
Copy link
Member

mnapoli commented Aug 6, 2021

Hey just FYI in case that helps, I had trouble with FPM too when adding tests for PHP 8 beta: shivammathur/setup-php#280 (comment)

@t-richard
Copy link
Member Author

@mnapoli thanks, I saw that, this issue is linked in the ci.yml file. Also just saw that @mykiwi had the same issue with the CI in #980 and ended up reverting the changes.

As far as I can tell, FPM is installed with 8.1, just have to find the right way to make both co-exist.

@t-richard
Copy link
Member Author

@mnapoli the workflow is fixed for 8.1 but the tests are failing because full_path was introduced in $_FILES so the assert is not an exact match at https://github.com/brefphp/bref/blob/master/tests/Handler/FpmHandlerTest.php#L735

See the php-src PR php/php-src#6917

I've provided a fix for it, let me know if you think that's ok to it this way.

- php: '7.3'
dependency-version: '--prefer-lowest'
- php: '8.1'
platform-reqs: '--ignore-platform-req=php' # see https://github.com/phpspec/prophecy/pull/533#issuecomment-891606803
Copy link
Member

Choose a reason for hiding this comment

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

Is this still relevant now that the PR has been merged?

Copy link
Member Author

Choose a reason for hiding this comment

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

The PR is merged but prophecy is not released and it won't be until php8.1 issues are fixed. I think it can be merged as is and removed when prophecy is compatible (it will be around RC1 apparently).

Copy link
Member

@mnapoli mnapoli left a comment

Choose a reason for hiding this comment

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

LGTM, thank you @t-richard!

@deleugpn deleugpn merged commit c67dae5 into brefphp:master Aug 25, 2021
@deleugpn
Copy link
Member

deleugpn commented Aug 25, 2021

Thank you @t-richard!

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.

3 participants