-
Notifications
You must be signed in to change notification settings - Fork 8
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
EZP-32214: Refactored config/packages/overrides/* to load from Extension #27
Conversation
6baf4a3
to
b8ce23f
Compare
src/EzPlatformCoreBundle/bundle/DependencyInjection/EzPlatformCoreExtension.php
Show resolved
Hide resolved
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.
As we discussed on Slack: would be good to move this code into dedicated platform.sh related package. But for now it's OK.
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.
Some refactoring in the future of this code would be much appreciated. I'll try to find some time.
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.
First, a small thing:
-
there's no handling of fastly purge_server (Fastly purge_server is not set:
https://github.com/ezsystems/ezplatform-ee/blob/master/config/packages/overrides/generic.php#L42-L44) which was a nice addition (and could break for people doing an upgrade). -
SESSION_SAVE_PATH and SESSION_HANDLER_ID seem to have no effect
For:
export SESSION_SAVE_PATH=tcp://redis-session:6379?weight=1
export SESSION_HANDLER_ID=ezplatform.core.session.handler.native_redis
php bin/console debug:config FrameworkBundle
still shows current values:
session:
handler_id: session.handler.native_file
save_path: /Users/mareknocon/Desktop/Sites/v3_2/var/sessions/dev
but it should show (current behaviour):
session:
handler_id: ezplatform.core.session.handler.native_redis
save_path: 'tcp://redis-session:6379?weight=1
I've also tested Commerce on platform.sh:
3) Purge type for Varnish is not configured automatically.
After Platform.sh deployment purge_type still seems to be "local".
If I understand correctly this part of code executes only if the HTTPCACHE_PURGE_TYPE variable is not set: https://github.com/ezsystems/ezplatform-core/pull/27/files#diff-f72b922417794d0e5c2291bf6eddcfdf0e1e32a18d00f5ff3e8b46c514baa100R280
and now we give it a default value.
- Solr is not configured when deploying on Platform.sh. It is set if I add SEARCH_ENGINE=solr variable to .platform.app.yaml. but without it it does not happen.
I will send you my Platform.sh project on Slack.
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.
QA approved.
Please remember to remove TMP changes in ezsystems/ezplatform#632 before merging.
master
Second attempt at refactoring
config/packages/overrides/*.php
files. Previously it wasn't fully working. It has to be a part ofExtension::load
in order for the data to be ready for compiler passes.