rybakit / msgpack.php

A pure PHP implementation of the MessagePack serialization format / msgpack.org[PHP]
MIT License
388 stars 18 forks source link

PHP 8 support #33

Closed nesk closed 3 years ago

nesk commented 3 years ago

Hi,

I ran your tests with PHP 8, everything works. It seems like the only missing thing is the PHP Decimal Extension, I saw you already have created php-decimal/ext-decimal#43 about this matter on their repository. Unfortunately, it has been several months and we still don't have any answer from a maintainer. Maybe its time to go forward?

I propose you to officially support PHP 8. We could add some phpDoc to the BIGINT_AS_DEC constant to tell users PHP 8 is not supported for the moment. We could also throw an exception if the user tries to use it and the Decimal extension is not detected, that way they are stopped from doing something stupid and forward compatibility is ensured because your users will not need to update your package to the latest version to start using Decimal once the extension is updated. Maybe this will require to release a major version to warn about the potential backward compatibility break?

I know this is not ideal, but your package works with PHP 8 and it would be a shame to not make it available to all PHP versions. 😕

nesk commented 3 years ago

Note: if you agree with supporting PHP 8 that way, I can make a PR if you want.

rybakit commented 3 years ago

Hey @nesk,

The good news is that the decimal extension has always been optional for non-dev environments. And the README already states that if one wants to use BIGINT_AS_DEC, they have to make sure that this extension is enabled.

We could also throw an exception if the user tries to use it and the Decimal extension is not detected,

I don't think this is necessary, if the extension is missing in the system user already gets the Class "Decimal\Decimal" not found core exception.

Maybe this will require to release a major version to warn about the potential backward compatibility break?

For the record, this library is still in the alpha phase, so users should be prepared to get incompatible API changes within the 0.x releases cycle :)

To conclude, in order to add support for PHP 8, the dev requirement needs to be relaxed to make ext-decimal optional, and dockerfile.sh and related tests need to be updated accordingly. I will try to find some time in the coming days to work on it.

nesk commented 3 years ago

Awesome, happy to see the migration to PHP 8 is not a blocked by the extension!

For the record, this library is still in the alpha phase, so users should be prepared to get incompatible API changes within the 0.x releases cycle :)

True, I totally forgot about that! 😅

If you need help, let me know. 🙂

nesk commented 3 years ago

That was fast, thank you so much! 🙇

rybakit commented 3 years ago

I just released 0.7.1 :)