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

IBX-570: Fixed respecting ezplatform.session.* parameters #40

Merged
merged 3 commits into from
Jun 25, 2021

Conversation

webhdx
Copy link
Contributor

@webhdx webhdx commented Jun 22, 2021

Question Answer
JIRA issue IBX-570
Bug yes
New feature no
Target version 2.3
BC breaks no
Tests pass yes
Doc needed no

This fixes IBX-570 where ezplatform.session.* parameters are not respected because our framework configration in ezplatform.yaml file is overwritten by framework.yaml. Since we can't really modify framework.yaml as it comes from Symfony, I just reconfigure session handler in compiler pass. Default behavior (=using framework.yaml configuration) can be still achieved by setting ezplatform.session.* params to null.

Here is the second PR which removes obsolete configuration from ezplatform.yaml: ibexa/recipes#32

TODO:

  • Implement feature / fix a bug.
  • Implement tests.
  • Fix new code according to Coding Standards ($ composer fix-cs).
  • Ask for Code Review.

@webhdx webhdx added Bug Something isn't working Ready for review labels Jun 22, 2021
@webhdx webhdx requested a review from a team June 22, 2021 11:32
@webhdx webhdx self-assigned this Jun 22, 2021
Copy link
Member

@alongosz alongosz left a comment

Choose a reason for hiding this comment

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

It might be risky to hide that configuration behind compiler pass magic. I would rather prefer documenting it properly, making end-developer adjust needed setting per project. Forcing loading of overrides with our custom Kernel is also interesting idea. Unfortunately it probably requires overriding Symfony-provided recipes/files, so back to the same issue as in case of pure framework.yaml.

My major concern would be that there's currently no way to disable the behavior forced by compiler pass. So any setup requiring different configuration (unable to predict on our end) would stop working.

That said, I see that this is the most "stable" from our POV way to ensure those parameters are set properly for our Product, so +1 for now due to lack of better idea.

Minor remarks:

@alongosz
Copy link
Member

Side: if we're gonna go with this, ezplatform.yaml in recipes needs to be cleaned to avoid confusion.

@webhdx
Copy link
Contributor Author

webhdx commented Jun 22, 2021

I agree it would be better to keep "magic" stuff as minimal as possible. Sadly it would greatly complicate installation and upgrade process.

Behavior of this compiler pass can be basically disabled by setting both parameters to null but I can't really imagine why would you do this. If you want to configure custom session handler i.e. redis then you'd probably refer to our documentation on how to do it. And the doc will tell you that you should use these parameters (or better - use env variables) instead of manually changing framework.session config.

Both of your suggestions have been implemented in 78055ec

@webhdx
Copy link
Contributor Author

webhdx commented Jun 22, 2021

PR to the recipes: ibexa/recipes#32

@micszo micszo requested a review from a team June 23, 2021 06:33
Copy link
Member

@mnocon mnocon left a comment

Choose a reason for hiding this comment

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

Tested locally and on Platform.sh, together with ibexa/recipes#32:

  • on Platform.sh sessions are by default stored in redis (depending on available services: either in rediscache or in redissession)
  • it's possible to configure the session handler using env variables and the settings are respected

QA Approved

@webhdx webhdx merged commit c7e9017 into 2.3 Jun 25, 2021
@webhdx webhdx deleted the IBX-570_respect_session_handler_id_parameter branch June 25, 2021 12:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
5 participants