NotificationsYou must be signed in to change notification settings
Fork9.7k
Star30.8k

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Jump to bottom

Merged

fabpot merged 1 commit intosymfony:masterfromlinaori:feature/appbundle-less

Feb 22, 2017

Merged

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

fabpot merged 1 commit intosymfony:masterfromlinaori:feature/appbundle-less

Feb 22, 2017

Conversation

Copy link

Contributor

linaori commentedSep 30, 2016•
edited
Loading

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.phpclass AppBundleextends Bundle{publicfunctionbuild(ContainerBuilder$container)    {$container->addCompilerPass(newSomeCompilerPass());$container->addCompilerPass(newAnotherCompilerPass());$container->addCompilerPass(newYetAnotherCompilerPass());    }}// src/DependencyInjection/AppExtension.phpclass AppExtensionextends Extension{publicfunctionload(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.phpclass AppKernelextends Kernel{protectedfunctionbuild(ContainerBuilder$container)    {$binary = ExecutableResolver::getPath($container->getParameter('kernel.root_dir').'/../');$snappyConfig = ['pdf' => ['binary' =>realpath($binary)]];$container->prependExtensionConfig('knp_snappy',$snappyConfig);$container->addCompilerPass(newSomeCompilerPass());$container->addCompilerPass(newAnotherCompilerPass());$container->addCompilerPass(newYetAnotherCompilerPass());    }}

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

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

Copy link

akomm commentedSep 30, 2016

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

Copy link

Member

GuilhemNSep 30, 2016•
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Is it still worth it when using php7 ?

Copy link

Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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.

Copy link

ContributorAuthor

linaoriSep 30, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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

Copy link

Contributor

GuilhemNSep 30, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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.

Copy link

Contributor

backbone87 commentedSep 30, 2016•
edited
Loading

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

Copy link

ContributorAuthor

linaori commentedSep 30, 2016

@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 reviewed

Oct 1, 2016

View reviewed changes

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

		*/
		publicfunctionprocess(ContainerBuilder$container)
		{
		$classes =array();

Copy link

Contributor

ro0NLOct 1, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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

Copy link

ContributorAuthor

linaoriOct 1, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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

Copy link

ContributorAuthor

linaoriOct 1, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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

Copy link

Contributor

ro0NLOct 1, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Exactly. The order seems more logical imo.

HeahDude suggested changes

Oct 1, 2016

View reviewed changes

src/Symfony/Component/HttpKernel/Kernel.php Outdated

		file_put_contents($this->getCacheDir().'/annotations.map',sprintf('<?php return %s;',var_export($annotatedClasses,true)));
		}

		/**

Copy link

Contributor

HeahDudeOct 1, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Returns (same for the other method below)

src/Symfony/Component/HttpKernel/Kernel.php Outdated


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

Copy link

Contributor

HeahDudeOct 1, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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

Copy link

Contributor

backbone87Oct 1, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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

javiereguiluz reviewed

Oct 1, 2016

View reviewed changes

src/Symfony/Component/HttpKernel/Kernel.php

		* Use this method to register compiler passes and manipulate the container during the building process.
		*
		* @param ContainerBuilder $container
		*/

Copy link

Member

javiereguiluzOct 1, 2016•
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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

Copy link

ContributorAuthor

linaoriOct 1, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Good point

Copy link

Contributor

ro0NLOct 1, 2016•
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

So isntKernel::buildContainer the methodBundle::build tends to be, ie.. do we still need that? Should it beBundle::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.

Copy link

ContributorAuthor

linaoriOct 3, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

@javiereguiluz buildContainer is already taken

Copy link

ContributorAuthor

linaori commentedOct 3, 2016

@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.

protectedfunctiondoLoadClassCache($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);        }    }

Copy link

ContributorAuthor

linaori commentedOct 3, 2016

Added tests, everything seems to work now.

Copy link

Member

nicolas-grekas commentedOct 3, 2016

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

Copy link

ContributorAuthor

linaori commentedOct 3, 2016

@nicolas-grekas I was wondering why the kerneldoes 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.

Copy link

Member

nicolas-grekas commentedOct 3, 2016

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 reviewed

Oct 3, 2016

View reviewed changes

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

		}

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

Copy link

Contributor

TaluuOct 3, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

why stop usingarray_merge rather than aforeach here ?

Copy link

ContributorAuthor

linaoriOct 4, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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

Copy link

Member

stofNov 29, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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

Copy link

ContributorAuthor

linaori commentedOct 22, 2016

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

Copy link

Contributor

ro0NL commentedOct 22, 2016•
edited
Loading

What about afinalize() method as well? Besides it would make sense to renamebuildContainer tocreateContainer, so we can then later usebuild/finalizeContainer (in favor ofbuild/finalize).

That would fix correct naming.

wouterj mentioned this pull request

Nov 9, 2016

Review all the articles related to bundlessymfony/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

src/Symfony/Component/HttpKernel/Kernel.php Outdated

		}
		}

		$this->build($container);

Copy link

Member

stofNov 29, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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

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.

src/Symfony/Component/HttpKernel/Kernel.php

		}
		}

		/**

Copy link

Member

stofNov 29, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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

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

		}

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

Copy link

Member

stofNov 29, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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

Copy link

ContributorAuthor

linaori commentedNov 30, 2016

Reverted the changes related to the class cache due toConsider deprecating ClassCollectionLoader & co. #20668
Decided to keep thebuild() 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 Kernelbuild() is exactly the same as the bundlebuild(), which makes it a lot more intuitive and easier to understand how it works
Thebuild() method for the kernel is now called last instead of first as mentioned by@stof.

nicolas-grekas added this to the3.x milestone

Dec 6, 2016

Copy link

ContributorAuthor

linaori commentedJan 9, 2017

Status: Needs Review

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

Copy link

Member

xabbuh commentedJan 12, 2017

👍

Status: Reviewed

carsonbot added Status: Reviewed and removed Status: Needs Review labels

Jan 12, 2017

Added build and class cache to kernel

62e80fc

Copy link

ContributorAuthor

linaori commentedFeb 22, 2017

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

Copy link

Member

fabpot commentedFeb 22, 2017

Thank you@iltar.

fabpot merged commit62e80fc intosymfony:master

Feb 22, 2017

fabpot added a commit that referenced this pull request

Feb 22, 2017

feature#20107Added a build method to the kernel to replace Bundle::…

99f60dc

…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.phpclass AppBundle extends Bundle{    public function build(ContainerBuilder $container)    {        $container->addCompilerPass(new SomeCompilerPass());        $container->addCompilerPass(new AnotherCompilerPass());        $container->addCompilerPass(new YetAnotherCompilerPass());    }}// src/DependencyInjection/AppExtension.phpclass 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.phpclass 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

linaori mentioned this pull request

Feb 22, 2017

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

Closed