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

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

Closed
plandolt opened this issue Nov 18, 2017 · 23 comments · Fixed by #333
Closed

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

plandolt opened this issue Nov 18, 2017 · 23 comments · Fixed by #333

Comments

@plandolt
Copy link
Contributor

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
Copy link
Member

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
Copy link

covex-nn commented Dec 8, 2017

@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
Copy link
Member

javiereguiluz commented Dec 8, 2017

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

@plandolt
Copy link
Contributor Author

plandolt commented Dec 8, 2017

@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
Copy link
Contributor

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
Copy link

covex-nn commented Dec 8, 2017

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

@plandolt
Copy link
Contributor Author

plandolt commented Dec 8, 2017

@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
Copy link
Contributor

Yeah, the problem is that many packages need the session 😕

@covex-nn
Copy link

covex-nn commented Dec 8, 2017

@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

@plandolt
Copy link
Contributor Author

plandolt commented Dec 8, 2017

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
Copy link
Contributor

... 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
Copy link
Member

See proposal at symfony/symfony#25508

@covex-nn
Copy link

@nicolas-grekas session was not enabled for test environment by default (see #223). Is there a way how to disable it after merging you proposal? Should it be disabled for test env?

@nicolas-grekas
Copy link
Member

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

@nicolas-grekas
Copy link
Member

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
Copy link
Contributor

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
Copy link

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
Copy link

slootjes commented Jan 2, 2018

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
Copy link
Member

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
Copy link
Member

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
Copy link

covex-nn commented Jan 5, 2018

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
Copy link
Member

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

@nicolas-grekas
Copy link
Member

Proposed fix in #333

nicolas-grekas added a commit to symfony/symfony that referenced this issue Jan 16, 2018
…sion* are loaded (nicolas-grekas)

This PR was merged into the 3.4 branch.

Discussion
----------

[FrameworkBundle] Auto-enable CSRF if the component *+ session* are loaded

| Q             | A
| ------------- | ---
| Branch?       | 3.4
| Bug fix?      | yes
| New feature?  | no
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | symfony/recipes#262
| License       | MIT
| Doc PR        | -

By binding CSRF and session default state, we provide better DX, but we also provide a way for bundles to enable session on its own: they just need to require "symfony/security-csrf".
Yes, that's a side effect, but I think that's a nice one for 3.4/4.0.
Of course, we might do better in 4.1, but for bug fix only releases, LGTM.

Commits
-------

9e8231f [FrameworkBundle] Automatically enable the CSRF if component *+ session* are loaded
symfony-splitter pushed a commit to symfony/framework-bundle that referenced this issue Jan 16, 2018
…sion* are loaded (nicolas-grekas)

This PR was merged into the 3.4 branch.

Discussion
----------

[FrameworkBundle] Auto-enable CSRF if the component *+ session* are loaded

| Q             | A
| ------------- | ---
| Branch?       | 3.4
| Bug fix?      | yes
| New feature?  | no
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | symfony/recipes#262
| License       | MIT
| Doc PR        | -

By binding CSRF and session default state, we provide better DX, but we also provide a way for bundles to enable session on its own: they just need to require "symfony/security-csrf".
Yes, that's a side effect, but I think that's a nice one for 3.4/4.0.
Of course, we might do better in 4.1, but for bug fix only releases, LGTM.

Commits
-------

9e8231f [FrameworkBundle] Automatically enable the CSRF if component *+ session* are loaded
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

Successfully merging a pull request may close this issue.

8 participants