Stack middleware suffers from incomplete service construction

Hmm, yes. I guess we have a couple of options, this goes for injected parameters and setters (as your demo patch shows):

- Remove the actual definitions from the service container after our compiler pass (patch attached for that). These should not be used outside of the stack anyway?
- Allow the httpkernel service to be passed in so we can create an instance but the stack would replace this anyway
- Implement our own builder (would be in a similar problem space though, and compiler pass could do similar)
- Don't use the StackBuilder at all; rely on passing the correct decorated kernel into the next. This could easily break and lead to strange things. A developer could easily add the wrong parameter to one of them and it would break the whole stack. Seems too fragile maybe?
- Just document this somewhere and leave it as that (meh).

Thoughts?

Yes, I guess this is because it adds the parameter from the last kernel id to the actual definition.

Here is pounards patch from mentioned issue in #6.

Indeed, this fixes the kernel definition problem. If you want to keep the StackBuilder dependency, it's still possible to fix the definitions during the compiler pass the exact same way, it only takes one or two lines of code to add.

As mentioned in the other issue, the only problem by getting rid of the StackBuilder is that we cannot chain terminate() calls, but the fact that the stackphp initiative does only document the KernelInterface and not the TerminableInterface mitigates that.

Yes, indeed. We could keep things how they are, let the stack do what it is doing, but fix the definitions in the compiler pass so it solves the problems this issue outlines? Something like this.

ok, well I don't think this is possible with the Stack/Builder class.

So if Stack/Builder doesn't provider what we need, let's just try to use the solution from pounard.
On the longrun moving that generic container feature could be moved upstream somehow.

I'm fine with dropping the stack library itself, since even with it we need to do Drupal-specific bridge code anyway. The important part is that stackable kernels work, not that they're added using a specific library to do it.

As for avoiding loading middlewares, they're a good example of a service that should be marked private. That would make $container->get() not work for them, which you should never be doing anyway.

As for avoiding loading middlewares, they're a good example of a service that should be marked private. That would make $container->get() not work for them, which you should never be doing anyway.

Yeah probably, this would be certainly good.

I think we need to decide here first if we want to support getting these services outside the context of the stack.

At first sight, I'd say we don't, except for Drupal specific services that may need configuration in specific use cases.

I cannot think of any reason to modify or reuse a middleware outside of, you know, being a middleware. The same applies for a lot of things; event subscribers, for instance, should all be private because there is no good reason to call $container->get() on them, ever. MOST of what we have in the container should be private, frankly.

Meh, yes. It has no bearing on that problem. Stack is not container aware in any way, so shall we favour pounards approach? Maybe with the addition of private services?

Here is a reroll of that for now.

I think that Crell is right, making those services private makes sense, they are not supposed to be used anywhere outside of the stack itself.

Ok, making them private services. So what we lose is the terminate calls for stacked items that the StackedHttpKernel provides. I guess people could use DestructableInterface if there were some uses? Not sure.

Wouldn't those services be set private directly in the yaml file instead ? I sounds odd to leave this responsability to the compiler pass ; While I agree that those services should be set private by the provider, I don't see the point of forcing them to be. For example, if a module set such service public for some various reason (I don't see any good reason right now, but some may rise anytime in the future in specific edge use cases) - why forcing at compilation time while it might be set directly in the definition file instead ?

ok, I was going with the enforcement route. We can add this just to the definitions though, I don't care either way.

I agree, let's just mark them private in the yaml file. No sense being fancy if we don't have a clear need to be.

Damian, I thought we said earlier that we were going to keep the Stack library for the Stacked kernel.

Damian, I thought we said earlier that we were going to keep the Stack library for the Stacked kernel.

The problem is that the container does not allow an array of references, sadly.

Marking the services in the YAML definition instead.

There is definitely a use case to get data out of the middleware service.

e.g.

A Page Cache Middleware that needs to store some data, but don't want to store it on the request object.

So you can then check e.g

$pageCache = container->get('page_cache_middleware');

if ($pageCache->hasX('whatever')) {
  // do something ...
}

The other option is to put the object or all the data on the request itself, but I don't think that is as clean.

If I understand the last comment correctly, according to znerol it should be possible to change the structure, so we can have the stack and still get back the individual services.

Yes, something like that is what I wanted to see originally if we keep the stack builder library (which is a good idea). If this resolves the dependencies properly this looks good.

Yes, this approach looks good is what I meant, I will make some changes now.

So here is some test coverage too.

Also added the check for the stack you mentioned znerol.

Reviewed already kinda during the process of damian writing it. +1

Not sure whether we want to have a dedicated test for the calls in order to ensure that the "regression" works in the future.

I am not sure about testing calls, as now we are always dealing with service definitions. So this would just be like us writing a test that the Symfony tests work, or not? :p

Yes, this ensures that the symfony implementation still works, though we have to have to ensure on a more high level way that things are still working.
I don't want to fight things here.

I am okay with making the services, public: false - if it cannot be done any other way. There are other ways Stack state can be stored somewhere, so RTBC for me too, just needs to pass tests again.

Ok, so the tests are good again. This was an issue with the test bot yesterday.

I have one concern:

- This means that whenever we add a middleware service we force the user to mark the service public: false, because else it breaks e.g. "console" which does an iteration over all public services.

Is it technically possible / feasible to either:

- a) do the marking private as part of the compiler pass and document somewhere that decorated services are hidden automatically?
- b) Output an error message if we try to decorate a public service?

Thanks!

As discussed in IRC:

- The public: false is a larger API change (in a way) and needs to be discussed separately.

Lets remove it.

Even with public: false removed, this successfully unbreaks "container:debug" in console! :)

Great work!

This will be RTBC once those three public: false lines are removed.

The rest makes perfect sense to me and I like this method of service definition much more that we use now.

Uploading a patch to get some commit credit :D (for reviewing).

Only implemented my reviewer feedback, so RTBC now.

As this could cause confusiion:

I did not RTBC my own patch here, I rather did not have a novice or anyone else remove those three lines we agreed in IRC to remove, but did it quickly myself as I had that diff applied to test it locally anyway.

Uploading a patch to get some commit credit :D (for reviewing).

Really, we should fix our process ... and our idea that credits count actually, because our model is best upon trust
which you should get when you give good reviews and all other kind of contributions.

+++ b/core/tests/Drupal/Tests/Core/DependencyInjection/Compiler/StackedKernelPassTest.php
@@ -0,0 +1,116 @@
+/**
+ * @coversDefaultClass \Drupal\Core\DependencyInjection\Compiler\StackedKernelPass
+ */
+class StackedKernelPassTest extends UnitTestCase {

Wait, don't we need a @group in order to appear it in the UI?

@znerol, I also created #2350491: Discuss/implement our usage of private services but did this on a train journey, and forgot to reference here.

Committed c033857 and pushed to 8.0.x. Thanks!

diff --git a/core/tests/Drupal/Tests/Core/DependencyInjection/Compiler/StackedKernelPassTest.php b/core/tests/Drupal/Tests/Core/DependencyInjection/Compiler/StackedKernelPassTest.php
index 654d9a8..5bd6001 100644
--- a/core/tests/Drupal/Tests/Core/DependencyInjection/Compiler/StackedKernelPassTest.php
+++ b/core/tests/Drupal/Tests/Core/DependencyInjection/Compiler/StackedKernelPassTest.php
@@ -14,6 +14,7 @@
 
 /**
  * @coversDefaultClass \Drupal\Core\DependencyInjection\Compiler\StackedKernelPass
+ * @group DependencyInjection
  */
 class StackedKernelPassTest extends UnitTestCase {
 

Fixed on commit.