Skip to content
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

Split application Kernel and rename default one #1387

Merged
merged 1 commit into from
Apr 25, 2014

Conversation

stloyd
Copy link
Contributor

@stloyd stloyd commented Apr 15, 2014

First step to conquer the world =)

@pjedrzejewski
Copy link
Member

But why? I understand that not only Symfony uses the bundles and kernel system, but core kernel depends on bundles which require symfony-framework bundle. So if you use Sylius core with Symfony2, the recommended way is to use CoreBundle.

@stloyd
Copy link
Contributor Author

stloyd commented Apr 15, 2014

@pjedrzejewski Maaan, sorry ;) Created PR from wrong branch... this was one of approaches but I dropped it.

Now it contains proper changes.

@stloyd stloyd changed the title Move Kernel into Core Component from CoreBundle Split application Kernel and rename default one Apr 15, 2014
@stloyd
Copy link
Contributor Author

stloyd commented Apr 17, 2014

@pjedrzejewski Did you looked at this one? =)

@stloyd
Copy link
Contributor Author

stloyd commented Apr 22, 2014

@Sylius Any reason this is not merged yet?

*
* @author Paweł Jędrzejewski <pawel@sylius.org>
*/
abstract class SyliusKernel extends Kernel
abstract class Kernel extends BaseKernel
Copy link
Contributor

Choose a reason for hiding this comment

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

actually, why would it be abstract?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I guess it was done because it's in bundle folder, but the real reason is known to @pjedrzejewski only =)

@winzou
Copy link
Contributor

winzou commented Apr 22, 2014

Seems good to me, but need rebase.

@pjedrzejewski
Copy link
Member

I'm not against, but just wondering what is the benefit of having separate Cli kernel? It will have separate and different cache/configuration than web kernel... Also, Sylius/Sylius is not the recommended starter for new projects, so this Kernel with "Put your bundles here" note is perfect, but for Sylius-Standard. What do you think?

@stloyd
Copy link
Contributor Author

stloyd commented Apr 22, 2014

As said in description, it's first step to done this properly, I don't want to create one big step that will broke everything once and for all, but do it in small parts, that don't require so many code adjustments.

About split of kernels, the advantage is that you use only bundles you need, you never use migrations + fixtures in other place than in CLI, of course there could be also some cons, as with everything, but this way it's much cleaner, and it's one of steps on "longer road" to make Sylius better =)

@webda2l
Copy link

webda2l commented Apr 22, 2014

Add to CliKernel, some people also split the AdminKernel and AppKernel for security reason. Each kernel have only its required bundles and the admin part and front part can be hosted on different server with only required bundles codes.

@jjanvier
Copy link
Contributor

The only drawback is that it makes Sylius a "non-standard" Symfony project.

But I like it. Not sure this is a "must have", but I like the idea of registering only what you need when you need it. The idea of @webda2l is really interesting too !

So 👍

@pjedrzejewski
Copy link
Member

Requires rebase now. :)

@stloyd
Copy link
Contributor Author

stloyd commented Apr 25, 2014

@pjedrzejewski Rebased, tests passed =)

pjedrzejewski pushed a commit that referenced this pull request Apr 25, 2014
Split application Kernel and rename default one
@pjedrzejewski pjedrzejewski merged commit 7241523 into Sylius:master Apr 25, 2014
@pjedrzejewski
Copy link
Member

Thank you Joseph! This also solves my problem with the demo, where I load the fixtures in production environment.

@stloyd stloyd deleted the feature/kernel_move branch April 25, 2014 08:00
@winzou
Copy link
Contributor

winzou commented Apr 25, 2014

@stloyd please fix:

InvalidArgumentException: There is no extension able to load the configuration for "doctrine_migrations"

doctrine_migrations is configured but the bundle not loaded while not in CLI => error.

@pjedrzejewski
Copy link
Member

Ah crap... I missed this. Does it mean a separate config for CLI? I'm not sure this is worth it... especially it makes us less Symfony standard compatible as @jjanvier said, with little benefit.

@winzou
Copy link
Contributor

winzou commented Apr 25, 2014

I'm not in favor of a separate CLI config, it's complicating things for no real advantage.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Enhancement Minor issues and PRs improving the current solutions (optimizations, typo fixes, etc.).
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants