Provide Symfony HttpCache as trait #233
Conversation
ddeboer
changed the title
ddeboer
force-pushed
the
httpcache-trait
branch
2 times, most recently
from
a01685f to
4390ce1
Compare
| * | ||
| * {@inheritDoc} | ||
| */ | ||
| public function fetch(Request $request, $catch = false) |
There was a problem hiding this comment.
Unfortunately, we have to add this method here. If I put this in the trait, I get:
Fatal error: Access level to Symfony\Component\HttpKernel\HttpCache\HttpCache::fetch() must be public (as in class FOS\HttpCache\SymfonyCache\CacheInvalidationInterface) in /vagrant/FOSHttpCache/tests/Functional/Fixtures/Symfony/AppCache.php on line 12
There was a problem hiding this comment.
strange that a trait can't change visibility of a method - or is there a way that would allow it?
There was a problem hiding this comment.
so actually it does change visibility but there seems to be some bug with interface vs trait somewhere: http://stackoverflow.com/questions/31877844/php-trait-exposing-a-method-and-interfaces
There was a problem hiding this comment.
@stof this is the solution i came up with, because of the problem i explain on stackoverflow. its annoying but i guess its really a bug (or illogic decision) of traits vs interfaces.
|
very cool! this will get us rid of a lot of duplication needs. |
|
@ddeboer i investigated a bit and at least found a workaround, albeit not an elegant one. there is one test reliably failing now: SymfonyProxyClientTest::testRefresh i have to leave now, if you want to further investigate, please do. |
| { | ||
| // http://stackoverflow.com/questions/31877844/php-trait-exposing-a-method-and-interfaces | ||
| use EventDispatchingHttpCache {fetch as public eventTriggeringFetch;} |
There was a problem hiding this comment.
There was a problem hiding this comment.
none of this is calling eventTriggeringFetch
There was a problem hiding this comment.
dbu
force-pushed
the
httpcache-trait
branch
2 times, most recently
from
8ebab9c to
b46b7d1
Compare
| */ | ||
| private function dispatchPostHandle(Request $request, Response $response) | ||
| protected function dispatch($name, Request $request, Response $response = null) |
There was a problem hiding this comment.
made this protected so that kernels using the trait can also use it, if they need to expand on a method and still need the pre and post event.
|
so this would be ready for a code review. doc is missing and some unit tests for the debug subscriber. |
|
@lolautruche can you or somebody else at ez have a look at this? it will affect ez i think, as you use the symfony http cache. i hope its an improvement. BC might be a bit tricky however - but then supporting both FOSHttpCache 1.x and 2.x would be really tricky i think, hope you do not need that. |
|
@dbu Thanks for fixing the test failure. Can you squash your commits? @lolautruche Will this change become a problem for eZ? Please keep in mind this change will only make it to the 2.0 version of the library. |
dbu
changed the title
dbu
force-pushed
the
httpcache-trait
branch
from
e7295ef to
44ab065
Compare
|
squashed and rebased. lets give @lolautruche a few more days to have a look at this before merging. |
| * | ||
| * <b>When using FOSHttpCacheBundle, look at FOS\HttpCacheBundle\HttpCache instead.</b> | ||
| * Your kernel needs to implement CacheInvalidatorInterface and redeclare the | ||
| * fetch method as public. (The later is needed because the trait declaring it |
|
Hi AFAICS, we can be able to use the trait without any hassle for the end developer since we rely on our own abstract Thanks for noticing (and sorry for the delay). |
|
Ping @andrerom for review as well |
|
Thanks for noticing (and sorry for the delay).
thanks for the review. so it seems it will also get a bit more elegant
for ez with the trait.
As for v2.0 of FOSHttpCache, any ETA? I also need to look into the
changelog to be sure our usage is not broken 😉 .
i think we need to wait for php-http (a http client abstraction so that
we don't depend on any specific implementation like a specific guzzle
version) to stabilize before we can call FOSHttpCache stable. the plan
there is to go alpha in the next days afaik, so i would hope we can
start tagging alpha in october and hopefully release end of october.
|
You mean the PHP extension? |
|
@lolautruche no, this thing: http://php-http.readthedocs.org/en/latest/ - it will get a more unique name soon as php-http is not google-friendly. |
|
Ah OK, cool |
Looks ok for me, sensible to move this into trait anyway.
makes perfect sense for 2.0 Side: @dbu / @ddeboer any chance of some feedback on the abstract tagging (#237) direction so we can have at least some abstraction in 2.0 to build upon for Sf HttpCache improvements? Me and @lolautruche have a need to work on this topic over the next month, we can't really wait much longer on this topic. |
|
thanks. so lets get this merged. @ddeboer any idea why hhvm fails: https://travis-ci.org/FriendsOfSymfony/FOSHttpCache/jobs/82876644 ? i already restarted the test, so its not random failure. but it looks like a varnish config issue - but as its specifically hhvm this is strange. |
|
It’s probably not the Varnish config per se, but HHVM not serving the responses correctly: I’m seeing 404s and 503s (backend fetch failed). |
|
i just restarted the last build of master in https://travis-ci.org/FriendsOfSymfony/FriendsOfSymfony/FOSHttpCache/jobs/77916788 it now fails with the same pattern. i think this means that travis updated their hhvm and we see some regression. ok if we merge as-is and look into fixing hhvm in a separate pull request? |
|
we see the same test failure on master as well. @ddeboer wdyt, should we just merge this? and would you have some time to look into what hhvm is unhappy about? |
|
fixed the last typo from the comments. @ddeboer is it ok if we merge this? hhvm fails the same way as on master, so imho we should not block this merge. and fix hhvm against master, or disable it if we can't fix it. |
Provide Symfony HttpCache as trait
Fix #191, fix #238.