Strip dot-prefixed build parameters from cached container

#3571858: Optimize OptimizedPhpArrayDumper further also attempts to reduce the container size, both of these together would be even better.

Maybe we should add a performance test for container size, so we get notified by a test failure if the size increases by a certain amount in any MR.

Yes, I think we explicitly used dot-prefixed assuming that they would get removed. FWIW, these structures are maybe 10-20% of the size that they had in 11.2 but it's of course still useful to remove every bit we don't need.

Yeah I guess we should have verified that the symfony docs aligned with drupal's implementation. We explicitly used .hook so it would be removed.

I guess the main question is do we want to add a compiler pass or manually delete the two we have.

The obvious benefit of the new pass is we can use . Parameters more freely and not worry about cleanup.

Alternatively we could just require manual cleanup when we use them and have a canary test that looks for .prefix parameters in the compiled container.

Adding the Symfony pass seems fine to me, it's only one line and to do this ourselves would basically mean copying the Symfony code. I'm also looking at optimising the pass configuration over in #3581926: Filter out unused compiler passes.

We could do with some kind of test here to prevent a regression.

Who makes that call?

If there is a disagreement it would be the framework managers.

But this is build time and would prevent needing to handle this is each time we use more dot prefixed parameters. I agree now that the Compiler pass is better.

However, I think we should use PassConfig::TYPE_REMOVE instead of a hard coded priority though.

And we do need the regression test.

Ah yes, I was talking about the , -2048 that you removed.

I think this is ready, there seems to be a random failure if that is rerun and green I think we can mark this RTBC.

I did pull this locally and ran the test only job and it failed as expected:

There was 1 failure:

1) Drupal\KernelTests\Core\DrupalKernel\DrupalKernelTest::testBuildParametersRemoved
Dot-prefixed build parameters should not be in the compiled container, but found: .hook_data, .theme_hook_data
Failed asserting that two arrays are identical.
--- Expected
+++ Actual
@@ @@
-Array &0 []
+Array &0 [
+    27 => '.hook_data',
+    28 => '.theme_hook_data',
+]

I was curious why RemoveBuildParametersPass wasn't running already, because it's part of the default PassConfig:

      $this->afterRemovingPasses = [
            0 => [
                new ResolveHotPathPass(),
                new ResolveNoPreloadPass(),
                new AliasDeprecatedPublicServicesPass(),
            ],
            // Let build parameters be available as late as possible
            // Don't remove array parameters since ResolveParameterPlaceHoldersPass doesn't resolve them
            -2048 => [new RemoveBuildParametersPass(true)],
        ];

Then I noticed that true is passed into the constructor, which per the comment means that parameters with array values aren't removed. Not sure why ResolveParameterPlaceHoldersPass is set not to resolve array parameters, though or why then dot prefixed array values shouldn't be removed.

The first false passed into ResolveParameterPlaceHoldersPass disables array resolving.

 $this->optimizationPasses = [[
            new AutoAliasServicePass(),
            new ValidateEnvPlaceholdersPass(),
            new ResolveDecoratorStackPass(),
            new ResolveAutowireInlineAttributesPass(),
            new ResolveChildDefinitionsPass(),
            new RegisterServiceSubscribersPass(),
            new ResolveParameterPlaceHoldersPass(false, false),

This doesn't seem to be in the repo, moving back to RTBC for now.

Committed and pushed 007adf94b0f to main and 7ff3e0390fe to 11.x. Thanks!