PHP 8 support #35

rieschl · 2020-10-22T13:22:02Z

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

Description

The minimum PHP version was bumped to 7.3, PHPUnit updated to 9.3 and all related problems fixed.
Also, return types were added for test methods and static invocation in assert calls.
Travis now checks PHP 7.3, 7.4 and 8.0.
- Code style checks (phpcs) are done in PHP 7.3 because codesniffer v2 throws some deprecation warnings in 7.4. Upgrading to the latest laminas-coding-standard will probably fix that.
- In nightly/PHP8 mongodb is disabled because the extension doesn't support PHP8, yet.
I also added a test for Validator\HttpUserAgent

ToDo:

drop laminas-servicemanager v2 support
drop laminas-eventmanager v2 support(?)
remove tests that rely on PHP 7.1 (and are therefore skipped in all builds)
add some more tests: I'd like to add some more tests, after dropping support for the old laminas-packages

@boesing Could you confirm that the todo list is ok?

fixes #33
required for #30

* update phpunit.xml schema * fix deprecated/removed/changed stuff in PHPUnit 9 Signed-off-by: Thomas Rieschl <thomas@trinet.at>

Signed-off-by: Thomas Rieschl <thomas@trinet.at>

…db in php8 Signed-off-by: Thomas Rieschl <thomas@trinet.at>

Signed-off-by: Thomas Rieschl <thomas@trinet.at>

boesing

So far, LGTM.
Had some minor change requests but overall, nice improvement here 👍

composer.json

test/ReflectionUtil.php

boesing · 2020-10-22T14:31:21Z

remove tests that rely on PHP 7.1 (and are therefore skipped in all builds)

Do you have examples? I am not into this repo (I've justed added 300 Issues for all packages ;-)) so I'd prefer having some examples. Overall: yes, remove 'em if they're really 7.1 related.

add some more tests: I'd like to add some more tests, after dropping support for the old laminas-packages

Would prefer a dedicated PR for improvements.

drop laminas-servicemanager v2 support
drop laminas-eventmanager v2 support(?)

~~I'll give you feedback ASAP.~~
Update: Yes! drop it like its hot! 👍

Ocramius · 2020-10-22T14:33:05Z

Removing old major versions of deps is fine, as long as it doesn't change our API 👍

…quirement in composer.json Signed-off-by: Thomas Rieschl <thomas@trinet.at>

…hould also cover AbstractContainer Signed-off-by: Thomas Rieschl <thomas@trinet.at>

rieschl · 2020-10-22T19:51:59Z

remove tests that rely on PHP 7.1 (and are therefore skipped in all builds)

Do you have examples? I am not into this repo (I've justed added 300 Issues for all packages ;-)) so I'd prefer having some examples. Overall: yes, remove 'em if they're really 7.1 related.

Yes, there are some tests which are skipped if PHP is less than 7.1 in IdTest, SessionConfigTest and StandardConfigTest

add some more tests: I'd like to add some more tests, after dropping support for the old laminas-packages

Would prefer a dedicated PR for improvements.

Yes, just a few to keep coveralls from failing :)

…ager v2 Signed-off-by: Thomas Rieschl <thomas@trinet.at>

boesing · 2020-10-22T21:20:04Z

Yes, just a few to keep coveralls from failing :)

This PR will get merged even without increasing coveralls. Just decreasing wont be that good ;-)

rieschl · 2020-10-22T21:24:32Z

Yes, just a few to keep coveralls from failing :)

This PR will get merged even without increasing coveralls. Just decreasing wont be that good ;-)

But apparently, coveralls thinks it decreased and marks the build as failed 🤷
But hopefully, the latest commit will do 🙂

Signed-off-by: Thomas Rieschl <thomas@trinet.at>

rieschl · 2020-10-23T07:30:02Z

Alright, I think I'm satisfied with my PR, now. However, coveralls still complains and I have no idea, why. If anyone can point me to the right direction, I'm happy to add another test.

boesing · 2020-10-23T07:47:52Z

However, coveralls still complains and I have no idea, why.

With the transition to laminas, we've added minimum of coverage which is not reached in this and many other projects. Thus, we are merging those projects even if coveralls is complaining. Thats what I wanted to say in one of my latest messages.

This is already a huge improvement you did here, thank you very much, @rieschl

I'll start review later on, but one thing I've already realized is the deletion of some files.
Even if we dropped support for EventManager v2, dropping source code is a BC break.
So either we find a way to keep those files and internally use the v3 event manager or we have to create a new major.
I'd prefer not bumping this to a new major. 😅

rieschl · 2020-10-23T08:11:26Z

I'll start review later on, but one thing I've already realized is the deletion of some files.
Even if we dropped support for EventManager v2, dropping source code is a BC break.
So either we find a way to keep those files and internally use the v3 event manager or we have to create a new major.
I'd prefer not bumping this to a new major. 😅

