Added a build method to the kernel to replace Bundle::build() #20107

linaori · 2016-09-30T14:55:35Z

Q	A
Branch?	master
Bug fix?	no
New feature?	yes
BC breaks?	no
Deprecations?	no
Tests pass?	yes
Fixed tickets	#20099
License	MIT
Doc PR	~

Adds a DX method to make it easier to omit using an AppBundle in your application.

Old situation

// src/AppBundle.php
class AppBundle extends Bundle
{
    public function build(ContainerBuilder $container)
    {
        $container->addCompilerPass(new SomeCompilerPass());
        $container->addCompilerPass(new AnotherCompilerPass());
        $container->addCompilerPass(new YetAnotherCompilerPass());
    }
}

// src/DependencyInjection/AppExtension.php
class AppExtension extends Extension
{
    public function load(array $config, ContainerBuilder $container)
    {
        $binary = ExecutableResolver::getPath($container->getParameter('kernel.root_dir').'/../');
        $snappyConfig = ['pdf' => ['binary' => realpath($binary)]];

        $container->prependExtensionConfig('knp_snappy', $snappyConfig);
    }
}

New situation

// rm src/AppBundle.php
// rm src/DependencyInjection/AppExtension.php

// app/AppKernel.php
class AppKernel extends Kernel
{
    protected function build(ContainerBuilder $container)
    {
        $binary = ExecutableResolver::getPath($container->getParameter('kernel.root_dir').'/../');
        $snappyConfig = ['pdf' => ['binary' => realpath($binary)]];

        $container->prependExtensionConfig('knp_snappy', $snappyConfig);
        $container->addCompilerPass(new SomeCompilerPass());
        $container->addCompilerPass(new AnotherCompilerPass());
        $container->addCompilerPass(new YetAnotherCompilerPass());
    }
}

Still missing tests, wondering if worth adding in this state first.

akomm · 2016-09-30T16:36:28Z

The less often you need to create an Extension in AppBundle, the better.

fabpot · 2016-09-30T16:39:30Z

I like this a lot!

GuilhemN · 2016-09-30T16:18:16Z

src/Symfony/Component/HttpKernel/Kernel.php

+     *
+     * @return string[]
+     */
+    public function getClassesToCompile()


Is it still worth it when using php7 ?

I'm planning to verify this soon. Yet this PR is right adding it: it's just moving existing things around, and we still support php 5.
If php7 makes this unnecessary, then the whole inline system could be deprecated.
To be proven first, and not in this PR imho.

@Ener-Getick if not, we could always patch it to only create and load this in php 5

This is mostly for new apps (which should use 7.0) so imo it doesn't make sense to allow that if that's not worth it in 7.0 but of course as @nicolas-grekas said, at worst we'll only have to deprecate it later.

backbone87 · 2016-09-30T20:19:23Z

Would it be worth to let the Kernel itself be a container extension?

linaori · 2016-09-30T20:55:23Z

@backbone87 which of those methods would you actually implement of the extension? https://github.com/symfony/symfony/blob/master/src/Symfony/Component/DependencyInjection/Extension/ExtensionInterface.php

ro0NL · 2016-10-01T13:00:19Z

src/Symfony/Component/HttpKernel/DependencyInjection/AddClassesToCachePass.php

@@ -39,12 +39,27 @@ public function process(ContainerBuilder $container)
        $classes = array();


What about $classes = $this->kernel->getClassesToCompile(); and drop all foreaches below.. (ie. is replacing the array_merge with foreach really needed for performance?)

Ideally I'd just append everything to 1 big array and then do array_merge([], ...$classes), but 5.5 doesn't support that

Actually, assigning everything from the kernel as defaults isn't a bad idea

Exactly. The order seems more logical imo.

HeahDude · 2016-10-01T14:54:44Z

src/Symfony/Component/HttpKernel/Kernel.php

@@ -344,6 +344,30 @@ public function setAnnotatedClassCache(array $annotatedClasses)
    }

    /**
+     * Return a list of classes to be cached away when the container is build.


Returns (same for the other method below)

HeahDude · 2016-10-01T14:55:33Z

src/Symfony/Component/HttpKernel/Kernel.php

@@ -344,6 +344,30 @@ public function setAnnotatedClassCache(array $annotatedClasses)
    }

    /**
+     * Return a list of classes to be cached away when the container is build.
+     *
+     * If a class contains annotations, add it to getAnnotatedClassesToCompile()instead.


Not sure this is worth a duplicated extra comment, the API looks explicit enough

I think its worth mentioning it here, although i would word it like so "Do not add classes that contain annotations, use ... instead"

javiereguiluz · 2016-10-01T15:00:31Z

src/Symfony/Component/HttpKernel/Kernel.php

+     *
+     * @param ContainerBuilder $container
+     */
+    protected function build(ContainerBuilder $container)


I don't like the name of this method. According to the Symfony naming conventions I think this should be called buildContainer() because we are not building the kernel, which is what the Kernel class is about.

