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

update to use 2.0 of FOSHttpCache #235

Merged
merged 3 commits into from
Dec 5, 2016
Merged

update to use 2.0 of FOSHttpCache #235

merged 3 commits into from
Dec 5, 2016

Conversation

dbu
Copy link
Contributor

@dbu dbu commented Jul 31, 2015

  • Updating to FOSHttpCache 2.0
  • Starting 2.0 development
  • Adjust documentation for support of custom ttl in the library.

TODO

fix #219

@ddeboer
Copy link
Member

ddeboer commented Aug 2, 2015

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.

@dbu
Copy link
Contributor Author

dbu commented Aug 3, 2015

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.

@dbu dbu force-pushed the update-2-0 branch 2 times, most recently from 4100017 to ffd6b9f Compare August 21, 2015 08:07
@dbu
Copy link
Contributor Author

dbu commented Aug 21, 2015

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

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

@ddeboer ddeboer mentioned this pull request Nov 15, 2015
@lsmith77 lsmith77 removed the wip/poc label Nov 18, 2015
@dbu dbu mentioned this pull request Dec 4, 2015
@dbu dbu changed the title [WIP] update for ttl support in the library [WIP] update to use 2.0 of FOSHttpCache Jul 1, 2016
@ddeboer
Copy link
Member

ddeboer commented Nov 21, 2016

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?

We need something to configure the http client: https://github.com/php-http/HttplugBundle/

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.

@dbu
Copy link
Contributor Author

dbu commented Nov 21, 2016

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
https://github.com/dbu Do you have some time to resolve the merge
conflicts?

yep, agree. will try to find some time.

We need something to configure the http client:
https://github.com/php-http/HttplugBundle/

In the library, we do most of the configuration on HttpDispatcher
http://foshttpcache.readthedocs.io/en/2.0.0-alpha1/proxy-clients.html#basic-http-setup-with-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.

i will have a look again at that. but i don't want to duplicate
configuration, maybe say by default we use autodiscovery otherwise
specify a http client service. and to get the service, configure
HttplugBundle or do your own thing.

@@ -88,6 +88,7 @@ public function testRefreshRoute()
*/
public function testTagResponse()
{
$this->markTestSkipped('TODO refactor to use tag handler');
Copy link
Contributor Author

Choose a reason for hiding this comment

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

needs to be adjusted

@ddeboer
Copy link
Member

ddeboer commented Dec 1, 2016

maybe say by default we use autodiscovery otherwise
specify a http client service. and to get the service, configure
HttplugBundle or do your own thing.

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.

@dbu dbu changed the title [WIP] update to use 2.0 of FOSHttpCache update to use 2.0 of FOSHttpCache Dec 4, 2016

* [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.
Copy link
Contributor Author

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.

Copy link
Member

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()
Copy link
Contributor Author

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.

@dbu
Copy link
Contributor Author

dbu commented Dec 4, 2016

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.

@ddeboer
Copy link
Member

ddeboer commented Dec 4, 2016

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.*'
Copy link
Contributor Author

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

@dbu dbu mentioned this pull request Dec 5, 2016
@dbu dbu merged commit ea4d715 into master Dec 5, 2016
@dbu dbu deleted the update-2-0 branch December 5, 2016 23:51
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 this pull request may close these issues.

Update inline C
3 participants