search

reactphp / promise

Promises/A implementation for PHP.
https://reactphp.org/promise/
MIT License
2.38k stars 146 forks source link

Use full function namespaces to avoid clashes with serialization #179

Closed brentkelly closed 3 years ago

brentkelly commented 4 years ago

Promise.php contains function calls (resolve and reject from src/functions.php) that expect the current namespace. However if you serialize & then later unserialize a Deferred instance, this namespace is lost, and the function calls clash with other defined global functions.

E.g. if using Laravel, you configure & then serialize a \React\Promise\Deferred instance, and then later unserialize & attempt to resolve it - the call to resolve on line 232 clashes with Laravel's global resolve function - causing it to throw an exception as it runs through the global Laravel function.

Using Opis/Closure for serializing.

By using the full namespace on the function call this eliminates this possible problem. Also makes the code an easier read IMO making it obvious that these are namespaced (and not global) functions.

clue commented 3 years ago

@brentkelly Thanks for bringing this up, this is an interesting one!

As discussed in #180, a promise should not usually be serialized because it contains transient state when it's still pending.

On top of this, it looks like this changeset would introduce a partial patch to work around a limitation in Opis Closure only. In other words, this isn't required to address an issue in this library, but rather make sure another library can work better. I think the way forward here would be to report this upstream and then see if it makes sense to patch this project.

That being said, I'm not opposed to getting this in once this is sorted out upstream. If you can come up with a way to reproduce this issue in this project and provide additional tests to highlight in which specific situation this is required, we can avoid future regressions and I'm happy to get this in.

Thanks for looking into this!

clue commented 3 years ago

I'm closing this for now as it hasn't received any input in a while and I believe this has been answered. If you feel this is still relevant, please come back with more details and we can always reopen this :+1:

Thank you for your effort!