Add sapi zts test

[Qard]

Description

Depends on #488.

I've pulled the test for that out as it had a problematic interaction with the closure feature. I'll have to figure out how to make that work properly. 🤔

Checklist

Check the boxes that apply (put an x in the brackets, like [x]). You can also check boxes after the PR is created.

• I have read the contribution guidelines.
• I have added tests that prove my code works as expected.
• I have added documentation if applicable.
• I have added a migration guide if applicable.

❤️ Thank you for your contribution!

[ptondereau]

Hello @Qard
We think your PR is a good addition.
Could it be possible to rebase here?

[Qard]

Yep, I can try a rebase. Though I never did get around to looking into the closure interaction. It may still be broken?

[Qard]

I've pushed a possible fix to the closure conflict, though I'm uncertain if that is actually correct. Anyone else know the closure subsystem better that can review that?

[ptondereau]

I've pushed a possible fix to the closure conflict, though I'm uncertain if that is actually correct. Anyone else know the closure subsystem better that can review that?

I'll try to have a look next week. Thanks for the work

[Qard]

I've pushed a possible fix to the closure conflict, though I'm uncertain if that is actually correct. Anyone else know the closure subsystem better that can review that?

I'll try to have a look next week. Thanks for the work

It seemed to get past the original failure, but now fails in a different. Unfortunately though it seems to only fail in CI for me and not locally, so I'm not too sure how to debug. If I get time at some point I might take another look, but I'm not too sure how much sense I'd be able to make of it. 🤔

[ptondereau]

I've pushed a possible fix to the closure conflict, though I'm uncertain if that is actually correct. Anyone else know the closure subsystem better that can review that?

I'll try to have a look next week. Thanks for the work

It seemed to get past the original failure, but now fails in a different. Unfortunately though it seems to only fail in CI for me and not locally, so I'm not too sure how to debug. If I get time at some point I might take another look, but I'm not too sure how much sense I'd be able to make of it. 🤔

Embed test runs only on 8.4 (embed sapi) debug nts with latest rust and llvm 17.

[ptondereau]

Ok, this is because I guess you have a ZTS build of PHP locally, and the Test embed runs only on a non-TS version.
This means that you probably need to add a test guard for ZTS build (we already have a #[cfg(php_zts)]) and add a matrix entry for test embed with PHP ZTS/NTS.

[Qard]

Where exactly? I'm not clear exactly what you're suggesting I change. 🤔

[coveralls]

Pull Request Test Coverage Report for Build 20101696758

Details

• 16 of 17 (94.12%) changed or added relevant lines in 2 files are covered.
• No unchanged relevant lines lost coverage.
• Overall coverage decreased (-0.08%) to 35.229%

---

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
src/closure.rs	1	2	50.0%

Totals
Change from base Build 20043754996:	-0.08%
Covered Lines:	1598
Relevant Lines:	4536

---

💛 - Coveralls

[ptondereau]

Now that the embed tests pass, we can introduce ZTS with a matrix test for test-embed job: https://github.com/extphprs/ext-php-rs/blob/master/.github/workflows/build.yml#L172
I can add a commit in your PR if you'd like.

[Qard]

Sure. I probably won't have the time to look at it this week. 😅

[ptondereau]

I struggle to push on your branch but you can see the diff here: https://github.com/extphprs/ext-php-rs/pull/589/files

[Qard]

Figured out the failure. It was missing a thread cleanup which was crashing the mongodb extension on shutdown.

[ptondereau]

Figured out the failure. It was missing a thread cleanup which was crashing the mongodb extension on shutdown.

Awesome thank you very much

[Qard]

@ptondereau Anything else you need for this, or can this be landed?

[ptondereau]

@ptondereau Anything else you need for this, or can this be landed?

Can you make a rebase just make sure. Thanks

[Qard]

Done. 🙂