Compatibility with PHP 7.x
Closed this issue · 19 comments
It took more than an year since #39 by @Danack but I finally had the time to sit down and work on polishing his initial contribution and put an end to the wait: phpiredis can be built and used on PHP 7.0, you can find the code on the php7 branch. The test suite is green on all the supported PHP versions and even the test suite of Predis (which has some connection backends that use the reader resource exposed by phpiredis) looks good. I'd like to test this branch a bit more before merging it into master and I'd also like to have some feedback from others so please try the new code and comment on this issue if you find any bug or memory leak using the extension on both PHP 5.x and 7.0.
PS: please note that for now phpiredis depends on hiredis up to v0.13 and it doesn't build with hiredis v1.0.0 which is still unreleased and a work-in-progress.
Thanks for the wait!
By the way, the library can be built even on PHP 7.1.0alpha1 and tests are green.
I'll plug this into our Docker setup and see how it handles itself. Nice work!!
Edit: Built successfully on our phusion/baseimage:latest container. I'll be using it in our development environment for a week or so. If all goes good I'll have it promoted to production.
Cheers!
Thanks @ellisio, looking forward to your feedback! Which version of PHP are you using?
We are using 7.0.7. I used it all day yesterday and it seems to work just fine. I plan to run some load testing here in a few days; just have some other stuff I have to get wrapped up before I do that.
@nrk sorry - I don't have time to look at this, but one thing you need to check is something I wasn't aware of when I did the PR.
Basically when you have any Z_TYPE_P, and the zval comes from a user, there ought to be a ZVAL_DEREF before the Z_TYPE_P to make it work properly, rather than falling over.
I think for you that might be just these two places:
https://github.com/nrk/phpiredis/blob/php7/phpiredis.c#L716
https://github.com/nrk/phpiredis/blob/php7/phpiredis.c#L744
need this:
#ifdef ZEND_ENGINE_3
ZVAL_DEREF(param);
#endif
As I said, I only found this out recently - the Imagick test that makes sure this works is there.
thank you so much nrk. nice work . i want donate to u for you work . 👍
@Danack fixed, thanks! Maybe it's just that I'm still a noob when it comes to PHP extensions, but the ZE API seems mostly an undocumented mass of macros, sometimes with only slight differences, that makes it hard for novices to start especially without digging into the ZE source.
the ZE API seems mostly an undocumented mass of macros, sometimes with only slight differences, that makes it hard for novices to start especially without digging into the ZE source.
It is not just you.
As I said, I missed that in my initial port of Imagick......and only found out about it when people complained about stuff breaking.....
After testing this branch for a good week I'm quite confident it's stable enough to get merged into master but since I'm busy for the next few days I'm going to wait a bit more just in case anyone has more feedback. After the merge I guess we can finally have v1.0.0 tagged.
any update nrk?
@erikwestlund the last update is my comment above yours from 6 hours ago as the time of writing.
@nrk, I'm sorry, for some reason I read "7 hours ago" as "7 days ago" .... whoops :)
looking forward to it. Thank you!
@erikwestlund haha it's fine don't worry :)
Been a little over a week, any chance of this being merged into master? I've been running it for almost a month now in development and production with zero hiccups.
Merge to master would be great
Whoa, whoa, whoa. You can't just rush into these things.
Finally merged the php7 branch into the master! Next week I'll be off on vacation, will tag v1.0.0 at the end of the month :-)
got
error LNK2001: unresolved external symbol _executor_globals
build with --disable-zts flag.