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

Rewrite on top of Aerys #267

Closed
wants to merge 5 commits into
base: master
from

Conversation

Projects
None yet
@kelunik
Contributor

kelunik commented Sep 4, 2017

See #146 for discussion.

Fixes #146, #249, #265.

See also php-pm/php-pm-httpkernel#61.

@kelunik kelunik referenced this pull request Sep 4, 2017

Closed

Rewrite based on Aerys #61

@andig

This comment has been minimized.

Show comment
Hide comment
@andig

andig Sep 4, 2017

Contributor

I'm really against this in its current form. Downstream applications like Volkszaehler add additional functionality on basis of reactPHP. Forcing those to move their platform is imho not an option.
If aerys is to be supported it should imho be in addition, not instead.

While reactPHP might be slow I really appreciate the quality of the code delivered.

Contributor

andig commented Sep 4, 2017

I'm really against this in its current form. Downstream applications like Volkszaehler add additional functionality on basis of reactPHP. Forcing those to move their platform is imho not an option.
If aerys is to be supported it should imho be in addition, not instead.

While reactPHP might be slow I really appreciate the quality of the code delivered.

@kelunik

This comment has been minimized.

Show comment
Hide comment
@kelunik

kelunik Sep 4, 2017

Contributor

This doesn't conflict with applications running on ReactPHP at all. If anything, then this improves the situation for them, because the ReactPHP versions used by PHP-PM are no longer conflicting with the ReactPHP versions used by the application.

However, there is an issue with applications using Amp currently, but I'll resolve that soon.

Contributor

kelunik commented Sep 4, 2017

This doesn't conflict with applications running on ReactPHP at all. If anything, then this improves the situation for them, because the ReactPHP versions used by PHP-PM are no longer conflicting with the ReactPHP versions used by the application.

However, there is an issue with applications using Amp currently, but I'll resolve that soon.

@marcj

This comment has been minimized.

Show comment
Hide comment
@marcj

marcj Sep 4, 2017

Member

😱 This is huge. Awesome! If Aerys brings us the features we need (http multipart parsing, file upload and more stable data routing) then this is a pretty huge milestone for PHP-PM, as it means we finally support HTTP, which we probably never will with ReactPHP as it never reached such a state and probably never will with the current man-power/commitment behind it. So, for me the only logical way to continue is to add Aerys support and drop ReactPHP. If we won't, we will wait another 1-2 years until ReactPHP's http library reaches full HTTP support.

Huge +1 from me.

Is is much work to add file upload and POST support? https://github.com/php-pm/php-pm-httpkernel/pull/61/files#diff-7a7e05cf20fe81b51c4f4b40d6a919a7R149

Member

marcj commented Sep 4, 2017

😱 This is huge. Awesome! If Aerys brings us the features we need (http multipart parsing, file upload and more stable data routing) then this is a pretty huge milestone for PHP-PM, as it means we finally support HTTP, which we probably never will with ReactPHP as it never reached such a state and probably never will with the current man-power/commitment behind it. So, for me the only logical way to continue is to add Aerys support and drop ReactPHP. If we won't, we will wait another 1-2 years until ReactPHP's http library reaches full HTTP support.

Huge +1 from me.

Is is much work to add file upload and POST support? https://github.com/php-pm/php-pm-httpkernel/pull/61/files#diff-7a7e05cf20fe81b51c4f4b40d6a919a7R149

@trowski

This comment has been minimized.

Show comment
Hide comment
@trowski

trowski Sep 5, 2017

@andig Note that Amp is completely compatible with any code written for ReactPHP. Any projects based on ReactPHP will only need to make a minor change to use the React\Loop\LoopInterface instance provided by the adapter package from Amp: https://github.com/amphp/react-adapter.

@marcj Glad to see you're excited about this – we certainly are excited to bring more features and better performance to PHP-PM.

trowski commented Sep 5, 2017

@andig Note that Amp is completely compatible with any code written for ReactPHP. Any projects based on ReactPHP will only need to make a minor change to use the React\Loop\LoopInterface instance provided by the adapter package from Amp: https://github.com/amphp/react-adapter.

@marcj Glad to see you're excited about this – we certainly are excited to bring more features and better performance to PHP-PM.

@kelunik kelunik referenced this pull request Sep 5, 2017

Closed

Concurrent Requests #268

@@ -1,11 +0,0 @@
<?php

This comment has been minimized.

@andig

andig Sep 5, 2017

Contributor

Why remove the Async interface? The idea was to give hosted applications access to the same loop for additional async functionality?

@andig

andig Sep 5, 2017

Contributor

Why remove the Async interface? The idea was to give hosted applications access to the same loop for additional async functionality?

This comment has been minimized.

@kelunik

kelunik Sep 5, 2017

Contributor

Because it's entirely unused. With #268 concurrent requests might be unsupported in the future and in that case applications should use their own loop instead of reuse the same loop. Reasons for that are for example that a stopping of the event loop would also stop the outer event loop run.

@kelunik

kelunik Sep 5, 2017

Contributor

Because it's entirely unused. With #268 concurrent requests might be unsupported in the future and in that case applications should use their own loop instead of reuse the same loop. Reasons for that are for example that a stopping of the event loop would also stop the outer event loop run.

