Add PHP 8 support #8

geerteltink · 2020-10-11T11:14:17Z

Update PHP constraint to ^7.3 || ~8.0.0
Update PHPUnit constraint to ^9.3
Remove constraint for phpspec/prophecy
Update Travis job matrix to cover 7.3, 7.4, and 8.0
Migrate tests from Prophecy to PHPUnit mocks
Update to coding standard constraint ~2.1.0

Fixes #7

Signed-off-by: Geert Eltink <geert.eltink@gmail.com>

geerteltink · 2020-10-11T11:16:55Z

src/ConfigProvider.php

@@ -14,13 +14,15 @@
 * Configuration provider for the package.
 *
 * @see https://docs.laminas.dev/laminas-component-installer/
+ *
+ * phpcs:disable WebimpressCodingStandard.PHP.DisallowFqn.FileName


38 | ERROR | Zend\ProblemDetails\ProblemDetailsMiddleware must be imported but file with name ProblemDetailsMiddleware exists in the namespace 39 | ERROR | Zend\ProblemDetails\ProblemDetailsNotFoundHandler must be imported but file with name ProblemDetailsNotFoundHandler exists in the namespace 40 | ERROR | Zend\ProblemDetails\ProblemDetailsResponseFactory must be imported but file with name ProblemDetailsResponseFactory exists in the namespace

@geerteltink Not sure how WebimpressCodingStandard.PHP.DisallowFqn.FileName is related to the issue you have reported ;-)

@michalbundyra Ah, I tagged the wrong line. This is causing:

FILE: src/ConfigProvider.php ---------------------------------------------------------------------------------------------------------------------------------------------------------- FOUND 3 ERRORS AFFECTING 3 LINES ---------------------------------------------------------------------------------------------------------------------------------------------------------- 38 | ERROR | Zend\ProblemDetails\ProblemDetailsMiddleware must be imported but file with name ProblemDetailsMiddleware exists in the namespace 39 | ERROR | Zend\ProblemDetails\ProblemDetailsNotFoundHandler must be imported but file with name ProblemDetailsNotFoundHandler exists in the namespace 40 | ERROR | Zend\ProblemDetails\ProblemDetailsResponseFactory must be imported but file with name ProblemDetailsResponseFactory exists in the namespace ----------------------------------------------------------------------------------------------------------------------------------------------------------

What I've done in other packages is to import these classes at the top of the file, but with the prefix "Legacy":

use Zend\ProblemDetails\ProblemDetailsResponseFactory as LegacyProblemDetailsResponseFactory;

When you reference them later:

LegacyProblemDetailsResponseFactory::class

will then work, and you no longer get those phpcs issues. It also makes it clear why we are importing them.

geerteltink · 2020-10-11T11:18:13Z

src/Exception/ProblemDetailsExceptionInterface.php

-    public function toArray() : array;
+    public function toArray(): array;
+
+    public function jsonSerialize(): array;


This is a fix for PhpUnit Mock. I'm not sure if I did something wrong, but without this it complaints about not being able to create a mock for jsonSerialize because it's an abstract method.

@weierophinney any idea on how to fix this properly?

geerteltink · 2020-10-11T11:22:57Z

test/ProblemDetailsResponseFactoryTest.php

    private $response;

    /** @var ProblemDetailsResponseFactory */
    private $factory;

    private const UTF_8_INVALID_2_OCTET_SEQUENCE = "\xc3\x28";

-    protected function setUp() : void
+    protected function setUp(): void


There are about 26 phpunit errors left in this file and I don't know where it is coming from:

Return value of PHPUnit\Framework\Constraint\Callback::matches() must be of the type bool, string returned

@weierophinney any idea what I miss here?

Signed-off-by: Geert Eltink <geert.eltink@gmail.com>

boesing · 2020-10-11T20:00:06Z

TBH: Its quite hard to review a PR which is meant to be a PHP upgrade with lots of codestyle changes.
I'd prefer having a dedicated CS 2.0 PR rather than combined with PHP 8.0.

geerteltink · 2020-10-12T07:11:00Z

TBH: Its quite hard to review a PR which is meant to be a PHP upgrade with lots of codestyle changes.
I'd prefer having a dedicated CS 2.0 PR rather than combined with PHP 8.0.

I can change the title if you want :D

I agree I could have made 2 MR's but after merging one, there I have to fix a ton of merge conflicts in the other. Besides that, in src it's code style only and in test it's mostly the change from prophecy to mock. If you think the MR is un reviewable, please close it and I'll make another one.

geerteltink · 2020-10-13T20:51:13Z

src/ProblemDetailsResponseFactory.php

@@ -4,6 +4,8 @@
 * @see       https://github.com/mezzio/mezzio-problem-details for the canonical source repository
 * @copyright https://github.com/mezzio/mezzio-problem-details/blob/master/COPYRIGHT.md
 * @license   https://github.com/mezzio/mezzio-problem-details/blob/master/LICENSE.md New BSD License
+ *
+ * phpcs:disable WebimpressCodingStandard.Namespaces.UnusedUseStatement.UnusedUse


Reported in webimpress/coding-standard#118

JSON_PRETTY_PRINT is not detected as a used contants.

@michalbundyra this is where the issue is.

Yeah, I know ;-) I have a fix already, will release tomorrow, thanks 👍

@geerteltink it has been resolved now in webimpress/coding-standard - 1.1.6 release.

I saw your new release. I'll update this PR with the change soon.

weierophinney

Looks sane; I don't see any BC breaks.

I've left some feedback on how to address the legacy ZF classes in the `ConfigProvider.

weierophinney · 2020-10-14T13:39:18Z

src/ConfigProvider.php

@@ -14,13 +14,15 @@
 * Configuration provider for the package.
 *
 * @see https://docs.laminas.dev/laminas-component-installer/
+ *
+ * phpcs:disable WebimpressCodingStandard.PHP.DisallowFqn.FileName


What I've done in other packages is to import these classes at the top of the file, but with the prefix "Legacy":

use Zend\ProblemDetails\ProblemDetailsResponseFactory as LegacyProblemDetailsResponseFactory;

When you reference them later:

LegacyProblemDetailsResponseFactory::class

will then work, and you no longer get those phpcs issues. It also makes it clear why we are importing them.

tux-rampage · 2020-11-20T15:03:23Z

composer.json

@@ -42,9 +38,8 @@
        "willdurand/negotiation": "^2.3"


This dependency is not compatible with PHP 8 - therfore this patch isn't as well.

It declares a class name which is a keyword in PHP 8: https://github.com/willdurand/Negotiation/blob/2.x/src/Negotiation/Match.php

It must be ^3.0 to support PHP 8 - See willdurand/Negotiation#106

boesing · 2020-12-04T23:16:53Z

We need willdurand/negotiation v3 support in here to properly support PHP 8.0 as stated in #10
Are you still on this @geerteltink or shall we take over?

acelaya · 2021-01-22T18:57:43Z

Any way I can help push this forward?

Ocramius · 2021-01-23T15:16:13Z

@acelaya you can take the branch, add willdurand/negotiation:^3 support, and then we'd merge that.

samsonasik · 2021-01-23T15:19:02Z

.travis.yml

      env:
        - DEPS=lowest
-    - php: 7.3
+    - php: nightly


8.0 can be used

acelaya · 2021-01-23T15:24:11Z

Sounds good. I can do that.

acelaya · 2021-01-23T15:50:26Z

I have just created a new PR #11, which includes this branch, plus the latest changes requested here.

acelaya · 2021-01-23T18:16:33Z

@Ocramius @samsonasik I did all required changes in #11 to make the build green. Feel free to review there.

Ocramius · 2021-01-24T20:59:54Z

Closing here: commits ported to #11 for now :-)

geerteltink added 2 commits October 11, 2020 12:51

feat: add PHP 8 support

f86bec0

Signed-off-by: Geert Eltink <geert.eltink@gmail.com>

feat: laminas coding standard 2

a16e48a

Signed-off-by: Geert Eltink <geert.eltink@gmail.com>

geerteltink added the Enhancement New feature or request label Oct 11, 2020

geerteltink added this to the 1.3.0 milestone Oct 11, 2020

geerteltink commented Oct 11, 2020

View reviewed changes

fix: use correct declaration

d3381ff

Signed-off-by: Geert Eltink <geert.eltink@gmail.com>

geerteltink commented Oct 13, 2020

View reviewed changes

weierophinney requested changes Oct 14, 2020

View reviewed changes

tux-rampage reviewed Nov 20, 2020

View reviewed changes

shaunfreeman mentioned this pull request Dec 3, 2020

PHP 8.0.0 error due to incompatable dependency version #10

Closed

acelaya mentioned this pull request Dec 4, 2020

PHP 8 support shlinkio/shlink#856

Closed

28 tasks

samsonasik reviewed Jan 23, 2021

View reviewed changes

.travis.yml

env:

- DEPS=lowest

- php: 7.3

- php: nightly

Copy link

Member

samsonasik Jan 23, 2021

Choose a reason for hiding this comment

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

8.0 can be used

acelaya reacted with thumbs up emoji

acelaya mentioned this pull request Jan 23, 2021

PHP 8 support #11

Merged

Ocramius self-assigned this Jan 24, 2021

Ocramius added the Duplicate This issue or pull request already exists label Jan 24, 2021

Ocramius closed this Jan 24, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add PHP 8 support #8

Add PHP 8 support #8

geerteltink commented Oct 11, 2020 •

edited

Loading

geerteltink Oct 11, 2020 •

edited

Loading

michalbundyra Oct 13, 2020

geerteltink Oct 13, 2020

weierophinney Oct 14, 2020

geerteltink Oct 11, 2020

geerteltink Oct 11, 2020

boesing commented Oct 11, 2020

geerteltink commented Oct 12, 2020

geerteltink Oct 13, 2020

geerteltink Oct 13, 2020

michalbundyra Oct 13, 2020

michalbundyra Oct 14, 2020

geerteltink Oct 14, 2020

weierophinney left a comment

weierophinney Oct 14, 2020

tux-rampage Nov 20, 2020

boesing commented Dec 4, 2020

acelaya commented Jan 22, 2021

Ocramius commented Jan 23, 2021

samsonasik Jan 23, 2021

acelaya commented Jan 23, 2021

acelaya commented Jan 23, 2021

acelaya commented Jan 23, 2021

Ocramius commented Jan 24, 2021

Add PHP 8 support #8

Add PHP 8 support #8

Conversation

geerteltink commented Oct 11, 2020 • edited Loading

geerteltink Oct 11, 2020 • edited Loading

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

boesing commented Oct 11, 2020

geerteltink commented Oct 12, 2020

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

weierophinney left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

boesing commented Dec 4, 2020

acelaya commented Jan 22, 2021

Ocramius commented Jan 23, 2021

Choose a reason for hiding this comment

acelaya commented Jan 23, 2021

acelaya commented Jan 23, 2021

acelaya commented Jan 23, 2021

Ocramius commented Jan 24, 2021

geerteltink commented Oct 11, 2020 •

edited

Loading

geerteltink Oct 11, 2020 •

edited

Loading