search

reactphp / filesystem

Evented filesystem access.
MIT License
135 stars 40 forks source link

[WIP] Libuv Adapter #69

Closed ghost closed 4 years ago

ghost commented 5 years ago

This PR adds a libuv based adapter to react/filesystem. This PR builds on top of #65 and #67 and as such stays as draft until these PRs get merged. The PR will be rebased once ready.

For the UV adapter the unit tests are functional and will build a foundation for a future pull request to refactor the unit tests. I've added a helper method to concat paths for cross compatibility.

This PR is not fully ready yet, but opening a PR will help getting some comments and feedback during development. This PR closes #9.

ghost commented 5 years ago

@clue Unit tests are added for libuv (which pass), however I noticed there are some underlying issues with the libuv event loop, because other unit tests are failing (with the libuv event loop and eio or child process adapter). This requires more investigation.

And the major dependency on #67 for this PR is performance and future security fixes. I don't like supporting old, unmaintained versions, simply because it encourages others to not upgrade (mostly comfort and laziness in their case). ReactPHP (and mostly the event loop and filesystem components) are targeting CLI applications anyway, which requires access through SSH (or similar mechanisms) to work. If you have SSH access you can simply upgrade the PHP version for any Linux distribution in a very simple way (for Windows you can just download a new binary). And let's be honest here, there's no reason not to upgrade to PHP7. Dropping support for PHP5 encourages others to upgrade.

It's just my opinion, but I see no reason why we should keep supporting PHP5. Bugfixes have stopped over two years ago.

clue commented 5 years ago

[…] other unit tests are failing (with the libuv event loop and eio or child process adapter). This requires more investigation.

Interesting, let's focus on this part to move this forward. Does it make sense to address this in a separate ticket? Does anybody have any insights?

ghost commented 5 years ago

[…] other unit tests are failing (with the libuv event loop and eio or child process adapter). This requires more investigation.

Interesting, let's focus on this part to move this forward. Does it make sense to address this in a separate ticket? Does anybody have any insights?

Maybe it makes sense to address this in a separate ticket. However I see no value gained if merging this PR causes everything else to break.

If you're interested in a travis build history, here's one, they're mostly timeouts. Some tests work with ext-uv and child process adapter and some don't - why is unknown to me.