This comment has been minimized.

@andig

andig Sep 5, 2017

Contributor

Because it's entirely unused.

It's used here: https://github.com/php-pm/php-pm-psr7/blob/d1c225469f1ad2d38c78c571018967cc0186c290/src/Psr7Bridge.php#L34. The idea was to add it to HttpKernel as well.

@andig

andig Sep 5, 2017

Contributor

Because it's entirely unused.

It's used here: https://github.com/php-pm/php-pm-psr7/blob/d1c225469f1ad2d38c78c571018967cc0186c290/src/Psr7Bridge.php#L34. The idea was to add it to HttpKernel as well.

This comment has been minimized.

@kelunik

kelunik Sep 5, 2017

Contributor

Let's decide on #268 first before continuing the discussion whether it's still something that's needed.

@kelunik

kelunik Sep 5, 2017

Contributor

Let's decide on #268 first before continuing the discussion whether it's still something that's needed.

@andig

This comment has been minimized.

Show comment
Hide comment
@andig

andig Sep 5, 2017

Contributor

Any projects based on ReactPHP will only need to make a minor change to use the React\Loop\LoopInterface

I'm beginning to see. Sounds good! My question would be similar to marcj- one thing we wanted to profit from from react was HTTP request parsing. Also the new PSR7 (half)compatibility is a pro. Are there plans for Aerys to support the same?

Contributor

andig commented Sep 5, 2017

Any projects based on ReactPHP will only need to make a minor change to use the React\Loop\LoopInterface

I'm beginning to see. Sounds good! My question would be similar to marcj- one thing we wanted to profit from from react was HTTP request parsing. Also the new PSR7 (half)compatibility is a pro. Are there plans for Aerys to support the same?

@kelunik

This comment has been minimized.

Show comment
Hide comment
@kelunik

kelunik Sep 5, 2017

Contributor

@andig Request parsing is definitely supported, but it might need some minor adjustments to be compatible with PHP's default parsing, i.e. the handling of foo[]=test creating an array instead of being available as foo[] => test. There's an open TODO in the PR for that, I'll definitely add that, but would prefer investing the time only if the other parts are accepted.

PSR-7 (half)compatibility will definitely not be supported in Aerys directly, simply because full support isn't possible in a sane way (i.e. not buffering the complete body into memory).

Contributor

kelunik commented Sep 5, 2017

@andig Request parsing is definitely supported, but it might need some minor adjustments to be compatible with PHP's default parsing, i.e. the handling of foo[]=test creating an array instead of being available as foo[] => test. There's an open TODO in the PR for that, I'll definitely add that, but would prefer investing the time only if the other parts are accepted.

PSR-7 (half)compatibility will definitely not be supported in Aerys directly, simply because full support isn't possible in a sane way (i.e. not buffering the complete body into memory).

@kelunik

This comment has been minimized.

Show comment
Hide comment
@kelunik

kelunik Sep 9, 2017

Contributor

@marcj I just added $_POST support. File support is still missing, I'm not sure whether we can even emulate that, I'll have to test that.

Contributor

kelunik commented Sep 9, 2017

@marcj I just added $_POST support. File support is still missing, I'm not sure whether we can even emulate that, I'll have to test that.

Rewrite original port back in slaves
This makes applications reconstructing the URL from the host header work.
@kelunik

This comment has been minimized.

Show comment
Hide comment
@kelunik

kelunik Sep 10, 2017

Contributor

I'd be glad if someone could provide some Symfony demo application with file uploads to test.

Contributor

kelunik commented Sep 10, 2017

I'd be glad if someone could provide some Symfony demo application with file uploads to test.

@kelunik

This comment has been minimized.

Show comment
Hide comment
@kelunik

kelunik Sep 11, 2017

Contributor

@marcj Seems like we can't get is_uploaded_file and move_uploaded_file working.

Contributor

kelunik commented Sep 11, 2017

@marcj Seems like we can't get is_uploaded_file and move_uploaded_file working.

@marcj

This comment has been minimized.

Show comment
Hide comment
@marcj

marcj Sep 11, 2017

Member

@kelunik that's ok :) you're forced to use an abstraction anyway, so no need to call those functions when you use PHP-PM.

Member

marcj commented Sep 11, 2017

@kelunik that's ok :) you're forced to use an abstraction anyway, so no need to call those functions when you use PHP-PM.

@kelunik

This comment has been minimized.

Show comment
Hide comment
@kelunik

kelunik Sep 11, 2017

Contributor

@marcj But Symfony uses these I think.

Contributor

kelunik commented Sep 11, 2017

@marcj But Symfony uses these I think.

@marcj

This comment has been minimized.

Show comment
Hide comment
@marcj

marcj Sep 11, 2017

Member

I believe it uses them only when your create a Request from globals, like in app.php.

Member

marcj commented Sep 11, 2017

I believe it uses them only when your create a Request from globals, like in app.php.

@andho

This comment has been minimized.

Show comment
Hide comment
@andho

andho Sep 22, 2017

