Implement stackphp; cleanup handlePageCache() and preHandle()

I'm actually shocked that it works; we've never gotten the kernel to actually wrap properly before. How does this relate to Mark's old issue that lawrolan had picked up?

Related issue is #1869548: Opt-in CORS support since there seems to be a CORS middleware in the list given by OP.

I'm going to go out on a limb and say yes, let's do this.

+++ b/core/core.services.yml
@@ -318,7 +318,19 @@ services:
+  http_kernel_test:
+    class: Drupal\Core\HttpKernel\TestMiddleware
+    arguments: ['@http_kernel.basic']
+    tags:
+      - { name: http_middleware }

But this should be taken out of core.services.yml and moved to somewhere that it can be used with just the test.

Want!

New library makes this Dries's call.

We definitely need an AbstractSingletonProxyFactoryBean next.

Perhaps help us understanding by providing some Drupal specific examples please?

Suppose one of the benefits could be more clean way of implementing a page cache (removal of $kernel->handlePageCache($request)) which is drupalism.
Also probably could open doors for #1597696: Consider whether HttpCache offers any significant benefit over the existing page cache

+++ b/core/lib/Drupal/Core/Form/FormBuilder.php
@@ -121,7 +121,7 @@ class FormBuilder implements FormBuilderInterface, FormValidatorInterface, FormS
    * @param \Drupal\Core\HttpKernel $http_kernel
    *   The HTTP kernel.
...
-  public function __construct(FormValidatorInterface $form_validator, FormSubmitterInterface $form_submitter, ModuleHandlerInterface $module_handler, KeyValueExpirableFactoryInterface $key_value_expirable_factory, EventDispatcherInterface $event_dispatcher, RequestStack $request_stack, ClassResolverInterface $class_resolver, CsrfTokenGenerator $csrf_token = NULL, HttpKernel $http_kernel = NULL) {
+  public function __construct(FormValidatorInterface $form_validator, FormSubmitterInterface $form_submitter, ModuleHandlerInterface $module_handler, KeyValueExpirableFactoryInterface $key_value_expirable_factory, EventDispatcherInterface $event_dispatcher, RequestStack $request_stack, ClassResolverInterface $class_resolver, CsrfTokenGenerator $csrf_token = NULL, HttpKernelInterface $http_kernel = NULL) {

docs needs update to interface too

Added another possible use case...

Back to RTBC as per #11.

I'd prefer to hold off adding this library until we better understand how we'd actually use it. The arguments in this issue feel too speculative. I'd like to see us do more work on the related issues to help us understand if stackphp is the best solution. If we agree that it is the best solution, I'll commit this library.

For example the CORS issue, for sure, could be solved differently (noone denies that), but solving things properly using an already existing
pattern costs less resources than introducing various ways for all the different usecases.

I see nothing in #31 that is recommending that we try to solve #1869548: Opt-in CORS support, #1505080: [META] Content Negotiation for Accept Headers, or any other issue that could benefit from this, in a way that doesn't use stackphp. Only that we pick at least one of those issues to actually solve with stackphp + a middleware library, so that we know for sure that that solution both works and is something we're happy with. I think that the assumption that the stack-cors or StackNegotiation libraries will solve our needs on those issues is what is currently speculative.

Hm. I first wanted to disagree with #31, but after studying the code, I think I agree.

I expected Stack to operate at the front-controller level (extra-app) instead of at the service container level (intra-app).

The concept implies and assumes a compatibility in terms of standardized service IDs, because all stacks are operating within the same container, which is not necessarily a given. Drupal diverges from Symfony CMF in various ways - partially for very good reasons, and also, Symfony full-stack is a custom application of its own, not a "standard".

Likewise, the "integrate a [native] Symfony project directly into Drupal" example equally assumes that your Drupal service container has all the custom services of your custom/native Symfony app.

I doubt that. Not only service IDs, but also the architecture of services would have to match (barely the case for e.g. Session handling). You'd have to have identical dependencies, up to a specific version level, registered and made available by one and the same service container. If that was the case, you'd have achieved a level of structural coherency that inherently makes the entire idea of multiple, individual applications obsolete/nonsensical.

Routing/processing every request through multiple HttpKernel instances (operating within the full scope of the same application) sounds detrimental to performance. The statistics.php example mentioned in the OP is most likely (not verified) outweighed by the negative impact / additional overhead of non-statistics.php requests, which would needlessly enter the Statistics-specific HttpKernel code path, just to say "No, but thanks for asking." - Same goes for IP blocking/banning.

The point of stacking multiple HTTP applications on top of each other would only make sense if the apps were consulted in a truly isolated fashion — e.g., ask a super-thin (separate) Statistics HttpKernel whether it can handle the request; otherwise, consult the (big & slow) main application.

Unless I'm missing something, the current architectural concept of Stack can be re-implemented (entirely) through HttpKernel Events (which, FWIW, have a much more reliable/robust API in terms of cross-project integration).

I'm not able to see a need for that, especially in Drupal. Modules are able to manipulate the application in plenty of (unholy) ways already. It is rather unlikely that there is a middleware application that is able to operate with Drupal's service container, so stacking HttpKernel services within the same, full-blown application does not seem to add any value?

It is possible that I'm lacking experience as well as use-cases in this area of (seemingly vertical) integration (that seemingly asks for horizontal integration in reality). Please correct me where I'm wrong.

I think its fair of #31 to ask for an actual implementation example and be it just a little test or example implementation / sandbox.

We provide that for most other issues, too.

I would also like to see an example as I am really curious, but cannot see how this could work / how it would look like ...

In that case, maintainer needs more info is probably more right :)

Routing/processing every request through multiple HttpKernel instances (operating within the full scope of the same application) sounds detrimental to performance

Why is this detrimental to performance? and why do they have to be HttpKernel instances? They just simply have to implement HttpKernelInterface, which last time I looked, just consisted of a handle() method. This could contain any level of complexity/functionality/overhead.

It is not about deciding which kernel will handle a request but creating a series of decorated classes that delegates to the next. This is a simple concept, that is IMO very effective and powerful because of this.

I agree we could do with an initial implementation of this.

@damiankloip: Good point. I didn't think of the possibility of "faking" an HttpKernel by implementing its interface only, in order to short-circuit a special behavior.

Nevertheless, I think the same can be achieved more reliably through events (operating at almost the exact same layer and point in time), and the other arguments about compatibility/assumptions/integration could use some counter-arguments, too. :-) (I do hope my arguments are wrong, so please prove me wrong.)

The "preferred" way to use Stack, as I understand it, is to edit your front controller directly. That way every kernel implementation is entirely independent and doesn't even know about each other; they could all be using different DICs, or no DIC, or whatever.

'course, Drupal doesn't let you edit index.php. We frown on that quite severely. That's why dawehner's patch stacks the kernels up via our container. However, if middlewares are properly written then it shouldn't matter. It just means we need a bridge module to set it up rather than instructions on how to edit index.php. Our DrupalKernel is already doing quite a bit to build an environment inside of which HttpKernels can do their thing. That's our limitation, not Stack's.

Support for stacking kernels like this (kernel interface implementations, rather) is a very deliberate design decision of Symfony's HttpKernel component, and I've wanted to get us compatible with that for over a year and a half. That we're able to do so now is a testament to the labor of lots of people to get us to that point, and my hats off to them. (Seriously, this rocks.)

It differs from "just use a request listener" in that it's a first-class citizen. Also, it doesn't require listeners. You can build a complete StackPHP setup that has no EventDispatcher at all. Or uses Zend's event dispatcher (if they have one), or Eventement, or whatever.

Dries: I don't know what you mean by "we better understand how we'd use it". There are multiple issues right now, linked from the summary, where we're saying "we know we want to use it here". Is that insufficient? Or is this another one of those places where we need a larger goldilocks to make the case? (IE, we need to also implement CORS or ConNeg or HttpCache or whatever in the same issue in order to "prove" that it's useful?)

I think the latest example exactly demos the way it works in useful manner (fixing an issue)

Here is the converted unit test, and the namespace fix. Setting back to NR.

If anyone is still wondering what this is about. I would recommend having a look at Rack.

Looks great, to solve the original issue we would need to extend that for X_FORWARDED_PROTO, right?

Though I can totally see how this makes it simpler and Decorator pattern is real clean here.

+++ b/core/lib/Drupal/Core/StackMiddleware/ReverseProxyMiddleware.php
@@ -0,0 +1,61 @@
+    $this->settings = $settings;

Once #2251113: Use container parameters instead of settings lands, we can inject the needed value directly rather than using the Settings object. (It will likely become just parameters in services.yml, so trivial to inject.) That would make this class entirely Drupal-decoupled, which is great.

That might likely conflict with the linked issue, but otherwise I think this does successfully demonstrate how we can use middlewares with stack, even without talking about 3rd party libs. Those are a bonus for follow up issues. Therefore, back to RTBC. (Whoever commits second rerolls, I guess.)

Dries: I don't know what you mean by "we better understand how we'd use it". There are multiple issues right now, linked from the summary, where we're saying "we know we want to use it here". Is that insufficient? Or is this another one of those places where we need a larger goldilocks to make the case? (IE, we need to also implement CORS or ConNeg or HttpCache or whatever in the same issue in order to "prove" that it's useful?)

Yep, the idea was that both the referenced issues just had sort of a nebulous "this could be useful here" rather than any actual demonstration-with-code that it was the best solution. Thanks @dawehner for adding an implementation!

Straight reroll, chasing HEAD.

+++ b/core/lib/Drupal/Core/StackMiddleware/ReverseProxyMiddleware.php
@@ -0,0 +1,61 @@
+  public function handle(Request $request, $type = self::MASTER_REQUEST, $catch = TRUE) {
+    // Initialize proxy settings.
+    if ($this->settings->get('reverse_proxy', FALSE)) {
+      $reverse_proxy_header = $this->settings->get('reverse_proxy_header', 'X_FORWARDED_FOR');
+      $request::setTrustedHeaderName($request::HEADER_CLIENT_IP, $reverse_proxy_header);
+      $proxies = $this->settings->get('reverse_proxy_addresses', array());
+      if (count($proxies) > 0) {
+        $request::setTrustedProxies($proxies);
+      }
+    }
+    return $this->httpKernel->handle($request, $type, $catch);
+  }

This looks great, and is a nice example of needing to do something with $request prior to kernel events. But how do we deal with handlePageCache(), which also calls initializeCookieGlobals() and therefore $request->isSecure()?

And also, this patch only wraps the HttpKernel, but don't we really need to wrap DrupalKernel, since it's in or before DrupalKernel's preHandle() that we need to set the trusted proxies?

Thanks for making a stronger case for stackphp. I'm leaning towards committing it but I'm wondering what the follow-up work will be. With one beta blocker left, we could be pretty close to beta 1 and I'm concerned with the follow-up work. I really don't want us to work on 'new projects' after beta. What is not clear is how much follow-up work is required to properly integrate it in Drupal? For example, what will be involved to solve #57 and #58? Are we comfortable stopping work on this after beta and shipping without #58? It would be great to dig in on this a bit more so we can have some firmer answers to the above and make the final decision.

I just reread Crell's comment in #39, which seems like saying #58 is a "won't fix", which I'm ok with, if there's consensus on that here. But what does that mean about #57: that we can't solve the trusted proxies issue with a middleware or that we plan on refactoring DrupalKernel such that $request->isSecure() isn't called until later? And if the latter, then per #59: how big an undertaking is that?

I believe a Stack-based new attempt at #1505080: [META] Content Negotiation for Accept Headers is entirely beta-safe. That's the chief follow-up in my WSCCI radar.

@Dries: HTTP Stack is definitely something that will allow Drupal Core extensibility in a region, where contrib will have a hard way currently still.

It allows core and contrib to use the Decorator pattern with our HttpKernel in a clean way.

This won't be possible otherwise and Drupal 8 is (at least in my understanding) in many ways about ensuring that contrib code can extend it in a clean way without resorting to hacks.

e.g. While we might not want to deal with VarnishHttp in core, a contrib module could do it and at the same time disable the CorePage cache, which renders the point of PageCache needing that information already during preHandle moot (as its never executed).

And for accept headers the stack is definitely a good solution as the route negotiation can be tricky otherwise.

#57: Can't we make the PageCache itself be a HttpStack around the main kernel? Or is the bootstrap itself the problem and also the reason why we can't use Symfony's HttpCache, yet? As it runs too late?

Okay here is the deal for a stacked approach:

- remove handlePageCache
- remove preHandle

Those are just procedural stacked approaches right now anyway.

Instead use:

DrupalKernel::handle(request, $type, $catch) {
  $this->boot();
  $this->getHttpKernel()->handle($request, $type, $catch);
}

and stack pageCacheHttpDecorator, preHandleHttpDecorator (I don't like the term middleware) either inside HttpKernel OR allow stacking both on DrupalKernel and HttpKernel.

Option 1) is:

DrupalKernel::handle -> HttpStack1::handle -> HttpStack2::handle > pageCacheHttpDecorator::handle > preHandleHttpDecorator::handle > HttpKernel::handle

Here would need to probably call from pageCacheHttpDecorator and preHandleHttpDecorator back inside the DrupalKernel unless we can move the code.

Option 2) is a custom drupal stack implementation:

DrupalKernel::handle(request, $type, $catch) {
  $this->boot();
  $this->getDrupalStack()->handle($this, $request, $type, $catch);
}

DrupalKernel::handle -> DrupalStack1::handle -> DrupalStack2::handle > pageCacheHttpDecorator::handle > preHandleHttpDecorator::handle > DrupalStackHttp::handle() > HttpStack1:handle > -> HttpKernel::handle

where DrupalStackHttp would just call $kernel->getHttpKernel->handle().

The advantage of option 2, is that the original DrupalKernel could be passed to the Drupal instances as we control the parameters for the DrupalStack, but would probably remove some of the benefits of HttpStack.

Overall a stacked approach has advantages for Drupal Core, too as we can remove special handling of handlePageCache and preHandle and implement many things that are difficult now in an optimized fashion.

e.g. content negotiation needs to come before the page cache, too.

I am in favor of option 1) if we can pull it off, but that someone like Crell needs to answer.

Talked with Dawehner and option 1) is feasible. The only thing we do more is to call the getContainer() (to get the HttpKernel), but because the page cache supports content negotiation already now, we need to get the container anyway, so we don't loose anything. And just getting the compiled container from Disk should be really quick and we need to do it in handlePageCache anyway (drupal_page_get_cache_cid()).

And then already our HttpStack takes over, which also means that someone can add a FastPageCache or Fast404 or whatever in contrib, which is something that was never possible cleanly before in Drupal.

+1 to that approach

EDIT:

I only have one request: Can we call it a http_decorator and not a http_middleware?

It _is_ a decorator by design and we use the term decorator in the rest of core.

I am okay to keep middleware as name, if that is word used upstream ...

EDIT: Wow, there are nice middleware's. I at least want to use a dozen right now :-D. e.g. StackNegotiation looks really useful.

This is truly beautiful!

Such very nice cleanup!

**RTBC** +1000 now!

Just a comment from IRC:

We don't need to change drupal_bootstrap() and DrupalKernel::prepareLegacyRequest, because those never call HttpKernel->handle(), but the old handlePageCache() and preHandle() methods, so we are good for that ones as well.

In #59, Dries asked:

What is not clear is how much follow-up work is required to properly integrate it in Drupal?

And with the patch in #69, the answer is: none! Great job! RTBC+1 from me as well.

I would argue that we want to clean things up, the call back to the Drupal kernel is not the best solution at the end.

Yes, but that is minor in terms of amount of work needed, not disruptive, and not release blocking, so I think that level of minor cleanup is not a concern.

Created some follow up issues for once this is in.

The ability to move logic out of the kernel and into decorators seems good. Also reducing our reliance on event subscribers. Not a fan (to put it very mildly) of the name middleware but meh.

The page cache/proxy changes look pretty minimal, would be interesting to see how the follow-ups turn out, but OK with that happening in the follow-ups.

StackNegotiation looks like where we could properly do some cleanup (with the caveat I didn't review the library at all).

Still assigned to Dries.

Not a fan (to put it very mildly) of the name middleware but meh.

Just a note: The term "middleware" is used pretty universally across the same type of thing in many programming environments, for example: WSGI in Python, Rack in Ruby, PSGI in Perl, etc. I come from a Python and Perl background (before I got into PHP), so when I hear "middleware", I know exactly what is meant. :-) It'd be super awesome to have this in Drupal!

Fixed a tiny typo in the description - "middlewars" :-)

Thanks for the follow-up work. The patch requires no followup work to achieve proper integration and the patch solves the critical trusted proxy issue. It convinced me to commit this patch.

Looks like I'm too late, but this patch seems pretty awkward to me.

Usually, in a middleware environment like stack/rack/etc, the middleware has only the incoming request as context. We've made them full blown services, which is very atypical from what I've seen.

I'm all for breaking up this bits into separate objects, but making them wrap the kernel doesn't seem any cleaner or easier to understand than using events or just calling these methods in the existing handle method.

I'm a bit surprised that we're lacking benchmarks for this patch.

#86:

The patch allows to add a middleware after Kernel boot, but before anything more significant happens - also from contrib, which means the full scale of Middlewares from Stack work with Drupal now. Those are middlewares that have the normal middleware contract and indeed only operate on the incoming request.

Using the container (which is a need as else how could we specify the middlewares?) it will always be possible to inject anything that is available at this time, which means everything that is available in the container. This is for sure not a middleware anymore, but a true HTTP Kernel Decorator: However, I don't think the added flexibility hurts.

It just means the http_middleware can be used in both ways: As a true middleware, which adheres to the contract and as a http kernel decorator that can use everything available in the Container, which is awesome in itself. Performance possibilities are endless that way.

Just adding these as methods in handle, would not achieve the same and not give the same flexibility as a decorator or as a chain of responsibility.

#87:

I don't think we need benchmarks here and I have been one of the most advocating benchmarks person in core, because all that we add are function calls, which are just called once -> micro optimization. The page cache happens at the same stage as before and we already proofed that it would use the container anyway, which means the net effect of a page cache hit is the same.

Is the stack builder really necessary ? Stack builder only means to give the code some sugar candyness at the cost of some runtime instrospection. Looking at the code, you are actually using a compiler pass to write this code using the stack builder. This means that the sugar candided code is actually only compiled code that no one will ever read but that will executed at every request. So just in order to make your compiler pass a bit more simple, you actually penalize the runtime, while a more complex compiler pass would have got rid of both the need of using the stack builder dependency, and the runtime introspection. You have all the cons there (both compile phase and introspection at runtime) while getting rid of stack builder and building a bit smater compiler pass would give you at least the pros of not doing runtime introspection. Or did I miss something reading the patch ?

Getting rid of the stack builder dependency would also reduce the previous patch down to 50% size, remove the need of having integration test with the external library, and would avoid to pollute composer files with an unnecessary package, and make everything way more maintainable.

Sorry I didn't mean to change the status.

I'm not complaining all day long, I didn't want to be rude, I was just surprised by this choice and I went reading the code, that is all. And for the sake of politeness, this was primarly just a question because I didn't see any arguments pro or against using this dependency in the thread, so I just wanted to have your opinion about why using it, and I trust you to have some. There is no reason for you to be aggressive about this.

Hum okay sounds fair, I might give it a try if I succeed in saving some time. Thanks for your answer.

Stack middlewares are skipped if a form-redirect occures: #2230121: Remove exit() from FormBuilder.

As a followup, here is a sample patch without the StackBuilder dependency, only the priority sorting is missing (just as a PoC and to see if all other tests actually pass or not).

Here is one with priorities handled.

I never have been able to correctly use that dawn array_multisort() function I guess !

+1 to this improvement.

Simple ordering mistake, priorities must be ordered ASC because the foreach() will reverse the execution order.

This should pass tests.

Can we please move this new development into a separate issue? I'm not for or against it but having anything other than a trivial follow-up gets very confusing.

It could be moved in another issue, but keeping it ensures that the original people that participated in making this happen are actually still following the issue are more likely to provide a review. Can we have anybody else's opinion about this ?

So you've just re implemented stack in the compiler pass and that dependency in DrupalKernel but with no test coverage etc..?

All I did is replacing:

new Builder()->push("a")->push("b")->push("c")->resolve()->handle()

By

new a(new b( new c()))->handle()

And the test in StackKernelIntegrationTest actually prooves that it works.

You are right, uncoupling it from DrupalKernel would probably be for the best, but as far as I can tell, DrupalKernel is already a very specific component, quite highly coupled to many stuff in that DIC. Still I do agree that untangling it would be better.

One last tiny detail I never saw is that the StackedHttpKernel implements the terminate() method, but my implementation actually doesn't guarantee that all kernels will be terminated (none of them will actually be terminated). As far as I know, I went reading the code of a few external kernels and never saw one implementing it (putting the Symfony HTTP kernel aside).

I did not see the http://stackphp.com/ initiative ever mention the TerminableInterface being part of the kernel stack convention.

The terminate() method is the only reason why I did expose the stack to the DrupalKernel class, because DrupalKernel class will call it by itself.

Seeing it that way, it does make sense. I did want to try, my PoC only determines that we cant get it work with or without the dependency easily.

pounard, new issue please. Don't scope creep this issue.

Agreed. This issue was fixed almost 3 weeks ago, and several follow-ups have been filed. Please do the same.

It's basically the same issue, I only was suggesting a different way of doing it. This was a very good way to learn a bit more D8 internals for me but I don't really care to see this pass in core, so I won't open a followup nor maintain my patch. If anyone ever think this is a good path to explore, please proceed.

Anyway dawehner thanks for your time.

Please note that I did make the effort of trying the patch, which I don't everytime :) The patch I did actually reproduced the exact same integration tests in case you missed. The issue did not only introduced the concept but also the implementation, my patch is only refining it a bit, so I don't think those are really different.

We need to revisit middleware construction, it is broken in several ways: #2343677: Stack middleware suffers from incomplete service construction.

#2347111: StackKernelIntegrationTest is not testing a successful request