Add PHP 8.1 support #533

javer · 2021-07-29T17:55:18Z

Fixes #531

alexpott · 2021-07-31T23:45:50Z

Thanks @javer for working on this. I've been testing this PR using Drupal's tests and it works great. Deprecation messages that were incorrectly triggered when mocking a class that implements \Iterator are no longer triggered. Nice one. 👍

alexpott · 2021-08-01T19:34:42Z

There's one issue that's come up from testing on Drupal. If you have a interface that extends \Traversable then on PHP 8.1 creating a double will trigger deprecation notices. If you add ->willImplement(\IteratorAggregator::class) then the deprecation notices go away. I think this is because \Traversable needs the object to implement either \IteratorAggregator or \Iterator - so it's a bit of an odd one.

ciaranmcnulty · 2021-08-01T19:46:37Z

composer.json

@@ -18,15 +18,15 @@
    ],

    "require": {
-        "php":                               "^7.2 || ~8.0, <8.1",
+        "php":                               "^7.2 || ^8.0",


I'd rather we didn't do this. It can be ~8.0, <8.2 once we've had a chance to test

Changed to ^7.2 || ~8.0, <8.2

Is there a reason why 8.2 is not supported? 🤔

ciaranmcnulty · 2021-08-01T19:47:21Z

spec/Prophecy/Argument/Token/ArrayEntryTokenSpec.php

@@ -113,8 +113,8 @@ function it_accepts_any_key_token_type_to_score_object_that_is_both_traversable_
            return 'value';
        });
        $object->key()->willReturn('key');
-        $object->rewind()->willReturn(null);
-        $object->next()->willReturn(null);
+        (\PHP_VERSION_ID < 80100) ? $object->rewind()->willReturn(null) : $object->rewind()->shouldBeCalled();


Can you explain what's causing this change from a stub to a mock?

Since PHP 8.1 many internal classes have got correct return types: https://wiki.php.net/rfc/internal_method_return_types

Now ArrayIterator::rewind() has return type void: https://github.com/php/php-src/blob/master/ext/spl/spl_array.stub.php#L201
That's why it cannot return null, and tests fail: https://github.com/javer/prophecy/runs/3215333259?check_suite_focus=true#step:6:27

Prophecy/Argument/Token/ArrayEntryToken 104 - it accepts any key token type to score object that is both traversable and array accessible the method "rewind" has a void return type, and so cannot return anything

shouldBeCalled() will make things break if the iterator is never used. I think we need something like canBeCalled() instead

ciaranmcnulty

I'd rather we didn't tag a version as working with 8.1 when there isn't yet an RC (and therefore more changes could occur).

Many of the other changes look ok, although I'm concerned in some places the tests have been changed rather than the implementation

javer · 2021-08-01T21:04:23Z

@ciaranmcnulty Feature freeze for PHP 8.1 was done on Jul 20 2021, so no changes in the language specs are expected since that moment.

Many changes in the tests related to the fact that they are checking implementation of the internal PHP classes which have been changed significantly in PHP 8.1, see:

So since PHP 8.1 many such tests became incorrect, because behavior of the tested methods was changed inside PHP.

I suggest not to wait for RC, because this library is special in the sense that 99% PHP packages indirectly depend on it, because all of them use phpunit/phpunit for unit testing, and all phpunit/phpunit versions except unreleased 10.x depends on this library.

In other words, missing support for PHP 8.1 in this library blocks almost all packages even to test whether they support PHP 8.1 or not, because all of them at least need to put this hack to CI flow https://github.com/php-amqplib/php-amqplib/pull/929/files#diff-ea12f60188cdd90bc99e5d0af2eb91647bbe4a9199176aa1ec5240f65efed510R79 and then they see many failed tests just because of incompatibility of this library with PHP 8.1, but not because of their failures.

javer · 2021-08-01T21:44:07Z

@alexpott Please check whether doubling \Traversable doesn't trigger deprecation notices with the latest changes regardless of the willImplement().

alexpott · 2021-08-01T22:20:18Z

~~@javer nope the recent changes don't appear to have changed the outcome of doing~~

    $t = $this->prophesize(\Traversable::class);
    $t->reveal();