In Symfony you can choose a different class for File(Upload) handling. I did this for a system which stored files on an NFS Share. move_uploaded_file didn't work with the NFS share, so my alternate class used stream_copy_to_stream instead.

Wait, that might be from the VichUploader bundle.

andho commented Sep 22, 2017

In Symfony you can choose a different class for File(Upload) handling. I did this for a system which stored files on an NFS Share. move_uploaded_file didn't work with the NFS share, so my alternate class used stream_copy_to_stream instead.

Wait, that might be from the VichUploader bundle.

@max-voloshin

This comment has been minimized.

Show comment
Hide comment
@max-voloshin

max-voloshin Sep 30, 2017

Hi, @kelunik! I'm going to use PHP-PM on top of Ayres in some new project. What is the easiest way to start writing controllers with that approach? Should I implement ApplicationEnvironmentAwareInterface, BootstrapInterface and BridgeInterface adapters with \Aerys\Router?

max-voloshin commented Sep 30, 2017

Hi, @kelunik! I'm going to use PHP-PM on top of Ayres in some new project. What is the easiest way to start writing controllers with that approach? Should I implement ApplicationEnvironmentAwareInterface, BootstrapInterface and BridgeInterface adapters with \Aerys\Router?

@kelunik

This comment has been minimized.

Show comment
Hide comment
@kelunik

kelunik Sep 30, 2017

Contributor

@max-voloshin If you're using Symfony, everything will be just as before. There's a PR for the Symfony kernel: php-pm/php-pm-httpkernel#61. If you plan to use the Aerys APIs directly, I'd recommend using Aerys directly instead of using PHP-PM as a layer in between. If you have any questions about that approach, feel free to reach out via any of the channels listed on https://amphp.org/support.

Contributor

kelunik commented Sep 30, 2017

@max-voloshin If you're using Symfony, everything will be just as before. There's a PR for the Symfony kernel: php-pm/php-pm-httpkernel#61. If you plan to use the Aerys APIs directly, I'd recommend using Aerys directly instead of using PHP-PM as a layer in between. If you have any questions about that approach, feel free to reach out via any of the channels listed on https://amphp.org/support.

@max-voloshin

This comment has been minimized.

Show comment
Hide comment

max-voloshin commented Oct 2, 2017

@marcj

This comment has been minimized.

Show comment
Hide comment
@marcj

marcj Oct 19, 2017

Member

Alrighty guys, how should we proceed? As already pointed out, we need Aerys to have the fundamental feature-set (http multipart parsing, file upload, ...) working. Maybe @clue can give us more insights about ReactPHP's development, so we can decide which way to go. I guess there's only one way: either Aerys or ReactPHP, but not both. Or do you guys see a different route?

The only reason I'd prefer Aerys is the features-set they provide. Code quality and other topics keep me away from it. But I'd swallow the pill if ReactPHP can't provide following features in the near feature:

  • Multipart parsing
  • File upload
  • HTTP/2 (would be cool)

Please throw out your opinions so we can make PHP-PM finally production ready. We need a decision soon as all PRs an dangling since we can't assume @kelunik will rebase all changes again. So we should wait until we get a decision on this one.

Member

marcj commented Oct 19, 2017

Alrighty guys, how should we proceed? As already pointed out, we need Aerys to have the fundamental feature-set (http multipart parsing, file upload, ...) working. Maybe @clue can give us more insights about ReactPHP's development, so we can decide which way to go. I guess there's only one way: either Aerys or ReactPHP, but not both. Or do you guys see a different route?

The only reason I'd prefer Aerys is the features-set they provide. Code quality and other topics keep me away from it. But I'd swallow the pill if ReactPHP can't provide following features in the near feature:

  • Multipart parsing
  • File upload
  • HTTP/2 (would be cool)

Please throw out your opinions so we can make PHP-PM finally production ready. We need a decision soon as all PRs an dangling since we can't assume @kelunik will rebase all changes again. So we should wait until we get a decision on this one.

@andig

This comment has been minimized.

Show comment
Hide comment
@andig

andig Oct 19, 2017

Contributor

Maybe @clue can give us more insights about ReactPHP's development, so we can decide which way to go.

React is almost there. All required PRs exist and its up to react to decide when they want to merge (dev-master) and what else need be included before 0.8 can be released. Its imho sooner rather than later. Only exception is HTTP/2.

Contributor

andig commented Oct 19, 2017

Maybe @clue can give us more insights about ReactPHP's development, so we can decide which way to go.

React is almost there. All required PRs exist and its up to react to decide when they want to merge (dev-master) and what else need be included before 0.8 can be released. Its imho sooner rather than later. Only exception is HTTP/2.

@marcj

This comment has been minimized.

Show comment
Hide comment
@marcj

marcj Oct 19, 2017

Member

Only exception is HTTP/2.

Is more optional than requirement I guess. (adjusted my list)

Member

marcj commented Oct 19, 2017

Only exception is HTTP/2.

Is more optional than requirement I guess. (adjusted my list)

@optiman

This comment has been minimized.

Show comment
Hide comment
@optiman

optiman Oct 19, 2017

Another reason for Aerys is much better performance:
https://github.com/torinaki/aerys-benchmark

