dflydev / dflydev-fig-cookies

Cookies for PSR-7 HTTP Message Interface.
MIT License
224 stars 29 forks source link

PHP 8 Support #39

Closed canvural closed 3 years ago

canvural commented 3 years ago

This PR:

canvural commented 3 years ago

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

simensen commented 3 years ago

@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 commented 3 years ago

@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 commented 3 years ago

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

canvural commented 3 years ago

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 commented 3 years ago

@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 commented 3 years ago

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:

hugochinchilla commented 3 years ago

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?

hugochinchilla commented 3 years ago

Thank you!