-
Notifications
You must be signed in to change notification settings - Fork 83
[WIP][RFC] Fastly support #427
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
will need to be updated once FriendsOfSymfony/FOSHttpCache#451 is wrapped up |
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.
+1
Besides the composer change suggested, and a rebase, anything else needed here @dbu?
Co-authored-by: André R. <289757+andrerom@users.noreply.github.com>
Co-authored-by: André R. <289757+andrerom@users.noreply.github.com>
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.
uh, i completely forgot to look at this again after fastly support was merged into FOSHttpCache
this looks good, i think there are some things to be removed.
and can you please add a test to ConfigurationTest? testSupportsFastly, similar to testSupportsVarnish and the others.
->end() | ||
->scalarNode('http_client') | ||
->defaultNull() | ||
->info('Httplug async client service name to use for sending the requests.') |
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.
if not specified, we use a default client, right? i notice that we do not have this option for the other caching systems like varnish, why do we need it for fastly?
} | ||
|
||
$definition = new Definition(HttpDispatcher::class, [ | ||
['api.fastly.com'], |
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.
imho we should have this as default value in the configuration. maybe some people have an experimental fastly api, or a test system with fastly or something... it feels bad to have this as a string in here.
$baseUrl = null; | ||
} | ||
|
||
$definition = new Definition(HttpDispatcher::class, [ |
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.
why do we create the definition for the dispatcher in PHP and not in the service configuration file? we could set the 3 parameters as container parameters instead, that would be more consistent with the other services.
|
||
<services> | ||
<service id="fos_http_cache.test.proxy_server.fastly" | ||
class="FOS\HttpCache\Test\Proxy\FastlyProxy"> |
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.
i don't think a FastlyProxy class exists.
we use this pattern to start a varnish / nginx instance during testing. i don't think we can do that for fastly.
can we remove this config file and the DI method that loads it?
@hpatoio You ok with doing the fix-ups or would you like us to take over on it? |
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.
To allow tag support, fastly also needs to be included here:
if (!in_array($client, ['varnish', 'symfony', 'custom', 'noop'])) { |
and with changes in Master, included here to disregard http.servers
: https://github.com/FriendsOfSymfony/FOSHttpCacheBundle/blob/master/src/DependencyInjection/Configuration.php#L492
@@ -318,6 +318,9 @@ private function loadProxyClient(ContainerBuilder $container, XmlFileLoader $loa | |||
if (isset($config['nginx'])) { | |||
$this->loadNginx($container, $loader, $config['nginx']); | |||
} | |||
if (isset($config['fastly'])) { | |||
$this->LoadFastly($container, $loader, $config['fastly']); |
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
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.
Hi @proeinfo
So did you try this as well? Would be great if that is the case.
Either way, would you be able to use the github suggest feature to suggest fixes to the two things you point out here?
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.
Hi @andrerom
I'm working on getting this to work, will post patch once that happens.
An additional issue: since Fastly doesn't use proxy servers for invalidating, the HttpDispatcher's approach of running through proxy servers (done in the A simple solution is to make a copy of the HttpDispatcher class and remove any mentions of servers and baseUris. Attached example of such a class. This would just require that the Fastly proxy client is instantiated with the FastlyDispatcher instead: $some_example_tags = [
'product1',
'product2',
];
$httpDispatcher = new \FOS\HttpCache\ProxyClient\FastlyDispatcher();
$options = [
'service_identifier' => '...',
'authentication_token' => '...',
'soft_purge' => true,
];
$fastly = new \FOS\HttpCache\ProxyClient\Fastly($httpDispatcher, $options);
$cacheInvalidator = new \FOS\HttpCache\CacheInvalidator($fastly);
$cacheInvalidator->invalidateTags($some_example_tags)->flush(); |
Additionally, since Fastly uses space separated surrogate keys, the default comma-separated tag header formatter won't work. Example implementation: <?php
namespace FOS\HttpCache\TagHeaderFormatter;
class FastlyTagHeaderFormatter implements TagHeaderFormatter
{
const FASTLY_TAG_HEADER = 'Surrogate-Key';
const FASTLY_TAG_HEADER_GLUE = ' ';
/**
* @var string
*/
private $headerName = self::FASTLY_TAG_HEADER;
/**
* @var string
*/
private $glue = self::FASTLY_TAG_HEADER_GLUE;
/**
* {@inheritdoc}
*/
public function getTagsHeaderName()
{
return $this->headerName;
}
/**
* {@inheritdoc}
*/
public function getTagsHeaderValue(array $tags)
{
return implode($this->glue, $tags);
}
} From there its a simple matter of overriding the service id in the user Symfony application: services:
fos_http_cache.tag_handler.header_formatter:
class: FOS\HttpCache\TagHeaderFormatter\FastlyTagHeaderFormatter
|
Adding Fastly support. At the moment only tag invalidation. Goes with FriendsOfSymfony/FOSHttpCache#403