optiman commented Oct 19, 2017

Another reason for Aerys is much better performance:
https://github.com/torinaki/aerys-benchmark

@dzubchik

This comment has been minimized.

Show comment
Hide comment
@dzubchik

dzubchik Oct 20, 2017

Contributor

@marcj and @andig, we see the work you have done and we really appreciate this. It would be nice to have file uploads and multipart parsing now. But, IMO, the project has a lack of tests (I mean integration tests with bridges) and without them, only crazy people would use it on production (as we do). Is it a big problem to choose react or aerys? Maybe we could write an adapter with interfaces like file uploads, http/2, etc?

Contributor

dzubchik commented Oct 20, 2017

@marcj and @andig, we see the work you have done and we really appreciate this. It would be nice to have file uploads and multipart parsing now. But, IMO, the project has a lack of tests (I mean integration tests with bridges) and without them, only crazy people would use it on production (as we do). Is it a big problem to choose react or aerys? Maybe we could write an adapter with interfaces like file uploads, http/2, etc?

@kelunik

This comment has been minimized.

Show comment
Hide comment
@kelunik

kelunik Oct 20, 2017

Contributor

@optiman The posted benchmark isn't really fair as ReactPHP doesn't support keep-alive, yet. But even without keep-alive Aerys serves 1.5k requests / s while ReactPHP serves 1.1k requests / s for me.

But, IMO, the project has a lack of tests (I mean integration tests with bridges) and without them, only crazy people would use it on production (as we do).

I'd say that the lack of releases is equally important. Relying on dev-master isn't a good thing.

While HTTP/2 support is nice to have, it's not as important as one might think. Currently I wouldn't recommend using PHP directly for TLS termination, but HTTP/2 will only be used for https:// sites by browsers. It might still be very useful if your reverse proxy supports h2c.

TBH, there are a few other things missing for production readiness. Currently PHP-PM just forwards all client data from a single process to the child processes to handle it. Usually that won't be a bottleneck I guess, but that might depend on the application and how many cores you have available.

A client is currently assigned to exactly one worker. The master process doesn't know anything about requests. That means a worker processes all requests from one connection. This might result in a sub-optimal request scheduling that PHP-PM is supposed to solve by choosing a free worker for a new connection. A solution to this is to directly run react/http / Aerys and using their request handlers to dispatch requests to workers instead of assigning connections to workers.

On the other hand this can be seen as unnecessary, as it works good enough without per-request scheduling. I know of people using Aerys directly for serving a totally synchronous applications and it works good enough for a high-traffic site. Maybe Aerys could be used directly in the future without PHP-PM and the bridges be built to map directly from Aerys requests. Aerys already includes a process manager and restarts crashing workers.

Contributor

kelunik commented Oct 20, 2017

@optiman The posted benchmark isn't really fair as ReactPHP doesn't support keep-alive, yet. But even without keep-alive Aerys serves 1.5k requests / s while ReactPHP serves 1.1k requests / s for me.

But, IMO, the project has a lack of tests (I mean integration tests with bridges) and without them, only crazy people would use it on production (as we do).

I'd say that the lack of releases is equally important. Relying on dev-master isn't a good thing.

While HTTP/2 support is nice to have, it's not as important as one might think. Currently I wouldn't recommend using PHP directly for TLS termination, but HTTP/2 will only be used for https:// sites by browsers. It might still be very useful if your reverse proxy supports h2c.

TBH, there are a few other things missing for production readiness. Currently PHP-PM just forwards all client data from a single process to the child processes to handle it. Usually that won't be a bottleneck I guess, but that might depend on the application and how many cores you have available.

A client is currently assigned to exactly one worker. The master process doesn't know anything about requests. That means a worker processes all requests from one connection. This might result in a sub-optimal request scheduling that PHP-PM is supposed to solve by choosing a free worker for a new connection. A solution to this is to directly run react/http / Aerys and using their request handlers to dispatch requests to workers instead of assigning connections to workers.

On the other hand this can be seen as unnecessary, as it works good enough without per-request scheduling. I know of people using Aerys directly for serving a totally synchronous applications and it works good enough for a high-traffic site. Maybe Aerys could be used directly in the future without PHP-PM and the bridges be built to map directly from Aerys requests. Aerys already includes a process manager and restarts crashing workers.

@andig

This comment has been minimized.

Show comment
Hide comment
@andig

andig Oct 20, 2017

Contributor

These are the things I can currently see on the priority list:

  • rely on stable releases (whatever those are) for as-is functionality including multipart and files- afaik react/http will provide a basis with 0.8 after a longish two-year wait
  • restructure and rewrite PPM (maybe with additional goals as outlined by @kelunik) - while the current code works it's highly complex (large methods, only few source files) and hard to maintain. A moot point unless dedicated capacity becomes available.
  • create a solid test infrastructure

React does imho provide battle-proven code of excellent quality and a solid base of maintainers. For react/http full PSR7 support via RequestBodyBufferMiddleware is a (small) plus that could remove the need for the PSR7 bridge.
For those who're interested in archeology both ppm and react still support php5. I can only assume that might at least contribute to the performance delta due to clumsier code.