Hm, I can just keep the files, but then there's really no reason to drop eventmanger v2? I could throw an Exception, if AbstractValidatorChainEM2 would be loaded?
Or just keep the files but only remove the logic in the ValidatorChain?

rieschl · 2020-10-23T13:31:47Z

I'll start review later on, but one thing I've already realized is the deletion of some files.
Even if we dropped support for EventManager v2, dropping source code is a BC break.
So either we find a way to keep those files and internally use the v3 event manager or we have to create a new major.
I'd prefer not bumping this to a new major. 😅

Hm, I can just keep the files, but then there's really no reason to drop eventmanger v2? I could throw an Exception, if AbstractValidatorChainEM2 would be loaded?
Or just keep the files but only remove the logic in the ValidatorChain?

I reverted the deletion of those files, but kept the new/changed ValidatorChain. Is this how you meant it?

boesing · 2020-10-31T12:13:02Z

Damn, my fault.
After reviewing this PR locally, I think its fine to drop v2 compat ValidatorChain.
I should spend more time in reviewing PRs while checking the code out.

Sorry for the trouble.

rieschl · 2020-10-31T12:55:24Z

Damn, my fault.
After reviewing this PR locally, I think its fine to drop v2 compat ValidatorChain.
I should spend more time in reviewing PRs while checking the code out.

Sorry for the trouble.

Thanks for the feedback!
I removed the latest commit (which re-added the previously deleted files), so the files are gone, now.

rieschl · 2020-10-31T13:11:14Z

I just found another thing. In SessionConfig there are setHashFunction and setHashBitsPerCharacter, and in StandardConfig even more PHP 7.1 specific stuff.
Now that PHP7.1 is dropped, should I make a no-op there? Removing the methods is a BC break, right? Or just mark it as deprecated, throw an Exception?
Thanks!

boesing · 2020-10-31T13:23:27Z

Just mark the methods as deprecated but keep them as is.
When marked as deprecated, we can drop these functions in the next major and are good to go. 👍

…d in PHP 7.1 Signed-off-by: Thomas Rieschl <thomas@trinet.at>

rieschl · 2020-10-31T13:47:32Z

Just mark the methods as deprecated but keep them as is.
When marked as deprecated, we can drop these functions in the next major and are good to go. 👍

Done!

boesing

LGTM

Signed-off-by: Thomas Rieschl <thomas@trinet.at>

boesing · 2020-10-31T15:11:13Z

@froschdesign As the functionality of some methods in StandardConfig was removed with PHP 7.1 (and we dropped 7.1), should we remove the parameters from documentation aswell or should we just mark them as deprecated in the documentation aswell?

Signed-off-by: Maximilian Bösing <2189546+boesing@users.noreply.github.com>

boesing · 2020-10-31T15:32:23Z

Thanks, @rieschl!

froschdesign · 2020-10-31T16:51:29Z

@boesing

…should we just mark them as deprecated in the documentation aswell?

Has already been done in the past. See: https://docs.laminas.dev/laminas-session/config/#basic-configuration-options

rieschl added 8 commits October 22, 2020 15:23

feat: bump minimum PHP version, update phpunit

a3e0b2c

* update phpunit.xml schema * fix deprecated/removed/changed stuff in PHPUnit 9 Signed-off-by: Thomas Rieschl <thomas@trinet.at>

qa: tests: import classes

70dc8fb

Signed-off-by: Thomas Rieschl <thomas@trinet.at>

qa: tests: use static invocation in assert calls

b2b1c36

Signed-off-by: Thomas Rieschl <thomas@trinet.at>

qa: tests: add return types

ead206e

Signed-off-by: Thomas Rieschl <thomas@trinet.at>

qa: fix code style

41e9266

Signed-off-by: Thomas Rieschl <thomas@trinet.at>

ci: check coverage on php 7.3, use nightly instead of 8.0, skip mongo…

4aaf056

…db in php8 Signed-off-by: Thomas Rieschl <thomas@trinet.at>

fix: fix tests which only run with xdebug

0c49545

Signed-off-by: Thomas Rieschl <thomas@trinet.at>

qa: add a test for HttpUserAgent validator

5beba2c

Signed-off-by: Thomas Rieschl <thomas@trinet.at>

rieschl force-pushed the feat/php8 branch from 4cdfa5b to 5beba2c Compare October 22, 2020 13:23

boesing requested changes Oct 22, 2020

View reviewed changes

composer.json Outdated Show resolved Hide resolved

test/ReflectionUtil.php Outdated Show resolved Hide resolved

test/ReflectionUtil.php Outdated Show resolved Hide resolved

boesing added this to In progress in PHP 8.0 via automation Oct 22, 2020

boesing added Enhancement hacktoberfest-accepted Issues/Pull-Requests which can be fixed during Hacktoberfest: https://hacktoberfest.digitalocean.com labels Oct 22, 2020

rieschl added 2 commits October 22, 2020 20:49

qa: convert ReflectionUtil to trait; use more specific php version re…

ce646c6

