Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

PHP 8.0 compatibility #172

Merged
merged 2 commits into from
Apr 11, 2022
Merged

PHP 8.0 compatibility #172

merged 2 commits into from
Apr 11, 2022

Conversation

mikhainin
Copy link
Contributor

Hopefully, 8.1 as well. At least, the tests on 8.1 pass

@Totktonada
Copy link
Member

Totktonada commented Apr 11, 2022

Thank you for the patches!

The first patch corresponds to the native-tls proposal and 1.c bullet ('TSRM changes') of the PHP 8.0 UPGRADING.INTERNALS document. It looks okay.

Those macros are no-op since PHP 7.0, so we can just remove them. We can push this minimal change (define as no-op) and remove them later, though.

The second patch corresponds to the negative_array_index proposal. There is no related entry in the PHP 8.0 UPGRADING.INTERNALS document, but the change is the following: php/php-src#3772. (It is strange that the problem stated as relevant only to PHP 8.1 in #170 (comment). It also affects PHP 8.0 according to php-src git logs and results of my local testing.)

I don't completely understand the php-src change, but our side fix looks working.

I'll do the following:

  • Update commit messages with relevant infomation.
  • Push the patch.

When time will permit I would also do:

  • Traverse over all PHP 8.0 and PHP 8.1 UPGRADING.INTERNALS documents to find more relevant bullets.
  • Revisit CI to make everything tested again (I'm in progress, but not ready to show results ATM).

@Totktonada Totktonada self-requested a review April 11, 2022 12:20
The TSRM* macros are no-op since PHP 7.0. PHP 8.0 drops them at all.
Define them as no-op in our code when building against PHP 8 and newer
to overcome a build fail. See [1] and [2] (1.c) for details.

Those macros could be removed from the code entirely (both definitions
and usages), since we support PHP 7.0+. It is to be done later.

[1]: https://wiki.php.net/rfc/native-tls
[2]: https://github.com/php/php-src/blob/php-8.0.18RC1/UPGRADING.INTERNALS

Part of tarantool#171
@Totktonada
Copy link
Member

NB: PHP 8.1 introduces zend_array_is_list() (see is_list proposal and php/php-src#6070), which can be used in php_mp_is_hash() for PHP >= 8.1 to guarantee forward compatibility, stability and speed.

The `nNextFreeElement` field is initialized with `ZEND_LONG_MIN` instead
of zero since PHP 8.0. Adjust our `php_mp_is_hash()` check accordingly.
See [1] and [2] for details.

NB: PHP 8.1 introduces `zend_array_is_list()`, which may be used here.
See [3] and [4] for details.

[1]: https://wiki.php.net/rfc/negative_array_index
[2]: php/php-src#3772
[3]: https://wiki.php.net/rfc/is_list
[4]: php/php-src#6070

Since I don't observe any other problems on PHP 8.1, closing the
relevant issue.

Fixes tarantool#171
@Totktonada Totktonada merged commit e0152fd into tarantool:master Apr 11, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants