Skip to content

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 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 2 commits into from

Conversation

weaverryan
Copy link
Member

Q A
Bug fix? no
New feature? yes
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets n/a
License MIT
Doc PR not yet

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:

  1. Allow services/extensions to be configured in the kernel.

Thanks!

return $this->containerBuilder;
}

public function getResourceLoader()
Copy link
Member

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
Copy link
Member Author

Agreed on not exposing the Loader - I've just fixed both issues!

@weaverryan
Copy link
Member Author

@stof I went backwards and re-added the getResourceLoader at sha: 92e419d. I did this so that I could pass the original LoaderInterface to configureServices(), instead of the wrapper ContainerBuilderAwareLoader, which could be more useful in edge cases.

@weaverryan weaverryan force-pushed the micro_kernel branch 3 times, most recently from 2cc23d7 to 6278eca Compare September 18, 2015 13:28
@weaverryan
Copy link
Member Author

I've just drastically simplified this PR and updated the description to describe it. This is ready for review!

*/
public function registerContainerConfiguration(LoaderInterface $loader)
{
if (!$loader instanceof ContainerBuilderAwareLoader) {
Copy link
Member Author

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 and real Loader out, so I don't know if there's a real-use case to pass other concrete implementation in.

Copy link
Member

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

@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
Copy link
Member Author

@dupuchba Good question. Here are some reasons:

  1. Remove the idea that Symfony is big and complex. In reality, we're very few steps from being able to create an entire app (services & routes) in a single file. That's a powerful concept to illustrate.

  2. Make Symfony better for creating micro-services and prototyping.

  3. This makes configuration simpler. You can load YAML files like normal, but you also have the option to use logic in here for configuration, which allows you to have environment-specific behavior, without the config_ENV.yml setup (which works well, but is overhead if you want to keep things small).

  4. Easier to submit bug reports - instead of cloning the SE, you could build a tiny project with this one file (+ whatever other classes, e.g. form types, needed for the bug).

I think this is superior to the normal Kernel. If the SE used this, nothing would change, except that users would be passed the ContainerBuilder and have the option to configure it in PHP (this is possible now with the ClosureLoader, but not obvious).

Cheers!

@dupuchba
Copy link
Contributor

@weaverryan you convinced me as at work we are currently migrating to a micro service architecture. Very interesting project!

* @param ContainerBuilder $c
* @param LoaderInterface $loader
*/
abstract protected function configureServices(ContainerBuilder $c, LoaderInterface $loader);
Copy link
Member

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 ?

Copy link
Member Author

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
Copy link
Member Author

ping @symfony/deciders

@dunglas
Copy link
Member

dunglas commented Sep 23, 2015

👍 another step in the right direction!

@xabbuh
Copy link
Member

xabbuh commented Sep 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
Copy link
Member Author

@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 configureServices is better than what we have now - I listed some reasons earlier: #15820 (comment).

Here's the big win for me: End users would now implement configureServices instead of registerContainerConfiguration and have the option to load an external file (like before), or add services directly (or load external files and then use some php logic to mutate those definitions, e.g. for environment differences).

Make sense?

*
* @param LoaderInterface $loader
*/
public function registerContainerConfiguration(LoaderInterface $loader)
Copy link
Member

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 the configureExtensions() and configureServices() methods)?

Copy link
Member Author

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

Copy link
Member Author

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

xabbuh commented Sep 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
Copy link
Member Author

@xabbuh thanks for the review!

@xabbuh
Copy link
Member

xabbuh commented Sep 28, 2015

👍

Status: Reviewed

@fabpot
Copy link
Member

fabpot commented Sep 29, 2015

What about this?

<?php

namespace Symfony\Component\HttpKernel;

use Symfony\Component\Config\Loader\LoaderInterface;
use Symfony\Component\DependencyInjection\ContainerBuilder;
use Symfony\Component\DependencyInjection\ContainerInterface;

abstract class MicroKernel extends Kernel
{
    public function registerContainerConfiguration(LoaderInterface $loader)
    {
        $loader->load(function (ContainerBuilder $container) use ($loader) {
            $this->configureExtensions($container, $loader);
            $this->configureServices($container, $loader);
        });
    }

    protected function configureExtensions(ContainerBuilder $c, LoaderInterface $loader)
    {
    }

    protected function configureServices(ContainerBuilder $c, LoaderInterface $loader)
    {
    }
}

@fabpot
Copy link
Member

fabpot commented Sep 29, 2015

I'm going to submit a PR soon with my own version of a micro-kernel.

@fabpot
Copy link
Member

fabpot commented Sep 29, 2015

see #15990

}

$this->configureExtensions($loader->getContainerBuilder(), $loader->getResourceLoader());
$this->configureServices($loader->getContainerBuilder(), $loader->getResourceLoader());
Copy link
Member

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(new FileResource(__FILE__));

@fabpot fabpot mentioned this pull request Sep 29, 2015
@fabpot
Copy link
Member

fabpot commented Oct 1, 2015

Closing this one as #15990 provides an easier approach.

@fabpot fabpot closed this Oct 1, 2015
@weaverryan weaverryan deleted the micro_kernel branch October 1, 2015 18:18
fabpot added a commit that referenced this pull request Nov 5, 2015
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/a

Related to #15948 and #15820

Commits
-------

eab0f0a added a micro kernel
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.

7 participants