~~This still result in the follow deprecation notices:~~

  1x: Return type of Double\Traversable\P1::current() should either be compatible with Iterator::current(): mixed, or the #[ReturnTypeWillChange] attribute should be used to temporarily suppress the notice

  1x: Return type of Double\Traversable\P1::next() should either be compatible with Iterator::next(): void, or the #[ReturnTypeWillChange] attribute should be used to temporarily suppress the notice

  1x: Return type of Double\Traversable\P1::key() should either be compatible with Iterator::key(): mixed, or the #[ReturnTypeWillChange] attribute should be used to temporarily suppress the notice

  1x: Return type of Double\Traversable\P1::valid() should either be compatible with Iterator::valid(): bool, or the #[ReturnTypeWillChange] attribute should be used to temporarily suppress the notice

  1x: Return type of Double\Traversable\P1::rewind() should either be compatible with Iterator::rewind(): void, or the #[ReturnTypeWillChange] attribute should be used to temporarily suppress the notice

~~in a test using PHPUnit with phpspec/prophecy-phpunit.~~

alexpott · 2021-08-01T22:29:32Z

@javer nope the recent changes don't appear to have changed the outcome of doing

    $t = $this->prophesize(\Traversable::class);
    $t->reveal();

This still result in the follow deprecation notices:

  1x: Return type of Double\Traversable\P1::current() should either be compatible with Iterator::current(): mixed, or the #[ReturnTypeWillChange] attribute should be used to temporarily suppress the notice

  1x: Return type of Double\Traversable\P1::next() should either be compatible with Iterator::next(): void, or the #[ReturnTypeWillChange] attribute should be used to temporarily suppress the notice

  1x: Return type of Double\Traversable\P1::key() should either be compatible with Iterator::key(): mixed, or the #[ReturnTypeWillChange] attribute should be used to temporarily suppress the notice

  1x: Return type of Double\Traversable\P1::valid() should either be compatible with Iterator::valid(): bool, or the #[ReturnTypeWillChange] attribute should be used to temporarily suppress the notice

  1x: Return type of Double\Traversable\P1::rewind() should either be compatible with Iterator::rewind(): void, or the #[ReturnTypeWillChange] attribute should be used to temporarily suppress the notice

in a test using PHPUnit with phpspec/prophecy-phpunit.

~~@javer nope the recent changes don't appear to have changed the outcome of doing~~

    $t = $this->prophesize(\Traversable::class);
    $t->reveal();

~~This still result in the follow deprecation notices:~~

  1x: Return type of Double\Traversable\P1::current() should either be compatible with Iterator::current(): mixed, or the #[ReturnTypeWillChange] attribute should be used to temporarily suppress the notice

  1x: Return type of Double\Traversable\P1::next() should either be compatible with Iterator::next(): void, or the #[ReturnTypeWillChange] attribute should be used to temporarily suppress the notice

  1x: Return type of Double\Traversable\P1::key() should either be compatible with Iterator::key(): mixed, or the #[ReturnTypeWillChange] attribute should be used to temporarily suppress the notice

  1x: Return type of Double\Traversable\P1::valid() should either be compatible with Iterator::valid(): bool, or the #[ReturnTypeWillChange] attribute should be used to temporarily suppress the notice

  1x: Return type of Double\Traversable\P1::rewind() should either be compatible with Iterator::rewind(): void, or the #[ReturnTypeWillChange] attribute should be used to temporarily suppress the notice

~~in a test using PHPUnit with phpspec/prophecy-phpunit.~~

@javer sorry I was wrong - cause of general awkwardness testing a PR with composer I missed the changes to src/Prophecy/Doubler/ClassPatch/TraversablePatch.php - your recent changes work great. Thanks!

javer · 2021-08-02T07:01:46Z

Added spec for testing new (and old) behavior of TraversablePatch, the build is green: https://github.com/javer/prophecy/runs/3218018475?check_suite_focus=true

alecpl · 2021-08-03T05:40:58Z

.github/workflows/build.yml

@@ -29,6 +34,10 @@ jobs:
            php-version: "${{ matrix.php }}"
            coverage: none

+      -   name: Configure for PHP 8.1
+          run: composer config platform.php 8.0.99


I think the point of this PR is that we don't need this step. It should work without this. Or is there any dependency not compatible with 8.1?

When other libraries depend on this one - yes, you are right. But this library itself has dev-dependency on phpspec/phpspec, which requires php: ^7.3 || 8.0.*.

prophecy/composer.json

Lines 28 to 31 in be1996e

"require-dev": {

"phpspec/phpspec": "^6.0",

"phpunit/phpunit": "^8.0 || ^9.0"

},

