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

Add async support using php-tokio #274

Merged
merged 13 commits into from
Oct 21, 2023
Merged

Add async support using php-tokio #274

merged 13 commits into from
Oct 21, 2023

Conversation

danog
Copy link
Collaborator

@danog danog commented Oct 21, 2023

No description provided.

@joelwurtz
Copy link
Contributor

joelwurtz commented Oct 21, 2023

Having async which support fiber is definitely a blast however i'm not sure this is the right way to do that, as you have a dep on php-tokio that have a dependency on this library, not a fan of circular deps even if it's well done by rust here. Also i don't think it should have a direct dependencies on tokio as there are other async runtime in rust, why not do the following :

  • Add code from php-tokio directly in this lib in a "async" crate
  • Split this async crate in two :
    • Wrap PHP Fiber with Future from rust
    • Add a tokio feature providing an event loop (which depends on tokio)

@danog
Copy link
Collaborator Author

danog commented Oct 21, 2023

Hi, I actually wrote php-tokio as part of a previous PR to ext-php-rs, I split it into a separate library and removed it from the PR because I felt it introduced code (interacting with a userland library like Revolt, macro transmute black magic) that had no place in ext-php-rs (which is a library that interacts with the Zend API).

I'll very gladly add a separate async feature for it (and I eventually plan to add support for other event loops other than tokio), but I'd like to include it as a separate crate from my repo, given that I'm a maintainer of both ext-php-rs and php-tokio.

@danog
Copy link
Collaborator Author

danog commented Oct 21, 2023

Actually, the cleanest solution imo is just to mention in the docs as a peer dependency to add async support, switched to this solution for now.

@danog
Copy link
Collaborator Author

danog commented Oct 21, 2023

The main reason I feel like it's the best solution is because it's basically an adaptor for the revolt event loop, not really something related to pure PHP itself (and that's because PHP has no event loop, thus there is no way to implement this using only PHP APIs in a way that's compatible with the current PHP async ecosystem).

@danog
Copy link
Collaborator Author

danog commented Oct 21, 2023

I.e.

Wrap PHP Fiber with Future from rust

We can't wrap Rust futures using pure PHP fibers, because the result would be yet another competing event loop implementation (and one that's not compatible with any of the implementations currently powering the async ecosystem, namely Amphp and reactphp).

On the other hand, relying on Revolt introduces a userland dependency, but allows for full compatibility with amphp, reactphp, psl and any other library based on revolt.

@danog danog merged commit aa88f5a into master Oct 21, 2023
20 checks passed
@danog danog deleted the php_tokio branch October 21, 2023 21:33
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