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

[Proposal] New FrameworkConfigurator to configure framework-bundle #247

Closed
covex-nn opened this issue Dec 20, 2017 · 10 comments
Closed

[Proposal] New FrameworkConfigurator to configure framework-bundle #247

covex-nn opened this issue Dec 20, 2017 · 10 comments

Comments

@covex-nn
Copy link
Contributor

See symfony/recipes#262

Right now bundles, that requires session service can not work properly if session is not configured in framework.yaml. But developer will have to configure session anyway to make a bundle work. And it is not about session only: some of them uses form, another - uses assets, liip/theme-bundle uses templating (symfony/recipes-contrib#209), sonata/core-bundle uses session (symfony/recipes-contrib#151).

Of course, bundles could contain some CompilerPass like CheckForSessionPass, created by @weaverryan for FOSUserBundle.

But if so, every time developer starts a project, he will see the same message like "FOSUserBundle requires the "session" service to be available. Uncomment the "session" section in "config/packages/framework.yaml" to activate it.". For assets or templating there will be instructions too.

And all these bundle will have their copies of CheckForSessionPass to tell developer what he has to do next. And if in the future there will be changes in FrameworkBundle, all these bundles will have to change instructions in compler passes too.

If there won't be any CompilerPass, developer will see message during cache:clear like this:

!! [Symfony\Component\DependencyInjection\Exception\ServiceNotFoundException]
!! The service "sonata.core.flashmessage.manager" has a dependency on a non-ex
!! istent service "session".


I believe that none of the described options are optimal. So, i suggest adding a new configurator FrameworkConfigurator. It will accept new recipe definition in manifest.json like this:

{
   "framework": [ "session", "assets", "form" ]
}

It means, that recipe/package requires session, assets and form modules to be enabled, to work properly.

To make FrameworkConfigurator work, we must fill config/packages/framework.yaml with commented default configuration for framework-bundle modules. All these configurations must be commented with double sharp sign.

framework:
    # uncomment this entire section to enable assets
    ##assets: ~

    # uncomment this entire section to enable sessions
    ##session:
    ##    # With this config, PHP's native session handling is used
    ##   handler_id: ~

So, then if FrameworkConfigurator will meet assets or session in framework section of manifest.json, it will remove double sharps in the corresponding lines automaticaly. Unfortunately removing double sharp cannot be unconfigured if several packages requires the same. But, if session or assets will be enabled, application will not became broken, so we can not pay attention to it.

@covex-nn
Copy link
Contributor Author

covex-nn commented Dec 20, 2017

Actualy, i have already created FrameworkConfigurator. So, if you like my idea, i will create PR

@Pierstoval
Copy link
Contributor

At first sight I'm not sure that FrameworkConfigurator is the best option. The framework entry is just an extension point for the FrameworkBundle, therefore I'd suggest having a BundleExtensionConfigurator that allows to add configuration to specific extensions, if they're available or not. Then, we would be able to have such config for the manifest.json:

{
    "extensions": {
        "symfony/framework-bundle": {
            "framework": {"session": null}
        }
}

This way, Flex might retrieve configuration files from this package's recipe, if it already has a recipe, and update its main config file. If no file exists, maybe we could have different options: create a new file, update current package's config according to these extension points, or throw an exception to force the creation of a recipe or specify a config file in the manifest...

WDYT?

@covex-nn
Copy link
Contributor Author

@Pierstoval i think, it will be difficult (if not imposible) for BundleExtensionConfigurator to merge similar (but not the same) configurations from several recipes. Example: what session cookie name should be chosen when two recipes will try to configure it? But it would be great if someone will create such configurator!

With my proposal, there will be only one default configuration for each framework extension, and these default configurations will be controlled by symfony/recipes owners. Each recipe, that will use FrameworkConfigurator to turn on framework extension, should work properly with its default configuration.

See config.yml from symfony/standard-edition! There are a lot of enabled framework extensions. With FrameworkConfigurator it would be easy to enable disabled serializer, and all other extensions btw! because they are disabled by default with Flex.

So, with FrameworkConfigurator it would be possible just to run composer require twig templating sonata-project/doctrine-orm-admin-bundle after composer create-project symfony/skeleton, and to see /admin/ web-page without lots of warnings, exceptions and questions in console.

@stof
Copy link
Member

stof commented Dec 21, 2017

FrameworkBundle auto-enables most of its components based on the fact that the corresponding package is installed.
The big exception is the session, because this cannot be detected this way (due to the Session code being part of HttpFoundation, which is always installed). However, as of 3.4.2 and 4.0.2, the session layer will be auto-enabled when the security-csrf component is installed (as enabling CSRF without enabling Session does not work).
So a way to enable session currently is to require the CSRF layer (which is one of the reasons why you might want it btw)

@covex-nn
Copy link
Contributor Author

@stof yes, i saw that PR about enabling session with symfony/security-csrf, it is not merged yet.

Right now my goal is to have recipe for sonata-project/admin-bundle to launch SonataAdminBundle "out from the box" with Flex. That bundle can not work without this framework configuration:

framework:
    assets: ~
    session: ~
    templating:
        engines: ["twig"]

But with FrameworkConfigurator it would be possible to create a recipe without need to configure framework-bundle. And SonataAdminBundle could be launched just with composer create-project symfony/skeleton . + composer require ...

@stof
Copy link
Member

stof commented Dec 21, 2017

Well, for the templating part, the admin bundle should rather migrate to using the native Twig notation for templates, and using the Twig service directly. We precisely want to discourage the usage of the Templating wrapper in Flex projects.

Regarding assets, it is already auto-enabled based on the detection of the asset component.
So it is better to require the component that you need (enabling it without having the component would break anyway)

@slootjes
Copy link
Contributor

I run into this issue for the same reason: trying to create a sonata admin recipe. Not only I have the issue with session but also with the "security" options which I also want to make part of the bundle. Maybe it can be handled in a similar way?

@covex-nn
Copy link
Contributor Author

@stof apparently, I missed all the fun with framework-bundle configuration =( Confusion arose from the session service (and because of inattention, of course).

But, anyway FrameworkConfigurator could be useful, it not - please close this issue

@stof
Copy link
Member

stof commented Dec 21, 2017

But, if session or assets will be enabled, application will not became broken, so we can not pay attention to it.

that's not true. If you remove a package and this removes the need to install symfony/asset, enabling the asset system explicitly will break (complaining that the component is missing).
This is why it is better to rely on the smart behavior of the FrameworkBundle configuration instead..

@covex-nn
Copy link
Contributor Author

I agree and give up. So, i'm closing this issue

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants