Skip to content

Conversation

kiike
Copy link

@kiike kiike commented Mar 27, 2025

First, thank you so much for ReactPHP and ReactPHP-Soap! I am using both in a small project and bring a very nice improvement in performance.

I noticed that:

  • the dependencies for this project are a bit old. For instance, the blocking library is deprectaed in favour or React/Async
  • plus there's an undocumented dependency on the RingCentral's PSR7 library

I went ahead and tried to address these observations:

  • updating dependencies,
  • moving from RingCentral's PSR7 library to Laminas Diactoros, which is maintained at the moment
  • moved from usage of '\Clue\React\Block' to 'Async\Await'

Looking forward to your feedback!

EDIT: Since promises aims to support older PHP versions, I am dropping the _doRequest signature update to not break compatibility with PHP<8.0

@kiike kiike force-pushed the chore/update_deps branch from 2c87600 to 1f64b79 Compare March 27, 2025 20:22
@kiike kiike force-pushed the chore/update_deps branch from 1f64b79 to 77ab32b Compare March 31, 2025 13:51
@kiike kiike changed the title Update dependencies, use Laminas Diactoros, update signature for Encoder::_doRequest and Decoder::_doRequest Update dependencies, use Laminas Diactoros, ~update signature for Encoder::_doRequest and Decoder::_doRequest~ Mar 31, 2025
@kiike kiike force-pushed the chore/update_deps branch 3 times, most recently from c0da8c0 to bb2b894 Compare March 31, 2025 14:05
@kiike kiike force-pushed the chore/update_deps branch from bb2b894 to e1c778d Compare March 31, 2025 14:06
@PaulRotmann
Copy link
Contributor

Thanks for this contribution! The updates you're proposing make sense in principle, but there are a few issues that make this difficult to review properly.

The main problem is that this PR combines too many changes at once, and the tests aren't passing. When multiple changes are bundled together like this, it becomes very difficult to identify which specific change might be causing issues.

Could you break this down into smaller, focused PRs? For example, one PR for dependency updates, another for switching to Laminas Diactoros, etc. This would make it much easier to review and test each change individually.

Once the changes are separated and the tests are passing, we'd be happy to review each one properly. The functionality improvements look good, they just need to be presented in a more manageable way.

Thanks for understanding!

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