So isnt Kernel::buildContainer the method Bundle::build tends to be, ie.. do we still need that? Should it be Bundle::buildContainer?

If you go bundles, you probably (should) go extensions anyway.

If you want a more simple bundle i'd prefer the approach from #19596.

@javiereguiluz buildContainer is already taken

linaori · 2016-10-03T07:36:46Z

@tgalopin in #19205 you've added the annotated class cache and in #18533 you've added the class cache warmer. However, the kernel never received a variant of this. What is this for? I noticed that it never loads the annotated classes but it does load the normal class cache. However, there's also a ClassCacheCacheWarmer that does this. @fabpot do you know what the idea behind this is?

I'm wondering if something is broken here.

    protected function doLoadClassCache($name, $extension)
    {
        if (!$this->booted && is_file($this->getCacheDir().'/classes.map')) {
            ClassCollectionLoader::load(include($this->getCacheDir().'/classes.map'), $this->getCacheDir(), $name, $this->debug, false, $extension);
        }
    }

linaori · 2016-10-03T10:17:50Z

Added tests, everything seems to work now.

nicolas-grekas · 2016-10-03T11:51:45Z

@iltar the annotated class requires an optimized composer autoloader to work, this maybe why you don't see it working?

linaori · 2016-10-03T11:54:05Z

@nicolas-grekas I was wondering why the kernel does load the class cache while there's also a class cache warmer, but only a class cache warmer for the annotation variant. It feels like the kernel variant could be removed.

nicolas-grekas · 2016-10-03T11:58:12Z

Could be yes, now that the cache warmer should be working as expected in 3.2.
The draw back is that cache warmers aren't always used, but maybe we shouldn't care :)

Taluu · 2016-10-03T14:58:02Z

src/Symfony/Component/HttpKernel/DependencyInjection/AddClassesToCachePass.php

+            }
+
+            foreach ($extension->getAnnotatedClassesToCompile() as $annotatedClass) {
+                $annotatedClasses[] = $annotatedClass;


why stop using array_merge rather than a foreach here ?

Basically due to performance. Array merge in loops is slow (at least in php5, haven't benchmarked 7 myself): https://github.com/dseguy/clearPHP/blob/master/rules/no-array_merge-in-loop.md

if this is a performance optimization, you should submit it separately, as it may be worth merging it into 3.2 already.

linaori · 2016-10-22T16:33:17Z

Anything still required for this PR? @javiereguiluz had a comment about the name of the method, but the suggested name is already taken.

ro0NL · 2016-10-22T16:46:50Z

What about a finalize() method as well? Besides it would make sense to rename buildContainer to createContainer, so we can then later use build/finalizeContainer (in favor of build/finalize).

That would fix correct naming.

stof · 2016-11-29T11:12:27Z

src/Symfony/Component/HttpKernel/Kernel.php

@@ -595,6 +630,9 @@ protected function prepareContainer(ContainerBuilder $container)
                $container->addObjectResource($bundle);
            }
        }
+
+        $this->build($container);


I would put it after bundles, to match the common convention to register your AppBundle last, which would match making AppKernel last when calling build.

Registering normal compiler passes of the project after the core normal compiler passes is more logical IMO, and would limit the need to use priority overrides.

stof · 2016-11-29T11:14:03Z

src/Symfony/Component/HttpKernel/Kernel.php