There is some kind of circular dependency phpspec -> prophecy -> phpspec, so one of these libraries should be fixed first, and because prophecy is runtime dependency for phpspec - it should be prophecy. After merging this PR and tagging the release we will be able to test phpspec and fix it to support PHP 8.1, release a new version and only after this specified above lines can be removed.

# composer install Problem 1 - phpspec/phpspec 6.0.0 requires php ^7.2, <7.4 -> your php version (8.1.0beta1) does not satisfy that requirement. - phpspec/phpspec[6.1.0, ..., 6.2.2] require php ^7.2, <7.5 -> your php version (8.1.0beta1) does not satisfy that requirement. - phpspec/phpspec[6.3.0, ..., 6.3.1] require php ^7.2 || 8.0.* -> your php version (8.1.0beta1) does not satisfy that requirement. - phpspec/phpspec[7.0.0, ..., 7.1.0] require php ^7.3 || 8.0.* -> your php version (8.1.0beta1) does not satisfy that requirement. - Root composer.json requires phpspec/phpspec ^6.0 || ^7.0 -> satisfiable by phpspec/phpspec[6.0.0, ..., 6.3.1, 7.0.0, 7.0.1, 7.1.0].

Ok, cross-ref: phpspec/phpspec#1387

ciaranmcnulty · 2021-08-03T07:28:53Z

I really don't want to tag a release of Prophecy that says it supports 8.1 before we've tested against a release candidate. If it's screwing up PHPUnit then I could maybe be persuaded (cc @sebastianbergmann)

Is there a reason libraries wanting to test can't use composer's ignore-platform-req=php flag?

sebastianbergmann · 2021-08-03T07:36:10Z

PHPUnit 8.5 and PHPUnit 9.5 still depend on Prophecy. This leads to

$ composer require --dev phpunit/phpunit ^8.5
./composer.json has been created
Running composer update phpunit/phpunit
Loading composer repositories with package information
Updating dependencies
Your requirements could not be resolved to an installable set of packages.

  Problem 1
    - phpunit/phpunit[8.5.12, ..., 8.5.19] require phpspec/prophecy ^1.10.3 -> satisfiable by phpspec/prophecy[v1.10.3, ..., 1.13.0].
    - phpunit/phpunit[8.5.0, ..., 8.5.11] require php ^7.2 -> your php version (8.1.0-dev) does not satisfy that requirement.
    - phpspec/prophecy[1.11.0, ..., 1.11.1] require php ^7.2 -> your php version (8.1.0-dev) does not satisfy that requirement.
    - phpspec/prophecy v1.10.3 requires php ^5.3|^7.0 -> your php version (8.1.0-dev) does not satisfy that requirement.
    - phpspec/prophecy[1.12.0, ..., 1.13.0] require php ^7.2 || ~8.0, <8.1 -> your php version (8.1.0-dev) does not satisfy that requirement.
    - Root composer.json requires phpunit/phpunit ^8.5 -> satisfiable by phpunit/phpunit[8.5.0, ..., 8.5.19].


Installation failed, deleting ./composer.json.

and

$ composer require --dev phpunit/phpunit ^9.5
./composer.json has been created
Running composer update phpunit/phpunit
Loading composer repositories with package information
Updating dependencies
Your requirements could not be resolved to an installable set of packages.

  Problem 1
    - phpunit/phpunit[9.5.0, ..., 9.5.8] require phpspec/prophecy ^1.12.1 -> satisfiable by phpspec/prophecy[1.12.1, 1.12.2, 1.13.0].
    - phpspec/prophecy[1.12.1, ..., 1.13.0] require php ^7.2 || ~8.0, <8.1 -> your php version (8.1.0-dev) does not satisfy that requirement.
    - Root composer.json requires phpunit/phpunit ^9.5 -> satisfiable by phpunit/phpunit[9.5.0, ..., 9.5.8].


Installation failed, deleting ./composer.json.

respectively.

PHPUnit 10, which is still in development, no longer depends on Prophecy.

ciaranmcnulty · 2021-08-03T07:37:41Z

@sebastianbergmann I'm happy to do whatever makes your life easier, but is there a reason --ignore-platform-reqs=php doesn't solve this issue?

sebastianbergmann · 2021-08-03T07:41:29Z

I think having to use --ignore-platform-reqs=php because of Prophecy should be fine until PHP 8.1 RC, PHP 8.1 GA at the latest. After that, I do not think it would be acceptable anymore.

In other words: are you planning to support PHP 8.1 in Prophecy and if so, when?

alecpl · 2021-08-03T07:46:23Z

With all respect. The number of changes in this PR shows clearly to me that ignore-platform-req=php is not a complete fix. It might depend on what features phpunit and peoples tests are using.

sebastianbergmann · 2021-08-03T07:56:12Z

It might depend on what features phpunit and peoples tests are using.

PHPUnit depends on Prophecy to provide the TestCase::prophesize() method:

protected function prophesize($classOrInterface = null): ObjectProphecy
{
    if (is_string($classOrInterface)) {
        $this->recordDoubledType($classOrInterface);
    }

    return $this->getProphet()->prophesize($classOrInterface);
}

And in the TestCase::verifyMockObjects() method, PHPUnit uses Prophecy like so:

if ($this->prophet !== null) {
    try {
        $this->prophet->checkPredictions();
    } finally {
        foreach ($this->prophet->getProphecies() as $objectProphecy) {
            foreach ($objectProphecy->getMethodProphecies() as $methodProphecies) {
                foreach ($methodProphecies as $methodProphecy) {
                    assert($methodProphecy instanceof MethodProphecy);

                    $this->numAssertions += count($methodProphecy->getCheckedPredictions());
                }
            }
        }
    }
}

Many years ago, I thought that it would be a good idea to 1) have PHPUnit depend on Prophecy to make the latter always available when PHPUnit is availabe and 2) provide a convenient integration of Prophecy with PHPUnit. This was deprecated in PHPUnit 9.1 and has been removed for PHPUnit 10.

ADmad · 2021-08-03T12:42:34Z

I really don't want to tag a release of Prophecy that says it supports 8.1 before we've tested against a release candidate.

You can merge this into a dev branch instead of master then. That way the whole downstream deps chain isn't blocked from upgrading/trying out their code with PHP 8.1.

ciaranmcnulty · 2021-08-10T08:57:21Z

I've added some issues for things we'll need to check on PHP 8.1

https://github.com/phpspec/prophecy/issues?q=is%3Aopen+is%3Aissue+label%3APHP8.1

Any extra issues people can think of, please raise them

ciaranmcnulty · 2021-08-10T08:58:04Z

@ADmad

You can merge this into a dev branch instead of master then. That way the whole downstream deps chain isn't blocked from upgrading/trying out their code with PHP 8.1.

If someone can depend on a dev branch instead, they can use the --ignore-platform-reqs=php flag too

ciaranmcnulty · 2021-08-10T08:58:40Z

@sebastianbergmann

In other words: are you planning to support PHP 8.1 in Prophecy and if so, when?

I'm happy to start testing against it and would aim to tag a release around the RC stage

sebastianbergmann · 2021-08-10T09:27:05Z

@sebastianbergmann In other words: are you planning to support PHP 8.1 in Prophecy and if so, when?

PHPUnit 8.5 and PHPUnit 9.5 are supported on PHP 8.1 and depend on Prophecy. I am not sure what you mean by "are you planning to support PHP 8.1 in Prophecy", though.

ciaranmcnulty · 2021-08-10T09:34:13Z

@sebastianbergmann Sorry that was meant to be quoting your question from upthread and then answering it

ciaranmcnulty · 2021-08-10T09:51:33Z

Thanks, I'm going to merge this so dev-master is 'compatible', but won't tag a release until the outstanding issues are closed off

ADmad · 2021-08-10T10:21:21Z

If someone can depend on a dev branch instead, they can use the --ignore-platform-reqs=php flag too

#533 (comment)

ciaranmcnulty · 2021-08-10T10:23:22Z

Thanks for the work @javer

ciaranmcnulty · 2021-09-06T10:10:59Z

It's demoralising to have so many on here emphasising how important it is for Prophecy to support 8.1, but no work has been done on any of the 8.1 issues in the month since, nor has anyone reviewed or commented on #538

It'll all eventually get done, of course, but at the moment it's bottlenecked on my time and availability

ciaranmcnulty · 2021-09-10T09:05:31Z

FYI https://github.com/phpspec/prophecy/releases/tag/1.14.0

Progi1984 mentioned this pull request Jul 29, 2021

Added support for PHP 8.1 PHPOffice/PHPPresentation#661

Merged

javer mentioned this pull request Jul 31, 2021

Add PHP 8.1 support php-amqplib/php-amqplib#929

Merged

ciaranmcnulty reviewed Aug 1, 2021

View reviewed changes

javer force-pushed the php81 branch from 73eed8d to 553076e Compare August 1, 2021 21:39

