Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
[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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
ro0NL commentedAug 11, 2016
Can we make route definitions available? |
GuilhemN commentedAug 11, 2016
@ro0NL you mean configuring them ? |
ro0NL commentedAug 11, 2016 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
GuilhemN commentedAug 11, 2016
@ro0NL the kernel is the root of the project, a bundle isn't. |
ro0NL commentedAug 11, 2016 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
Understand, im talking about registering (mounting) |
GuilhemN commentedAug 11, 2016
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. |
ro0NL commentedAug 11, 2016
Asusual? // 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 commentedAug 11, 2016
@ro0NL ok got it, you would like to be able to use the bundle class as a resource. |
sstok commentedAug 12, 2016 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
@ro0NL Does this bundle solve your use-casehttps://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. |
ro0NL commentedAug 12, 2016 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
I guess for now i would workaround using the kernel as a service, which couples perhaps better anyway. // SomeBundlepublicfunctiongetRoutes() {returnnewRouteCollection(/*...*/);}// KernelpublicfunctionloadRoutes(LoaderInterface$loader) {$bundleRoutes =$loader->import('kernel:getSomeBundleRoutes','service');$bundleRoutes->addPrefix('/some-bundle');}publicfunctiongetSomeBundleRoutes() {return$this->getBundle('Some')->getRoutes();} Im not even sure it's supported though... importing routes from a service? |
| * | ||
| * @author Guilhem N. <egetick@gmail.com> | ||
| */ | ||
| abstractclass MicroBundleextends Bundleimplements ExtensionInterface, ConfigurationExtensionInterface, ConfigurationInterface |
There was a problem hiding this comment.
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 aConfigurationInterface.. we can eventually just callProcessor::process...
Instead of that, implementingPrependExtensionInterface could be useful.
There was a problem hiding this comment.
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 :)
There was a problem hiding this comment.
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 asConfigDebugCommand andConfigDumpReferenceCommand (should be at least, they won't work if the extension doesn't implement this interface).
And asConfigurationExtensionInterface returns an instance ofConfigurationInterface, theMicroBundle must be an instance ofConfigurationInterface.
AboutPrependExtensionInterface 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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
And what aboutCompilerPassInterface?
There was a problem hiding this comment.
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 commentedSep 16, 2016
Would such feature be accepted? |
ro0NL commentedSep 16, 2016
Imo it should be, as it's the missing part of the micro kernel right now. |
fabpot commentedSep 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 commentedSep 21, 2016 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
@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 commentedSep 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 commentedSep 21, 2016
@fabpot i understand but i don't think micro apps are the biggest target here. |
fabpot commentedSep 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 commentedSep 21, 2016
@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 commentedSep 21, 2016 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
Disagree, i truly consider a micro app built from simple bundles.
Separation of concerns and reuseability.
They are :) In the sense that a kernel can load bundles. However naming it micro could cause users to think it'srequired to have micro bundles, when using a micro kernel. Which indeed is not the case. Maybe we can do better naming... edit: |
| /** | ||
| * {@inheritdoc} | ||
| */ | ||
| publicfunctiongetNamespace() |
There was a problem hiding this comment.
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 withBundle::getNamespace, perhaps it works.. but the intention is different. What about not implementingExtensionInterface, but rather return a generic/callback extension fromMicroBundle::getContainerExtension. WDYT?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
8866522 toa1d3431Compare| { | ||
| } | ||
| protectedfunctionbuildContainer(array$config,ContainerBuilder$container) |
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
This follows theExtension::load signature but both are fine for me.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Maybe it should be calledloadContainer then, to clarify a bit more and dont create too much confusion withBundle::build.
There was a problem hiding this comment.
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) whileBundle::buildContainer builds the container (which happens much later in the kernel life).
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 aboutconfigureBundle ? orconfigureContainer ?
| private$bundle; | ||
| private$buildContainer; | ||
| publicfunction__construct(SimpleBundle$bundle,Closure$buildContainer) |
There was a problem hiding this comment.
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 :)
There was a problem hiding this comment.
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 useClosure::bind, i hesitated.
There was a problem hiding this comment.
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 toSimpleBundle, not bridging it with a closure, ie. you cant abuse it.
| return$treeBuilder; | ||
| } | ||
| protectedfunctionbuildConfiguration(NodeParentInterface$rootNode) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Can beArrayNodeDefinition which is probably more convenient.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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 useTreeBuilder i don't think there's an use case to use something else anyway.
| finalpublicfunctiongetConfigTreeBuilder() | ||
| { | ||
| $treeBuilder =newTreeBuilder(); | ||
| $rootNode =$treeBuilder->root($this->getAlias()); |
There was a problem hiding this comment.
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 overrideSimpleExtension::getAlias and create a unique alias for each simple bundle class.
| publicfunction__construct(SimpleBundle$bundle,Closure$buildContainer) | ||
| { | ||
| $this->bundle =$bundle; | ||
| $this->buildContainer = Closure::bind(function (array$config,ContainerBuilder$container) { |
There was a problem hiding this comment.
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}. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
{@link SimpleBundle}
24180d1 to6128e57Compare| // check naming convention | ||
| $basename =preg_replace('/Bundle$/','',$this->bundle->getName()); | ||
| return Container::underscore($basename); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Container is not imported :)
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 👍
There was a problem hiding this comment.
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; | ||
| } | ||
| protectedfunctionbuildConfiguration(TreeBuilder$rootNode) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
$rootNode is not of typeTreeBuilder ;-) I'd preferArrayNodeDefinition (or the interface otherwise) and letgetConfigTreeBuilder root with the right alias.
| { | ||
| } | ||
| protectedfunctionbuildContainer(array$config,ContainerBuilder$container) |
There was a problem hiding this comment.
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 withBundle::build(ContainerBuilder $container).
I think it makes more sense to haveSimpleBundle::loadExtension (as we're always called fromSimpleExtension::load).
What about configureBundle ? or configureContainer ?
We really should consider havingBundle::build already and what is best DX. So.., it's called fromKernel::prepareContainer, which is called fromKernel::buildContainer. Ie. it's easy to assumebuild already acts asbuildContainer, it's just poor naming imo.
However, in that spirit, we could also go with simplySimpleBundle::load. Orconfigure from your pov.
There was a problem hiding this comment.
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 toResources/config?
Then imo it would be more appropriate to name this methodload.
| { | ||
| $config =$this->processConfiguration($this->bundle,$configs); | ||
| $f = Closure::bind(function (array$config,ContainerBuilder$container) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Should be\Closure.
| } | ||
| protectedfunctionbuildContainer(array$config,ContainerBuilder$container) | ||
| protectedfunctionload(array$config,ContainerBuilder$container,LoaderInterface$loader) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Love it :D
GuilhemN commentedOct 26, 2016 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
@ro0NL thanks for your reviews, I fixed your last comments :) The last version injects a loader in class AppBundleextends SimpleBundle{protectedfunctionload(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 |
ro0NL commentedOct 26, 2016 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
We could also consider |
GuilhemN commentedOct 26, 2016 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
@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 commentedOct 26, 2016 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
Yeah.. like This path is usually user-defined (as symfony is flexible).. not sure introducing some special new path (i cant think of name either.. |
ro0NL commentedDec 29, 2016 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
@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 commentedDec 29, 2016 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
@ro0NL why would we need to update the config in a bundle ? A bundle should only configure services/parameters. |
ro0NL commentedDec 29, 2016 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
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 commentedMar 1, 2017
Still 👎 on adding this in core. ping @symfony/deciders |
jakzal commentedMar 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. |
stof commentedMar 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 |
Uh oh!
There was an error while loading.Please reload this page.
This PR introduces a new class:
MicroBundle.It is a all-in-one class replacing the usual
Bundle-Extension-Configurationscheme by a simple interface that is enough for many app bundles:This api is strongly inspiredfrom the
Bundleclass of theKnpRadBundle.