Replace Symfony container with a Drupal one, stored in cache

Overall I think our container building is very problematic also for multiple web heads and coordination.

My proposal for that is to use three steps:

- a) Compile the container using the symfony container builder
- b) Dump the container to a list of services and parameters - read an array, store that in a cache (DB / memcache / redis)
- c) On each web head compile the container definition to e.g. /tmp/container/container_{hash-of-configuration}.php for speed reasons

Then the only thing we need to take care of is:

If /tmp/container/container_{hash-of-configuration}.php does not exist, but someone builds it already, should we wait on the lock or run somehow just with the configuration.

This also allows us to change the configuration e.g. for dynamic language parts in the Kernel, would then just compile to different containers.

more of a scalability issue than a performance issue.

Report from the front:

- Dumping into XML and back to YAML is already supported.

Only "public: false" services fail with YAML dumper, because those are put as definitions, e.g.

    <service id="page_cache_response_policy" class="Drupal\Core\PageCache\ChainResponsePolicy" lazy="true">
      <tag name="service_collector" tag="page_cache_response_policy" call="addPolicy"/>
      <property name="_serviceId">page_cache_response_policy</property>
      <call method="addPolicy">
        <argument type="service">
          <service class="Drupal\node\PageCache\DenyNodePreview" public="false">
            <tag name="page_cache_response_policy"/>
            <argument type="service" id="current_route_match"/>
          </service>
        </argument>
      </call>

Note that the service is inlined here and not asked for by ID.

If I resolve this back to the original ID OR create a random id OR just support nesting in YAML (which we could do in PHP) then we can nicely use all dumpers.

Note also that private services are never auto-serializable atm. as they miss our _serviceId property ... (should revisit in some issue)

Writing a PHP Definition dumper is probably not much effort either ...

The only thing missing is then:

- checksum
- store in DB / D7-like cache layer
- add checksum to class
- Create / Load service_container based on checksum of definitions

and see how we can _efficiently_ get from e.g. XML or PHP or whatever to a PhpDumper dumped container again.

I will also give service_container (my own symfony interface compatible container taking PHP arrays) a whirl and do some performance comparisons.

I have a patch running my own service_container successfully. Only around 5% function calls more and from -2 ms to +2 ms for an admin page (not yet profiled in depth).

Dumping a container builder to PhpArray itself takes around 7-8 ms.

And with omitting the ResolveParameters Kernel pass we can even change the config (with the caveat that container passes don't see such changes obviously) before loading the container.

Container is 140k in definition, so easily storable in the database or memcache or redis or ... as a serialized array.

Will upload a patch around Monday.

Looking forward to reading that patch :)

It is looking pretty good:

Container using "compiled" PHP array of services and parameters, % parameters are resolved at run-time, container loaded from serialized.data file.

~ 2.3 ms to give us back sanity, yes!

And we can still compile to PHP, too.

I am not even sure we need to compile to PHP anymore (we still compile), with some performance hacks (not calling any functions, but preparing output partially in a typed, but fast way), I get:

554 of those calls are to substr and could easily be avoided.

Means we have probably around 500 services loaded.

Granted, the variation is still 1 ms (see below), BUT:

Being completely independent of anything we could compile this PHP Array based container to a native PHP extension - similar to how Pimple can and _then_ we are likely faster than native compiled PHP code.

( https://github.com/LionsAd/service_container/blob/7.x-1.x/src/Dependency... is the basis - needs some small bug fixes to work with D8 core, but around 300 loc, maybe in the end 400 loc ... That is doable to convert to a PHP extension.)

Variation:

=== SUM: 8_0_x-without-proxy-summary..hack-the-container-summary compared (556addd042899..556addd7c32f3):

[ DIVIDE NUMBERS BY 100 for real number ]

ct  : 6,732,282|6,800,233|67,951|1.0%
wt  : 12,547,608|12,676,685|129,077|1.0%
mu  : 2,418,491,112|2,429,629,752|11,138,640|0.5%
pmu : 2,427,074,488|2,438,754,216|11,679,728|0.5%

---
ct = function calls, wt = wall time, cpu = cpu time used, mu = memory usage, pmu = peak memory usage

### XHPROF-LIB REPORT

+---------------------+------------+------------+------------+------------+------------+
| namespace           |        min |        max |       mean |     median |       95th |
+---------------------+------------+------------+------------+------------+------------+
| Calls               |            |            |            |            |            |
|                     |            |            |            |            |            |
| hack-the-container  |     67,962 |     71,995 |     68,002 |     67,962 |     67,962 |
| 8_0_x-without-proxy |     67,282 |     71,364 |     67,323 |     67,282 |     67,282 |
|                     |            |            |            |            |            |
| Wall time           |            |            |            |            |            |
|                     |            |            |            |            |            |
| hack-the-container  |    117,969 |    187,877 |    126,767 |    126,207 |    132,188 |
| 8_0_x-without-proxy |    116,410 |    183,320 |    125,476 |    124,976 |    129,246 |
|                     |            |            |            |            |            |
| Memory usage        |            |            |            |            |            |
|                     |            |            |            |            |            |
| hack-the-container  | 24,270,496 | 25,001,472 | 24,296,298 | 24,270,496 | 24,304,163 |
| 8_0_x-without-proxy | 24,163,688 | 24,895,392 | 24,184,911 | 24,163,688 | 24,164,332 |
|                     |            |            |            |            |            |
| Peak memory usage   |            |            |            |            |            |
|                     |            |            |            |            |            |
| hack-the-container  | 24,361,448 | 25,093,560 | 24,387,542 | 24,361,688 | 24,402,259 |
| 8_0_x-without-proxy | 24,248,584 | 24,975,072 | 24,270,745 | 24,248,584 | 24,249,647 |
|                     |            |            |            |            |            |
+---------------------+------------+------------+------------+------------+------------+

----

Completely unrelated thought, we should allow _one_ drupal_static to hold the container, which we clear in DrupalKernel when rebuilding, so e.g. t() and other legacy-call-the-container functions can use the container without going via \Drupal::service() and use good old static fast pattern ...

I did not want to unassign ...

It turns out state() is not "stateless", but depends on system module being installed via hook_schema().

Therefore this first version still caches to a PHP array, but it works with state(), too - just not in tests ...

Performance (as seen) is really good.

Also:

"I heard you like your containers, so I put a container for your container so you can load your container."

--

In this patch:

- Adds a PhpArrayDumper
- Adds a PhpArray/Container.php (based on service_container)
- Removes the variable replacement pass via a hack (@todo need to do properly)
- Uses the same PhpArray container to add a quick bootstrap container (6 loc!), that can be changed via settings.php ('bootstrap_container_definition')
- Has @todo's for all stages where locking and caching needs to be implemented.

I think the default variable_initialize D7 is a good role model for a first start.

I think in the ideal case it is stored in the chained_fast backend and the counter result persisted from bootstrap_container to real container.

No idea if that is sane or not, but for re-using the same cache_bootstrap counter it would be fast and still use DB as backend.

--

First patch mostly for testbot.

Testbot fluke?

Removed the initial setting of settings as testbot did not like that.

Override via settings.php is now all or nothing, which is fine with me, too.

KernelTestBase oddities and some hardening on serialized services.

Real interdiff ...

Alias support was missing (should have been resolved by compiler passes, but tests can do runtime things ...)

Hardening is hard, forgot that ::set takes a NULL argument as well.

Just a quick note.

+++ b/core/lib/Drupal/Core/DependencyInjection/Dumper/PhpArrayDumper.php
@@ -0,0 +1,358 @@
+
+    if ($definition->getFactoryClass()) {
+      $service['factory_class'] = $definition->getFactoryClass();
+    }
...
+
+    if ($definition->getFactoryMethod()) {
+      $service['factory_method'] = $definition->getFactoryMethod();
+    }
+
+    if ($definition->getFactoryService()) {
+      $service['factory_service'] = $definition->getFactoryService();
+    }

+++ b/core/lib/Drupal/Core/DependencyInjection/PhpArray/Container.php
@@ -0,0 +1,390 @@
+      'factory_class' => '',
+      'factory_method' => '',
+      'factory_service' => '',
...
+      if (!empty($definition['factory_method'])) {
+        $method = $definition['factory_method'];
+
+        if (!empty($definition['factory_class'])) {
+          $factory = $definition['factory_class'];
+        }
+        elseif (!empty($definition['factory_service'])) {
+          $factory = $this->get($definition['factory_service'], $invalidBehavior);
+        }
+        else {
+          throw new RuntimeException(sprintf('Cannot create service "%s" from factory method without a factory service or factory class.', $name));
+        }
+        $service = call_user_func_array(array($factory, $method), $arguments);
+      }

Do you mind just having either service:method or class::method ? See symfony 2.7 update patch

That was funny:

KernelTestBase leaked the container builder into the test class.

Hence suddenly there were two different containers ...

This fixes it ...

Also fixed one dependency on the ContainerBuilder. getDefinition() exists on the container, because it originally had the pluginManager Interface.

Hence suddenly there were two different containers ...

Well, it used to work indeed because the same container builder object got used and assigned in \Drupal\simpletest\KernelTestBase::containerBuild()

Also fixed one dependency on the ContainerBuilder. getDefinition() exists on the container, because it originally had the pluginManager Interface.

So do we really need to allow to fetch this information on runtime? I would vote to better not allow to do that, we should hide the implementation detail of our container, as much as possible.

1. +++ b/core/lib/Drupal/Core/DependencyInjection/PhpArray/Container.php
   @@ -0,0 +1,390 @@
   +      if ($argument instanceof \stdClass) {
   +        // We need a unique service name.
   +        $name = 'private__' . $this->privateServicesCounter;
   +        $this->privateServicesCounter++;
   +        $this->serviceDefinitions[$name] = $argument->value;
   +        $arguments[$key] = $this->get($name, $invalidBehavior);
   +        unset($this->serviceDefinitions[$name]);
   +        unset($this->services[$name]);
   +        continue;
   +      }
   

   What kind of special feature of the container is that?

2. +++ b/core/lib/Drupal/Core/DrupalKernel.php
   @@ -56,6 +60,45 @@ class DrupalKernel implements DrupalKernelInterface, TerminableInterface {
   +  protected static $bootstrapContainerDefinition = [
   

   should we name this defaultBootstrapContainerDefinition? What is the reason that this is static?

3. +++ b/core/lib/Drupal/Core/DrupalKernel.php
   @@ -735,25 +780,30 @@ protected function initializeContainer($rebuild = FALSE) {
   +      // @todo State service does fail tests atm. as table is not created automatically.
   +      // @todo Use cache service?
   

   At least a @todo reference to #2371709: [PP-x] Move the on-demand-table creation into the database API would be cool. I would really like to get that one in one day.

In this patch:

- Add factory support. (2.7 ready, yeah!)
- Fix Drupal Kernel Test for new assumptions
- Fix keyvalue.memory service that should have been synthetic.
- Expose container builder for simple definition checks during tests.
- Fix services with uppercase. (argh! - Symfony during compile moves all parameters and services to have lowercase. Can we standardize in Drupal on lowercase and throw an Exception in case an uppercase is used somewhere? 1000s of unnecessary strtolower() calls are not nice to have ...)

Should be green! :)

#32:

Thanks for the review!

1. Private services, to make those efficient and distinguish from other services, a stdClass with [ 'type' => 'service' and 'value' => $definition ] is used.

I used to have a PrivateServiceDefinition object, but performance was way way slower, so using pseudo-typed objects instead ...

2. Yes we should rename, it originally was set on Settings before those were initialized, but the DrupalTestRunnerKernel did not like that as it thought there was a settings.php already ...

3. Will speak with catch where to store and what services to use for that.

Theoretically we can persist services from bootstrap container to real container ...

Re-roll ... Symfony 2.7 got in ...

Proper factory support, removed deprecated methods from dumper to support Symfony 2.7 better.

Can we standardize in Drupal on lowercase and throw an Exception in case an uppercase is used somewhere? 1000s of unnecessary strtolower() calls are not nice to have ...)

This sounds super sensible!

Pinged @msonnabaum & @beejeebus for reviews.

#40: Little early for that - we still need to decide where to store the PHP dumped array so that we can efficiently retrieve it.

Fix services with uppercase. (argh! - Symfony during compile moves all parameters and services to have lowercase. Can we standardize in Drupal on lowercase and throw an Exception in case an uppercase is used somewhere? 1000s of unnecessary strtolower() calls are not nice to have ...)

Yes! (I really, really dislike this about the Symfony container, and https://github.com/symfony/symfony/pull/14558 was closed recently, so while I didn't put any effort into resolving the complaints on that PR there doesn't seem to be much appetite for fixing this upstream).

Haven't reviewed patch yet.

#42: To be fair, they did merge https://github.com/symfony/symfony/pull/14643/files, which resolves the issue.

I still think it makes no sense however ...

Opened #2498293: Only allow lowercase service and parameter names for the mean time, this can go in anyway as its a legit performance improvement, saves one unnecessary for loop and one levenshtein.

--

Spoke with catch.

To avoid taking a performance hit we will use a 'atomic_counter' service for chained fast backend.

That 'atomic_counter' service and the 'database' we can then persist to the real container, that should be the best compromise.

Removed all strtolower calls again (Performance!) and incorporated #2498293: Only allow lowercase service and parameter names briefly for now.

Quick fix for private services as discussed with dawehner.

Not sure how PhpDumper knows the ID of the private service as its not part of the definition. (Oh it does not distinguish, it only optimizes away for private services only used once.)

Anyway, this is not a problem, the hash as unique ID is better approach and next patch will include:

diff --git a/core/lib/Drupal/Core/DependencyInjection/PhpArray/Container.php b/core/lib/Drupal/Core/DependencyInjection/PhpArray/Container.php
index 5bcc6f6..2ca8a91 100644
--- a/core/lib/Drupal/Core/DependencyInjection/PhpArray/Container.php
+++ b/core/lib/Drupal/Core/DependencyInjection/PhpArray/Container.php
@@ -285,10 +285,10 @@ protected function expandArguments($arguments, $invalidBehavior = ContainerInter
     foreach ($arguments as $key => $argument) {
       if ($argument instanceof \stdClass) {
         $name = $argument->id;
-        if (!isset($this->serviceDefinitions[$name])) {
-          $this->serviceDefinitions[$name] = $argument->value;
-        }
+        $this->serviceDefinitions[$name] = $argument->value;
         $arguments[$key] = $this->get($name, $invalidBehavior);
+        unset($this->serviceDefinitions[$name]);
+        unset($this->services[$name]);
         continue;
       }

Uploaded the wrong patch, interdiff see #46 ...

Out of time for now, unassigning.

Actionable items:

- #32.2 - rename and make non-static
- Register a cache factory and bin in the bootstrap container (Database for now) and use it (no idea how that works)
- Remove the hack from Symfony - can be follow-up to be able to change parameters after loading config. (should still pass tests with the compiler pass again in).
- Implement locking around rebuilding the container to avoid stampedes.

Suggestion:

cache_miss:

acquire_lock()

yes => compileContainer

no => wait short time (to be determined)

cache_get

cache_miss => compileContainer, but do not dump\

- Reviews

Improving the IS.

It's far, far too late to change direction now but I am curious, why wasn't the Reflection API employed to manage the majority of DI rather than build this monster table and class -- like this one I've used in the past

https://r.je/dice.html

@Aki Tendo
We want to have a DI which maps as directly to the existing configuration as possible. Having a DI based upon reflection IMHO doens't solve all the usecases
we have, like for example having multiple cache backends with the same class at the end.

1. +++ b/core/lib/Drupal/Core/DependencyInjection/Dumper/PhpArrayDumper.php
   @@ -0,0 +1,351 @@
   +    if (!$this->container->getParameterBag()->all()) {
   +      return [];
   +    }
   

   This is not obvious, why would ->all() not return something properly?

2. +++ b/core/lib/Drupal/Core/DependencyInjection/PhpArray/Container.php
   @@ -0,0 +1,404 @@
   + * @file
   + * Contains \Drupal\Core\DependencyInjection\PhpArray\Container
   + */
   

   I'm curious, is anything in that class drupal specific? Could we move that to a component?

#54:

a) Verbatim copied from Dumper/YamlDumper.php (have we a policy how to credit that?) and also taken inspiration from XmlDumper.php.

- So ask upstream? :) It looks fine however for the case that a container has no parameters.

b) No, both classes can totally be Component. (could theoretically even live upstream, but I think its fine to ship our own for the moment to get D8 releasable).

Can you add that to the IS tasks to move over from core to component - if we think its a good idea?

Probably my main reason was that there was already Core/DependencyInjection/... while there was nothing for Component.

However maybe the _serviceId things are Core code - not sure if we need that, but we have quite some in Container right now - that we need to re-visit ...

Why #45, #46 and #48 are not sent to testbots?

Re-uploading, testbot fail I assume.

re-roll

Oh, whw was the last patch 5KB smaller?

Because the changes to set and the addition of register and setParameter in ContainerBuilder were done in #2498293: Only allow lowercase service and parameter names . This comment was generated by your friendly diffdiff service :) interdiff won't show it.

+++ b/core/includes/common.inc
diff --git a/core/lib/Drupal/Core/DependencyInjection/Dumper/PhpArrayDumper.php b/core/lib/Drupal/Core/DependencyInjection/Dumper/PhpArrayDumper.php
new file mode 100644

+++ b/core/lib/Drupal/Core/DependencyInjection/PhpArray/Container.php
@@ -0,0 +1,387 @@
+        // @todo Allow dynamic class definitions via parameters.
+        $class = $definition['class'];
+        $length = count($arguments);
+
+        switch ($length) {
+          case 0:
+            $service = new $class();
+            break;
+          case 1:
+            $service = new $class($arguments[0]);
+            break;
+          case 2:
+            $service = new $class($arguments[0], $arguments[1]);
+            break;
+          case 3:
+            $service = new $class($arguments[0], $arguments[1], $arguments[2]);
+            break;
+          case 4:
+            $service = new $class($arguments[0], $arguments[1], $arguments[2], $arguments[3]);
+            break;
+          case 5:
+            $service = new $class($arguments[0], $arguments[1], $arguments[2], $arguments[3], $arguments[4]);
+            break;
+          case 6:

Can't we just use call_user_func_array($class, $arguments) here?

Can't we just use call_user_func_array($class, $arguments) here?

Well the reason here was better performance

#61: Unfortunately no, and that (rather ugly) code is just there to avoid a performance regression due to ReflectionClass being quite costly.

#2296557-100: [policy] Require PHP 5.5

Argument unpacking with ... came in PHP 5.6

And the comment I linked argues for requiring 5.6 because of variadics. Sorry if that was not clear.

Ah sorry I didn't click through. I should have known! 5.6, wouldn't that be awesome :)

We discussed this on the maintainer call and agreed the issue is definitely critical to resolve, because... endless stampede.

1. +++ b/core/includes/common.inc
   @@ -1309,6 +1309,10 @@ function drupal_flush_all_caches() {
   +  // Remove container definition.
   +  //$keys[] = 'container_definition';
   

   I think its fine to not clear the container definitions, given that its just an implementation detail that they are stored in cache and not somewhere else. Ideally you would never rebuild them.

2. +++ b/core/lib/Drupal/Core/DependencyInjection/Dumper/PhpArrayDumper.php
   @@ -0,0 +1,362 @@
   +/**
   + * PhpArrayDumper dumps a service container as a serialized PHP array.
   + */
   +class PhpArrayDumper extends Dumper
   +{
   

   We should document the design decision to not resolve the parameters on compile time, but rather make it potential dynamically on runtime, before a freeze.

3. +++ b/core/lib/Drupal/Core/DependencyInjection/Dumper/PhpArrayDumper.php
   @@ -0,0 +1,362 @@
   +    if (($scope = $definition->getScope()) !== ContainerInterface::SCOPE_CONTAINER) {
   +      $service['scope'] = $scope;
   +    }
   

   Should we skip the support for scope right in the beginning?

4. +++ b/core/lib/Drupal/Core/DependencyInjection/Dumper/PhpArrayDumper.php
   @@ -0,0 +1,362 @@
   +   * Dumps callable to YAML format
   

   Nitpick: its php :)

5. +++ b/core/lib/Drupal/Core/DependencyInjection/PhpArray/Container.php
   @@ -0,0 +1,387 @@
   +/**
   + * Container is a DI container that provides services to users of the class.
   + *
   + * @ingroup dic
   + */
   +class Container implements IntrospectableContainerInterface {
   

   The important part we should document here is that its using a PHP array in order to know which services exists etc. Related question: Do we need our container to be introspectable? We did not needed that before ...

6. +++ b/core/lib/Drupal/Core/DependencyInjection/PhpArray/Container.php
   @@ -0,0 +1,387 @@
   +    if (isset($this->services[$name]) || ($invalidBehavior === ContainerInterface::NULL_ON_INVALID_REFERENCE && array_key_exists($name, $this->services))) {
   

   Do we need the check here? array_key_exists seems to be enough, given otherwise we would have already thrown the exception on the previous call

7. +++ b/core/lib/Drupal/Core/DependencyInjection/PhpArray/Container.php
   @@ -0,0 +1,387 @@
   +        // @todo Allow dynamic class definitions via parameters.
   

   I'm not sure whether we should really support that ... Even symfony itself realized that its actually quite a bad thing to do, because you have to change the dependencies so often that just changing the class is not that helpful at all. Its also pretty annoying to have that extra level of lookup as a developer

8. +++ b/core/lib/Drupal/Core/DependencyInjection/PhpArray/Container.php
   @@ -0,0 +1,387 @@
   +      throw new \BadMethodCallException("Container parameters can't be changed on runtime.");
   

   Let's adapt the message and tell people that the container is frozen instead

9. +++ b/core/lib/Drupal/Core/DrupalKernel.php
   @@ -1038,6 +1088,16 @@ protected function attachSynthetic(ContainerInterface $container) {
   +  protected function compileContainerWithLock() {
   +    // @todo Implement lock here.
   +    return $this->compileContainer();
   +  }
   

   General question ... in case we would use state, we would have a relyable storage across webheads, right? Could we then rebuild the container, like we do now, just when we need it, and not "lazy" like here? In that case we would have much less of a problem of locking.

10. +++ b/core/lib/Drupal/Core/DrupalKernel.php
    @@ -1161,36 +1221,24 @@ protected function getContainerBuilder() {
    +    $code = str_replace("stdClass::__set_state", "(object)", $code);
    

    Do we need that? I thought that its optional ... is that done to make the string smaller?

11. +++ b/core/lib/Drupal/Core/DrupalKernel.php
    @@ -1161,36 +1221,24 @@ protected function getContainerBuilder() {
    +    return $this->storage()->save('container_definition.php', '<?php return ' . $code . ';');
    

    storage() is not really a helpful method name to begin with, but that is not the fault of this patch

12. +++ b/core/vendor/symfony/dependency-injection/Compiler/ResolveParameterPlaceHoldersPass.php
    @@ -30,6 +30,8 @@ class ResolveParameterPlaceHoldersPass implements CompilerPassInterface
    +// @todo Remove compiler pass via getPasses() / setPasses().
    +return;
    

    That part shouldn't be that hard, right?

I chatted with Fabianx, and am going to work on integrating the atomic counter concept from #2301163: Create a phpstorage backend that works with a local disk but coordinates across multiple webheads into the ChainedFastBackend.

After further discussion with Fabianx, we decided to pursue using cache tags as a centralized invalidator for the chained fast backend rather than the atomic counter. I ran out of time today for this, but here is progress in that direction.

Current stumbling blocks:

• This implementation makes assumptions about the cache backends in use (eg, it assumes $item->checksum will be set)
• Given that, the unit test is failing since the memory cache backend doesn't set checksum at all
• There are also some other failing cache tests that need to be resolved in this approach

I also did not have time to address any of #69 in this patch.

Opened #2508417: ChainedFastBackend should have a centralized invalidation service for the work to move toward a centralized invalidation service for the chained fast backend. Patch in #71 can be ignored.

#74 says to ignore #71. Latest patch before that is #58, but that needs a reroll.

Rerolled the patch as per #58.

Removing the tag of Rerolling the patch.

Since we are dumping into PHP why are we serializing?

+ // @todo State service does fail tests atm. as table is not created automatically.
+ // @todo Use cache service?

@chx
The idea is to store it in state, and don't deal with the issues with storing it on disk.
@Fabianx
Why don't you just use $this->installSchema() in KernelTestBase.php, this should pretty much allow this, if I understand it correctly.
As alternative we could simply use a PHP based KV there and be happy.

Using cache is the correct way as that can be easier made pluggable. I have the factory setup locally.

Thanks for the re-rolls @all :).

I have another idea. We are going PHP 5.5 and PHP 5.5 is opcache land and opcache can cache file:// and phar:// wrappers. Let's write a stream wrapper then which loads the specified files from database and then we can opcache them!

For a quick verification this is doable I have overridden phar:// with the existing public stream wrapper https://twitter.com/chx/status/613882731215237120 and I am planning to write a class that will coopt file://

file://foo will be safely passed along while file://///bar or some other crazy invalid path (throw in a few chr(0) for good measure?) will be going against the database.

Edit: phenaproxima suggests file://./foo which indeed looks like quite invalid unlike my idea of slashes.

Edit2: nope, we can't do file:// because there's not really a good and efficient way after that to reach the normal file handling functions but we definitely can do phar://. Here's a wrapper https://gist.github.com/chx/0db97d97661b50381907 that maintains normal phar functionality. The same dance would be necessary for file:// which is a bit excessive as it would effect every file open after.

Edit3: my idea is compatible with Fabian's: write the array as a bit of PHP code into the database and save serialize / unserialize.

1. +++ b/core/includes/common.inc
   @@ -1304,6 +1304,19 @@ function drupal_flush_all_caches() {
   +  // Remove the cache of the active theme's info file information in state.
   +  // @see \Drupal\Core\Theme\ThemeInitialization::getActiveByThemeName()
   +  $keys = [];
   +  foreach ($theme_handler->listInfo() as $theme_name => $theme) {
   +    $keys[] = 'theme.active_theme.' . $theme_name;
   +  }
   

   Given #2499943: system.active_theme.THEMENAME state should not cache theme data, is this still needed?

2. +++ b/core/lib/Drupal/Core/DependencyInjection/Dumper/PhpArrayDumper.php
   @@ -0,0 +1,362 @@
   +/*
   +    $tagsCode = '';
   +    foreach ($definition->getTags() as $name => $tags) {
   +      foreach ($tags as $attributes) {
   +        $att = array();
   +        foreach ($attributes as $key => $value) {
   +          $att[] = sprintf('%s: %s', $this->dumper->dump($key), $this->dumper->dump($value));
   +        }
   +        $att = $att ? ', '.implode(', ', $att) : '';
   +
   +        $tagsCode .= sprintf("      - { name: %s%s }\n", $this->dumper->dump($name), $att);
   +      }
   +    }
   +    if ($tagsCode) {
   +      $code .= "    tags:\n".$tagsCode;
   +    }
   +*/
   

   This code is commented out, so just remove it?

+++ b/core/lib/Drupal/Core/DependencyInjection/Dumper/PhpArrayDumper.php
@@ -0,0 +1,362 @@
+use Symfony\Component\ExpressionLanguage\Expression;

We don't currently include this component, so either this needs to be removed (and the corresponding code the uses Expression), or we need to add that component to core.

error: patch failed: core/lib/Drupal/Core/DrupalKernel.php:1161
error: core/lib/Drupal/Core/DrupalKernel.php: patch does not apply

I'm working on the re-roll.

@jhedstrom,

Do you want me to created a patch with the suggestion from #83 and #84 ? Or are we are waiting feedback ?

Here is the re-roll patch. This patch does not have any of the suggestion from #83 or #84.

I forgot to close a bracket. Sorry..

I think you might be having some trouble with manually applying the changes. here's a rebase/merge.

I tested this some and this patch does seem to keep rebuilds from writing millions of containers. However, if I delete the container directory a couple times I can cause it to never get recreated. watch find sites/default/files/php just shows an empty container directory for 100's of thousands of requests.

Uhm, yes I am working on loading the definition from the DB.

To give an update:

- The Container passes Symfony test suite now - it was a little difficult, because Symfony does test a lot of internals too (e.g. compile(), frozen(), getDefinedServiceIds()) and does rely very much on Fixtures for everything. (e.g. of the compiled container)
- The PhpArrayDumper passes the converted Yaml Test suite, which however is not very extensive - so I have added my own.

The main thing missing was 'scope' and 'configurator' support.

I have an OptimizedPhpArrayDumper, which still uses an array format, but does several optimizations so we can avoid digging e.g. into deep arrays, that don't need any service resolving.

I have an PhpArray/OptimizedContainer, which just overrides two methods to parse the optimized format, but needs test coverage.

--

I will roll however a patch next that fixes the test failure (just need to add to the cache key) and uses the normal cache backend for loading the definition, so some more progress can be directly seen here.

Cleanup, Fixes, Next: Tests

->set does not return a value, so the best we can do is to check for an Exception happening.

Just some early/quick feedback

1. +++ b/core/lib/Drupal/Core/DrupalKernel.php
   @@ -805,7 +828,7 @@ protected function initializeContainer($rebuild = FALSE) {
   +    if ($this->containerNeedsDumping && !$this->cacheDrupalContainer($container_definition)) {
          $this->container->get('logger.factory')->get('DrupalKernel')->notice('Container cannot be written to disk');
        }
   

   This throws potentially an exception right? In case you start with no container, there is also no way to call out to the container and log things. Maaybe try catch it, and provide a message that makes it clear we are dealing with a helpless situation

2. +++ b/core/lib/Drupal/Core/DrupalKernel.php
   @@ -1159,35 +1182,23 @@ protected function getContainerBuilder() {
   +      // -1 == Cache::Permanent, hardcoded for performance reasons.
   +      $this->bootstrapContainer->get('cache.container')->set($this->getCacheKey(), $container_definition, -1, ['container']);
   

   I really don't get that. How are you able to deal with the cache, but then not load CacheBackendInterface, where the constant exists ...

3. +++ b/core/lib/Drupal/Core/Site/Settings.php
   @@ -107,15 +107,17 @@ public static function getAll() {
       *   exposed to the local scope of settings.php, so as to allow it to be
       *   decorated with Symfony's ApcClassLoader, for example.
   +   * @param array $settings
   +   *   Initial settings, set by DrupalKernel.
       *
       * @see default.settings.php
       */
      public static function initialize($app_root, $site_path, &$class_loader) {
        // Export these settings.php variables to the global namespace.
        global $base_url, $config_directories, $config;
   -    $settings = array();
        $config = array();
        $databases = array();
   +    $settings = array();
    
        if (is_readable($app_root . '/' . $site_path . '/settings.php')) {
          require $app_root . '/' . $site_path . '/settings.php';
   

   All the changes here are pretty pointless

4. +++ b/core/lib/Drupal/Core/Site/Settings.php
   @@ -107,15 +107,17 @@ public static function getAll() {
   +   * @param array $settings
   +   *   Initial settings, set by DrupalKernel.
   ...
      public static function initialize($app_root, $site_path, &$class_loader) {
   

   Seems not :)

5. +++ b/core/modules/simpletest/src/KernelTestBase.php
   @@ -298,7 +299,7 @@ public function containerBuild(ContainerBuilder $container) {
   -      ->addArgument(Database::getConnection())
   +      ->addArgument(new Reference('database'))
   

   How did that ever worked?

New patch should be green again - I fixed all occurrences of direct access to PhpStorage and added a proper invalidation API to DrupalKernel, where it IMHO belongs.

Feedback:

#102

1. No longer, exception thrown by the cache backend is caught in cacheDrupalContainer; The logger should not fail at this stage, because the container builder is already successfully build and the container long loaded.

2. The cache backends have their own constant on CacheBackendInterface, but removed as I decided against using tags to reduce complexity for now.

3. / 4. Yes, removed and cleaned up.

5. It did work, because tests had been run with the container builder. They are run with a real container now.

The Installer did not like the database to throw an Exception.

Also split ServiceProviderTest into one KernelTestBase and one WebTest - as a web test cannot get the definition of a service by design, That was only due to the side effect of running tests with the container builder.

Self-review:

Also need to port the unit tests over from service_container, but needs to port them to prophecy (as they use Mockery currently).

1. +++ b/core/lib/Drupal/Core/DependencyInjection/Dumper/PhpArrayDumper.php
   @@ -0,0 +1,344 @@
   +    if ($definition->getFile()) {
   +      $service['file'] = $definition->getFile();
   +    }
   

   @todo Need to support that in PhpArray/Container.

2. +++ b/core/lib/Drupal/Core/DependencyInjection/Dumper/PhpArrayDumper.php
   @@ -0,0 +1,344 @@
   +    if (($scope = $definition->getScope()) !== ContainerInterface::SCOPE_CONTAINER) {
   +      $service['scope'] = $scope;
   +    }
   

   @todo per it being deprecated in Symfony 3.0, we should probably throw an Exception already here and not at run-time.

3. +++ b/core/lib/Drupal/Core/DependencyInjection/Dumper/PhpArrayDumper.php
   @@ -0,0 +1,344 @@
   +    if (($decorated = $definition->getDecoratedService()) !== NULL) {
   +      $service['decorates'] = $decorated;
   +    }
   

   @todo Need to support that in PhpArray/Container. (Symfony test suite seems to have not tested that in the tests that I did run ...)

4. +++ b/core/lib/Drupal/Core/DependencyInjection/Dumper/PhpArrayDumper.php
   @@ -0,0 +1,344 @@
   +    if ($callable = $definition->getConfigurator()) {
   +      $service['configurator'] = $this->dumpCallable($callable);
   +    }
   

   @todo Need to support that in PhpArray/Container.

   The current implementation is buggy.

   Unit tests will show.

5. +++ b/core/lib/Drupal/Core/DependencyInjection/Dumper/PhpArrayDumper.php
   @@ -0,0 +1,344 @@
   +        'alias' => '@' . $id,
   +      ];
   +    } else {
   +      return [
   +        'alias' => '@' . $id,
   +        'public' => FALSE,
   +      ];
   

   That is the wrong syntax as found out in 'service_container' module.

   Need to sync fix from "upstream" base.

6. +++ b/core/lib/Drupal/Core/DependencyInjection/Dumper/PhpArrayDumper.php
   @@ -0,0 +1,344 @@
   +      'type' => 'service',
   

   Should be:

   type => 'private_service'

7. +++ b/core/lib/Drupal/Core/DependencyInjection/Dumper/PhpArrayDumper.php
   @@ -0,0 +1,344 @@
   +    } elseif ($value instanceof Expression) {
   +      return $this->getExpressionCall((string) $value);
   

   We should already throw an Exception here that Expression component does not exist.

   I don't think we will add it to Drupal anytime soon.

8. +++ b/core/lib/Drupal/Core/DependencyInjection/Dumper/PhpArrayDumper.php
   @@ -0,0 +1,344 @@
   +      if (isset($value->_serviceId)) {
   +        return '@' . $value->_serviceId;
   +      }
   

   This is Drupal specific, but that best we can do, when someone sets a service into the container.

   But both the compiler passed and ContainerBuilder::set and Container::set enforce it.

   So should be fine, might need a comment.

9. +++ b/core/lib/Drupal/Core/DependencyInjection/Dumper/PhpArrayDumper.php
   @@ -0,0 +1,344 @@
   +    if (null !== $reference && ContainerInterface::EXCEPTION_ON_INVALID_REFERENCE !== $reference->getInvalidBehavior()) {
   +      return sprintf('@?%s', $id);
   +    }
   

   This is supported by current container, but a little buggy as the default behavior is overwritten, needs to be fixed.

10. +++ b/core/lib/Drupal/Core/DependencyInjection/PhpArray/Container.php
    @@ -0,0 +1,389 @@
    +  /**
    +   * The instantiated services.
    +   *
    +   * @var array
    +   */
    +  protected $services = array();
    

    The best way to ensure private services are shared (yes they are), is to add a $privateServices array here.

11. +++ b/core/lib/Drupal/Core/DependencyInjection/PhpArray/Container.php
    @@ -0,0 +1,389 @@
    +    ); // @codeCoverageIgnore
    

    Just remove and push default definition as a property of the class instead.

12. +++ b/core/lib/Drupal/Core/DependencyInjection/PhpArray/Container.php
    @@ -0,0 +1,389 @@
    +    try {
    +      if (!empty($definition['arguments'])) {
    +        $arguments = $this->expandArguments($definition['arguments'], $invalidBehavior);
    +      } else {
    +        $arguments = array();
    +      }
    ...
    +    catch (\Exception $e) {
    +      unset($this->loading[$name]);
    +      throw $e;
    +    }
    

    This try { catch block is the one to deal with the passed invalidBehavior value, not expandArguments, etc.

13. +++ b/core/lib/Drupal/Core/DependencyInjection/PhpArray/Container.php
    @@ -0,0 +1,389 @@
    +      elseif (!empty($definition['factory_method'])) {
    +        $method = $definition['factory_method'];
    +
    +        if (!empty($definition['factory_class'])) {
    +          $factory = $definition['factory_class'];
    +        }
    +        elseif (!empty($definition['factory_service'])) {
    +          $factory = $this->get($definition['factory_service'], $invalidBehavior);
    +        }
    +        else {
    +          throw new RuntimeException(sprintf('Cannot create service "%s" from factory method without a factory service or factory class.', $name));
    +        }
    

    This is deprecated and can be removed.

14. +++ b/core/lib/Drupal/Core/DependencyInjection/PhpArray/Container.php
    @@ -0,0 +1,389 @@
    +        // @todo Allow dynamic class definitions via parameters.
    

    Remove the @todo, the ContainerBuilder already took care of resolving that and I don't think we want to allow changing that on run-time.

15. +++ b/core/lib/Drupal/Core/DependencyInjection/PhpArray/Container.php
    @@ -0,0 +1,389 @@
    +    foreach ($definition['calls'] as $call) {
    +      $method = $call[0];
    +      $arguments = array();
    +      if (!empty($call[1])) {
    +        $arguments = $this->expandArguments($call[1], $invalidBehavior);
    +      }
    +      call_user_func_array(array($service, $method), $arguments);
    +    }
    

    This is usually part of the other try { catch } block.

16. +++ b/core/lib/Drupal/Core/DependencyInjection/PhpArray/Container.php
    @@ -0,0 +1,389 @@
    +    if (isset($definition['configurator'])) {
    +      $configurator = $definition['configurator'];
    +      if (is_array($configurator)) {
    +        $configurator = $this->expandArguments($configurator);
    +      }
    ...
    +      call_user_func_array(array($service, $method), $arguments);
    

    This is wrong and needs to be done like factory, but documentation on configurator was sparse.

    Also missed decorated - unless that is a ContainerBuilder thing (need to check).

17. +++ b/core/lib/Drupal/Core/DependencyInjection/PhpArray/Container.php
    @@ -0,0 +1,389 @@
    +  public function set($id, $service, $scope = self::SCOPE_CONTAINER) {
    +    if (isset($service)) {
    +      $service->_serviceId = $id;
    +    }
    +    $this->services[$id] = $service;
    +  }
    

    Should throw an Exception for any scope not self::SCOPE_CONTAINER.

18. +++ b/core/lib/Drupal/Core/DependencyInjection/PhpArray/Container.php
    @@ -0,0 +1,389 @@
    +      if ($argument instanceof \stdClass) {
    +        $name = $argument->id;
    +        $this->serviceDefinitions[$name] = $argument->value;
    +        $arguments[$key] = $this->get($name, $invalidBehavior);
    +        unset($this->serviceDefinitions[$name]);
    +        unset($this->services[$name]);
    +        continue;
    +      }
    

    Missing comment:

    // This is for private service instantiations.

    Also private services can safely be 'cached' as they can and usually are shared.

19. +++ b/core/lib/Drupal/Core/DrupalKernel.php
    @@ -55,6 +55,38 @@ class DrupalKernel implements DrupalKernelInterface, TerminableInterface {
    +      'cache_tags_provider.container' => [
    +        'class' => 'Drupal\Core\Cache\DatabaseCacheTagsChecksum',
    +        'arguments' => ['@database'],
    +      ],
    

    It might be better to use a NULL cache tags backend , but on the other hand if no tags are passed or loaded maybe checksum is not computed?

If someone wants to, they could work on it now.

Lots of actionable items.

Likely the best one to start though:

- Port unit tests from https://github.com/LionsAd/service_container/blob/7.x-1.x/tests/src/Depe... over and needs to use Prophecy instead of mockery.

Adding issue summary template with proposed resolution section.

Adding remaining tasks section

Re #54

+++ b/core/lib/Drupal/Core/DependencyInjection/PhpArray/Container.php
@@ -0,0 +1,404 @@
+ * @file
+ * Contains \Drupal\Core\DependencyInjection\PhpArray\Container
+ */

I'm curious, is anything in that class drupal specific? Could we move that to a component?

+++ b/core/lib/Drupal/Core/DependencyInjection/PhpArray/Container.php
@@ -0,0 +1,389 @@
+  public function set($id, $service, $scope = self::SCOPE_CONTAINER) {
+    if (isset($service)) {
+      $service->_serviceId = $id;
+    }
+    $this->services[$id] = $service;
+  }

The _serviceId attribute on service instances is Drupal specific.

#110: Yes, it is, but besides PhpArrayDumper, it is just an implementation detail.

Nothing in the interface says that this can or cannot be done.

So I am not sure that would prevent us to move it to a Drupal Component - as its generically useful.

Nothing in the interface says that this can or cannot be done.

Not wanting to be pedantic here, but the interface is only part of the contract. In this case we are adding application specific behavior and adding that to a generic component violates the Liskov substitution principle. It is subtle though.

Instead of moving this to a component, we also might simply merge in the remaining customizations from \Drupal\Core\DependencyInjection\Container.

Anyway, I'll start porting the tests now.

This contains the minimum amount of changes to make tests pass (except alias, which currently does not work).

+++ b/core/lib/Drupal/Core/DependencyInjection/PhpArray/Container.php
@@ -0,0 +1,389 @@
+          default:
+            $r = new ReflectionClass($class);
+            $service = $r->newInstanceArgs($arguments);
+            break;
...
+    if (isset($definition['configurator'])) {
+      $configurator = $definition['configurator'];
+      if (is_array($configurator)) {
+        $configurator = $this->expandArguments($configurator);
+      }
+
+      $method = $configurator[1];
+      $arguments = array();
+      if (!empty($configurator[0])) {
+        $arguments = [ $configurator[0] ];
+      }
+
+      call_user_func_array(array($service, $method), $arguments);
+    }
...
+      throw new InvalidArgumentException(sprintf('You have requested a non-existent parameter "%s".', $name));

Coverage report indicates missing test coverage in this areas.

Moving the test double into the container test.

It seems to me that the container expects alias being references including the @ prefix (i.e. 'alias' => '@orig.class'). Also replicated upstream Fixed tests and alias handling in PhpArrayDumper.

Taking over again

Filed #2527478: Resolve infinite stampede in mtime protected PHP storage which fixes the issues with NFS outlined in the issue summary.

Thanks znerol,

Note: This issue also solves #2301163: Create a phpstorage backend that works with a local disk but coordinates across multiple webheads implictly and not only the race condition, while it could still allow people for single server web things to put things to a raw php storage cache backend - if wanted.

Also another one is solved implictly here to have a central invaliation service for the service container.

I am gonna work on the reviews now.

Spoke with catch, we can do the invalidation service anyway, which is helpful to reduce fragility regardless of the state of this issue.

#2493665: Add centralized container invalidation method

Quick note to self:

- Prevent overwriting 'service_container' in set

#2493665: Add centralized container invalidation method is fixed.

The 6 Tests fails have been generically covered with:

- #2527710: Decouple Error testing from running with container builder - fixed
- #2528292: Decouple Error testing from relying on a cached on disk-container that is created by a different Kernel - green, waiting for reviews

Re-roll now that the blockers are in.

Little fixes to unit-tests and replacing main Container.php.

No interdiff - sorry (will see if I can provide one later)

Edit:

There is more test-only changes that are useful in general that I can split off from here.

Here is the interdiff from before - not that helpful as it contained newer changes fixed in other issues, but provided for completeness.

Fixed tests, now that the container is used always, rebuildContainer() obviously fails, so we can finally switch it over to invalidateContainer(), which makes more sense anyway.

Cleanup, preparation to split all test changes out.

Yes, I obviously missed something.

Fixed by updating the container after updateModules calls.

Opened #2529516: Decouple tests from relying that $container is a container builder for the test changes.

This is now build on-top of that (see --on-top.txt)

Also reverted some more changes and made the container unfrozen by default, so that new Container() works.

Hi. I'm a crufty old Drupal core developer that has not contributed to Drupal core in any meaningful way in five years. I've never looked at Drupal 8 before this week. However, I'm trying to get up to speed so that I can help push Drupal 8 out the door, and pwolanin suggested I take a look at this issue.

You should assume I have no idea what I'm talking about. :-) If this is actually not a good issue for me to "practice review" on, just let me know, and I'll shut up.

1. +++ b/core/lib/Drupal/Core/DependencyInjection/Container.php
   @@ -1,29 +1,393 @@
   + * Container is a DI container that provides services to users of the class.
   

   Comparing this implementation to Symfony\Component\DependencyInjection\Container, it seems like the major differences are that service names are treated as fixed strings (as opposed to being case insensitive, etc.), presumably for efficiency, and several methods are not implemented (presumably because Drupal does not need/use them). If that's true, it might be helpful to include that motivation in the class documentation. If that's false, then since I'm confused it seems even more important to include the real motivation in the class documentation. :-)

2. +++ b/core/lib/Drupal/Core/DrupalKernel.php
   @@ -770,25 +789,30 @@ protected function initializeContainer() {
   +    $class = Settings::get('container_base_class', '\Drupal\Core\DependencyInjection\Container');
   

   It is not obvious to me why this is not BOOTSTRAP_CONTAINER_BASE_CLASS. Worth an explanatory comment?

> Introduce a bootstrap container providing the cache.container service which by default uses the database cache backend

We decided that was not a great idea in #1849004: One Service Container To Rule Them All

#136: Thanks for the great feedback!

1. Indeed, well spotted that this does not allow lowercase service IDs. Fortunately we already have taken care of that in #2498293: Only allow lowercase service and parameter names for speed reasons, we should document the difference however - yes.
2. Because bootstrap container and real container are two different things. The bootstrap container provides low-level DIC services only to be consumed by DrupalKernel, while the real container is for the rest of the system.

#137: Thanks for the reminder. I remember that discussion. However that would have not blocked e.g. donquixotes low-level DIC from going in.

The difference is that this is a very lightweight container that uses a human-readable format and is easily overridable. It is also completely and 100% separate and isolated from the real container (e.g. no one besides DrupalKernel will ever interface with it), while what we had before was a weird hybrid of services (as far as I remember).

Therefore, even a mid-request cache clear would still fallback to the original cache service of the bootstrap container.

It would also allow easily allow pre-container middlewares, which can be handy for great performance. (e.g. #2501989: [meta] Page Cache Performance)

What concerns do you have?

Started with the cleanup, other issue is in good shape, but likely need a different solution.

Added lots of documentation to the Container class.

Opened #2530586: Read-Only Container is not working properly. as I found that while re-working the DrupalKernelTest to match the Expectations.

And postponed #2529516: Decouple tests from relying that $container is a container builder, because I was able to change the logic to take $this->allowDumping into account which means that KernelTestBase can continue to always use the ContainerBuilder for everything.

So lots of reverts in the interdiff and less changes to tests in this patch.

Re-organized the whole container and made a line-by-line comparison of both Container and ContainerBuilder by Symfony.

This is now an exact replica of ContainerBuilder, just way faster and based on arrays and \stdClass.

createService will move back (for performance reasons) into the main get(), but for now this is for being able to compare safely.

This is now an exact replica of ContainerBuilder, just way faster and based on arrays and \stdClass.

Probably the answer is no, but I feel like we have to ask the question at least: Any chance we can get this into upstream?.

#142: The chance exists obviously, but I would like to not make a critical dependent on an upstream decision. Also it means code style will need to be re-worked again, though apparently CS-Fixer can do that automatically.

What do you think? Should we open an upstream issue?

but I would like to not make a critical dependent on an upstream decision

Of course not! :) Major follow-up.

And it turns out ContainerBuilder has a bug if a service is public: false and shared ...

That means whenever we run with ContainerBuilder a ->get('menu.tree_storage') will always instantiate a new version ...

Fixed in my version, by splitting of private services to its own $privateServices space and not mixing them.

Uploading a work-in-progress on the machine format for test bot, benchmarks look good.

I decided on a lazy unserialize strategy per service definition, that ensures the default unserialize call is very very fast and over 242 services that we load on an empty front-page (242? uh, wow ....) that is still 1-2 ms faster than loading them all at once.

Overall performance now is back on-par with compiled Container, sometimes it was even faster. (within the normal fluctuations)

Next task:

- Split off human-readable format to BootstrapContainer.php
- Cleanup
- Test some more optimizations
- Unit tests
- Done

Okay, thats it:

- Split off: done
- Cleanup: done
- Benchmarks: done

Missing:

- Full unit test coverage (65% currently)
- Reviews

Current Benchmarks

Empty front-page

Fluctuating a little, the most is the 242 unserialize() calls (1.6 ms), but given we load from DB cache and lazy unserialize this is a linear effort, which should be acceptable.

Page Cache

Overall kinda the same (within fluctuations), it can be faster, it can be 100 us (0.1 ms) slower, but that again is okay and given in "[meta] Page Cache" I show lots of possibilities to save overall 2 ms and that we load from the DB here and we could use memcache, APC, whatever here.

unserialize() is called 4 times here and takes 200-300 us in total, so that is fine.

This is a lot to review. Here's a partial review. Many nitpicks, because they make it harder to read the patch. But also some questions. I think several of my remarks/questions are stupid/silly from the POV of somebody who knows the container implementation very well. But I don't. Which also makes me a less than ideal person to review this patch probably, but hopefully my review is useful to ensure that the final code is understandable by somebody who does not know container internals.

1. +++ b/core/lib/Drupal/Core/DependencyInjection/BootstrapContainer.php
   @@ -0,0 +1,245 @@
   + * Provides an interface compatible alternative to the default Symfony
   

   s/interface compatible/interface-compatible/

2. +++ b/core/lib/Drupal/Core/DependencyInjection/BootstrapContainer.php
   @@ -0,0 +1,245 @@
   + * dependency injection container specially optimized for Drupal's needs.
   

   s/specially//

3. +++ b/core/lib/Drupal/Core/DependencyInjection/BootstrapContainer.php
   @@ -0,0 +1,245 @@
   + * with a structure very similar to the YAML file format.
   

   s/to the YAML file format/to the YAML container definition/

4. +++ b/core/lib/Drupal/Core/DependencyInjection/BootstrapContainer.php
   @@ -0,0 +1,245 @@
   +class BootstrapContainer extends Container {
   

   This suggests it's not a generic container, but one for bootstrap services only. Which contradicts with the class docs.

5. +++ b/core/lib/Drupal/Core/DependencyInjection/BootstrapContainer.php
   @@ -0,0 +1,245 @@
   +    if (isset($container_definition['machine_format']) && $container_definition['machine_format'] === TRUE) {
   +      throw new InvalidArgumentException('The machine format is not supported by this class. Use a human-readable format instead.');
   +    }
   

   When can this occur? An @see could be helpful here.

6. +++ b/core/lib/Drupal/Core/DependencyInjection/BootstrapContainer.php
   @@ -0,0 +1,245 @@
   +    // Do not call the parents constructor as it would bail on the machine format.
   

   80 cols.

7. +++ b/core/lib/Drupal/Core/DependencyInjection/BootstrapContainer.php
   @@ -0,0 +1,245 @@
   +    if (isset($definition['synthetic']) && $definition['synthetic'] === TRUE) {
   +      throw new RuntimeException(sprintf('You have requested a synthetic service ("%s"). The DIC does not know how to construct this service.', $id));
   +    }
   

   I think this means synthetic services are not supported? If so, why not have the exception say that instead?

8. +++ b/core/lib/Drupal/Core/DependencyInjection/BootstrapContainer.php
   @@ -0,0 +1,245 @@
   +      // @todo All of this is deprecated in Symfony 2.7 - remove it?
   +      $method = $definition['factory_method'];
   

   D8 HEAD just started using Symfony 2.7. Does this mean we can already remove this?

9. +++ b/core/lib/Drupal/Core/DependencyInjection/BootstrapContainer.php
   @@ -0,0 +1,245 @@
   +      switch ($length) {
   +        case 0:
   ...
   +        case 1:
   ...
   +        case 10:
   ...
   +        default:
   

   So we optimize for services with up to 10 arguments, after that we fall back to reflection.

   Should this be documented? Not sure. If my initial understanding is correct, it's probably clear enough. Just want to make sure.

10. +++ b/core/lib/Drupal/Core/DependencyInjection/BootstrapContainer.php
    @@ -0,0 +1,245 @@
    +    if (isset($definition['configurator'])) {
    ...
    +        throw new InvalidArgumentException(sprintf('The configure callable for class "%s" is not a callable.', get_class($service)));
    

    s/configure/configurator/ ?

11. +++ b/core/lib/Drupal/Core/DependencyInjection/BootstrapContainer.php
    @@ -0,0 +1,245 @@
    +        // Create private service.
    +        if ($type == 'private_service') {
    

    Why does this only need to care about private services, and not public services?

12. +++ b/core/lib/Drupal/Core/DependencyInjection/BootstrapContainer.php
    @@ -0,0 +1,245 @@
    +          // Does the private service already exist.
    

    Bad sentence.

13. +++ b/core/lib/Drupal/Core/DependencyInjection/BootstrapContainer.php
    @@ -0,0 +1,245 @@
    +          // Create the private service.
    

    Same comment as above; confusing.

14. +++ b/core/lib/Drupal/Core/DependencyInjection/Container.php
    @@ -7,23 +7,554 @@
    + * Provides an interface compatible alternative to the default Symfony
    + * dependency injection container specially optimized for Drupal's needs.
    

    Hrm, same as my first comment. I suspect that therefore the docs in my first comment are wrong, and theyre they are correct.

15. +++ b/core/lib/Drupal/Core/DependencyInjection/Container.php
    @@ -7,23 +7,554 @@
    + * This container is based on a PHP array container definition with a structure
    + * very similar to the YAML file format.
    

    Same remarks as my second comment.

16. +++ b/core/lib/Drupal/Core/DependencyInjection/Container.php
    @@ -7,23 +7,554 @@
    + * - It only allows lowercase service and parameter names, though it does not
    + *   enforce it for performance reasons.
    

    Let's add a todo to add an assertion for checking the lowercaseness once #2408013: Adding Assertions to Drupal - Test Tools. lands?

17. +++ b/core/lib/Drupal/Core/DependencyInjection/Container.php
    @@ -7,23 +7,554 @@
    +  protected $parameters = array();
    ...
    +  protected $aliases = array();
    ...
    +  protected $serviceDefinitions = array();
    ...
    +  protected $privateServices = array();
    ...
    +  protected $loading = array();
    

    Let's use the short array syntax notation.

18. +++ b/core/lib/Drupal/Core/DependencyInjection/Container.php
    @@ -7,23 +7,554 @@
    +    if (!empty($container_definition) && (!isset($container_definition['machine_format']) || $container_definition['machine_format'] !== TRUE)) {
    +      throw new InvalidArgumentException('The human readable format is not supported by this class. Use a machine-readable format instead.');
    +    }
    

    Same remark as earlier.

19. +++ b/core/lib/Drupal/Core/DependencyInjection/Container.php
    @@ -7,23 +7,554 @@
    +    // is used, the actual wanted behavior is to re-try getting the service at a later point.
    

    80 cols.

20. +++ b/core/lib/Drupal/Core/DependencyInjection/Container.php
    @@ -7,23 +7,554 @@
    +  protected function createService($definition, $id) {
    

    Missing docs.

21. +++ b/core/lib/Drupal/Core/DependencyInjection/Container.php
    @@ -7,23 +7,554 @@
    +      throw new RuntimeException(sprintf('You have requested a synthetic service ("%s"). The DIC does not know how to construct this service.', $id));
    

    Same remark as above.

22. +++ b/core/lib/Drupal/Core/DependencyInjection/Container.php
    @@ -7,23 +7,554 @@
    +      // @todo All of this is deprecated in Symfony 2.7 - remove it?
    

    Same remark as above.

23. +++ b/core/lib/Drupal/Core/DependencyInjection/Container.php
    @@ -7,23 +7,554 @@
    +   * For performance reasons also a \stdClass based approach is supported.
    

    What does this mean?

24. +++ b/core/lib/Drupal/Core/DependencyInjection/Dumper/DebugPhpArrayDumper.php
    @@ -0,0 +1,76 @@
    +class DebugPhpArrayDumper extends PhpArrayDumper
    +{
    
    +++ b/core/lib/Drupal/Core/DependencyInjection/Dumper/PhpArrayDumper.php
    @@ -0,0 +1,477 @@
    +class PhpArrayDumper extends Dumper
    +{
    

    We put opening curly braces on the same line.

25. +++ b/core/lib/Drupal/Core/DependencyInjection/Dumper/PhpArrayDumper.php
    @@ -0,0 +1,477 @@
    +   * Returns the aliases as a PHP Array.
    

    s/Array/array/

1. +++ b/core/lib/Drupal/Core/DependencyInjection/BootstrapContainer.php
   @@ -0,0 +1,245 @@
   + * @file
   + * Contains \Drupal\Core\DependencyInjection\Container.
   + */
   ...
   + * Provides an interface compatible alternative to the default Symfony
   + * dependency injection container specially optimized for Drupal's needs.
   ...
   +class BootstrapContainer extends Container {
   

   Is that a copy and paste from somewhere? Why is that called BootstrapContainer ....

2. +++ b/core/lib/Drupal/Core/DependencyInjection/BootstrapContainer.php
   @@ -0,0 +1,245 @@
   +    if (isset($container_definition['machine_format']) && $container_definition['machine_format'] === TRUE) {
   ...
   +    if (isset($definition['synthetic']) && $definition['synthetic'] === TRUE) {
   

   You could use just !empty() instead.

3. +++ b/core/lib/Drupal/Core/DependencyInjection/BootstrapContainer.php
   @@ -0,0 +1,245 @@
   +  /**
   +   * {@inheritdoc}
   +   */
   +  protected function createService($definition, $id) {
   

   It would be great to explain here what we override from the parent method ... I mean its a big method so some overview would be nice

4. +++ b/core/lib/Drupal/Core/DependencyInjection/BootstrapContainer.php
   @@ -0,0 +1,245 @@
   +      throw new RuntimeException(sprintf('You have requested a synthetic service ("%s"). The DIC does not know how to construct this service.', $id));
   

   Let's not use short names here.

5. +++ b/core/lib/Drupal/Core/DependencyInjection/BootstrapContainer.php
   @@ -0,0 +1,245 @@
   +    if (isset($definition['file'])) {
   +      $file = $this->frozen ? $definition['file'] : current($this->resolveServicesAndParameters(array($definition['file'])));
   +      require_once $file;
   +    }
   

   Does that mean we could have used 'file' for proxies?

6. +++ b/core/lib/Drupal/Core/DependencyInjection/BootstrapContainer.php
   @@ -0,0 +1,245 @@
   +      // @todo All of this is deprecated in Symfony 2.7 - remove it?
   +      $method = $definition['factory_method'];
   +
   +      if (!empty($definition['factory_class'])) {
   +        $factory = $definition['factory_class'];
   +      }
   +      elseif (!empty($definition['factory_service'])) {
   +        $factory = $this->get($definition['factory_service']);
   +      }
   +      else {
   +        throw new RuntimeException(sprintf('Cannot create service "%s" from factory method without a factory service or factory class.', $id));
   +      }
   

   I would be totally fine to not keep BC code here and JUST use 'factory' and otherwise throw an exception. Drupal does not need to be BC compatible yet

7. +++ b/core/lib/Drupal/Core/DependencyInjection/BootstrapContainer.php
   @@ -0,0 +1,245 @@
   +
   +      switch ($length) {
   +        case 0:
   

   Do you mind putting some documentation why we need this construct (avoid reflection, if possible)

8. +++ b/core/lib/Drupal/Core/DependencyInjection/BootstrapContainer.php
   @@ -0,0 +1,245 @@
   +      // Optimized code path.
   

   Do you mind explaining in which way this is a optimized code path?

9. +++ b/core/lib/Drupal/Core/DependencyInjection/Container.php
   @@ -7,23 +7,554 @@
   +  public function __construct(array $container_definition = array(), $frozen = FALSE) {
   

   I'm curious, why would it make sense to have a container without any container definition?

10. +++ b/core/lib/Drupal/Core/DependencyInjection/Container.php
    @@ -7,23 +7,554 @@
    +
    +  protected function createService($definition, $id) {
    

    Needs docs with maybe some high level overview

11. +++ b/core/lib/Drupal/Core/DependencyInjection/Container.php
    @@ -7,23 +7,554 @@
    +   *
    +   * For performance reasons also a \stdClass based approach is supported.
    +   *
    

    Do you mind explaining why a \stdClass based approach is faster? Is it because we don't copy the memory of the array around? I thought php is copy on write. Such thing s would be nice as part of the documentation

12. +++ b/core/lib/Drupal/Core/DependencyInjection/Container.php
    @@ -7,23 +7,554 @@
    +    throw new \BadMethodCallException(sprintf("'%s' is not implemented", __FUNCTION__));
    ...
    +    throw new \BadMethodCallException(sprintf("'%s' is not implemented", __FUNCTION__));
    ...
    +    throw new \BadMethodCallException(sprintf("'%s' is not implemented", __FUNCTION__));
    ...
    +    throw new \BadMethodCallException(sprintf("'%s' is not implemented", __FUNCTION__));
    ...
    +    throw new \BadMethodCallException(sprintf("'%s' is not implemented", __FUNCTION__));
    

    We could explicit tell that its not supported, you see the small difference to not implemented

13. +++ b/core/lib/Drupal/Core/DependencyInjection/Dumper/DebugPhpArrayDumper.php
    @@ -0,0 +1,76 @@
    +   $code = array();
    +
    ...
    +      return sprintf('@?%s', $id);
    ...
    +    return sprintf('@%s', $id);
    ...
    +    return sprintf('%%%s%%', $name);
    

    Any reason why we can't just use string concat.?

14. +++ b/core/lib/Drupal/Core/DependencyInjection/Dumper/PhpArrayDumper.php
    @@ -0,0 +1,477 @@
    +   * @return array
    

    Note: Many places in this patch could use string[]

15. +++ b/core/lib/Drupal/Core/DependencyInjection/Dumper/PhpArrayDumper.php
    @@ -0,0 +1,477 @@
    +      while (isset($aliases[$id])) {
    +        $id = (string) $aliases[$id];
    +      }
    +      $alias_definitions[$alias] = $id;
    

    Do we need protection against infinite aliases or is that solved already on the container build time?

16. +++ b/core/lib/Drupal/Core/DrupalKernel.php
    @@ -770,25 +789,40 @@ protected function initializeContainer() {
    +      } catch (\Exception $e) {
    +        // Ignore exceptions thrown by the database. In case it is really not
    +        // available, it will fail during container compilation.
    +        $cache = FALSE;
    

    I don't see why we need the protection here. In case you have no database you are screwed anyway and we should fallback to the exception handler ... Are there other exceptions beside the database?

17. +++ b/core/lib/Drupal/Core/DrupalKernel.php
    @@ -1186,35 +1218,22 @@ protected function getContainerBuilder() {
    +    catch (\Exception $e) {
    +      $saved = FALSE;
         }
    

    Another catch all, which is IMHO a bad idea

18. +++ b/core/tests/Drupal/Tests/Core/DependencyInjection/PhpArray/ContainerTest.php
    @@ -0,0 +1,734 @@
    +class ContainerTest extends \PHPUnit_Framework_TestCase {
    

    +1 for not extending from UnitTestCase unless you need it

19. +++ b/core/tests/Drupal/Tests/Core/DependencyInjection/PhpArray/ContainerTest.php
    @@ -0,0 +1,734 @@
    +  public function test_resolveServicesAndParameters_optional() {
    

    Is there any reason you did not went with camelCase?

20. +++ b/core/tests/Drupal/Tests/Core/DependencyInjection/PhpArray/ContainerTest.php
    @@ -0,0 +1,734 @@
    +  protected function getMockContainerDefinition() {
    

    I don't see properties nor configurator in the definitions. I think for proper test coverage we should provide them as well, right?

+++ b/core/lib/Drupal/Core/DrupalKernel.php
@@ -52,6 +50,41 @@
+   * Holds the boostrap container instance.

ghostly boostrap typo. Also it doesn't hold the interface it hold the bootstrap container.

#148:

High level feedback for helping with understanding:

There is 2 different "container definition format"s:

1. Human readable, very similar to YAML syntax - just in PHP -- used here for BootstrapContainer. Drupal\Core\DependencyInjection\Dumper\DebugPhpArrayDumper is able to produce this format. It is a joy to write this format by hand. The DebugPhpArrayDumper is a strange name, maybe HRFPhpArrayDumper (HRF == Human-readable-format?)

2. Machine readable, still similar to YAML syntax, but collections, variable references and private and public services are specially encoded as \stdClass objects for performance reasons. Drupal\Core\DependencyInjection\Dumper\PhpArrayDumper is able to produce this format and Container can read it. It is a pain to write this format yourself.

1. fixed
2. fixed
3. fixed
4. HumanContainer or HumanReadableContainer sounded strange. Any ideas for a better name? And yes, it is generic, just within Drupal used as BootstrapContainer class. Maybe HRFContainer or such?
5. This can occur when someone dumps data with PhpArrayDumper() and tries to put the resulting definition into BootstrapContainer().
6. fixed
7. This is the verbatim message from Symfony upstream. Happy to change that, just saying ... ;)
8. Yes, we could remove support for factory_method, etc.. Should we?
9. That understanding is correct. I added a comment for clarification.
10. Again that is a message from Symfony upstream. Happy to change, though.
11. Private services are processed in a special way by the ContainerBuilder. If a private service is just used once, then the ID is removed and just the Definition of the service is left in the tree. Public services are denoted (in this human readable container format) with '@service' as in Yaml in the code below this one. In the Container class also normal services are handled.
12. Fixed to: "Check if the private service already exists."
13. Fixed to: "Create a private service from a service definition."
14. Typo fixed, rest: ?
15. fixed
16. Genius idea! - fixed
17. No, don't want to as I still need this for D7 / PHP 5.3 and per our coding conventions one can choose IIRC. Not a blocker though if someone insists.
18. See above and high level explanation.
19. Fixed
20. Oops, fixed
21. See above
22. See above
23. This should be removed as only a \stdClass approach is supported now. Fixed by removal.
24. Yes, indeed that slipped through. Fixed.
25. Fixed

--

Patch with the fixes above will come shortly.

#149:

Thanks for the great review, here are the answers:

1. See #148 high level answer

2. This uses specifically only isset() whenever possible. The reason is that it is then easier to port over to a compiled PHP extension. (I had problems when I compiled the container to C code with !empty vs. isset). Could be changed though.

3. Sure, added some comments. Overall the only thing that is changed is that the parent has an optimization to only resolveServicesAndParameters when it encounters a collection, which is checked for via "instanceof \stdClass". This could be "fixed" by using a $this->machineFormat in the parent, but I felt it was complicating the code for no reason.

4. Okay, message is from Symfony upstream. Will fix.

5. Uhm, yes possibly.

6. Will remove, thanks for a decision.

7. Yes, will add a comment. This is purely performance optimization as ReflectionClass is pretty costly on run-time.

8. The comment is no longer needed as its the only code path now. Could say: Machine-readable format.

9. A container can be used as is:

$container = new Container();
$container->setParameter('x','y');
$container->set('foo', $foo);
$container->get('foo');

10. Uhm, yes for sure. I originally had planned to inline that into get(), but after benchmarking this turned out to not be neceessary.

11. The reason I use \stdClass objects is to distinguish between arrays and objects. It is just to be able to use instanceof and optimize some code paths.

e.g. a "collection" / array that is 5 levels deep would in the human-readable-format need to be traversed fully even if there is no '%' or '@' used (no variable or service to resolve).

In the optimized format only collections are resolved and only recursively as long as 'resolve' => TRUE.

12. That's _your_ original wording :p. Yes, lets use not supported.

13. We can, that comes from YAML dumper upstream. Fixed.

14. Will fix, have to find them all first. (@todo)

15. Should be solved by ContainerBuilder pass. Could add if we really wanted to.

16. InstallerTest fails without it. The database is not available at first.

17. This is the only way to see if saving worked. There is no status code coming back from $cache API. Also the caller takes care of the error, so should be fine.

18. :)

19. Yes, that is my own convention. I like to be able to see things clearly as in test_{method}_{context}. But as its not a core convention and coding standards violation I will change it.

20. Properties should be there, Configurator is indeed missing. Test coverage shows that.

--

#150: Ghostly boostrap typo fixed :-D.

Thank you fabian for the quick response!

12. That's _your_ original wording :p. Yes, lets use not supported.

Ha :)

15. Should be solved by ContainerBuilder pass. Could add if we really wanted to.

Are you sure there is not something already? Well I think this is a small detail.

16. InstallerTest fails without it. The database is not available at first.

We should document this then. This is about early installer then, I guess?

17. This is the only way to see if saving worked. There is no status code coming back from $cache API. Also the caller takes care of the error, so should be fine.

Another place where some docs would be great!

20. Properties should be there, Configurator is indeed missing. Test coverage shows that.

Ah, great!

Please don't be shocked about the new patch size. That is just unit tests added.

In this patch:

- interdiff-cleanup.txt: Address Wim's and dawehner's review.
- interdiff.txt - Cleanup and 100% test coverage for Container, PhpArrayContainer (BootstrapContainer), PhpArrayDumper (formerly DebugDumper) and OptimizedPhpArrayDumper.

Test coverage with this patch is now complete.

Discussed naming in IRC:

Container + PhpArrayContainer
OptimizedPhpArrayDumper + PhpArrayDumper

- Renaming for that is next.
- Then: Removal of deprecated factory_method, etc. + associated test coverage
- Then: Needs more reviews.

Clean rebase, conflicted on UncaughtExceptionTest, but resolved automatically.

Fixtures need to have a non-php extension.

#2151103: Zend feed plugins are incorrectly registered broke the deprecation of scope we use here.

So I added intermediate support for scope: prototype to the dumper and forward compatibility with Symfony 2.8 for the 'shared' flag.

See: https://github.com/symfony/symfony/blob/2.8/UPGRADE-2.8.md#dependencyinj...

Also added tests for 'scope: prototype' to the Dumper test and tests for non-shared services to the ContainerTest.

1. +++ b/core/lib/Drupal/Core/DependencyInjection/BootstrapContainer.php
   @@ -0,0 +1,269 @@
   +use ReflectionClass;
   ...
   +          $r = new ReflectionClass($class);
   
   +++ b/core/lib/Drupal/Core/DependencyInjection/Container.php
   @@ -7,23 +7,570 @@
   +use ReflectionClass;
   

   nitpick: We don't use use statements for global level classes but rather \ReflectionClass itself

2. +++ b/core/lib/Drupal/Core/DependencyInjection/BootstrapContainer.php
   @@ -0,0 +1,269 @@
   +    elseif (isset($definition['factory_method'])) {
   +      // @todo All of this is deprecated in Symfony 2.7 - remove it?
   +      $method = $definition['factory_method'];
   +
   +      if (!empty($definition['factory_class'])) {
   +        $factory = $definition['factory_class'];
   +      }
   +      elseif (!empty($definition['factory_service'])) {
   +        $factory = $this->get($definition['factory_service']);
   +      }
   +      else {
   +        throw new RuntimeException(sprintf('Cannot create service "%s" from factory method without a factory service or factory class.', $id));
   +      }
   +      $service = call_user_func_array(array($factory, $method), $arguments);
   

   Doesn't the original container throws an E_DEPRECTATED warning if factory_method/factory_class/factory_service is used?

3. +++ b/core/lib/Drupal/Core/DependencyInjection/BootstrapContainer.php
   @@ -0,0 +1,269 @@
   +      $length = isset($definition['arguments_count']) ? $definition['arguments_count'] : count($arguments);
   +
   +      $length = count($arguments);
   

   I think the second $length = is wrong

4. +++ b/core/lib/Drupal/Core/DependencyInjection/BootstrapContainer.php
   @@ -0,0 +1,269 @@
   +
   +        if ($type !== NULL) {
   +          throw new InvalidArgumentException(sprintf('Undefined type "%s" while resolving parameters and services.', $type));
   +        }
   

   Would that be a good place for an assertion in the future?

5. +++ b/core/lib/Drupal/Core/DependencyInjection/BootstrapContainer.php
   @@ -0,0 +1,269 @@
   +  }
   +}
   
   +++ b/core/lib/Drupal/Core/DependencyInjection/Dumper/PhpArrayDumper.php
   @@ -0,0 +1,477 @@
   +class PhpArrayDumper extends Dumper {
   +  /**
   +   * {@inheritdoc}
   

   Missing new line

6. +++ b/core/lib/Drupal/Core/DependencyInjection/Container.php
   @@ -7,23 +7,570 @@
   +use Symfony\Component\DependencyInjection\Exception\ParameterNotFoundException;
   

   not used

7. +++ b/core/lib/Drupal/Core/DependencyInjection/Container.php
   @@ -7,23 +7,570 @@
   + * Provides an interface compatible alternative to the default Symfony
   + * dependency injection container optimized for Drupal's needs.
   

   What about splitting this up into: Provides a container optimized for Drupal's need.\n\nThis Container implementation is compatible with the default Syfmony dependency injection container ...

8. +++ b/core/lib/Drupal/Core/DependencyInjection/Container.php
   @@ -7,23 +7,570 @@
   + *
   + * As the container is unfrozen by default, a second parameter can be passed to
   + * the container to "freeze" the parameter bag.
   

   Should we explain when the parameters should be frozen? I would argue that it should be frozen at least once $this->handle() is executed?

9. +++ b/core/lib/Drupal/Core/DependencyInjection/Container.php
   @@ -7,23 +7,570 @@
   + * - Scopes are explicitly not allowed, because Symfony 3.0 has deprecated
   + *   them.
   + * - Synchronized services are explicitly not supported, because Symfony 3.0 has
   + *   deprecated them.
   

   Afaik symfony 3.0 will remove both of those features

10. +++ b/core/lib/Drupal/Core/DependencyInjection/Dumper/DebugPhpArrayDumper.php
    @@ -0,0 +1,75 @@
    +/*
    
    +++ b/core/lib/Drupal/Core/DependencyInjection/Dumper/PhpArrayDumper.php
    @@ -0,0 +1,477 @@
    +/*
    

    Missing "*"

11. +++ b/core/lib/Drupal/Core/DependencyInjection/Dumper/DebugPhpArrayDumper.php
    @@ -0,0 +1,75 @@
    +use Symfony\Component\DependencyInjection\Alias;
    +use Symfony\Component\DependencyInjection\ContainerInterface;
    +use Symfony\Component\DependencyInjection\Definition;
    +use Symfony\Component\DependencyInjection\Parameter;
    +use Symfony\Component\DependencyInjection\Reference;
    +use Symfony\Component\DependencyInjection\Exception\RuntimeException;
    +use Symfony\Component\DependencyInjection\ContainerBuilder;
    +use Symfony\Component\DependencyInjection\Dumper\Dumper;
    +use Symfony\Component\ExpressionLanguage\Expression;
    

    Just ContainerInterface is used

12. +++ b/core/lib/Drupal/Core/DependencyInjection/Dumper/PhpArrayDumper.php
    @@ -0,0 +1,477 @@
    +use Symfony\Component\DependencyInjection\Alias;
    ...
    +use Symfony\Component\DependencyInjection\ContainerBuilder;
    

    unused

13. +++ b/core/lib/Drupal/Core/DependencyInjection/Dumper/PhpArrayDumper.php
    @@ -0,0 +1,477 @@
    + * The format of this dumper is very similar to the ContainerBuilder based
    + * format, but based on PHP arrays and \stdClass objects instead of rich value
    + * objects for performance reasons.
    

    I think it would be still really nice if you could explain here, why this is faster

14. +++ b/core/lib/Drupal/Core/DependencyInjection/Dumper/PhpArrayDumper.php
    @@ -0,0 +1,477 @@
    +  public function getArray() {
    

    Nothing uses this method afaik, so this could be protected?

15. +++ b/core/lib/Drupal/Core/DependencyInjection/Dumper/PhpArrayDumper.php
    @@ -0,0 +1,477 @@
    +   * @param \Symfony\Component\DependencyInjection\Definition $definition
    ...
    +  protected function getServiceDefinition($definition) {
    ...
    +   * @param callable $callable
    +   *   The callable to process.
    ...
    +  protected function dumpCallable($callable) {
    

    Do you mind adding a typehint?

16. +++ b/core/lib/Drupal/Core/DependencyInjection/Dumper/PhpArrayDumper.php
    @@ -0,0 +1,477 @@
    +   * @param mixed $collection
    +   *   A collection to process.
    +   *
    ...
    +  protected function dumpCollection($collection, &$resolve = FALSE) {
    

    Resolve is not documented yet

17. +++ b/core/lib/Drupal/Core/DependencyInjection/Dumper/PhpArrayDumper.php
    @@ -0,0 +1,477 @@
    +  protected function getReferenceCall($id, Reference $reference = null) {
    ...
    +    if ($reference !== NULL) {
    

    null vs. NULL

18. +++ b/core/lib/Drupal/Core/DrupalKernel.php
    @@ -52,6 +50,41 @@
    +  const BOOTSTRAP_CONTAINER_BASE_CLASS = '\Drupal\Core\DependencyInjection\BootstrapContainer';
    +  const PHP_ARRAY_DUMPER_CLASS = '\Drupal\Core\DependencyInjection\Dumper\PhpArrayDumper';
    

    Is there a reason why we can't make them protected variables on the class and then maybe at some point make it actually possible to swap them out?

19. +++ b/core/lib/Drupal/Core/DrupalKernel.php
    @@ -711,24 +740,14 @@ public function updateModules(array $module_list, array $module_filenames = arra
       /**
    -   * Returns the classname based on environment.
    +   * Returns the cache key based on environment.
        *
        * @return string
    -   *   The class name.
    +   *   The cache key.
        */
    -  protected function getClassName() {
    +  protected function getCacheKey() {
    

    I'm curious whether you mind at least adapting the docs to talk about the cache key for the container? Its so not obvious from the method name that we deal with that

20. +++ b/core/tests/Drupal/Tests/Core/DependencyInjection/Dumper/PhpArrayDumperTest.php
    @@ -0,0 +1,579 @@
    +use Symfony\Component\DependencyInjection\Exception\LogicException;
    +use Symfony\Component\DependencyInjection\Exception\InvalidArgumentException;
    

    Unused

21. +++ b/core/tests/Drupal/Tests/Core/DependencyInjection/Dumper/PhpArrayDumperTest.php
    @@ -0,0 +1,579 @@
    +    $this->containerBuilder = $this->prophesize('\Symfony\Component\DependencyInjection\ContainerBuilder');
    +    $this->containerBuilder->getAliases()->willReturn(array());
    +    $this->containerBuilder->getParameterBag()->willReturn(new ParameterBag());
    +    $this->containerBuilder->getDefinitions()->willReturn(NULL);
    

    Jus that shows how much easier it is than ordinary phpunit mocking

22. +++ b/core/tests/Drupal/Tests/Core/DependencyInjection/PhpArray/ContainerTest.php
    @@ -0,0 +1,1185 @@
    + * Contains \Drupal\Tests\Core\DependencyInjection\PhpArray\ContainerTest.
    ...
    +namespace Drupal\Tests\Core\DependencyInjection\PhpArray;
    

    Do you mind moving it to the more appropriate namespace: Drupal\Tests\Core\DependencyInjection ?

23. +++ b/core/tests/Drupal/Tests/Core/DependencyInjection/PhpArray/ContainerTest.php
    @@ -0,0 +1,1185 @@
    +   * @var \Drupal\Core\DependencyInjection\PhpArray\Container
    

    That class does not exist

#161:

1. Fixed
2. We could do that, but we could also just not support it as the dumper already does not support it (else the deprecation notice would have led to failed tests. I vote to just remove it.
3. Indeed, fixed.
4. No, that is a real run-time exception and should never happen unless someone creates a machine-readable definition manually.
5. Fixed
6. Fixed by using it.
7. Fixed
8. I think this is an internal detail.
9. Okay, fixed by saying deprecated in 2.8, will be removed in 3.0.
10. Oops, fixed.
11. Yes, thanks!
12. Fixed.
13. Explained in IRC. TL;DR: Less objects to load, function calls saved, etc.
14. Uhm, not really. DrupalKernel uses it, to avoid a serialize / unserialize dance, because dump() must return a string.
15. I can do that sure, is callable a valid typehint in 5.5+? I assume yes ...
16. Oh, very good point. Fixed.
17. Good catch, fixed.
18. We can totally do that, fixed.
19. Lets call it getContainerCacheKey, that is way more obvious.
20. Fixed
21. YES!!!
22. Yes, There is already a ContainerTest, I guess I should just merge things in.
23. Good catch, fixed.