-
-
Notifications
You must be signed in to change notification settings - Fork 9.6k
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
added a micro kernel #15990
Conversation
07ae17f
to
558229f
Compare
* @author Ryan Weaver <ryan@knpuniversity.com> | ||
* @author Fabien Potencier <fabien@symfony.com> | ||
*/ | ||
abstract class MicroKernel extends Kernel |
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.
MicroKernel
is probably not the right class name, it's just a regular Kernel with some method helpers.
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.
Hmm, yes. Will we need to split this class into 2 parts (with service stuff in HttpKernel and route stuff in FrameworkBundle)?
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 can't think of a good name, so let me throw out some not great names for brainstorming:
- ConfigurableKernel
- CompleteKernel
- AutonomousKernel
- BuildableKernel
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 love MicroKernel
name. I cannot think of a better alternative :)
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.
It's not about a sexy name, but having a descriptive one; there is nothing Micro 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.
I like ConfigurableKernel
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.
ConfigurableKernel is cool 👍
What about StandardKernel or SimpleKernel ? Even though I'd vote for ConfigurableKernel :)
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 this is "the" ConfigurableKernel ... does that mean that the other kernels are not configurable?
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 Kernel is abstract. Why not AbstractKernel
then?
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.
Kernel is abstract as well...
558229f
to
bea6db2
Compare
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__)); |
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 think we need this to include the sub-class:
$container->addObjectResource($this);
9573350
to
fc24976
Compare
|
||
return $this->configureRoutes(new RouteCollectionBuilder($loader))->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.
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
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 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
75daaa2
to
d140c61
Compare
The updated routing method looks good to me. 👍 in general - except for the name of course :) |
}); | ||
} | ||
|
||
private function loadRoutes(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.
I think this will need to be public
@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!) |
05d7106
to
8a94c87
Compare
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). |
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 |
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. |
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. |
@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 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 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 |
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. |
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 |
@weaverryan assume i have several routing files i want added, if both have the same prefix i assume the latter will override the former? |
@henrikbjorn it uses the same mechanism as normal route importing. So if you import 5 routes from |
Maybe we should allow mounting without a prefix? |
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". |
Let's describe what I think (as a non-native english speaker) the discussed function names mean:
It turns out Furthermore, it turns out |
About About |
We can rename it to |
@weaverryan i think the confusing part for me was the 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 |
All I see is references to 2.8. Just verifying that this will this be in 3.0 too? |
It will be merged in the next "merge 2.8 on 3.0" I guess :) |
@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 ? |
@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. |
@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 Not respecting that is kind of alarming for me because I can never know what to expect. I would constantly ask myself:
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 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 |
There are many packages in the 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! |
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! |
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. |
I totally agree with @SoboLAN. |
@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. |
gedmo/doctrine-extensions has a PHP requirement of It's been already said:
For me, these reasons are enough to keep it in the 2.8 branch |
Btw we already had traits in symfony before (ContainerAwareTrait). Not sure what the goal of this discussion is. Nobody forces you to use it. |
@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:
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. |
@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. |
…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
…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
Related to #15948 and #15820