Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

[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

Closed
GuilhemN wants to merge1 commit intosymfony:masterfromGuilhemN:BUNDLE

Conversation

@GuilhemN
Copy link
Contributor

@GuilhemNGuilhemN commentedAug 11, 2016
edited
Loading

QA
Branch?"master"
Bug fix?no
New feature?yes
BC breaks?no
Deprecations?no
Tests pass?yes
Fixed tickets
LicenseMIT
Doc PR

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

useSymfony\Component\Config\Definition\Builder\NodeParentInterface;useSymfony\Component\DependencyInjection\ContainerBuilder;useSymfony\Component\HttpKernel\Bundle\MicroBundle;class AppBundleextends MicroBundle{protectedfunctionbuildConfiguration(NodeParentInterface$rootNode)    {$rootNode            ->children()                ->scalarNode('foo')->defaultValue('bar')->end()            ->end();    }protectedfunctionbuildContainer(array$config,ContainerBuilder$container)    {$container->setParameter('app.foo',$config['foo']);    }}

This api is strongly inspiredfrom theBundle class of theKnpRadBundle.

ro0NL, Koc, jameshalsall, tyx, shouze, and yceruto reacted with thumbs up emoji
@ro0NL
Copy link
Contributor

Can we make route definitions available?

@GuilhemN
Copy link
ContributorAuthor

@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 commentedAug 11, 2016
edited
Loading

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

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

@GuilhemN
Copy link
ContributorAuthor

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

@ro0NL
Copy link
Contributor

ro0NL commentedAug 11, 2016
edited
Loading

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

@GuilhemN
Copy link
ContributorAuthor

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

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
Copy link
ContributorAuthor

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

ro0NL and apfelbox reacted with thumbs up emoji

@sstok
Copy link
Contributor

sstok commentedAug 12, 2016
edited
Loading

@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.
I have seen projects with a simple Extension class to load only simple XML services file 😝

@ro0NL
Copy link
Contributor

ro0NL commentedAug 12, 2016
edited
Loading

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
Copy link
Contributor

@ro0NLro0NLAug 13, 2016
edited
Loading

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.

Copy link
ContributorAuthor

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
ContributorAuthor

@GuilhemNGuilhemNAug 14, 2016
edited
Loading

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.

ogizanagi and ro0NL reacted with thumbs up emoji
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 aboutCompilerPassInterface?

Copy link
ContributorAuthor

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

ro0NL reacted with thumbs up emoji
@GuilhemN
Copy link
ContributorAuthor

Would such feature be accepted?

@ro0NL
Copy link
Contributor

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

GuilhemN reacted with thumbs up emoji

@fabpot
Copy link
Member

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
ContributorAuthor

GuilhemN commentedSep 21, 2016
edited
Loading

@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

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
ContributorAuthor

@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

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
ContributorAuthor

@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 commentedSep 21, 2016
edited
Loading

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'srequired 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}
*/
publicfunctiongetNamespace()
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 withBundle::getNamespace, perhaps it works.. but the intention is different. What about not implementingExtensionInterface, but rather return a generic/callback extension fromMicroBundle::getContainerExtension. WDYT?

Copy link
ContributorAuthor

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
ContributorAuthor

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

{
}

protectedfunctionbuildContainer(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
ContributorAuthor

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.

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 calledloadContainer then, to clarify a bit more and dont create too much confusion withBundle::build.

Copy link
ContributorAuthor

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

Copy link
Contributor

@ro0NLro0NLOct 20, 2016
edited
Loading

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
ContributorAuthor

@GuilhemNGuilhemNOct 20, 2016
edited
Loading

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)
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
ContributorAuthor

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.

Copy link
Contributor

@ro0NLro0NLOct 20, 2016
edited
Loading

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)
Copy link
Contributor

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.

Copy link
ContributorAuthor

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
ContributorAuthor

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());
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 overrideSimpleExtension::getAlias and create a unique alias for each simple bundle class.

GuilhemN reacted with thumbs up emoji
publicfunction__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 ;-)

GuilhemN reacted with thumbs up emoji
}

/**
* 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 reacted with thumbs up emoji
@GuilhemNGuilhemNforce-pushed theBUNDLE branch 2 times, most recently from24180d1 to6128e57CompareOctober 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 :)

GuilhemN reacted with thumbs up emoji
Copy link
ContributorAuthor

@GuilhemNGuilhemNOct 25, 2016
edited
Loading

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
ContributorAuthor

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)
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 typeTreeBuilder ;-) I'd preferArrayNodeDefinition (or the interface otherwise) and letgetConfigTreeBuilder root with the right alias.

{
}

protectedfunctionbuildContainer(array$config,ContainerBuilder$container)
Copy link
Contributor

@ro0NLro0NLOct 23, 2016
edited
Loading

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.

Copy link
ContributorAuthor

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) {
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.

GuilhemN reacted with thumbs up emoji
}

protectedfunctionbuildContainer(array$config,ContainerBuilder$container)
protectedfunctionload(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 reacted with hooray emoji
@GuilhemN
Copy link
ContributorAuthor

GuilhemN commentedOct 26, 2016
edited
Loading

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

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

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 toResources/config is hardcoded but I'd like to create a new method inSimpleBundle allowing to customize it. I just don't know what to call it...

@ro0NL
Copy link
Contributor

ro0NL commentedOct 26, 2016
edited
Loading

SimpleBundle::__construct?

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

@GuilhemN
Copy link
ContributorAuthor

GuilhemN commentedOct 26, 2016
edited
Loading

@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 commentedOct 26, 2016
edited
Loading

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 commentedDec 29, 2016
edited
Loading

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

GuilhemN commentedDec 29, 2016
edited
Loading

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

@ro0NL
Copy link
Contributor

ro0NL commentedDec 29, 2016
edited
Loading

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 :)

GuilhemN reacted with thumbs up emoji

@fabpot
Copy link
Member

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

@jakzal
Copy link
Contributor

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.

@fabpotfabpot closed thisMar 1, 2017
@stof
Copy link
Member

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

@GuilhemNGuilhemN deleted the BUNDLE branchMarch 3, 2017 16:03
@nicolas-grekasnicolas-grekas modified the milestones:3.x,3.3Mar 24, 2017
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment

Reviewers

1 more reviewer

@ro0NLro0NLro0NL left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Projects

None yet

Milestone

3.3

Development

Successfully merging this pull request may close these issues.

9 participants

@GuilhemN@ro0NL@sstok@fabpot@jakzal@stof@hason@nicolas-grekas@carsonbot

[8]ページ先頭

©2009-2025 Movatter.jp