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

Fix PHP 8.0 tests under ZTS #46

Closed
wants to merge 3 commits into from

Conversation

johnpbloch
Copy link

It looks like the version of PHP on Travis was compiled with ZTS mode turned on and the tests are failing because the macro for the now undefined tsrm_set_interpreter_context function was replacing everything but the semicolon, and the semicolon was causing compilation errors.

So in this fix, instead of a macro I just used a function that returns a null pointer, so it should compile correctly now. I suppose we'll see if that's true once the travis tests run 😅

@coveralls
Copy link

coveralls commented Mar 29, 2021

Coverage Status

Coverage increased (+1.02%) to 86.677% when pulling f05dc3f on johnpbloch:fix/php-8.0-tests into f055bcd on php-zookeeper:master.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.8%) to 84.835% when pulling 90a1842 on johnpbloch:fix/php-8.0-tests into f055bcd on php-zookeeper:master.

@johnpbloch
Copy link
Author

🤦🏼‍♂️ looks like zookeeper just released zookeeper 3.7 in the last few weeks, so now 3.5 is no longer in the standard apache mirror setup. I'll switch 3.5 to build from archive.apache.org instead, but it's probably worthwhile to also test on 3.7 eventually.

@johnpbloch
Copy link
Author

I'm going to close this PR. I've spent a bit of time working through some of the TSRM internals and the complexity is way beyond what I'm able to handle at the moment. I can get the extension to compile in ZTS builds of PHP 8, but it still segfaults for those 3 tests (watcher-related tests). At this point, I think the TSRM code really does need to be migrated to the 7.x way of handling those resources.

@johnpbloch johnpbloch closed this Mar 30, 2021
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

2 participants