Add PHP 8.1 support

3b22bd6

javer force-pushed the php81 branch from 553076e to 3b22bd6 Compare August 2, 2021 06:56

oliverklee mentioned this pull request Aug 2, 2021

Test with PHP 8.1 as well t-regx/phpunit-data-provider#52

Merged

ruudk mentioned this pull request Aug 2, 2021

Test with PHP 8.1 LongRunning/LongRunning#62

Merged

BenMorel mentioned this pull request Aug 2, 2021

PHP 8.1 support / composer #531

Closed

alecpl reviewed Aug 3, 2021

View reviewed changes

ciaranmcnulty merged commit 2052039 into phpspec:master Aug 10, 2021

asgrim mentioned this pull request Aug 24, 2021

Added PHP 8.1 support scoutapp/scout-apm-php#235

Merged

alexander-schranz mentioned this pull request Sep 2, 2021

Add PHP 8.1 Support sulu/sulu#6208

Merged

11 tasks

javer mentioned this pull request Sep 16, 2021

PHP 8.1 compatibility googleapis/google-api-php-client#2121

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add PHP 8.1 support #533

Add PHP 8.1 support #533

javer commented Jul 29, 2021

alexpott commented Jul 31, 2021

alexpott commented Aug 1, 2021

ciaranmcnulty Aug 1, 2021

javer Aug 1, 2021

Dgame Nov 25, 2021

ciaranmcnulty Aug 1, 2021

javer Aug 1, 2021

stof Sep 6, 2021

ciaranmcnulty left a comment

javer commented Aug 1, 2021

javer commented Aug 1, 2021

alexpott commented Aug 1, 2021 •

edited

Loading

alexpott commented Aug 1, 2021

javer commented Aug 2, 2021

alecpl Aug 3, 2021 •

edited

Loading

javer Aug 3, 2021

alecpl Aug 3, 2021

ciaranmcnulty commented Aug 3, 2021

sebastianbergmann commented Aug 3, 2021

ciaranmcnulty commented Aug 3, 2021

sebastianbergmann commented Aug 3, 2021

alecpl commented Aug 3, 2021

sebastianbergmann commented Aug 3, 2021

ADmad commented Aug 3, 2021

ciaranmcnulty commented Aug 10, 2021

ciaranmcnulty commented Aug 10, 2021

ciaranmcnulty commented Aug 10, 2021 •

edited

Loading

sebastianbergmann commented Aug 10, 2021

ciaranmcnulty commented Aug 10, 2021

ciaranmcnulty commented Aug 10, 2021

ADmad commented Aug 10, 2021

ciaranmcnulty commented Aug 10, 2021

ciaranmcnulty commented Sep 6, 2021

ciaranmcnulty commented Sep 10, 2021

	"require-dev": {
	"phpspec/phpspec": "^6.0",
	"phpunit/phpunit": "^8.0 \|\| ^9.0"
	},

Add PHP 8.1 support #533

Add PHP 8.1 support #533

Conversation

javer commented Jul 29, 2021

alexpott commented Jul 31, 2021

alexpott commented Aug 1, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ciaranmcnulty left a comment

Choose a reason for hiding this comment

javer commented Aug 1, 2021

javer commented Aug 1, 2021

alexpott commented Aug 1, 2021 • edited Loading

alexpott commented Aug 1, 2021

javer commented Aug 2, 2021

alecpl Aug 3, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ciaranmcnulty commented Aug 3, 2021

sebastianbergmann commented Aug 3, 2021

ciaranmcnulty commented Aug 3, 2021

sebastianbergmann commented Aug 3, 2021

alecpl commented Aug 3, 2021

sebastianbergmann commented Aug 3, 2021

ADmad commented Aug 3, 2021

ciaranmcnulty commented Aug 10, 2021

ciaranmcnulty commented Aug 10, 2021

ciaranmcnulty commented Aug 10, 2021 • edited Loading

sebastianbergmann commented Aug 10, 2021

ciaranmcnulty commented Aug 10, 2021

ciaranmcnulty commented Aug 10, 2021

ADmad commented Aug 10, 2021

ciaranmcnulty commented Aug 10, 2021

ciaranmcnulty commented Sep 6, 2021

ciaranmcnulty commented Sep 10, 2021

alexpott commented Aug 1, 2021 •

edited

Loading

alecpl Aug 3, 2021 •

edited

Loading

ciaranmcnulty commented Aug 10, 2021 •

edited

Loading