Maybe Aerys could be used directly in the future without PHP-PM and the bridges be built to map directly from Aerys requests. Aerys already includes a process manager and restarts crashing workers.

Interesting. Would this speak against all of the above topics?

Contributor

andig commented Oct 20, 2017

These are the things I can currently see on the priority list:

  • rely on stable releases (whatever those are) for as-is functionality including multipart and files- afaik react/http will provide a basis with 0.8 after a longish two-year wait
  • restructure and rewrite PPM (maybe with additional goals as outlined by @kelunik) - while the current code works it's highly complex (large methods, only few source files) and hard to maintain. A moot point unless dedicated capacity becomes available.
  • create a solid test infrastructure

React does imho provide battle-proven code of excellent quality and a solid base of maintainers. For react/http full PSR7 support via RequestBodyBufferMiddleware is a (small) plus that could remove the need for the PSR7 bridge.
For those who're interested in archeology both ppm and react still support php5. I can only assume that might at least contribute to the performance delta due to clumsier code.

Maybe Aerys could be used directly in the future without PHP-PM and the bridges be built to map directly from Aerys requests. Aerys already includes a process manager and restarts crashing workers.

Interesting. Would this speak against all of the above topics?

@andig

This comment has been minimized.

Show comment
Hide comment
@andig

andig Oct 22, 2017

Contributor

The only reason I'd prefer Aerys is the features-set they provide. ... Multipart parsing ... File upload

Please see #271. I would appreciate testing as I'm not fluent with symfony. File uploads should be working (not tested!) as the react MultipartParser already supports files.

not working yet is UDS on OSX (waiting for react/socket release this weekend, workaround included) and chunked request handling (waiting for react/http merge).

Contributor

andig commented Oct 22, 2017

The only reason I'd prefer Aerys is the features-set they provide. ... Multipart parsing ... File upload

Please see #271. I would appreciate testing as I'm not fluent with symfony. File uploads should be working (not tested!) as the react MultipartParser already supports files.

not working yet is UDS on OSX (waiting for react/socket release this weekend, workaround included) and chunked request handling (waiting for react/http merge).

@clue

This comment has been minimized.

Show comment
Hide comment
@clue

clue Oct 23, 2017

@marcj Thank your for your patience and friendly ping. I think that you brought up some very valid points and I very much appreciate this PR and the related discussion!

I understand that it may be frustrating that @reactphp may not always develop at the speed we all wish :-) That being said, I think we've made some major improvements in the past months and are actively working on getting these features out. All of your above wishlist items are already implemented in the current development version and we're currently actively working with @andig (thank you!) to get some minor additional features released that are also used as part of php-pm.

I mostly agree with your assertion, I can assure you the @reactphp teams values stable, sustainable and high quality components over quick hacks. We're currently in the process of stabilizing things and can assure you we'll get a stable release out as soon as possible.

Given that your above wishlist items are already implemented, you can try this out by relying on dev-master (see also @andig's excellent PR!). Of course, relying on a branch long-term is not a good idea, but this should give you a good overview of the current state which should be really stable already and will be part of the next release.

I'm giving a few talks in the next two weeks, so I won't be able to make any promises in this time frame. But rest assured, this is a high priority item on my list immediately thereafter. :shipit:

Thank you! <3

clue commented Oct 23, 2017

@marcj Thank your for your patience and friendly ping. I think that you brought up some very valid points and I very much appreciate this PR and the related discussion!

I understand that it may be frustrating that @reactphp may not always develop at the speed we all wish :-) That being said, I think we've made some major improvements in the past months and are actively working on getting these features out. All of your above wishlist items are already implemented in the current development version and we're currently actively working with @andig (thank you!) to get some minor additional features released that are also used as part of php-pm.

I mostly agree with your assertion, I can assure you the @reactphp teams values stable, sustainable and high quality components over quick hacks. We're currently in the process of stabilizing things and can assure you we'll get a stable release out as soon as possible.

Given that your above wishlist items are already implemented, you can try this out by relying on dev-master (see also @andig's excellent PR!). Of course, relying on a branch long-term is not a good idea, but this should give you a good overview of the current state which should be really stable already and will be part of the next release.

I'm giving a few talks in the next two weeks, so I won't be able to make any promises in this time frame. But rest assured, this is a high priority item on my list immediately thereafter. :shipit:

Thank you! <3

@trowski

This comment has been minimized.

Show comment
Hide comment
@trowski

trowski Oct 30, 2017

Note that choosing Amp/Aerys as a basis for PHP-PM has the advantage of offering both a ReactPHP-compatible environment and an Amp-compatible environment. Amp is capable of running any code written for React via a simple loop adaptor.

trowski commented Oct 30, 2017

Note that choosing Amp/Aerys as a basis for PHP-PM has the advantage of offering both a ReactPHP-compatible environment and an Amp-compatible environment. Amp is capable of running any code written for React via a simple loop adaptor.

*/
interface BootstrapInterface
{
public function getApplication();
public function getStaticDirectory();
public function getStaticDirectory(): string;

This comment has been minimized.

@mvhirsch

mvhirsch Oct 31, 2017

This change forces to use php 7.0 or higher. composer.json only requires php 5.6 or higher.
This results in a BC-break.

@mvhirsch

mvhirsch Oct 31, 2017

This change forces to use php 7.0 or higher. composer.json only requires php 5.6 or higher.
This results in a BC-break.

This comment has been minimized.

@kelunik

kelunik Oct 31, 2017

Contributor

There are anyway no releases, yet. There are bigger breaks. And increasing the required PHP version isn't a BC break.

@kelunik

kelunik Oct 31, 2017

Contributor

There are anyway no releases, yet. There are bigger breaks. And increasing the required PHP version isn't a BC break.

This comment has been minimized.

@trowski

trowski Oct 31, 2017

I'm certain that not changing composer.json to require 7.0 instead of 5.6 was simply an oversight and will be fixed if this PR is merged. Composer will not pull versions that increase the PHP version requirement if at least that version of PHP is not installed.

@trowski

trowski Oct 31, 2017

I'm certain that not changing composer.json to require 7.0 instead of 5.6 was simply an oversight and will be fixed if this PR is merged. Composer will not pull versions that increase the PHP version requirement if at least that version of PHP is not installed.

@@ -32,13 +32,13 @@ protected function execute(InputInterface $input, OutputInterface $output)
{
$config = $this->initializeConfig($input, $output);
$class = isset($config['processmanager']) ? $config['processmanager'] : ProcessManager::class;
$class = $config['processmanager'] ?? ProcessManager::class;

This comment has been minimized.

@mvhirsch

mvhirsch Oct 31, 2017

The null coalescing operator is only avaiable since php 7.0 or higher. composer.json allows php 5.6 or higher. This results in a BC break.

@mvhirsch

mvhirsch Oct 31, 2017

The null coalescing operator is only avaiable since php 7.0 or higher. composer.json allows php 5.6 or higher. This results in a BC break.

This comment has been minimized.

@kelunik

kelunik Oct 31, 2017

Contributor

7.0 is anyway required by this PR.

@kelunik

kelunik Oct 31, 2017

Contributor

7.0 is anyway required by this PR.

@mvhirsch

This comment has been minimized.

Show comment
Hide comment
@mvhirsch

mvhirsch Oct 31, 2017

Please be aware, that this PR has syntactic changes which requires PHP 7.0 or higher!
I've noted two warnings about that in the diff (Null Coalescing Operator && Return Type Hinting).

Since composer.json currently allows PHP 5.6 or higher, this MR will break user's projects if they depend on this! Please keep in mind, that using new versions of PHP is good, but not by breaking backwards compatibility and leaving users behind (not knowing that they need to upgrade to PHP 7; or even worse: by forcing them to upgrade to PHP 7).

Please provide an upgrade-path (changelog, documentation, ...), so users of this package can still use PHP-PM on ~5.6.0 and smoothly upgrade to PHP 7.

mvhirsch commented Oct 31, 2017

Please be aware, that this PR has syntactic changes which requires PHP 7.0 or higher!
I've noted two warnings about that in the diff (Null Coalescing Operator && Return Type Hinting).

Since composer.json currently allows PHP 5.6 or higher, this MR will break user's projects if they depend on this! Please keep in mind, that using new versions of PHP is good, but not by breaking backwards compatibility and leaving users behind (not knowing that they need to upgrade to PHP 7; or even worse: by forcing them to upgrade to PHP 7).

Please provide an upgrade-path (changelog, documentation, ...), so users of this package can still use PHP-PM on ~5.6.0 and smoothly upgrade to PHP 7.

@dzubchik

This comment has been minimized.

Show comment
Hide comment
@dzubchik

dzubchik Oct 31, 2017

Contributor

this MR will break user's projects if they depend on this

No, will not. Because if you use composer correctly, have composer.lock in the repository it will install the latest working version of php-pm that does not have new requirements.

Contributor

dzubchik commented Oct 31, 2017

this MR will break user's projects if they depend on this

No, will not. Because if you use composer correctly, have composer.lock in the repository it will install the latest working version of php-pm that does not have new requirements.

@mvhirsch

This comment has been minimized.

Show comment
Hide comment
@mvhirsch

mvhirsch Nov 2, 2017

No, will not. Because if you use composer correctly, have composer.lock in the repository it will install the latest working version of php-pm that does not have new requirements.

Sure? Requiring php-pm and updating it afterwards via composer on PHP 5.6.32 this won't work in my opinion. Anyway, just wanted to hint on this, before it gets merged. :-)

mvhirsch commented Nov 2, 2017

No, will not. Because if you use composer correctly, have composer.lock in the repository it will install the latest working version of php-pm that does not have new requirements.

Sure? Requiring php-pm and updating it afterwards via composer on PHP 5.6.32 this won't work in my opinion. Anyway, just wanted to hint on this, before it gets merged. :-)

@dzubchik

This comment has been minimized.

Show comment
Hide comment
@dzubchik

dzubchik Nov 2, 2017

Contributor

@mvhirsch , unless you run composer update without --ignore-platform-reqs, it will not upgrade package to unsupported PHP version (to PHP 7, if you use PHP 5.6).

Contributor

dzubchik commented Nov 2, 2017

@mvhirsch , unless you run composer update without --ignore-platform-reqs, it will not upgrade package to unsupported PHP version (to PHP 7, if you use PHP 5.6).

@andig

This comment has been minimized.

Show comment
Hide comment
@andig

andig Nov 2, 2017

Contributor

Please be assured that we're aware of the fact that Amp requires PHP7 while as of now php-pm still supports PHP 5.6. Since this PR has evolved into a roadmap discussion I'd like to make the following proposal for proceeding:

  1. Upgrade ppm to stable react releases (i.e. #271). This work is ready for testing but currently lacking any feedback. At this stage development is stuck.
  2. Release a ppm 1.0 version with PHP 5.6 support on this basis.
  3. Upgrade master to PHP7.
  4. Re-assess Amp vs. React. At this stage I don't see any advantage in one or the other. The painpoints with ppm are imho more in the complexity and lack of tests, less in the underlying library once react has been upgraded to stable. If one library provides better base features and can help reduce code for worker management, scheduling etc it might be the more suitable candidate.
Contributor

andig commented Nov 2, 2017

Please be assured that we're aware of the fact that Amp requires PHP7 while as of now php-pm still supports PHP 5.6. Since this PR has evolved into a roadmap discussion I'd like to make the following proposal for proceeding:

  1. Upgrade ppm to stable react releases (i.e. #271). This work is ready for testing but currently lacking any feedback. At this stage development is stuck.
  2. Release a ppm 1.0 version with PHP 5.6 support on this basis.
  3. Upgrade master to PHP7.
  4. Re-assess Amp vs. React. At this stage I don't see any advantage in one or the other. The painpoints with ppm are imho more in the complexity and lack of tests, less in the underlying library once react has been upgraded to stable. If one library provides better base features and can help reduce code for worker management, scheduling etc it might be the more suitable candidate.
@marcj

This comment has been minimized.

Show comment
Hide comment
@marcj

marcj Nov 20, 2017

Member

The biggest pain point and showstopper for most people is missing file upload and multipart parsing support. Without that, it is basically useless if you have a web-service that allows users to upload things. We need that in React or we can't rely on React. Also I haven't read yet a thing about that support anywhere. Do you know the progress @andig? I'd go ReactPHP when we get that support. Any ideas @clue?

Member

marcj commented Nov 20, 2017

The biggest pain point and showstopper for most people is missing file upload and multipart parsing support. Without that, it is basically useless if you have a web-service that allows users to upload things. We need that in React or we can't rely on React. Also I haven't read yet a thing about that support anywhere. Do you know the progress @andig? I'd go ReactPHP when we get that support. Any ideas @clue?

@andig

This comment has been minimized.

Show comment
Hide comment
@andig

andig Nov 20, 2017

Contributor

The biggest pain point and showstopper for most people is missing file upload and multipart parsing support.

React already has file upload and multipart in dev-master afaik. It's also implemented as part of #271.

See https://github.com/reactphp/http/blob/master/src/Io/MultipartParser.php and https://github.com/reactphp/http/blob/master/src/Io/UploadedFile.php.

Contributor

andig commented Nov 20, 2017

The biggest pain point and showstopper for most people is missing file upload and multipart parsing support.

React already has file upload and multipart in dev-master afaik. It's also implemented as part of #271.

See https://github.com/reactphp/http/blob/master/src/Io/MultipartParser.php and https://github.com/reactphp/http/blob/master/src/Io/UploadedFile.php.

@colinmollenhour

This comment has been minimized.

Show comment
Hide comment
@colinmollenhour

colinmollenhour Nov 20, 2017

Regarding users depending on file upload, perhaps this could be worked around using nginx locations that proxy to fcgi for example.

server {
    root /app;
    server_name example.com;

    # Uploads are not yet supported
    location = /my/app/uploader/ {
        fastcgi_pass 127.0.0.1:9000;
        include fastcgi_params;
    }

    # All other requests can go to PPM
    location / {
        try_files $uri @ppm;
    }
    location @ppm {
        proxy_set_header Host $http_host;
        proxy_set_header X-Real-IP $remote_addr;
        proxy_set_header X-Forwarded-For $proxy_add_x_forwarded_for;
        proxy_set_header X-Forwarded-Proto $scheme;
        proxy_pass http://127.0.0.1:8080;
    }
}

colinmollenhour commented Nov 20, 2017

Regarding users depending on file upload, perhaps this could be worked around using nginx locations that proxy to fcgi for example.

server {
    root /app;
    server_name example.com;

    # Uploads are not yet supported
    location = /my/app/uploader/ {
        fastcgi_pass 127.0.0.1:9000;
        include fastcgi_params;
    }

    # All other requests can go to PPM
    location / {
        try_files $uri @ppm;
    }
    location @ppm {
        proxy_set_header Host $http_host;
        proxy_set_header X-Real-IP $remote_addr;
        proxy_set_header X-Forwarded-For $proxy_add_x_forwarded_for;
        proxy_set_header X-Forwarded-Proto $scheme;
        proxy_pass http://127.0.0.1:8080;
    }
}
@clue

This comment has been minimized.

Show comment
Hide comment
@clue

clue Nov 21, 2017

Small progress upgrade: Conference time is over for me this year and I've picked up working on the outstanding PRs again. We've released a number of feature releases in the last weeks and are currently working on stabilizing the HTTP component. As far as I can tell everything that is needed for PHP-PM should already be in :shipit:

@marcj The biggest pain point and showstopper for most people is missing file upload and multipart parsing support.

This feature has already been implemented a few weeks back, have you seen https://github.com/reactphp/http#requestbodyparsermiddleware yet?

I'm unsure what the current status of this ticket is. I would like to suggest continuing with @andig's excellent PR #271 which updates to latest ReactPHP components. This should be a solid base to then look into evaluating multipart parsing / file uploads 👍

If you feel anything's missing from ReactPHP's side of things, please let me know and I'm happy to look into this.

clue commented Nov 21, 2017

Small progress upgrade: Conference time is over for me this year and I've picked up working on the outstanding PRs again. We've released a number of feature releases in the last weeks and are currently working on stabilizing the HTTP component. As far as I can tell everything that is needed for PHP-PM should already be in :shipit:

@marcj The biggest pain point and showstopper for most people is missing file upload and multipart parsing support.

This feature has already been implemented a few weeks back, have you seen https://github.com/reactphp/http#requestbodyparsermiddleware yet?

I'm unsure what the current status of this ticket is. I would like to suggest continuing with @andig's excellent PR #271 which updates to latest ReactPHP components. This should be a solid base to then look into evaluating multipart parsing / file uploads 👍

If you feel anything's missing from ReactPHP's side of things, please let me know and I'm happy to look into this.

@marcj

This comment has been minimized.

Show comment
Hide comment
@marcj

marcj Nov 21, 2017

Member

@clue @andig ok awesome, that was actually the only points that would have made Aerys lucrative.

By all hard work from @kelunik, we really appreciate your effort in building such a huge PR, but for the time being I'm closing this PR in favour of #271. Thanks anyways again! 🍺❤️

Why?

Well, we started with ReactPHP and we think that ReactPHP is a great library (beside its lack of development in the last 1 year, but that got fixed obviously). The reasons we wanted to go away from ReactPHP are mainly the lack of a tagged version with HTTP support, concretely multipart handling. That seems now to be fixed, which let us now provide a PHP-PM version that has everything necessary to let it run for production system. I had it running for aetros.com which has ~15mio php request/month, and it worked great, but had to remove it duo to multiparse requests.

Aerys would have been a great replacement for ReactPHP if latter wouldn't have increased its dev speed lately and introduced the missing features. Having said that, I liked the features of Aerys but they mainly conflicts with PHP-PM as the features Aerys is providing is either from the same use-case as PHP-PM's or is not necessary by PHP-PM. The small part of the actual HTTP handling is not worth bringing a huge library like Aerys under the hood of PHP-PM.

Also, Aerys does not bring any features on the table we're currently missing (anymore). It's actually vice-versa, Aerys brings features we don't need, which makes it very bloated for our use-case. A very well abstracted and in little parts split library like ReactPHP is here better. Also there're other reasons I'd definitely prefer ReactPHP, but I think that addressed ones are enough to justify to choose ReactPHP for now.

Thanks anyone for contributing!

Member

marcj commented Nov 21, 2017

@clue @andig ok awesome, that was actually the only points that would have made Aerys lucrative.

By all hard work from @kelunik, we really appreciate your effort in building such a huge PR, but for the time being I'm closing this PR in favour of #271. Thanks anyways again! 🍺❤️

Why?

Well, we started with ReactPHP and we think that ReactPHP is a great library (beside its lack of development in the last 1 year, but that got fixed obviously). The reasons we wanted to go away from ReactPHP are mainly the lack of a tagged version with HTTP support, concretely multipart handling. That seems now to be fixed, which let us now provide a PHP-PM version that has everything necessary to let it run for production system. I had it running for aetros.com which has ~15mio php request/month, and it worked great, but had to remove it duo to multiparse requests.

Aerys would have been a great replacement for ReactPHP if latter wouldn't have increased its dev speed lately and introduced the missing features. Having said that, I liked the features of Aerys but they mainly conflicts with PHP-PM as the features Aerys is providing is either from the same use-case as PHP-PM's or is not necessary by PHP-PM. The small part of the actual HTTP handling is not worth bringing a huge library like Aerys under the hood of PHP-PM.

Also, Aerys does not bring any features on the table we're currently missing (anymore). It's actually vice-versa, Aerys brings features we don't need, which makes it very bloated for our use-case. A very well abstracted and in little parts split library like ReactPHP is here better. Also there're other reasons I'd definitely prefer ReactPHP, but I think that addressed ones are enough to justify to choose ReactPHP for now.

Thanks anyone for contributing!

@marcj marcj closed this Nov 21, 2017

@kelunik kelunik referenced this pull request Nov 21, 2017

Closed

RFC: Remove AsyncInterface #284

@andig andig referenced this pull request Dec 8, 2017

Closed

Moving from reactphp to amphp #146

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment