Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
Adding a MicroKernel that holds service configuration#15820
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
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.
does this actually need to be exposed ?
weaverryan commentedSep 17, 2015
Agreed on not exposing the Loader - I've just fixed both issues! |
weaverryan commentedSep 18, 2015
2cc23d7 to6278ecaCompareweaverryan commentedSep 18, 2015
I've just drastically simplified this PR and updated the description to describe it. This is ready for review! |
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 should maybe be an interface, but the class is so simple and we just use it to fetch the ContainerBuilder andreal Loader out, so I don't know if there's a real-use case to pass other concrete implementation in.
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 agree with you here
dupuchba commentedSep 18, 2015
@weaverryan can you give me a use case on why you would choose to have a new kernel whose goal is to be able to build an application without any external files ? Thank you :-) |
weaverryan commentedSep 18, 2015
@dupuchba Good question. Here are some reasons:
I think this is superior to the normal Cheers! |
dupuchba commentedSep 18, 2015
@weaverryan you convinced me as at work we are currently migrating to a micro service architecture. Very interesting project! |
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 implementing these methods with an empty body, so that you only need to overwrite the ones you need ?
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.
good idea - I like that better
weaverryan commentedSep 22, 2015
ping @symfony/deciders |
dunglas commentedSep 23, 2015
👍 another step in the right direction! |
xabbuh commentedSep 26, 2015
Probably I miss an important point here, but I don't see right now how this really makes things much easier. @weaverryan Can you show a simple example on how I would use this in a real application? |
weaverryan commentedSep 26, 2015
@xabbuh I have a (somewhat complex) kernel example here (+route integration, which isn't part of this PR, but you can see how the kernel could handle both):https://gist.github.com/weaverryan/e4b19cabb1d9286c4217 Adding services in php is certainly not objectively better, but I think the approach of Here's the big win for me: End users would now implement Make sense? |
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.
Usually we put public methods before protected ones. Is there a special reason to keep this method here (for example, to make it easier to spot theconfigureExtensions() andconfigureServices() methods)?
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.
that was exactly my thinking (making them more visible) - but I'd be happy to change the order if this is a hard rule we want to stick to
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.
actually, I just changed the order :)
xabbuh commentedSep 27, 2015
@weaverryan Thanks for the example. I think I now understand your approach better. :) Besides the minor comments I left, I like this approach. 👍 |
weaverryan commentedSep 27, 2015
@xabbuh thanks for the review! |
xabbuh commentedSep 28, 2015
👍 Status: Reviewed |
…xtensions) can be configured direclty inside.
fabpot commentedSep 29, 2015
What about this? <?phpnamespaceSymfony\Component\HttpKernel;useSymfony\Component\Config\Loader\LoaderInterface;useSymfony\Component\DependencyInjection\ContainerBuilder;useSymfony\Component\DependencyInjection\ContainerInterface;abstractclass MicroKernelextends Kernel{publicfunctionregisterContainerConfiguration(LoaderInterface$loader) {$loader->load(function (ContainerBuilder$container)use ($loader) {$this->configureExtensions($container,$loader);$this->configureServices($container,$loader); }); }protectedfunctionconfigureExtensions(ContainerBuilder$c,LoaderInterface$loader) { }protectedfunctionconfigureServices(ContainerBuilder$c,LoaderInterface$loader) { }} |
fabpot commentedSep 29, 2015
I'm going to submit a PR soon with my own version of a micro-kernel. |
fabpot commentedSep 29, 2015
see#15990 |
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.
We should probably add:
$loader->addResource(newFileResource(__FILE__));
fabpot commentedOct 1, 2015
Closing this one as#15990 provides an easier approach. |
This PR was merged into the 2.8 branch.Discussion----------added a micro kernel| Q | A| ------------- | ---| Bug fix? | no| New feature? | yes| BC breaks? | no| Deprecations? | no| Tests pass? | yes| Fixed tickets | n/a| License | MIT| Doc PR | n/aRelated to#15948 and#15820Commits-------eab0f0a added a micro kernel
Hi guys!
This introduces a new kernel whose goal is to be able to build an application without any external files (and to make loading external files easier, since conditional logic can be in PHP).
This allows you to register services and configure DI extensions right in the kernel. With#15742, this (or a sub-class) would be updated to also allow routes to be configured inside the kernel as well.
There is really only one goal of this PR:
Thanks!