search

AlexaCRM / dynamics-webapi-toolkit

Dynamics 365 Web API Toolkit for PHP
MIT License
75 stars 58 forks source link

PHP 8.0 compatibility #84

Closed SpartakusMd closed 1 year ago

SpartakusMd commented 2 years ago

Last commit was to have compatibility with PSR cache & log versions ^3.0. They require PHP >8.0. They also introduced typed parameters and return types. This PR comes with fixing the crashes due to incompatibility

Fatal error:  Declaration of AlexaCRM\Cache\NullAdapter: :getItem($key) must be compatible with Psr\Cache\CacheItemPoolInterface: :getItem(string $key): Psr\Cache\CacheItemInterface in /srv/app/vendor/alexacrm/dynamics-webapi-toolkit/src/Cache/NullAdapter.php on line 48
SpartakusMd commented 2 years ago

A suggestion I would make is to replace the comments in NullAdapter and NullCacheItem with @inheritDoc in order to use the phpdocs from the interface itself. WDYT?

SpartakusMd commented 1 year ago

Hi @georged, in that case, the PR #82 needs to be reverted as the src/Cache/NullAdapter.php and src/Cache/NullCacheItem.php are not compatible with psr/cache v3 (see the PR description with the error).

I would suggest reverting to psr/cache ^1.0 || ^2.0 for version 2.x and use psr/cache ^2.0 || ^3.0 + PHP ^8.0 for version 3.x.

What do you think on this?

Note: I'm using the library on PHP 8.1 without issues :)

georged commented 1 year ago

@SpartakusMd yes, already had people complaining so reverted this one.

I would suggest reverting to psr/cache ^1.0 || ^2.0 for version 2.x and use psr/cache ^2.0 || ^3.0 + PHP ^8.0 for version 3.x.

That's a great idea - can you create a PR for that? I won't be able to get to it for a few days at least.

SpartakusMd commented 1 year ago

Hey @georged, I rebased the code on master. This PR contains drops support for PHP 7.4 and requires PHP 8.0. Also drops psr/cache: ^1.0 and psr/log: ^1.0 support.

It can be merged and released as version 3 to not break the code for other users.

SpartakusMd commented 1 year ago

Hey @georged. Did you have time to look at this?

georged commented 1 year ago

@SpartakusMd we haven't merged yet but we did decide to create a separate branch min-php8.0 for php 8+ and keep 7.4 as is. The new branch will have all the changes to support psr 3. @wizardist can you take a look how best to merge this pull request into the new branch?

SpartakusMd commented 1 year ago

Hey @georged @wizardist any plans on this?

georged commented 1 year ago

@SpartakusMd yes there are :)

Did you check if php 80 branch is more suitable for this?

SpartakusMd commented 1 year ago

Hi @georged, the branch can run on php 8.0 but not on php 8.1 where return type hints were added. It will crash due to widening the return types in the implementation of cache 3.0.

I would suggest merging this branch into the php 8 one. WDYT?

georged commented 1 year ago

Your commit removes 7.4 from what I can see. We can't do that as we need this for backward compatibility. @wizardist do you think we can bring relevant changes to v8 branch?

SpartakusMd commented 1 year ago

Hello @georged, @wizardist. I see there was a 4.0.0-alpha version released for which minimum php version is 8. Unfortunately there is no php8 branch anymore as I wanted to move the pr to it. Could you create it back so that I would adjust my PR?

georged commented 1 year ago

@SpartakusMd I've [re]-created 4.x branch to support at least 8.0. Support for 7.4 dropped in that branch.

SpartakusMd commented 1 year ago

@georged I have updated the PR to target that branch. If the changes are ok, could you merge it and tag it?

paclw-cory commented 1 year ago

Greetings! Loving the package, but it's broken with PHP8+. Is anyone going to merge these changes or should I just make my own fork?

georged commented 1 year ago

@paclw-cory we've merged now, give it a shot!