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

PHP 8.0 support #15

Merged
merged 18 commits into from
Jan 26, 2021
Merged

PHP 8.0 support #15

merged 18 commits into from
Jan 26, 2021

Conversation

arueckauer
Copy link
Member

@arueckauer arueckauer commented Nov 14, 2020

Q A
Documentation no
Bugfix no
BC Break yes/no
New Feature no
RFC no
QA yes

Description

This PR provides provides changes described in #12.

Fixes #12

Upgrade phpunit/phpunit to 9.3

Signed-off-by: Andi Rückauer <arueckauer@gmail.com>
Signed-off-by: Andi Rückauer <arueckauer@gmail.com>
Signed-off-by: Andi Rückauer <arueckauer@gmail.com>
Signed-off-by: Andi Rückauer <arueckauer@gmail.com>
Drop support for PHP less then 7.3

Signed-off-by: Andi Rückauer <arueckauer@gmail.com>
…havior

Signed-off-by: Andi Rückauer <arueckauer@gmail.com>
@arueckauer
Copy link
Member Author

Hi @boesing 👋🏻

It seems Travis configuration needs some update for PHP nightly builds. I am unsure how to fix that. Has this issue been solved in another component where PHP 8 support has been added?

@boesing boesing added this to In progress in PHP 8.0 via automation Nov 17, 2020
@boesing boesing linked an issue Nov 17, 2020 that may be closed by this pull request
6 tasks
@boesing
Copy link
Member

boesing commented Nov 17, 2020

Hm, when I understand this correct, its because of the TESTS_LAMINAS_DIAGNOSTICS_APCU_ENABLED global environment var.

We should install apcu 5.1.19 as it is compatible with PHP 8.0.
You can have a look at laminas-cache-storage-adapter-apcu to see how to install pecl extensions during CI.

Signed-off-by: Andi Rückauer <arueckauer@gmail.com>
Signed-off-by: Andi Rückauer <arueckauer@gmail.com>
Signed-off-by: Andi Rückauer <arueckauer@gmail.com>
Signed-off-by: Andi Rückauer <arueckauer@gmail.com>
Signed-off-by: Andi Rückauer <arueckauer@gmail.com>
@arueckauer
Copy link
Member Author

Thanks for the input. APCu now works.

Since Memcached is not yet supported for PHP 8.0 (see latest version 3.1.5), I disabled it.

For MongoDB I added the 1.9.0RC1 which adds support for PHP 8. Unfortunately the build was not executed, due to a negative credit balance in Travis.

Owner laminas does not have enough credits.

@arueckauer arueckauer marked this pull request as ready for review November 18, 2020 08:47
@boesing
Copy link
Member

boesing commented Nov 18, 2020

Thanks for the changes. We might want to have a look when travis is back up and running. 👍

Signed-off-by: Andi Rückauer <arueckauer@gmail.com>
@arueckauer
Copy link
Member Author

@boesing Builds are generally running again, but the latest push did not trigger Travis. Can you manually start the build process?

Signed-off-by: Andi Rückauer <arueckauer@gmail.com>
Change PHP version to 8.0

Signed-off-by: Andi Rückauer <arueckauer@gmail.com>
Signed-off-by: Andi Rückauer <arueckauer@gmail.com>
@mathieupetrini
Copy link

Hi,

bovigo/vfsStream#232 has been merged.
Is it possible to restart failing jobs ?

@samsonasik
Copy link
Member

restarted

PHP 8.0 automation moved this from In progress to Review in progress Dec 28, 2020
phpunit.xml.dist Show resolved Hide resolved
{
self::markTestSkipped('Clarify what to do with assertion of protected property.');
Copy link
Member

Choose a reason for hiding this comment

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

To be discussed before merging

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, just use reflection for now as it might test something which has been fixed before.
I dont really like checking protected/private properties at all but this is done in so many packages and I dont get the reason behind that...

Copy link
Member

Choose a reason for hiding this comment

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

Urgh, this went under my radar: OK to merge without it

Copy link
Member

@boesing boesing left a comment

Choose a reason for hiding this comment

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

LGTM

Please remove the skipped test for guzzle and use reflection for now to make the test pass.

{
self::markTestSkipped('Clarify what to do with assertion of protected property.');
Copy link
Member

Choose a reason for hiding this comment

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

Yeah, just use reflection for now as it might test something which has been fixed before.
I dont really like checking protected/private properties at all but this is done in so many packages and I dont get the reason behind that...

- Adds a coding standards workflow
- Adds a unit tests workflow
- Removes Travis configuation

Signed-off-by: Matthew Weier O'Phinney <matthew@weierophinney.net>
Unreleased, so having to alias for now.

Signed-off-by: Matthew Weier O'Phinney <matthew@weierophinney.net>
warning is no longer raised; type error is raised instead.

Signed-off-by: Matthew Weier O'Phinney <matthew@weierophinney.net>
@weierophinney
Copy link
Member

I've taken the liberty of amending this with changes to the CI pipeline. We're moving off of Travis to GHA, and I plan to provide fixes per the feedback provided already... but we're out of Travis minutes for the month, so first step is getting a working CI pipeline. 😄

As you can see, all green at this stage, and we have tests against all expected services.

"laminas/laminas-zendframework-bridge": "^1.0"
},
"require-dev": {
"doctrine/migrations": "^1.0 || ^2.0",
"guzzlehttp/guzzle": "^5.3.3 || ^6.3.3",
"laminas/laminas-coding-standard": "~1.0.0",
"laminas/laminas-loader": "^2.0",
"mikey179/vfsstream": "^1.6",
"mikey179/vfsstream": "dev-master#08a798577d677436f4f4ea3d0b3c949f64655548 as 1.6.9",
Copy link
Member

Choose a reason for hiding this comment

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

To be changed before merge

Copy link
Member

Choose a reason for hiding this comment

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

Hmm, no, only require-dev: I'd say "roll with it"

Copy link
Member

Choose a reason for hiding this comment

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

Until they have a release that includes that commit... it is what it is. :)

@Ocramius
Copy link
Member

We're moving off of Travis to GHA

@weierophinney the GHA setup used here seems to be a bit wonky, but in the interest of getting anything running at this point, I would say 🚢

@Ocramius Ocramius changed the base branch from 1.6.x to 1.7.x January 26, 2021 10:37
@Ocramius Ocramius self-assigned this Jan 26, 2021
@Ocramius Ocramius added this to the 1.7.0 milestone Jan 26, 2021
@Ocramius Ocramius merged commit e396469 into laminas:1.7.x Jan 26, 2021
PHP 8.0 automation moved this from Review in progress to Done Jan 26, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
No open projects
PHP 8.0
  
Done
Development

Successfully merging this pull request may close these issues.

PHP 8.0 support
6 participants