php 8 Support [WIP]

[svbackend]

Hello, I will try to update tests to support php 8, just started with bumping versions in composer.json and cleaning up travis builds, but need help from maintainers to keep going, I have couple of questions:

1. Can someone please create a new branch for php8 as it will be not backward compatible with enq 0.10.x, so next major version (0.11.x) will be merged there?

2. What's the best possible way to update docker, as I need php8 but docker images that used in this project do not support php 8 yet (https://github.com/makasim/docker-nginx-php-fpm - latest is 7.3 pushed more than year ago) so what would you suggest - create PR to https://github.com/makasim/docker-nginx-php-fpm with php8 image and wait till it will be pushed to registry or better to put local Dockerfile right into this repository?

Roadmap:

• Add docker php 8 image
• Check that composer dependencies installing without issues
• Update packages if needed to support php 8
• Update codebase to comply with new packages requirements
• Update tests, make sure everything works

[Steveb-p]

Is there any particular reason why you'd like to drop support for PHP7? Any particular functionality?

The thing is, codebase-wise those two branches (0.10 and 0.11) would look really similar, while you'd have to maintain both since one would only work for PHP7 and the other for PHP8 - and not everyone wants to switch to newer version for various, often important reasons.

Also, there should be no BC breaks, so it's not really a major in terms of semver (granted, it's an upgrade with new functionality and semver-wise library is still "not stable"). PHP8 and PHP7 aren't that different in terms of language basics, and most PHP7 code is PHP8 compatible.

As for docker images, unfortunately I can't help you with it, since afaik only @makasim can do anything about it.

[svbackend]

@Steveb-p should I just change requirements for php to: "php^7 | php^8" and let composer decide which version to use? But how tests could work if phpunit not backward compatible and thus if I update tests to work on php8 & phpunit 9.5 they will stop work on php 7 & phpunit 7.5, I'm not sure I understand how we can support both versions in the same branch?

[Steveb-p]

Exactly, just let composer decide on it's own. You can be more explicit and specify php: "^7.3 | ^8.0" or php: ">=7.3". If tests include composer.lock file then just create one for each major php version and rename them before tests for a version are run.

if phpunit not backward compatible and thus if I update tests to work on php8 & phpunit 9.5 they will stop work on php 7 & phpunit 7.5

That's incorrect. Phpunit 9 is backwards incompatible with Phpunit 8 in the sense that code written for Phpunit 8 might not work on Phpunit 9. To be precise, Phpunit authors are free to make changes that are usually "forbidden", i.e. changing or removing methods / interfaces, etc.
Phpunit 9 is specifically compatible with PHP 7.3 and up. You can find this information in their composer.json declaration: https://github.com/sebastianbergmann/phpunit/blob/5702c50e0490f913684ffc02c34901278428c17f/composer.json#L24

This means once you upgrade Phpunit and related test code to version 9, it will be perfectly ok to run on both PHP 7 and 8.

To be frank, most changes that are BC breaks in php 8 occured in internals. Most PHP7 code will run PHP8 just fine (with a few specifically noted exceptions).

[svbackend]

@Steveb-p thank you! That's really helpful, ok so I'm not even blocked because of docker image now as I can just update phpunit to newer version and make sure it work on current php 7.3 setup while we're waiting for some information about docker php 8 image

[svbackend]

Current issue:

PHP Fatal error:  Declaration of Interop\Queue\Spec\SendAndReceiveDelayedMessageFromQueueSpec::tearDown() must be compatible with PHPUnit\Framework\TestCase::tearDown(): void in /mqdev/vendor/queue-interop/queue-spec/src/SendAndReceiveDelayedMessageFromQueueSpec.php on line 71

Need to update https://github.com/queue-interop/queue-spec to be compatible with phpunit 9.5 first, will do a PR

upd: queue-interop/queue-spec#2

upd2: Tests results currently looks like this:
ERRORS!
Tests: 3251, Assertions: 5637, Errors: 309, Failures: 10, Warnings: 52, Skipped: 1, Incomplete: 2, Risky: 142.

[svbackend]

@Steveb-p how do you think, would be ok to just remove tests like this that rely on private properties?

For example in test below we are creating RouterProcessor and in constructor it takes "DriverInterface $driver" and sets it into "private $driver" property, and then this test just checks this that this property was initialized with given driver instance:

public function testCouldBeConstructedWithDriver()
    {
        $driver = $this->createDriverStub(); // DriverInterface

        $processor = new RouterProcessor($driver);

        $this->assertAttributeSame($driver, 'driver', $processor); // <-- This method was removed. 
        // it checks does $processor->driver === $driver
    }

I'm curious because assertAttributeSame actually was deprecated and removed from phpunit 9.5 (see sebastianbergmann/phpunit#3338) therefore currently there's couple of options to replace this -

1. add getter (getDriver()) and use it in test
2. Use reflection to read private property
3. Remove tests that rely on private props instead of object public api

I personally would prefer to remove such tests as they don't really testing anything, if something like this brakes we will see it from other tests that trying to use this private property, and we will see pretty specific error message "trying to access $driver property which is not initialized yet"

[svbackend]

Current progress:
ERRORS!
Tests: 3250, Assertions: 5851, Errors: 265, Failures: 10, Warnings: 52, Skipped: 1, Incomplete: 2, Risky: 144.

[makasim]

You could remove such tests. That's fine

[svbackend]

ERRORS!
Tests: 3240, Assertions: 6027, Errors: 195, Failures: 10, Warnings: 52, Skipped: 1, Incomplete: 2, Risky: 144.

[svbackend]

ERRORS!
Tests: 3240, Assertions: 6079, Errors: 150, Failures: 10, Warnings: 52, Skipped: 1, Incomplete: 2, Risky: 144.

[martinssipenko]

@svbackend would you be open to any help with this?

[svbackend]

@martinssipenko sure, it's pretty easy to do it in parallel, I'm currently updating tests for Stomp client, just let me know on which part you want to work so I won't touch it

https://github.com/php-enqueue/enqueue-dev/tree/master/pkg/stomp

[Jurigag]

I guess i could also help a bit, we use enqueue/sns, enqueue/snsqs, enqueue/sqs in our codebase.

[andrewmy]

@svbackend please check out adspons#1 — WIP but let's discuss it there not relevant any more, everything done there already merged

[andrewmy]

Extracted the phpunit part of work to a separate PR to upstream (here) — #1131 — hope it gets us to PHP 8 quicker

[Jurigag]

Can you rebase your branch @svbackend so you get changes from @andrewmy work?

[andrewmy]

Continuing here #1132