Skip to content

[RAD][HttpKernel] Create a MicroBundle base class #19596

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 our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 1 commit into from

Conversation

GuilhemN
Copy link
Contributor

@GuilhemN GuilhemN commented Aug 11, 2016

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

This PR introduces a new class: MicroBundle.
It is a all-in-one class replacing the usual Bundle-Extension-Configuration scheme by a simple interface that is enough for many app bundles:

use Symfony\Component\Config\Definition\Builder\NodeParentInterface;
use Symfony\Component\DependencyInjection\ContainerBuilder;
use Symfony\Component\HttpKernel\Bundle\MicroBundle;

class AppBundle extends MicroBundle
{
    protected function buildConfiguration(NodeParentInterface $rootNode)
    {
        $rootNode
            ->children()
                ->scalarNode('foo')->defaultValue('bar')->end()
            ->end();
    }

    protected function buildContainer(array $config, ContainerBuilder $container)
    {
        $container->setParameter('app.foo', $config['foo']);
    }
}

This api is strongly inspired from the Bundle class of the KnpRadBundle.

@ro0NL
Copy link
Contributor

ro0NL commented Aug 11, 2016

Can we make route definitions available?

@GuilhemN
Copy link
Contributor Author

@ro0NL you mean configuring them ?
That's not possible as the routing component is build around resources that must be loaded at the root of the project.

@ro0NL
Copy link
Contributor

ro0NL commented Aug 11, 2016

Yes, as a resource. A bit like the the MicroKernelTrait does (ref).

edit: this is somewhat related to #19594 using the kernel as a service (hence a route resource)

@GuilhemN
Copy link
Contributor Author

@ro0NL the kernel is the root of the project, a bundle isn't.

@ro0NL
Copy link
Contributor

ro0NL commented Aug 11, 2016

Understand, im talking about registering (mounting) bundle:SomeBundleName:loadRoutes from the kernel:loadRoutes resource. Imagine a micro kernel, built from micro bundles.

@GuilhemN
Copy link
Contributor Author

This is possible in the micro kernel trait because the kernel is also a service but bundles aren't and i don't think they should be.
I understand your motivation but it would create weird cases (what about prefix, etc when loading bundles routing?).

@ro0NL
Copy link
Contributor

ro0NL commented Aug 11, 2016

As usual?

// AppKernel::loadRoutes
$bundleRoutes = $loader->import('bundle:SomeBundle:getRoutes', 'service'); // getRoutes to clarify
$bundleRoutes->addPrefix('/some-bundle');

I understand this is not as easy (kernel is a service like you mentioned). However for me, having the kernel as a service is just as weird as having a bundle as a service. Hence i want to propose separating this (kernel/bundle in the ecosystem vs. kernel/bundle as a service).

@GuilhemN
Copy link
Contributor Author

@ro0NL ok got it, you would like to be able to use the bundle class as a resource.
Well I'm not especially against that but I don't how it should be done and adding such support is out of the scope of this PR. We'll still be able to add such support later, this PR is already proposing a huge change as is.

@sstok
Copy link
Contributor

sstok commented Aug 12, 2016

@ro0NL Does this bundle solve your use-case https://github.com/rollerworks/RouteAutowiringBundle?

Edit. Hmm, you want to define routes as a service completely? It should be possible, but you would have to register the Bundle class as a service which seems like a terrible idea 😉

Or you need to create a complete route service definition, which is not easy I know of experience 💀

The main advantage of MicroBundle I see is the removal is Extension class.
I have seen projects with a simple Extension class to load only simple XML services file 😝

@ro0NL
Copy link
Contributor

ro0NL commented Aug 12, 2016

I guess for now i would workaround using the kernel as a service, which couples perhaps better anyway.

// SomeBundle
public function getRoutes() {
    return new RouteCollection(/*...*/);
}

// Kernel
public function loadRoutes(LoaderInterface $loader) {
    $bundleRoutes = $loader->import('kernel:getSomeBundleRoutes', 'service');
    $bundleRoutes->addPrefix('/some-bundle');
}
public function getSomeBundleRoutes() {
    return $this->getBundle('Some')->getRoutes();
}

Im not even sure it's supported though... importing routes from a service?

*
* @author Guilhem N. <egetick@gmail.com>
*/
abstract class MicroBundle extends Bundle implements ExtensionInterface, ConfigurationExtensionInterface, ConfigurationInterface
Copy link
Contributor

