Problem/Motivation

Improve YamlFileLoader so that we can cache built service definition objects and not just the raw data.

Proposed resolution

@todo

Remaining tasks

@todo

User interface changes

API changes

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Fabianx’s picture

Status: Active » Postponed
mgifford’s picture

Status: Postponed » Active

The parent issue is fixed, so assuming this should be active now.

Version: 8.0.x-dev » 8.1.x-dev

Drupal 8.0.6 was released on April 6 and is the final bugfix release for the Drupal 8.0.x series. Drupal 8.0.x will not receive any further development aside from security fixes. Drupal 8.1.0-rc1 is now available and sites should prepare to update to 8.1.0.

Bug reports should be targeted against the 8.1.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.2.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Fabianx’s picture

Title: Improve YamlFileLoader so that we can cache built service definition objects and not just the raw data. » Remove @todo to Improve YamlFileLoader so that we can cache built service definition objects and not just the raw data.
Priority: Major » Normal
Status: Active » Needs work
Issue tags: +Novice, +php-novice

This issue is actually:

"Closed: Won't Fix"

but there is a dangling todo in core/lib/Drupal/Core/DependencyInjection/YamlFileLoader.php.

which references this issue.

Lets remove it!

The reason this is Won't fix is because I benchmarked it and the savings had been minimal (-168 ms) for 5-6 container rebuilds, but the effort / complexity is pretty huge.

=> Not worth it.

Diff here to preserve the approach, but it is _not_ worth it.

diff --git a/core/lib/Drupal/Core/DependencyInjection/YamlFileLoader.php b/core/lib/Drupal/Core/DependencyInjection/YamlFileLoader.php
index 2a55542..865c6b6 100644
--- a/core/lib/Drupal/Core/DependencyInjection/YamlFileLoader.php
+++ b/core/lib/Drupal/Core/DependencyInjection/YamlFileLoader.php
@@ -3,9 +3,11 @@
 
 namespace Drupal\Core\DependencyInjection;
 
+use Drupal\Component\DependencyInjection\Dumper\PhpArrayDumper;
 use Drupal\Component\FileCache\FileCacheFactory;
 use Drupal\Component\Serialization\Yaml;
 use Symfony\Component\DependencyInjection\Alias;
+use Symfony\Component\DependencyInjection\ContainerBuilder as SymfonyContainerBuilder;
 use Symfony\Component\DependencyInjection\ContainerInterface;
 use Symfony\Component\DependencyInjection\Definition;
 use Symfony\Component\DependencyInjection\DefinitionDecorator;
@@ -33,6 +35,11 @@ class YamlFileLoader
     /**
      * @var \Drupal\Core\DependencyInjection\ContainerBuilder $container
      */
+    protected $containerBuilder;
+    
+    /**
+     * @var \Symfony\Component\DependencyInjection\ContainerBuilder $container
+     */
     protected $container;
 
     /**
@@ -45,7 +52,7 @@ class YamlFileLoader
 
     public function __construct(ContainerBuilder $container)
     {
-        $this->container = $container;
+        $this->containerBuilder = $container;
         $this->fileCache = FileCacheFactory::get('container_yaml_loader');
     }
 
@@ -60,12 +67,17 @@ public function load($file)
         // Load from the file cache, fall back to loading the file.
         // @todo Refactor this to cache parsed definition objects in
         //   https://www.drupal.org/node/2464053
-        $content = $this->fileCache->get($file);
-        if (!$content) {
-            $content = $this->loadFile($file);
-            $this->fileCache->set($file, $content);
+        $container = $this->fileCache->get($file);
+        if ($container instanceof SymfonyContainerBuilder) {
+          $this->mergeContainerBuilder($container);
+          return;
         }
 
+        $this->container = new SymfonyContainerBuilder();
+
+        $content = $this->loadFile($file);
+
         // Not supported.
         //$this->container->addResource(new FileResource($path));
 
@@ -95,6 +107,24 @@ public function load($file)
 
         // services
         $this->parseDefinitions($content, $file);
+
+        $this->fileCache->set($file, $this->container);
+        $this->mergeContainerBuilder($this->container);
+    }
+
+    private function mergeContainerBuilder($container) {
+        if ($this->containerBuilder->isFrozen()) {
+            throw new BadMethodCallException('Cannot merge on a frozen container.');
+        }
+
+        $definitions = [];
+        foreach ($container->getDefinitions() as $key => $definition) {
+            $definitions[$key] = clone $definition;
+        }
+
+        $this->containerBuilder->addDefinitions($definitions);
+        $this->containerBuilder->addAliases($container->getAliases());
+        $this->containerBuilder->getParameterBag()->add($container->getParameterBag()->all());
     }
 
     /**
marcoscano’s picture

Status: Needs work » Needs review
FileSize
725 bytes

"@todo removed" :)

Fabianx’s picture

Status: Needs review » Reviewed & tested by the community

RTBC - Thank you!

Fabianx’s picture

Version: 8.1.x-dev » 8.2.x-dev
alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Committed 8fade5d and pushed to 8.1.x and 8.2.x. Thanks!

  • alexpott committed a02f143 on 8.2.x
    Issue #2464053 by marcoscano, Fabianx: Remove @todo to Improve...

  • alexpott committed 8fade5d on 8.1.x
    Issue #2464053 by marcoscano, Fabianx: Remove @todo to Improve...
Fabianx’s picture

Version: 8.2.x-dev » 8.1.x-dev

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.