symfony / recipes

Symfony Recipes Repository
https://github.com/symfony/recipes/blob/flex/main/RECIPES.md
MIT License
960 stars 476 forks source link

What to do if a bundle (recipe) needs session? #262

Closed scuben closed 6 years ago

scuben commented 6 years ago

The defaults in framework uncomments the session and thus the session service is not being created. But what can one do if a bundle (recipe) needs the session to be available?

weaverryan commented 6 years ago

I've been thinking a lot about this too. It's a real outlier: one of the few critical services that is activated by uncommenting config, versus installing a package. But, it's more than that: 3rd-party bundles (I'm working on FOSUserBundle right now) have no easy way to detect if the session is missing: there is no class_exists(Session::class) that can be done, like many other things (the only way I know of is to create a compile pass that looks for a session service).

I can think of 3 solutions:

A) Do nothing: use compiler passes when necessary to throw a clear exception if your bundle needs the session.

B) Create a fake symfony/session package that is empty, but has a recipe attached to it that will auto-activate the session.

C) As a follow-up to B, move the session logic into its own package so that it works a bit like the other pieces (e.g. translator, serializer, etc).

Thoughts?

covex-nn commented 6 years ago

@weaverryan session service is enabled only when framework.session presents in configuration.

So, if a new recipe requires session or assets or form, we could just add this to new recipe's yaml file:

framework:
    assets: ~
    form: ~
    session: ~
javiereguiluz commented 6 years ago

Some people in the community have been asking to make a new Session component since 2014:

scuben commented 6 years ago

@weaverryan as @covex-nn mentioned it's a wider problem not only related to "activate session" in the framework. All solutions A to C seems just to be workarounds. C) is a good idea, but actually not related to this issue. Moving session in its own component should be done nonetheless. A) is a bad DX. And B) is - as you said - fake :)

What about a new possibility to set config other than just copy your bundlename.yaml? Something like:

"set-config": {
    "framework.session": "~"
}

During composer req flex could check the current configuration value and compare it to the one which the recipe wants to set. If they diverge then flex might ask the user (like incenteev/composer-parameter-handler does) if he want to change the configuration value (in this example directly in framework.yaml. Of course this idea is currently just a jumpstart and maybe its not possible with setting configs in the manifest.json, but there will be solutions (e.g. reference a .yaml file in "set-config".

@covex-nn is this actually working to add a framework.yaml in your (contrib) recipe? and the config gets merged? can image this would lead to serious "merge conflicts".

Pierstoval commented 6 years ago

Some people in the community have been asking to make Session a new componente since 2014:

This could be nice, but the problem is that we then have to find a BC layer for current session from HttpFoundation. I think the best idea is to make sub-splits of HttpFoundation like symfony/http-session.

covex-nn commented 6 years ago

@scuben my suggestion was to activate session inside new config/packages/vendor_name.yaml, not in config/packages/framework.yaml

scuben commented 6 years ago

@covex-nn yes, but that would lead to a config key in many .yaml files and the question then will be: which one will be used if the values are divergent.

Pierstoval commented 6 years ago

Yeah, the problem is that many packages need the session πŸ˜•

covex-nn commented 6 years ago

@scuben @Pierstoval if many packages needs session, then there will be many...

framework:
    session: ~

... and there will be only one service, configured by default.

I created new project with composer create-project symfony-skeleton ^4.0, then i added 2 different files with "session activation config". And here is the result:

C:\2\4.0>php bin\console debug:container session

Information for Service "session"
=================================

 ---------------- --------------------------------------------------
  Option           Value
 ---------------- --------------------------------------------------
  Service ID       session
  Class            Symfony\Component\HttpFoundation\Session\Session
  Tags             -
  Public           yes
  Synthetic        no
  Lazy             no
  Shared           yes
  Abstract         no
  Autowired        no
  Autoconfigured   no
 ---------------- --------------------------------------------------

So, no problem here

scuben commented 6 years ago

But what if the config value is not just on/off?

Like in this example (yeah, not a good example. but one gets it):

framework:
    default_locale: en

versus

framework:
    default_locale: de

Which one should be used? I think it should be the customers choice during flex installation.

Pierstoval commented 6 years ago

... and there will be only one service, configured by default.

I'm not sure having the same config in tons of files is the best option. If we start today with session, I'm afraid other components will be activated this way and if we want to deactivate one, we'll need to disable it in several files, it's probably gonna be a PITA to debug... πŸ˜•

nicolas-grekas commented 6 years ago

See proposal at https://github.com/symfony/symfony/pull/25508

covex-nn commented 6 years ago

@nicolas-grekas session was not enabled for test environment by default (see https://github.com/symfony/recipes/pull/223). Is there a way how to disable it after merging you proposal? Should it be disabled for test env?

nicolas-grekas commented 6 years ago

My proposal just changes the default value. So yes, you can change it: just set it and overwrite the default.

nicolas-grekas commented 6 years ago

What about creating a session.yaml file, that would contain the session enabling lines? Every bundle that would need session would ship this file in its recipe.

Pierstoval commented 6 years ago

Then it would have to be kinda "standard": by default, framework.yaml would have no session configured, but any bundle having session.yaml then must enable the session in this file. If it is not enabled, other bundles requiring the session and having the same file in their recipe will not overwrite the file when Flex will install the recipe, and there can be some bugs then πŸ˜• But the session.yaml file in every package needing it sounds like a good idea IMO πŸ‘

covex-nn commented 6 years ago

What if session will be activated by default, but it will use some NullSessionStorage/NullSessionHandler as session storage/handler? Then there will be no problems with recipes, that using session and real php session will not start.

Also there could be a comment in config/packages/framework.yaml how to activate real session and some message about it in post-install.txt:

framework:
    # to activate session do this and that
    # or remove session block if you do not want to use session at all
    session:
        storage_id: session.storage.null
        handler_id: session.handler.null
slootjes commented 6 years ago

Just like in #258 I'm running into the same issue with the security config. An option to merge/update framework and/or security entries would be very helpful.

weaverryan commented 6 years ago

I like the session.yaml idea. But to accomplish this, would every bundle that needs a session need to duplicate this in their recipe? There is also session config needed for the test environment (currently done in config/packages/test/framework.yaml, and you need to know to also uncomment it there).

If we had a session component, this would be much easier (give it a recipe, and other packages depend on it). But we don’t...

javiereguiluz commented 6 years ago

Just asking: would creating a Session component be a huge task? We wouldn't start from scratch. It could be doable pretty quickly ... if we really want to do that.

covex-nn commented 6 years ago

If for some reason Session must be a part of symfony/http-foundation in Symfony4 under Symfony\Component\HttpFoundation\Session namespace, then i see 2 options:

  1. Create a package symfony/session with composer.json without dependencies just for Flex. After composer require symfony/session, session will be enabled. Recipe with config/packages/session.yaml file, will contain uncommented session configuration:

    framework:
        session: ~
  2. Create a bundle symfony/session-bundle with copy of session.xml from FrameworkBundle. Session will be enabled by default, and will be configured from session root node.

    To disable session for test environment, this configuration could be inside config/packages/test/session.yaml file:

    session:
        # Remove next line if you're using sessions
        enabled: false
        # Uncomment this section if you're using sessions
        #storage_id: session.storage.mock_file
nicolas-grekas commented 6 years ago

What's the downside of always enabling the session? Looks the most sensible to me for now, isn't it?

nicolas-grekas commented 6 years ago

Proposed fix in #333