@ro0NL ro0NL Aug 13, 2016

Choose a reason for hiding this comment

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

Not sure this needs to be a ConfigurationInterface.. we can eventually just call Processor::process...

Instead of that, implementing PrependExtensionInterface could be useful.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes indeed i didn't know this method. I'll change that asap, thanks :)

Copy link
Contributor Author

@GuilhemN GuilhemN Aug 14, 2016

Choose a reason for hiding this comment

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

After all I don't think we should do that. ConfigurationExtensionInterface is used in commands such as ConfigDebugCommand and ConfigDumpReferenceCommand (should be at least, they won't work if the extension doesn't implement this interface).
And as ConfigurationExtensionInterface returns an instance of ConfigurationInterface, the MicroBundle must be an instance of ConfigurationInterface.

About PrependExtensionInterface it will only allow people to not implement the interface and it won't be used most of the time so I don't see any benefits.

Copy link
Contributor

Choose a reason for hiding this comment

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

And what about CompilerPassInterface?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@hason this interface is already easy to implement (no need of code to make it work on a bundle).

@GuilhemN
Copy link
Contributor Author

Would such feature be accepted?

@ro0NL
Copy link
Contributor

ro0NL commented Sep 16, 2016

Imo it should be, as it's the missing part of the micro kernel right now.

@fabpot
Copy link
Member

fabpot commented Sep 21, 2016

I don't really understand why this is useful in the context of a micro-*. The application using the MicroKernelTrait should probably be bundle-less. What's the benefit or introducing a Bundle in such cases?

@GuilhemN
Copy link
Contributor Author

GuilhemN commented Sep 21, 2016

@fabpot that's useful for tests for the micro kernel but it is mostly useful for most small bundles that doesn't need a complex structure.

@fabpot
Copy link
Member

fabpot commented Sep 21, 2016

Sorry, I wasn't clear in my previous comment. I'm not saying that this is not simpler that the default Bundle class; I just think you don't need a bundle at all.

@GuilhemN
Copy link
Contributor Author

@fabpot i understand but i don't think micro apps are the biggest target here.
This micro bundle can be used for small third party bundles that don't need a big configuration/extension.
It may also be used to explain to newcomers the extension system (knowing a class FQCN is simpler that an entire folder structure, and it is also easier to understand).

@fabpot
Copy link
Member

fabpot commented Sep 21, 2016

I think that the extension system should never be used for anything but Open-Source/shared bundles. In which case, there is no need for something simpler. In any case, naming it like MicroKernel as one might think that there are related, but they are not.

@GuilhemN
Copy link
Contributor Author

@fabpot why wouldn't we need something simpler? An extension may only contain services loading with maybe a few configuration parameters to manage and in this case it looks overkill to have 3 classes to do that.

@ro0NL
Copy link
Contributor

ro0NL commented Sep 21, 2016

The application using the MicroKernelTrait should probably be bundle-less.

Disagree, i truly consider a micro app built from simple bundles.

What's the benefit or introducing a Bundle in such cases?

Separation of concerns and reuseability.

In any case, naming it like MicroKernel as one might think that there are related, but they are not.

They are :) In the sense that a kernel can load bundles. However naming it micro could cause users to think it's required to have micro bundles, when using a micro kernel. Which indeed is not the case.

Maybe we can do better naming... SimpleBundle?

edit:
As this easily can be done in user-land it could be a reason to not ship it with the core framework. Totally legit... but that also counted for the micro kernel trait. Hence i think at this point it should be shipped for the same reason. And eventually they play well together 👍

/**
* {@inheritdoc}
*/
public function getNamespace()
Copy link
Contributor

Choose a reason for hiding this comment

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

@Ener-Getick i think this is not truly compatible with Bundle::getNamespace, perhaps it works.. but the intention is different. What about not implementing ExtensionInterface, but rather return a generic/callback extension from MicroBundle::getContainerExtension. WDYT?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Indeed you're right... I have to find some time to rework that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done, sorry for the delay

{
}

protected function buildContainer(array $config, ContainerBuilder $container)
Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure but i'd prefer the arguments in reverse order (builder first).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This follows the Extension::load signature but both are fine for me.

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe it should be called loadContainer then, to clarify a bit more and dont create too much confusion with Bundle::build.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't think so, Bundle::build builds the bundle (so just after the kernel is created) while Bundle::buildContainer builds the container (which happens much later in the kernel life).

Copy link
Contributor

@ro0NL ro0NL Oct 20, 2016

Choose a reason for hiding this comment

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

public function build(ContainerBuilder $container).. it builds the container ;-) (ref)

Copy link
Contributor

Choose a reason for hiding this comment

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

@Ener-Getick maybe just mimic (public) ExtensionInterface::load as (protected) SimpleBundle::loadExtension.

Copy link
Contributor Author

@GuilhemN GuilhemN Oct 20, 2016

Choose a reason for hiding this comment

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

If we consider adding bundle passes to the container as part of the container building, it does build the container of course :P When i talk about container building, I mostly think about services/parameters which should not be added at this level.

The naming is kind of hard at this level, build is one the first methods executed (it may not be clear enough), about the new method I'd like it to be clear that it is based on configuration.
What about configureBundle ? or configureContainer ?

private $bundle;
private $buildContainer;

public function __construct(SimpleBundle $bundle, Closure $buildContainer)
Copy link
Contributor

Choose a reason for hiding this comment

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

As this class is internal, we could just call $bundle->buildContainer() with a little reflection magic :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure but i don't like much accessing private functions with reflection. Maybe we could use Closure::bind, i hesitated.

Copy link
Contributor

@ro0NL ro0NL Oct 20, 2016

Choose a reason for hiding this comment

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

The API would be cleaner.. :) public function __construct(SimpleBundle $bundle) and we are truly tight to SimpleBundle, not bridging it with a closure, ie. you cant abuse it.

return $treeBuilder;
}

protected function buildConfiguration(NodeParentInterface $rootNode)
Copy link
Contributor

Choose a reason for hiding this comment

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

Can be ArrayNodeDefinition which is probably more convenient.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This would limit the possibilities in case someone only want a scalar definition here.

Copy link
Contributor

Choose a reason for hiding this comment

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

Each bundle should still define it's config under an alias namespace.. right? The root node is an array node by default: https://github.com/symfony/symfony/blob/master/src/Symfony/Component/Config/Definition/Builder/TreeBuilder.php#L34

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Indeed you're right. Maybe we can even use TreeBuilder i don't think there's an use case to use something else anyway.

final public function getConfigTreeBuilder()
{
$treeBuilder = new TreeBuilder();
$rootNode = $treeBuilder->root($this->getAlias());
Copy link
Contributor

Choose a reason for hiding this comment

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

getAlias does not exists ;) you should override SimpleExtension::getAlias and create a unique alias for each simple bundle class.