@@ -447,6 +471,17 @@ protected function initializeBundles()
    }

    /**
+     * The extension point similar to the Bundle::build() method.


technically, you can already achieve this by overriding Kernel::buildContainer, as long as you take care to start by calling the parent method.

stof · 2016-11-29T11:15:35Z

src/Symfony/Component/HttpKernel/DependencyInjection/AddClassesToCachePass.php

+            }
+
+            foreach ($extension->getAnnotatedClassesToCompile() as $annotatedClass) {
+                $annotatedClasses[] = $annotatedClass;


if this is a performance optimization, you should submit it separately, as it may be worth merging it into 3.2 already.

linaori · 2016-11-30T09:45:46Z

Reverted the changes related to the class cache due to Consider deprecating ClassCollectionLoader & co. #20668
Decided to keep the build() method as is:
- Deprecated/renaming methods in the kernel was not part of this scope (but I do agree naming could be a bit better now)
- The Kernel build() is exactly the same as the bundle build(), which makes it a lot more intuitive and easier to understand how it works
The build() method for the kernel is now called last instead of first as mentioned by @stof.

linaori · 2017-01-09T07:53:09Z

Status: Needs Review

xabbuh · 2017-01-12T05:52:39Z

👍

Status: Reviewed

linaori · 2017-02-22T09:14:36Z

Failure is unrelated: Symfony\Component\Process\Tests\ProcessTest::testInputStreamWithCallable, PR is merged against the latest master

fabpot · 2017-02-22T17:07:14Z

Thank you @iltar.

…build() (iltar) This PR was merged into the 3.3-dev branch. Discussion ---------- Added a build method to the kernel to replace Bundle::build() | Q | A | | --- | --- | | Branch? | master | | Bug fix? | no | | New feature? | yes | | BC breaks? | no | | Deprecations? | no | | Tests pass? | yes | | Fixed tickets | #20099 | | License | MIT | | Doc PR | ~ | Adds a DX method to make it easier to omit using an AppBundle in your application. **Old situation** ``` php // src/AppBundle.php class AppBundle extends Bundle { public function build(ContainerBuilder $container) { $container->addCompilerPass(new SomeCompilerPass()); $container->addCompilerPass(new AnotherCompilerPass()); $container->addCompilerPass(new YetAnotherCompilerPass()); } } // src/DependencyInjection/AppExtension.php class AppExtension extends Extension { public function load(array $config, ContainerBuilder $container) { $binary = ExecutableResolver::getPath($container->getParameter('kernel.root_dir').'/../'); $snappyConfig = ['pdf' => ['binary' => realpath($binary)]]; $container->prependExtensionConfig('knp_snappy', $snappyConfig); } } ``` **New situation** ``` php // rm src/AppBundle.php // rm src/DependencyInjection/AppExtension.php // app/AppKernel.php class AppKernel extends Kernel { protected function build(ContainerBuilder $container) { $binary = ExecutableResolver::getPath($container->getParameter('kernel.root_dir').'/../'); $snappyConfig = ['pdf' => ['binary' => realpath($binary)]]; $container->prependExtensionConfig('knp_snappy', $snappyConfig); $container->addCompilerPass(new SomeCompilerPass()); $container->addCompilerPass(new AnotherCompilerPass()); $container->addCompilerPass(new YetAnotherCompilerPass()); } } ``` Still missing tests, wondering if worth adding in this state first. Commits ------- 62e80fc Added build and class cache to kernel

carsonbot added Status: Needs Review Feature labels Sep 30, 2016

linaori mentioned this pull request Sep 30, 2016

[RFC] Enhance Kernel to allow removal of AppBundle concept (Advanced Usages) #20099

Closed

GuilhemN reviewed Sep 30, 2016

View reviewed changes

ro0NL reviewed Oct 1, 2016

View reviewed changes

HeahDude suggested changes Oct 1, 2016

View reviewed changes

javiereguiluz reviewed Oct 1, 2016

View reviewed changes

Taluu reviewed Oct 3, 2016

View reviewed changes

wouterj mentioned this pull request Nov 9, 2016

Review all the articles related to bundles symfony/symfony-docs#7126

Closed

linaori mentioned this pull request Nov 29, 2016

Consider deprecating ClassCollectionLoader & co. #20668

Closed

stof reviewed Nov 29, 2016

View reviewed changes

nicolas-grekas added this to the 3.x milestone Dec 6, 2016

linaori changed the title ~~Added build and class cache to kernel~~ Added a build method to the kernel to replace Bundle::build() Jan 11, 2017

carsonbot added Status: Reviewed and removed Status: Needs Review labels Jan 12, 2017

Added build and class cache to kernel

62e80fc

fabpot merged commit 62e80fc into symfony:master Feb 22, 2017

linaori mentioned this pull request Feb 22, 2017

[do not merge] Change the default directory structure and use new features of 3.3 symfony/symfony-standard#1034

Closed

javiereguiluz mentioned this pull request Feb 26, 2017

Document the build() method for Kernel class symfony/symfony-docs#7535

Closed

nicolas-grekas modified the milestones: 3.x, 3.3 Mar 24, 2017

fabpot mentioned this pull request May 1, 2017

Release v3.3.0-BETA1 #22603

Merged

linaori deleted the feature/appbundle-less branch February 8, 2019 13:38

		@@ -39,12 +39,27 @@ public function process(ContainerBuilder $container)
		$classes = array();

Added a build method to the kernel to replace Bundle::build() #20107

Added a build method to the kernel to replace Bundle::build() #20107

Conversation

linaori commented Sep 30, 2016 • edited Loading

akomm commented Sep 30, 2016

fabpot commented Sep 30, 2016

GuilhemN Sep 30, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

backbone87 commented Sep 30, 2016 • edited Loading

linaori commented Sep 30, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

javiereguiluz Oct 1, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ro0NL Oct 1, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

linaori commented Oct 3, 2016

linaori commented Oct 3, 2016

nicolas-grekas commented Oct 3, 2016

linaori commented Oct 3, 2016

nicolas-grekas commented Oct 3, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

linaori commented Oct 22, 2016

ro0NL commented Oct 22, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

linaori commented Nov 30, 2016

linaori commented Jan 9, 2017

xabbuh commented Jan 12, 2017

linaori commented Feb 22, 2017

fabpot commented Feb 22, 2017

linaori commented Sep 30, 2016 •

edited

Loading

GuilhemN Sep 30, 2016 •

edited

Loading

backbone87 commented Sep 30, 2016 •

edited

Loading

javiereguiluz Oct 1, 2016 •

edited

Loading

ro0NL Oct 1, 2016 •

edited

Loading

ro0NL commented Oct 22, 2016 •

edited

Loading