-
Notifications
You must be signed in to change notification settings - Fork 149
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
Fix usage of session env variables and move some over to use env() #308
Conversation
app/config/default_parameters.yml
Outdated
purge_type: local | ||
|
||
## Session handler, by default set to file based in order to be able to use %session.save_path% | ||
# evn: SESSION_HANDLER_ID |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
typo: # env
Looks good. Everything is still working as expected. Using ezlaunchpad and still working. Deploy to p.sh still working. |
@@ -1,8 +1,6 @@ | |||
# This file contains defaults for optional parameters, which you can override in parameters.yml | |||
parameters: | |||
locale_fallback: en | |||
ezplatform.session.handler_id: session.handler.native_file | |||
ezplatform.session.save_path: '%kernel.project_dir%/var/sessions/%kernel.environment%' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Isn't this a BC break? I had ezplatform.session.save_path
overridden on my project and this change broke my installation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ezplatform.session.save_path
was afaik introduced here and has not been made part of any release yet:
fc0e356
So it's maybe just because you use 2.2-dev? (we don't have BC policy for that) Or was it somewhere else before also?
However there is a BC break here on ezplatform.session.handler_id
, which has been around across several branches for quite a while. So the renaming here should maybe anyway be renamed back to also avoid your issue.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe you are right and I did it between those two changes. If this wasn't released yet I think we are ok with the ezplatform.session.save_path
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah, but I changed them to:
session.handler_id
session.save_path
to be somewhat more consistent with other params.
However change on first param change is BC break as it's been around since 2.1.0 and 1.13.1.
So ok for you if I change it back to the following before 2.2.2?
ezplatform.session.handler_id
ezplatform.session.save_path
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, works for me.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed in 8b3d4e0, looks ok to you? If so I'll merge up to the other repos
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks ok, thanks.
Some further cleanup on session env variables, and move some non handler based params to use
env()
and hence fix inline todo.Besides cleanup, this fixes that platformsh.php was setting wrong session.save_path parameter.
This is followup to #307 (done in 2.2-dev) and #304
It also tries to clarify a bit the confusion in #306
review ping @alongosz, @raupie, @Plopix