public function __construct(SimpleBundle $bundle, Closure $buildContainer)
{
$this->bundle = $bundle;
$this->buildContainer = Closure::bind(function (array $config, ContainerBuilder $container) {
Copy link
Contributor

Choose a reason for hiding this comment

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

This is usually called only once during building phase right? ie. do we really need property cache? 2nd. constructor arg is unused now ;-)

}

/**
* Simple {@link ExtensionInterface} supporting {@link MicroBundle}.
Copy link
Contributor

Choose a reason for hiding this comment

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

{@link SimpleBundle}

@GuilhemN GuilhemN force-pushed the BUNDLE branch 2 times, most recently from 24180d1 to 6128e57 Compare October 20, 2016 21:02
// check naming convention
$basename = preg_replace('/Bundle$/', '', $this->bundle->getName());

return Container::underscore($basename);
Copy link
Contributor

Choose a reason for hiding this comment

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

Container is not imported :)

Copy link
Contributor Author

@GuilhemN GuilhemN Oct 25, 2016

Choose a reason for hiding this comment

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

I should definitely add tests... But i'd like the symfony members' point of view before spending too much time on them.

Copy link
Contributor

Choose a reason for hiding this comment

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

Understand, i already planned taking it (if you're ok with it ;-)) and it's not accepted (hence the review :)). Perhaps create a composer gist then? Anyway.. hope this gets excepted 👍

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No problem, that's made to be reused ;)
I would prefer seing this merged in symfony, i think it loses most of its interest if it's not simple and quick to use.

return $treeBuilder;
}

protected function buildConfiguration(TreeBuilder $rootNode)
Copy link
Contributor

Choose a reason for hiding this comment

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

$rootNode is not of type TreeBuilder ;-) I'd prefer ArrayNodeDefinition (or the interface otherwise) and let getConfigTreeBuilder root with the right alias.

{
}

protected function buildContainer(array $config, ContainerBuilder $container)
Copy link
Contributor

@ro0NL ro0NL Oct 23, 2016

Choose a reason for hiding this comment

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

I really dont like this name 😕 imo. it's confusing with Bundle::build(ContainerBuilder $container).

I think it makes more sense to have SimpleBundle::loadExtension (as we're always called from SimpleExtension::load).

What about configureBundle ? or configureContainer ?

We really should consider having Bundle::build already and what is best DX. So.., it's called from Kernel::prepareContainer, which is called from Kernel::buildContainer. Ie. it's easy to assume build already acts as buildContainer, it's just poor naming imo.

However, in that spirit, we could also go with simply SimpleBundle::load. Or configure from your pov.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What about injecting a chained loader pointing to Resources/config?
Then imo it would be more appropriate to name this method load.

{
$config = $this->processConfiguration($this->bundle, $configs);

$f = Closure::bind(function (array $config, ContainerBuilder $container) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Should be \Closure.

{
}

protected function buildContainer(array $config, ContainerBuilder $container)
protected function load(array $config, ContainerBuilder $container, LoaderInterface $loader)
Copy link
Contributor

Choose a reason for hiding this comment

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

Love it :D

@GuilhemN
Copy link
Contributor Author

GuilhemN commented Oct 26, 2016

@ro0NL thanks for your reviews, I fixed your last comments :)

The last version injects a loader in SimpleBundle::load() which allows to have something even lighter:

class AppBundle extends SimpleBundle
{
    protected function load(array $config, ContainerBuilder $container, LoaderInterface $loader)
    {
        // Loads a directory
        $loader->load('store');
        // Loads a single file
        $loader->load('basket.yml');
    }
}

For now the path to Resources/config is hardcoded but I'd like to create a new method in SimpleBundle allowing to customize it. I just don't know what to call it...

@ro0NL
Copy link
Contributor

ro0NL commented Oct 26, 2016

SimpleBundle::__construct?

We could also consider $loader->load('Resources/config/store'); and keep things simple :)

@GuilhemN
Copy link
Contributor Author

GuilhemN commented Oct 26, 2016

@ro0NL in the constructor? I was thinking about a protected getter.

Keeping things verbose work as well but it's not very friendly when you have a lot of files to load. Letting the user choose the paths he wants to use is better imo.

@ro0NL
Copy link
Contributor

ro0NL commented Oct 26, 2016

Yeah.. like __construct($loaderPath = 'Resources/config'). And override it if you load a special bundle :)

This path is usually user-defined (as symfony is flexible).. not sure introducing some special new path (i cant think of name either.. getConfigPath?) is the right way. Thats why i prefer $loader->load('whatever/bundle/path') as it keep things tight to the user. Opposed to having it tight to the bundle's definition.

@ro0NL
Copy link
Contributor

ro0NL commented Dec 29, 2016

@GuilhemN given PHPArray file config doesnt come to core, and this is practically a big self-bridging class... im :-+0: i guess. It gave some good ideas for smaller apps though 👍

@GuilhemN
Copy link
Contributor Author

GuilhemN commented Dec 29, 2016

@ro0NL why would we need to update the config in a bundle ? A bundle should only configure services/parameters.

@ro0NL
Copy link
Contributor

ro0NL commented Dec 29, 2016

You dont :) I really like the combination of it all in that sense; for having smaller apps and bundles. Basically the whole philosophy on it's lowest level. Not ending up with a big fat dir. structure and fancy config :)

@fabpot
Copy link
Member

fabpot commented Mar 1, 2017

Still 👎 on adding this in core. ping @symfony/deciders

@jakzal
Copy link
Contributor

jakzal commented Mar 1, 2017

I'm also 👎

The argument a single class is simpler doesn't convince me. Number of classes is not necessarily and indicator of complexity. What worries me with the "SimpleBundle" is that it mixes several responsibilities and can quickly get out of hand. I also think it's better to have one way of doing things.

Btw, if you insist on having everything in a single place perhaps anonymous classes could be helpful.

@fabpot fabpot closed this Mar 1, 2017
@stof
Copy link
Member

stof commented Mar 1, 2017

Thus, this PR still wants to support all features available with an extension, but puts them all in the bundle class. This does not make any sense IMO. If you want to process configuration, define the appropriate classes

@GuilhemN GuilhemN deleted the BUNDLE branch March 3, 2017 16:03
@nicolas-grekas nicolas-grekas modified the milestones: 3.x, 3.3 Mar 24, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants