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 7.3 compatibility and bugfixes #195

Merged
merged 2 commits into from Apr 2, 2019
Merged

PHP 7.3 compatibility and bugfixes #195

merged 2 commits into from Apr 2, 2019

Conversation

ghost
Copy link

@ghost ghost commented Dec 11, 2018

Yeah I know the original project is seemingly dead, but okay, let this pull request still be there.


  • Define new GC_ADDREF/DELREF/SET_REFCOUNT macros for older PHP versions and use them instead of direct GC reference counter access

  • Fixup all necessary 'long' type parameters to 'zend_long', PHP 7.3 makes it mandatory, also fixup some direct function implementations to accept the same

  • In php_zmq_recv(), zend_string_init() was wrongly called with third parameter as '1', marking new string with IS_STR_PERSISTENT, this caused heap corruption and/or segfaults with PHP 7.3 and could possibly cause other sorts of bugs under any 7.x version
    With ZVAL_STRINGL macro, this last '1' parameter meant to copy the string and was seemingly erroneously moved to zend_string_init(). zend_string_init() copies string by default, and last parameter has totally different meaning here

  • In poll(), flag ZVAL separation on passed arrays (PHP 7.3 makes it mandatory)

  • Test 19 (exception on connect callback with forced reference parameter): skip on PHP 7.1 and higher, PHP >= 7.1 started to fallback to passing argument by value instead of failing

  • Test 21 (warning generation from callback): it is ok, but PHP 7.3 uses 'int' instead of 'integer' for constants, so allow any word in place of the word 'integer'

AlexAT and others added 2 commits December 10, 2018 06:39
- Define new GC_ADDREF/DELREF/SET_REFCOUNT macros for older PHP versions and use them instead of direct GC reference counter access

- Fixup all necessary 'long' type parameters to 'zend_long', PHP 7.3 makes it mandatory, also fixup some direct function implementations to accept the same

- In php_zmq_recv(), zend_string_init() was wrongly called with third parameter as '1', marking new string with IS_STR_PERSISTENT, this caused heap corruption and/or segfaults with PHP 7.3 and could possibly cause other sorts of bugs under any 7.x version
  With ZVAL_STRINGL macro, this last '1' parameter meant to copy the string and was seemingly erroneously moved to zend_string_init(). zend_string_init() copies string by default, and last parameter has totally different meaning here

- In poll(), flag ZVAL separation on passed arrays (PHP 7.3 makes it mandatory)

- Test 19 (exception on connect callback with forced reference parameter): skip on PHP 7.1 and higher, PHP >= 7.1 started to fallback to passing argument by value instead of failing

- Test 21 (warning generation from callback): it is ok, but PHP 7.3 uses 'int' instead of 'integer' for constants, so allow any word in place of the word 'integer'
@ghost
Copy link
Author

ghost commented Dec 11, 2018

Second commit got in from my master branch as well, it removes the link to build tests which are apparently dead/unmaintained as well, don't import this if you want to stay with original one.

Probably more commits will get in as well when I update my copy for libzmq 4.2.5 and latest CZMQ, so read carefully before taking it for yourself.

@ghost ghost mentioned this pull request Dec 13, 2018
@remicollet
Copy link
Contributor

Fedora build with this PR applied, on various arch, NTS + ZTS, with test suite.
https://koji.fedoraproject.org/koji/taskinfo?taskID=31457365

LGTM

@oriceon
Copy link

oriceon commented Dec 27, 2018

@mkoppanen can you test and merge this PR please? Also build dll`s for windows?
Thanks.

buildroot-auto-update pushed a commit to buildroot/buildroot that referenced this pull request Jan 6, 2019
This includes an patch that fixes the following error:

```
/home/buildroot/build/instance-0/output/build/php-zmq-1.1.3/zmq.c: In function 'php_zmq_context_get':
/home/buildroot/build/instance-0/output/build/php-zmq-1.1.3/zmq.c:238:20: error: lvalue required as left operand of assignment
   GC_REFCOUNT(&le) = 1;
                    ^
/home/buildroot/build/instance-0/output/build/php-zmq-1.1.3/zmq.c: In function 'php_zmq_socket_store':
/home/buildroot/build/instance-0/output/build/php-zmq-1.1.3/zmq.c:538:19: error: lvalue required as left operand of assignment
  GC_REFCOUNT(&le) = 1;
```

The patch was created from the PR at:

zeromq/php-zmq#195

Upstream has not merged the PR. Fixes:

http://autobuild.buildroot.org/results/3f2/3f258fbc7352c3d7205bc6402145be1102d69683

Signed-off-by: Frank Hunleth <fhunleth@troodon-software.com>
Signed-off-by: Peter Korsgaard <peter@korsgaard.com>
@glendmaatita
Copy link

this probably will fix my issue. @mkoppanen could you check and merge?

@budby
Copy link

budby commented Mar 27, 2019

It probably will fix my issue also

@mkoppanen mkoppanen merged commit fc6e966 into zeromq:master Apr 2, 2019
@jankremlacek
Copy link

thank you @mkoppanen 🙏🏻

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.

None yet

7 participants