-
Notifications
You must be signed in to change notification settings - Fork 83
update to use 2.0 of FOSHttpCache #235
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
Conversation
FriendsOfSymfony/FOSHttpCache#214 has been merged, so we can merge this, too. I think we need to update the http-cache requirement to @dev, here. |
right. updated - but we should first merge the bugfix #236 and then tag a last 1.3 version. this PR will start the 2.0 version of the bundle. |
4100017
to
ffd6b9f
Compare
created the 1.3 branch, now getting serious :-) lots of question still, however. mainly how to handle the http client... maybe a default service with a configured class if the user specifies neither class nor service? |
|
||
before_script: | ||
- sh -c 'if [ "$SYMFONY_VERSION" != "" ]; then composer require --dev --no-update symfony/symfony=$SYMFONY_VERSION; fi;' | ||
- sh -c 'if [ "$FRAMEWORK_EXTRA_VERSION" != "" ]; then composer require --dev --no-update sensio/framework-extra-bundle=$FRAMEWORK_EXTRA_VERSION; fi;' | ||
- composer install | ||
- if [[ "$TRAVIS_PHP_VERSION" == "5.4" || "$TRAVIS_PHP_VERSION" == "hhvm" ]]; then composer remove "php-http/guzzle6-adapter" --dev --no-update; fi | ||
- if [[ "$TRAVIS_PHP_VERSION" == "5.4" || "$TRAVIS_PHP_VERSION" == "hhvm" ]]; then composer require "php-http/guzzle5-adapter" --dev --no-update; fi |
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 in FOSHttpCache 2.0
As we tagged FOSHttpCache 2.0-alpha1, it would be a good time to update the bundle to work with 2.0 so users can test both in tandem. @dbu Do you have some time to resolve the merge conflicts?
In the library, we do most of the configuration on HttpDispatcher, which wraps the Httplug client; not on Httplug itself. So while I’m okay with adding HttplugBundle as a dependency for users that want to tweak the underlying Httplug client’s settings (such as timeouts etc.), I’m not sure all users need the extra bundle. For most, being able to choose your own HTTP client adapter plus the configuration options that we already have should be sufficient. |
yep, agree. will try to find some time.
i will have a look again at that. but i don't want to duplicate |
@@ -88,6 +88,7 @@ public function testRefreshRoute() | |||
*/ | |||
public function testTagResponse() | |||
{ | |||
$this->markTestSkipped('TODO refactor to use tag handler'); |
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.
needs to be adjusted
I like this approach: let’s suggest HttplugBundle (it’s good bundle 😉) but leave it open for users to configure the service any way they like. FOSHttpCacheBundle then only needs the service id. |
|
||
* [Test] Dropped the proxy client services as they where not used anywhere. The | ||
services `fos_http_cache.test.client.varnish` and `fos_http_cache.test.client.nginx` | ||
no longer exist. |
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.
@ddeboer can you agree with this? i feel that with httplug and discovery, its easy enough to do this now on your own in tests.
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.
Agreed!
->info('Default host name and optional path for path based invalidation.') | ||
->end() | ||
->scalarNode('http_client') | ||
->defaultNull() |
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.
we don't need any reference to the httplug-bundle. by default, we pass no client to HttpDispatcher, which will prompt that to do a discovery. if discovery is not desired, a client can be configured.
imho this is ready to merge now. there is a bunch of things we should clean up in this bundle, but i think we should not do that in the same PR. would prefer to merge this ASAP and then do the other things in separate PRs. |
49c6757
to
d629ef7
Compare
Ok, fixed the docs build, so almost green. There’s a dep problem on prefer-lowest, do you have an idea? |
# - php: 5.5 | ||
# env: | ||
# - COMPOSER_FLAGS="--prefer-lowest" | ||
# - SYMFONY_VERSION='2.8.*' |
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.
jordi thinks we hit some hard to tell edge case: https://twitter.com/seldaek/status/805698883456270336
i comment this build out for now and try again when we released the library
TODO
fix #219