PHP 8 Support #39

canvural · 2020-11-01T10:44:52Z

This PR:

Updates dependencies to support PHP 8
Fixes code styles and PHPStan errors

dominikzogg · 2020-11-06T05:51:46Z

src/Dflydev/FigCookies/Cookie.php

    {
-        return new static($name, $value);
+        return new self($name, $value);


class is not final, means this is a behavior change

dominikzogg · 2020-11-06T05:52:42Z

src/Dflydev/FigCookies/Cookies.php

    {
-        return new static(Cookie::listFromCookieString($string));
+        return new self(Cookie::listFromCookieString($string));


class is not final, means this is a behavior change

dominikzogg · 2020-11-06T05:53:02Z

src/Dflydev/FigCookies/Cookie.php


-        /** @var Cookie $cookie */
-        $cookie = new static($cookieName);
+        $cookie = new self($cookieName);


class is not final, means this is a behavior change

this one isn't reverted yet

Indeed, can this one get updated, too?

dominikzogg · 2020-11-06T05:53:19Z

src/Dflydev/FigCookies/SetCookie.php

    {
-        return new static($name, $value);
+        return new self($name, $value);


class is not final, means this is a behavior change

dominikzogg · 2020-11-06T05:53:25Z

src/Dflydev/FigCookies/SetCookie.php


-        /** @var SetCookie $setCookie */
-        $setCookie = new static($cookieName);
+        $setCookie = new self($cookieName);


class is not final, means this is a behavior change

this one isn't reverted yet

Indeed, can this one get updated, too?

dominikzogg · 2020-11-06T05:53:38Z

src/Dflydev/FigCookies/SetCookies.php

    {
-        return new static(array_map(function (string $setCookieString) : SetCookie {
+        return new self(array_map(static function (string $setCookieString): SetCookie {


class is not final, means this is a behavior change

dominikzogg · 2020-11-06T05:53:47Z

src/Dflydev/FigCookies/SetCookies.php

    {
-        return new static(array_map(function (string $setCookieString) : SetCookie {
+        return new self(array_map(static function (string $setCookieString): SetCookie {


class is not final, means this is a behavior change

canvural · 2020-11-26T00:16:03Z

@dominikzogg Is there something else that needs to be done here? Can we merge this?

dominikzogg

@canvural i am only a user like you are, but i tried to help @simensen to review this PR.

dominikzogg · 2020-11-26T05:08:20Z

src/Dflydev/FigCookies/Cookie.php


-        /** @var Cookie $cookie */
-        $cookie = new static($cookieName);
+        $cookie = new self($cookieName);


this one isn't reverted yet

dominikzogg · 2020-11-26T05:09:26Z

src/Dflydev/FigCookies/Cookies.php

    {
        $cookieString = implode('; ', $this->cookies);

-        $request = $request->withHeader(static::COOKIE_HEADER, $cookieString);
+        $request = $request->withHeader(self::COOKIE_HEADER, $cookieString);


changes are low (http spec), but also this constant could be overridden

I think I've completely underestimated the static:: vs self:: implications in a few places... Is doing static:: bad here for some reason?

static:: means it will take the called class, while self:: will take the class the self:: is done in the class that does the self:: call:

https://3v4l.org/soaZR

dominikzogg · 2020-11-26T05:09:34Z

src/Dflydev/FigCookies/Cookies.php

    {
-        $cookieString = $request->getHeaderLine(static::COOKIE_HEADER);
+        $cookieString = $request->getHeaderLine(self::COOKIE_HEADER);


changes are low (http spec), but also this constant could be overridden

I think I've completely underestimated the static:: vs self:: implications in a few places... Is doing static:: bad here for some reason?

dominikzogg · 2020-11-26T05:10:26Z

src/Dflydev/FigCookies/Modifier/SameSite.php

    {
-        return new self(self::STRICT);
+        return new static(self::STRICT);


it's a final class, static is useless here

But is it wrong?

dominikzogg · 2020-11-26T05:10:32Z

src/Dflydev/FigCookies/Modifier/SameSite.php

    {
-        return new self(self::LAX);
+        return new static(self::LAX);


it's a final class, static is useless here

But is it wrong?

dominikzogg · 2020-11-26T05:14:27Z

src/Dflydev/FigCookies/SetCookies.php

            return SetCookie::fromSetCookieString($setCookieString);
-        }, $response->getHeader(static::SET_COOKIE_HEADER)));
+        }, $response->getHeader(self::SET_COOKIE_HEADER)));


changes are low (http spec), but also this constant could be overridden

I think I've completely underestimated the static:: vs self:: implications in a few places... Is doing static:: bad here for some reason?

dominikzogg · 2020-11-26T05:15:18Z

tests/Dflydev/FigCookies/CookiesTest.php

-        $request = $this->prophesize(static::INTERFACE_PSR_HTTP_MESSAGE_REQUEST);
-        $request->getHeaderLine(Cookies::COOKIE_HEADER)->willReturn($cookieString);
+        /** @var RequestInterface|MockObject $request */
+        $request = $this->createMock(self::INTERFACE_PSR_HTTP_MESSAGE_REQUEST);


prophecy is still usable, by loading a trait

i have no idea if @simensen wants to keep prophecy

I don't care about Prophecy. I think this was maybe added by @Ocramius at some point?

dominikzogg · 2020-11-26T05:17:00Z

tests/Dflydev/FigCookies/CookiesTest.php

@@ -210,7 +207,7 @@ public function provideCookieStringAndExpectedCookiesData() : array
    }

    /** @return string[][]|Cookie[][] */
-    public function provideGetsCookieByNameData()
+    public function provideGetsCookieByNameData(): array


test is not final could be overridden (i would take the risk of a BC here)

dominikzogg · 2020-11-26T05:17:21Z

tests/Dflydev/FigCookies/FigCookieTestingMessage.php

@@ -57,7 +59,7 @@ public function withAddedHeader($name, $value)
    /** {@inheritDoc} */
    public function withoutHeader($name)
    {
-        $clone = clone($this);
+        $clone = clone$this;


Suggested change

$clone = clone$this;

$clone = clone $this;

Let's get this change added to the PR.

dominikzogg · 2020-11-26T05:19:28Z

tests/Dflydev/FigCookies/SetCookiesTest.php

-        /** @var ResponseInterface|ObjectProphecy $response */
-        $response = $this->prophesize(static::INTERFACE_PSR_HTTP_MESSAGE_RESPONSE);
-        $response->getHeader(SetCookies::SET_COOKIE_HEADER)->willReturn($setCookieStrings);
+        /** @var ResponseInterface|MockObject $response */


prophecy is still usable, by loading a trait

i have no idea if @simensen wants to keep prophecy

Happy to drop it if it's easier at this point.

simensen · 2020-11-30T02:31:49Z

@dominikzogg How are we doing on this one? My bad for not catching this earlier. @Ocramius, are you able to take a look at this? I think your big changes were some of the last things to go in here.

dominikzogg · 2020-11-30T05:08:31Z

@simensen i get aware about this PR, cause i like to create an issue about the lack of PHP 8 support. In my opinion its PR is much larger than it needs to be, but its not my repository, so i only mentioned the things that i believe are critical like (self vs static changes).

simensen

This looks good. Agreed it is maybe larger than needed (especially the style changes) but none of them look altogether bad. Would love to get sign off from @Ocramius, though, as he did a lot of work on this recently.

simensen · 2020-11-30T17:30:13Z

src/Dflydev/FigCookies/Cookie.php


-        /** @var Cookie $cookie */
-        $cookie = new static($cookieName);
+        $cookie = new self($cookieName);


Indeed, can this one get updated, too?

simensen · 2020-11-30T17:31:02Z

src/Dflydev/FigCookies/Cookies.php

    {
        $cookieString = implode('; ', $this->cookies);

-        $request = $request->withHeader(static::COOKIE_HEADER, $cookieString);
+        $request = $request->withHeader(self::COOKIE_HEADER, $cookieString);


I think I've completely underestimated the static:: vs self:: implications in a few places... Is doing static:: bad here for some reason?

simensen · 2020-11-30T17:31:15Z

src/Dflydev/FigCookies/Cookies.php

    {
-        $cookieString = $request->getHeaderLine(static::COOKIE_HEADER);
+        $cookieString = $request->getHeaderLine(self::COOKIE_HEADER);


I think I've completely underestimated the static:: vs self:: implications in a few places... Is doing static:: bad here for some reason?

simensen · 2020-11-30T17:33:32Z

src/Dflydev/FigCookies/Modifier/SameSite.php

    {
-        return new self(self::STRICT);
+        return new static(self::STRICT);


But is it wrong?

simensen · 2020-11-30T17:33:36Z

src/Dflydev/FigCookies/Modifier/SameSite.php

    {
-        return new self(self::LAX);
+        return new static(self::LAX);


But is it wrong?

simensen · 2020-11-30T17:36:08Z

src/Dflydev/FigCookies/SetCookies.php

        foreach ($this->setCookies as $setCookie) {
-            $response = $response->withAddedHeader(static::SET_COOKIE_HEADER, (string) $setCookie);
+            $response = $response->withAddedHeader(self::SET_COOKIE_HEADER, (string) $setCookie);


I think I've completely underestimated the static:: vs self:: implications in a few places... Is doing static:: bad here for some reason?

simensen · 2020-11-30T17:36:15Z

src/Dflydev/FigCookies/SetCookies.php

            return SetCookie::fromSetCookieString($setCookieString);
-        }, $response->getHeader(static::SET_COOKIE_HEADER)));
+        }, $response->getHeader(self::SET_COOKIE_HEADER)));


I think I've completely underestimated the static:: vs self:: implications in a few places... Is doing static:: bad here for some reason?

simensen · 2020-11-30T17:37:12Z

tests/Dflydev/FigCookies/CookiesTest.php

-        $request = $this->prophesize(static::INTERFACE_PSR_HTTP_MESSAGE_REQUEST);
-        $request->getHeaderLine(Cookies::COOKIE_HEADER)->willReturn($cookieString);
+        /** @var RequestInterface|MockObject $request */
+        $request = $this->createMock(self::INTERFACE_PSR_HTTP_MESSAGE_REQUEST);


I don't care about Prophecy. I think this was maybe added by @Ocramius at some point?

simensen · 2020-11-30T17:38:15Z

tests/Dflydev/FigCookies/FigCookieTestingMessage.php

@@ -57,7 +59,7 @@ public function withAddedHeader($name, $value)
    /** {@inheritDoc} */
    public function withoutHeader($name)
    {
-        $clone = clone($this);
+        $clone = clone$this;


Let's get this change added to the PR.

simensen · 2020-11-30T17:38:47Z

tests/Dflydev/FigCookies/SetCookiesTest.php

-        /** @var ResponseInterface|ObjectProphecy $response */
-        $response = $this->prophesize(static::INTERFACE_PSR_HTTP_MESSAGE_RESPONSE);
-        $response->getHeader(SetCookies::SET_COOKIE_HEADER)->willReturn($setCookieStrings);
+        /** @var ResponseInterface|MockObject $response */


Happy to drop it if it's easier at this point.

simensen · 2020-11-30T17:41:18Z

@canvural are you up for making the requested changes? :) If not, I can see if I can take over.

canvural · 2020-11-30T18:21:43Z

I can fix it, but Wednesday earliest.

Style changes are because of doctrine/coding-standard 8.0 So it's normal I'd say.

static to self changes are because of PHPStan. Basically, it warns us that if the user extends the class and does something unexpected our code here will not work as expected. Cause static:class will refer to the extended class, not our class. One way to fix it making the class or constructors final or changing it to self::class So, it's up to you to decide. I can revert the changes and ignore those error in PHPStan.

simensen · 2020-11-30T19:57:17Z

@canvural Awesome, thanks.

Indeed, it makes sense re: the style changes. :) However, it might have been possible to not upgrade that dependency so far just yet. I'm happy with it as-is, to be clear. It does make it harder to review the PR, tho.

Re static/self, I think if you can revert just the few that @dominikzogg called out if that works. If PHPStan gives errors, we can look into it more. The solution might need to be to make __construct final but I'll need to think about that.

Thanks again for your help!

dominikzogg · 2020-11-30T21:04:45Z

self:
against final classes
against final methods / constants

static:
against the rest (if you want extend ability)

Change from static to self is a BC, except it matches already the self:

waghanza

I think PHP 8 should be added to CI's matrix (I can mot do it myself right now)

BackEndTea · 2020-12-29T13:20:40Z

composer.json

@@ -20,10 +20,12 @@
        }
    },
    "require-dev": {
-        "phpunit/phpunit": "^7.2.6",
+        "phpunit/phpunit": "^7.2.6 || ^9",


The minimum php version of this lib is php 7.2, so the minimum vesion of phpunit could be 8, as that still supports php 7.2

BackEndTea · 2020-12-29T13:24:20Z

src/Dflydev/FigCookies/Cookies.php

    {
        $cookieString = implode('; ', $this->cookies);

-        $request = $request->withHeader(static::COOKIE_HEADER, $cookieString);
+        $request = $request->withHeader(self::COOKIE_HEADER, $cookieString);


static:: means it will take the called class, while self:: will take the class the self:: is done in the class that does the self:: call:

https://3v4l.org/soaZR

BackEndTea · 2020-12-29T13:26:00Z

src/Dflydev/FigCookies/SetCookie.php

    {
        if ($expires === null) {
            return 0;
        }

        if ($expires instanceof DateTimeInterface) {
-            return $expires->getTimestamp();
+            return (int) $expires->getTimestamp();


Before php 8, this method could return false on failure.

hugochinchilla · 2021-01-21T12:15:45Z

Hi, this is the last dependency preventing my project from runnin on php 8.0, can I do something to help get this ready? should I fork the pull-request and complete the pending changes?

Extending and finalizing #39

hugochinchilla · 2021-01-22T06:51:47Z

Thank you!

Update dependencies for PHP8

120bfe8

canvural mentioned this pull request Nov 2, 2020

Support PHP 8 thephpleague/openapi-psr7-validator#91

Closed

dominikzogg reviewed Nov 6, 2020

View reviewed changes

Fix code style and PHPStan errors

60675f7

canvural force-pushed the php8 branch from d4ca238 to 60675f7 Compare November 7, 2020 21:35

dominikzogg suggested changes Nov 26, 2020

View reviewed changes

simensen mentioned this pull request Nov 30, 2020

PHP8 Support #38

Closed

simensen requested changes Nov 30, 2020

View reviewed changes

acelaya mentioned this pull request Nov 30, 2020

PHP 8 support shlinkio/shlink#856

Closed

28 tasks

iLexN mentioned this pull request Dec 21, 2020

PHP8 support iLexN/swoole-psr7#3

Closed

waghanza suggested changes Dec 27, 2020

View reviewed changes

BackEndTea reviewed Dec 29, 2020

View reviewed changes

dominikzogg mentioned this pull request Dec 30, 2020

[PHP] Upgrade to 8.0 the-benchmarker/web-frameworks#3655

Merged

osteel mentioned this pull request Jan 6, 2021

PHP 8 support osteel/openapi-httpfoundation-testing#2

Merged

simensen mentioned this pull request Jan 22, 2021

Extending and finalizing #39 #44

Merged

simensen added a commit that referenced this pull request Jan 22, 2021

Merge pull request #44 from simensen/php8

8002650

Extending and finalizing #39

simensen merged commit 60675f7 into dflydev:master Jan 22, 2021

simensen mentioned this pull request Jan 22, 2021

Add PHP8 support #43

Closed

PHP 8 Support #39

PHP 8 Support #39

Conversation

canvural commented Nov 1, 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

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

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

canvural commented Nov 26, 2020

dominikzogg left a 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

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

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

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

simensen commented Nov 30, 2020

dominikzogg commented Nov 30, 2020

simensen left a 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

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

simensen commented Nov 30, 2020

canvural commented Nov 30, 2020

simensen commented Nov 30, 2020

dominikzogg commented Nov 30, 2020 • edited Loading

waghanza left a 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

hugochinchilla commented Jan 21, 2021

hugochinchilla commented Jan 22, 2021

canvural commented Nov 1, 2020 •

edited

Loading

dominikzogg commented Nov 30, 2020 •

edited

Loading