…quirement in composer.json Signed-off-by: Thomas Rieschl <thomas@trinet.at>

qa: add some tests to prevent coveralls from failing; ContainerTest s…

193d8fd

…hould also cover AbstractContainer Signed-off-by: Thomas Rieschl <thomas@trinet.at>

feat: drop support for laminas-eventmanager v2 and laminas-serviceman…

a8d0b3f

…ager v2 Signed-off-by: Thomas Rieschl <thomas@trinet.at>

rieschl force-pushed the feat/php8 branch from 1055c76 to d0d3312 Compare October 22, 2020 21:21

qa: add a test for ValidatorChain

cc6e67a

Signed-off-by: Thomas Rieschl <thomas@trinet.at>

rieschl force-pushed the feat/php8 branch from d0d3312 to cc6e67a Compare October 23, 2020 07:01

qa: remove tests which require PHP < 7.1

c0200b1

Signed-off-by: Thomas Rieschl <thomas@trinet.at>

rieschl marked this pull request as ready for review October 23, 2020 07:27

rieschl requested a review from boesing October 23, 2020 07:27

rieschl force-pushed the feat/php8 branch from 89ec686 to c0200b1 Compare October 31, 2020 12:53

deprecate methods which operate on session settings which were remove…

e87c20d

…d in PHP 7.1 Signed-off-by: Thomas Rieschl <thomas@trinet.at>

PHP 8.0 automation moved this from In progress to Reviewer approved Oct 31, 2020

boesing approved these changes Oct 31, 2020

View reviewed changes

boesing added this to the 2.10.0 milestone Oct 31, 2020

boesing mentioned this pull request Oct 31, 2020

Add cookie_samesite parameter #30

Closed

rieschl added 2 commits October 31, 2020 15:35

feat: don't delete unneeded classes just yet, just deprecate them

fa7461b

Signed-off-by: Thomas Rieschl <thomas@trinet.at>

add a dummy AbstractValidatorChain class to keep bc

a0319f9

Signed-off-by: Thomas Rieschl <thomas@trinet.at>

boesing force-pushed the feat/php8 branch from 24bea37 to ba8b68a Compare October 31, 2020 15:16

boesing added 2 commits October 31, 2020 16:25

Add CHANGELOG entry

14af97c

Signed-off-by: Maximilian Bösing <2189546+boesing@users.noreply.github.com>

Add lowest dependencies for PHP 7.4 and 8.0

f633855

Signed-off-by: Maximilian Bösing <2189546+boesing@users.noreply.github.com>

boesing force-pushed the feat/php8 branch from c1b0fc2 to f633855 Compare October 31, 2020 15:25

boesing merged commit 990a9d1 into laminas:2.10.x Oct 31, 2020

PHP 8.0 automation moved this from Reviewer approved to Done Oct 31, 2020

rieschl mentioned this pull request Dec 10, 2020

Add PHP 8.0 support laminas/laminas-form#75

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

PHP 8 support #35

PHP 8 support #35

rieschl commented Oct 22, 2020 •

edited

Loading

boesing left a comment

boesing commented Oct 22, 2020 •

edited

Loading

Ocramius commented Oct 22, 2020

rieschl commented Oct 22, 2020

boesing commented Oct 22, 2020

rieschl commented Oct 22, 2020

rieschl commented Oct 23, 2020

boesing commented Oct 23, 2020

rieschl commented Oct 23, 2020

rieschl commented Oct 23, 2020 •

edited

Loading

boesing commented Oct 31, 2020

rieschl commented Oct 31, 2020

rieschl commented Oct 31, 2020

boesing commented Oct 31, 2020

rieschl commented Oct 31, 2020

boesing left a comment

boesing commented Oct 31, 2020

boesing commented Oct 31, 2020

froschdesign commented Oct 31, 2020

PHP 8 support #35

PHP 8 support #35

Conversation

rieschl commented Oct 22, 2020 • edited Loading

Description

ToDo:

boesing left a comment

Choose a reason for hiding this comment

boesing commented Oct 22, 2020 • edited Loading

Ocramius commented Oct 22, 2020

rieschl commented Oct 22, 2020

boesing commented Oct 22, 2020

rieschl commented Oct 22, 2020

rieschl commented Oct 23, 2020

boesing commented Oct 23, 2020

rieschl commented Oct 23, 2020

rieschl commented Oct 23, 2020 • edited Loading

boesing commented Oct 31, 2020

rieschl commented Oct 31, 2020

rieschl commented Oct 31, 2020

boesing commented Oct 31, 2020

rieschl commented Oct 31, 2020

boesing left a comment

Choose a reason for hiding this comment

boesing commented Oct 31, 2020

boesing commented Oct 31, 2020

froschdesign commented Oct 31, 2020

rieschl commented Oct 22, 2020 •

edited

Loading

boesing commented Oct 22, 2020 •

edited

Loading

rieschl commented Oct 23, 2020 •

edited

Loading