Skip to content

added a micro kernel #15990

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

Merged
merged 1 commit into from
Nov 5, 2015
Merged

added a micro kernel #15990

merged 1 commit into from
Nov 5, 2015

Conversation

fabpot
Copy link
Member

@fabpot fabpot commented Sep 29, 2015

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

* @author Ryan Weaver <ryan@knpuniversity.com>
* @author Fabien Potencier <fabien@symfony.com>
*/
abstract class MicroKernel extends Kernel
Copy link
Member Author

Choose a reason for hiding this comment

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

MicroKernel is probably not the right class name, it's just a regular Kernel with some method helpers.

Copy link
Member

Choose a reason for hiding this comment

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

Hmm, yes. Will we need to split this class into 2 parts (with service stuff in HttpKernel and route stuff in FrameworkBundle)?

Copy link
Member

Choose a reason for hiding this comment

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

I can't think of a good name, so let me throw out some not great names for brainstorming:

  • ConfigurableKernel
  • CompleteKernel
  • AutonomousKernel
  • BuildableKernel

Copy link
Member

Choose a reason for hiding this comment

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

I love MicroKernel name. I cannot think of a better alternative :)

Copy link
Member Author

Choose a reason for hiding this comment

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

It's not about a sexy name, but having a descriptive one; there is nothing Micro here.

Copy link
Member

Choose a reason for hiding this comment

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

I like ConfigurableKernel

Copy link
Contributor

Choose a reason for hiding this comment

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

ConfigurableKernel is cool 👍

What about StandardKernel or SimpleKernel ? Even though I'd vote for ConfigurableKernel :)

Copy link
Member

Choose a reason for hiding this comment

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

If this is "the" ConfigurableKernel ... does that mean that the other kernels are not configurable?

Copy link
Contributor

Choose a reason for hiding this comment

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

This Kernel is abstract. Why not AbstractKernel then?

Copy link
Member

Choose a reason for hiding this comment

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

Kernel is abstract as well...

@weaverryan
Copy link
Member

Obviously, there are a few minor todos as you mentioned, but this accomplishes all of the goals of my other PR's that you listed, but indeed, in a much simpler way (and a smaller code footprint). I'm very pleased by this version!

$this->configureExtensions($container, $loader);
$this->configureServices($container, $loader);

$loader->addResource(new FileResource(__FILE__));
Copy link
Member

Choose a reason for hiding this comment

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

I think we need this to include the sub-class:

$container->addObjectResource($this);

@fabpot fabpot force-pushed the micro-kernel branch 3 times, most recently from 9573350 to fc24976 Compare September 29, 2015 16:25

return $this->configureRoutes(new RouteCollectionBuilder($loader))->build();
});
}
Copy link
Member

Choose a reason for hiding this comment

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

This starts to feel a bit worse than the ServiceRouteLoader approach, which also had some supporters for other use-cases. If there were a way to pull this off in a nice way without any new code, that would be great. But otherwise, I like #15742

Copy link
Member

Choose a reason for hiding this comment

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

I updated that PR, and have a suggestion that would remove the need for putting the RouteLoaderInterface on the kernel: https://github.com/symfony/symfony/pull/15742/files#r40755658

@fabpot fabpot force-pushed the micro-kernel branch 2 times, most recently from 75daaa2 to d140c61 Compare October 1, 2015 13:41
@weaverryan
Copy link
Member

The updated routing method looks good to me. 👍 in general - except for the name of course :)

});
}

private function loadRoutes(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.

I think this will need to be public

@weaverryan
Copy link
Member

@fabpot if we want some tests (there's not much to test, but there could be some) or other changes and I can help - let me know and I can steal this PR and make those changes (and take all the credit!)

@fabpot fabpot force-pushed the micro-kernel branch 2 times, most recently from 05d7106 to 8a94c87 Compare October 1, 2015 20:51
@fabpot
Copy link
Member Author

fabpot commented Oct 1, 2015

Here is a small usage example:

<?php

require_once __DIR__.'/../vendor/autoload.php';

use Symfony\Component\Config\Loader\LoaderInterface;
use Symfony\Component\Debug\Debug;
use Symfony\Component\DependencyInjection\ContainerBuilder;
use Symfony\Component\HttpFoundation\Request;
use Symfony\Component\HttpFoundation\Response;
use Symfony\Component\HttpKernel\MicroKernel;
use Symfony\Component\Routing\RouteCollectionBuilder;

class AppKernel extends MicroKernel
{
    public function home()
    {
        return new Response('Foo');
    }

    public function registerBundles()
    {
        return array(
            new Symfony\Bundle\FrameworkBundle\FrameworkBundle(),
        );
    }

    protected function configureRoutes(RouteCollectionBuilder $routes)
    {
        $routes->add('/', 'kernel:home');
    }

    protected function configureServices(ContainerBuilder $c, LoaderInterface $loader)
    {
        $c->setParameter('kernel.secret', 'foo');
    }
}

$kernel = new AppKernel('dev', true);
$request = Request::createFromGlobals();
$response = $kernel->handle($request);
$response->send();
$kernel->terminate($request, $response);

I'm not quite satisfied as we cannot use a closure for controllers (as Symfony dumps the routes as a PHP file).

@fabpot fabpot changed the title [WIP] added a micro kernel added a micro kernel Oct 1, 2015
@fabpot
Copy link
Member Author

fabpot commented Oct 1, 2015

Question: is it really something we want to package in Symfony itself. As you can see, the code is quite minimal, so isn't it something for a small/micro distribution? Or part of a tutorial in the docs? For me, that's more like a starting point rather than some kind of infrastructure that needs to be standardized.

@xabbuh
Copy link
Member

xabbuh commented Oct 1, 2015

If this is something that will be asked for frequently, I would say yes (personally I don't see any use cases for myself right now). The code that would reside inside the core looks good so far.

@fabpot
Copy link
Member Author

fabpot commented Oct 1, 2015

The fact that closures cannot be used defeats the initial purpose (at least for me). With closure support, that would make it something interesting for small websites, APIs, things that Silex is good at.

@weaverryan
Copy link
Member

@fabpot I want the code somewhere where people can just use it, so people can get a minimal Symfony project up and running really quickly and with minimal setup. If we make the user do that boilerplate, it defeats the purpose.

My vote is for core because I want say: "you can create a symfony app by requiring symfony/symfony and creating a single file that extends MicroKernel" (or whatever we call it).

Also, I think the kernel should be used in the SE: I think loads things in a way that's more clear, but basically works the same (you could easily remove routing_dev.yml for example).

And finally, yes, Silex is awesome. But it doesn't have bundle support, which means I forfeit a lot of solutions I might be accustomed to using. A Silex app also can't evolve into a full Symfony app very easily if it grows. The idea behind this was - as much as possible - to have my cake and eat it too: get full Symfony, but get it small.

Anyways, I see your point - after all, this class is quite small - but that's my vote. We could move it into FrameworkBundle if we want to hide it a bit from the core components (especially since it requires FrameworkBundle.

@weaverryan
Copy link
Member

Also, I don't envision that you'll normally have just one file, though that ability is cool. More that it's easy to create a minimal application easily: a kernel, controller classes, services and some configuration files if you choose.

@wouterj
Copy link
Member

wouterj commented Oct 2, 2015

I don't like the idea of using this in the Standard Edition. The Standard Edition should be the base template for common Symfony applications, which tend to be quite advanced. This still is a nice feature, but for 20% or so of the Symfony developers.

What about creating a new package, like symfony/micro, which requires symfony/symfony and this class. Then you can say "Require the symfony/micro package, use the MicroKernel class and you're ready".

@henrikbjorn
Copy link
Contributor

@weaverryan assume i have several routing files i want added, if both have the same prefix i assume the latter will override the former?

@weaverryan
Copy link
Member

@henrikbjorn it uses the same mechanism as normal route importing. So if you import 5 routes from admin.yml and 5 routes from admin2.yml - both with the prefix /admin, and you will end up with 10 routes. The only time later routes would override earlier routes is if they have the same route name - the prefix won't matter with that.

@wouterj
Copy link
Member

wouterj commented Nov 5, 2015

Maybe we should allow mounting without a prefix?

@weaverryan
Copy link
Member

Before I answer that, what is the problem with the current implementation? :) I have some small things that I don't think are necessarily perfect, but I only want to change things if people like @henrikbjorn say "Hey, I was confused by X, it would have been more clear if I could just call Y".

@wouterj
Copy link
Member

wouterj commented Nov 5, 2015

Let's describe what I think (as a non-native english speaker) the discussed function names mean:

  • mount() bind 2 things (first and second argument) together
  • import() includes something into the current stack

It turns out mount() doesn't mount argument 1 and 2, it mounts argument 2 in the current route collection with an optional prefix defined by 1. If I didn't knew the Yaml syntax behind this (a resource with a prefix option), I would actually think that I cannot bind 2 different route collections to the same mountpoint.

Furthermore, it turns out import actually doesn't do anything more than loading and returning what it just loaded. The thing you loaded now has to be attached/mounted to the main route collection.

@weaverryan
Copy link
Member

About mount(), it's exactly as it is in Silex (http://silex.sensiolabs.org/doc/organizing_controllers.html). So if it hasn't been a problem there for people, we can assume it won't be a problem here (if it has been a problem, we should't repeat it).

About import(), I share your concerns. However, if you automatically put the new routes into the RouteCollectionBuilder, then you're not able to modify then further - e.g. add defaults or requirements to the routes in just that collection. Again, this idea comes from Silex (http://silex.sensiolabs.org/doc/providers.html#controller-providers), though it's a bit more obvious there, because you create new class, so it's obvious that it's not being imported until you actually add it to your app. Is it as simple as calling import() something else? Or making import() actually import and adding a less-used method that returns the new RouteCollectionBuilder without mounting it?

@wouterj
Copy link
Member

wouterj commented Nov 5, 2015

We can rename it to load() or import the route collection and return the imported route collection, allowing to change the instance. (this is done with registering Container definitions for instance)

@weaverryan
Copy link
Member

@wouterj you're right - I didn't think about that - that's great :). See #16477.

@henrikbjorn
Copy link
Contributor

@weaverryan i think the confusing part for me was the $prefix in mount is the first argument, and i would the assume it would be mandatory and therefor override my ealier prefix. But if the argument order was different eg mount($routes, $prefix = null) then i would make sense.

But that would proberly be a bigger BC break than your proposed solution.

Also a lot of the Bundles seems they are geared very much at usage in a directory structure that is the same as the standard edition which is a shame (eg app/Resources/views)

edit: my playground thingy https://github.com/henrikbjorn/Muse

@janit
Copy link

janit commented Nov 6, 2015

All I see is references to 2.8. Just verifying that this will this be in 3.0 too?

@Pierstoval
Copy link
Contributor

It will be merged in the next "merge 2.8 on 3.0" I guess :)

@SoboLAN
Copy link

SoboLAN commented Nov 8, 2015

@fabpot I see this was merged in 2.8 branch. But 2.8 requires PHP 5.3.9, while traits are only available in PHP 5.4+ .

Are you sure it's correct to have this in 2.8 ?

@wouterj
Copy link
Member

wouterj commented Nov 8, 2015

@SoboLAN this is an additional feature. Symfony doesn't require you to use this feature, so the minimum requirement is still 5.3.9.

If you want to use this feature, you have to use PHP 5.4.

@SoboLAN
Copy link

SoboLAN commented Nov 8, 2015

@wouterj You could say that about most of the functionality provided by Symfony.

If a minimum PHP version is specified, then I expect that absolutely everything will run without problems if I use Symfony on that PHP version (or newer). That's the whole point of why Composer allows specifying something like this.

I think it's a very reasonable assumption. Why ? Because everyone who has some restrictions on production systems (and most everyone has) and needs to decide what Symfony version to use, will rely on that 1 line in composer.json (which specifies minimum PHP version) to make his decision. It is the single most important factor in these kind of decisions.

Not respecting that is kind of alarming for me because I can never know what to expect. I would constantly ask myself:

If I use this method in my project, will it work in production servers ? (where I have a locked and usually older PHP version than on my dev machine)

If you (the deciders of Symfony) strongly believe that it should be like this, then fine. Either way I have no authority to dictate anything around here. But think about it: if you do this now, then where do you draw the line ? If you go down this path, where do you stop ?

I think that it's a dangerous precedent because it takes away all the confidence in that requires PHP version X statement.

Without that confidence, what is a developer or devops going to do ? What if he has a big Symfony project (or 20 projects) in his production systems ? Test absolutely every functionality when he makes a routine patch update (e.g. 2.6.9 -> 2.6.10) ? Just to be sure that what is written in composer.json is not a lie and that his website won't blow up ?

@wouterj
Copy link
Member

wouterj commented Nov 8, 2015

There are many packages in the suggest sections of Symfony's framework, components and bundles. These are all libraries that have some kind of integration in the component/bundle, but aren't required for the main usage (for instance, doctrine/annotations is required when using annotations with the Validator component, but as you can use Yaml/Xml/PHP, it's in the suggest section).

Also, 95% of the people that are going to use Symfony are not going to use this trait. It's a really nice feature, but more to advocate Symfony ("Symfony isn't that massive huge framework you think it is!") and for some people that are not going to base on top of the Standard Edition (and most people installing 2.8 will stick with the Standard Edition).

At last, it's very clear (or debuggable within 30 seconds) that this feature requires PHP 5.4 as you need to use the trait yourself. A developer using this feature on a PHP 5.3 server will either know the trait feature and identifies it as something that requires PHP 5.4 or will get an error message, search for "php trait" on google and find out that it requries PHP 5.4. Furthermore, I'm sure the documentation about this feature will mention the PHP 5.4 requirement "above the fold" of the article.

So for these reasons, I think it's safe to put this PHP-5.4-requiring-feature in Symfony 2.8. I don't think you need to be afraid that Symfony will cross the line many times in the future. The Symfony framework has a core team that's very focussed on it's usage (with the BC promise, easy upgrade paths, etc. as results).

At last, I want to thank you for sharing your worries in such a clear and long comment!

@weaverryan
Copy link
Member

I agree with Wouter - this is a very special situation - so don't worry @SoboLAN, we take this thing quite seriously.

Btw, nothing depends on this class in Symfony, it's a total extra feature. If you want it on 5.3, you can safely copy this class and tweak it to work with 5.3.

Cheers!

@Pierstoval
Copy link
Contributor

Plus, to add another argument, this feature is mostly for new projects, and I doubt that developers creating new projects will use an old PHP version.

@psampaz
Copy link
Contributor

psampaz commented Nov 8, 2015

I totally agree with @SoboLAN.

@javiereguiluz
Copy link
Member

@wouterj @weaverryan what you say is true and reasonable. But at the same time I find @SoboLAN's comments very reasonable too. If some code claims 5.3 compatibility, I understad that it's for its entire codebase.

@Pierstoval
Copy link
Contributor

gedmo/doctrine-extensions has a PHP requirement of >=5.3.2 and it has traits. If you don't want to use them, you don't have to, and your code won't break.

It's been already said:

  • 95% of the people that are going to use Symfony are not going to use this trait
  • nothing depends on this class in Symfony, it's a total extra feature
  • this feature is mostly for new projects

For me, these reasons are enough to keep it in the 2.8 branch

@Tobion
Copy link
Contributor

Tobion commented Nov 8, 2015

Btw we already had traits in symfony before (ContainerAwareTrait). Not sure what the goal of this discussion is. Nobody forces you to use it.

@SoboLAN
Copy link

SoboLAN commented Nov 10, 2015

@Pierstoval @Tobion I totally understand what you're saying. No one is making you use these features.

But the problem in these situations is that you may use some classes/functionalities without knowing it.

Let's take a simple example and say that I have the following line in one of my Controllers:

$this->get('validator')->validate($someObject, array('group1', 'group2'));

I'm not using what I'm not supposed to. But, behind the scenes, how much is going on in order to validate that object ? You have to:

  • build the validator object (which is not very simple)
  • let's say read a validation.yml file
  • interpret the file
  • have the Validator go through it and determine which constraints contain those specified groups
  • apply those constraints
  • build violation errors
  • etc.

There are probably thousands of lines of code being executed inside that 1 line from above.

If somewhere in all that code you add code (like for example a trait) that is not compatible with what PHP version you specify in composer, then my project will maybe fail, usually in ways that can be difficult to reproduce.

@Pierstoval @Tobion I only presented above the reason for why this whole discussion started.

@wouterj assured me that something like this would never happen. I hope he's right. I'm very familiar with Symfony's promise to have excellent backwards-compatibility. I will just have faith that bad scenarios like the one above will never happen :) .

I hope I did not piss people off with my concerns. If I did, it was not my intention. I was just somewhat worried.

@nicolas-grekas
Copy link
Member

@SoboLAN Symfony does not rely on the MicroKernelTrait, which means it's true that Symfony works with 5.3. No project is going to break because we added the trait.
If someone or some lib start relying on the trait to provide some feature, then it's their responsibility to enforce the correct minimum PHP version.

@fabpot fabpot mentioned this pull request Nov 16, 2015
fabpot added a commit that referenced this pull request Nov 23, 2015
…or to add to the builder (weaverryan)

This PR was squashed before being merged into the 2.8 branch (closes #16477).

Discussion
----------

[Routing] Changing RouteCollectionBuilder::import() behavior to add to the builder

| Q             | A
| ------------- | ---
| Bug fix?      | behavior change
| New feature?  | behavior change
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | n/a
| License       | MIT
| Doc PR        | n/a

Based on conversation starting here: #15990 (comment).

```php
// Before:
$routes->mount('/admin', $routes->import(__DIR__.'/config/admin.yml');

// After:
$routes->import(__DIR__.'/config/admin.yml', '/admin');
```

This makes `import()` actually add the `RouteCollectionBuilder` into itself. We didn't do this before at Fabien's request, and actually the current implementation (before this PR) is quite "clean". However, I agree with @wouterj that `import()` really sounds/looks like it will actually *import* those routes *into* this `RouteCollectionBuilder`.

This change is subjective - we just need to pick which way we like better and run full steam with it.

Commits
-------

8feb9ef [Routing] Changing RouteCollectionBuilder::import() behavior to add to the builder
fabpot added a commit to symfony/routing that referenced this pull request Nov 23, 2015
…or to add to the builder (weaverryan)

This PR was squashed before being merged into the 2.8 branch (closes #16477).

Discussion
----------

[Routing] Changing RouteCollectionBuilder::import() behavior to add to the builder

| Q             | A
| ------------- | ---
| Bug fix?      | behavior change
| New feature?  | behavior change
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | n/a
| License       | MIT
| Doc PR        | n/a

Based on conversation starting here: symfony/symfony#15990 (comment).

```php
// Before:
$routes->mount('/admin', $routes->import(__DIR__.'/config/admin.yml');

// After:
$routes->import(__DIR__.'/config/admin.yml', '/admin');
```

This makes `import()` actually add the `RouteCollectionBuilder` into itself. We didn't do this before at Fabien's request, and actually the current implementation (before this PR) is quite "clean". However, I agree with @wouterj that `import()` really sounds/looks like it will actually *import* those routes *into* this `RouteCollectionBuilder`.

This change is subjective - we just need to pick which way we like better and run full steam with it.

Commits
-------

8feb9ef [Routing] Changing RouteCollectionBuilder::import() behavior to add to the builder
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.