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.

Sign up for GitHub

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

Jump to bottom

config of module.php cache headers #1189

Merged
merged 4 commits into from Sep 11, 2019
Merged

config of module.php cache headers #1189

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
96558bb
config of module.php cache headers
melanger Aug 21, 2019
54c806f
Simplify Module.php static asset cache configuration
jornane Sep 6, 2019
fa02e49
Add forgotten newline
jornane Sep 6, 2019
0c14180
Document static asset cache configuration
jornane Sep 6, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
31 changes: 31 additions & 0 deletions config-templates/config.php
View file
Open in desktop
Expand Up @@ -861,6 +861,37 @@
*/
'production' => true,

/*
* Set this option to adjust basic caching directives sent by module.php.
*
* Defaults to ['public' => true, 'max_age' => 86400]
*/
// 'module.php.cache' => ['public' => true, 'max_age' => 86400],
Copy link
Member

Choose a reason for hiding this comment

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

I think the configuration could use some refactor and rewording. Naming is always hard. In this case, I don't think "module.php.*" tells anything of what this config options do to someone who's not looking at this PR. Also, our current practice is to try to group together configuration options that are related (e.g. the application option or the debug option). This makes it easier to configure the software, in my opinion, and to document options that are related. I would follow the same approach.

As I said, naming is hard, but since this is related to caching of assets, would it make sense to call it just assets, then add a caching option inside? In my head, this makes sense as we might want to configure other stuff about assets in the future (e.g. the base URL for them, if we want to offload assets to their own server). In this case, and as far as I can see, almost all the available options that can be passed to symfony are set in the Cache-Control HTTP header, and there are directives for ETag and Last-Modified. I think it makes sense to have something like this then:

    /**
     * Explanation of the assets directive
     */
    'assets' => [
        'caching' => [
           /**
            * Explanation of the supported caching-related directives
            */
            'public' => true,
            'max_age' => 86400,
            'etag' => false,
            'last_modified' => ...
        ],
    ],


/*
* Set this option to adjust expiration timestamp sent by module.php.
* The value (in seconds) is added to current time.
* If set to null, the Expiration header is not sent at all.
*
* Defaults to 10 * 60 (ten minutes).
*/
// 'module.php.expires' => 10 * 60,
Copy link
Member

Choose a reason for hiding this comment

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

I think this should go as well inside a general caching group (name again is orientative).


/*
* Set this option to make module.php automatically send Etag.
*
* Defaults to false.
*/
// 'module.php.etag' => false,
Copy link
Member

Choose a reason for hiding this comment

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

This one would be covered already by my previous suggestion.


/*
* Set this option to adjust more caching directives sent by module.php,
* for example must-revalidate. The option takes an array
* where keys are the caching directive names and values are the corresponding values.
*
* Defaults to [] (empty array).
*/
// 'module.php.directives' => [],
Copy link
Member

Choose a reason for hiding this comment

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

This is very confusing. I think we need to either move every directive we want to support to the general caching configuration group, or remove it. My main problem with this is that it forces you to know Symfony in order to configure it. Flexibility is nice, but I wouldn't depend on any Symfony-specific code.



/*********************
Expand Down
18 changes: 14 additions & 4 deletions lib/SimpleSAML/Module.php
View file
Open in desktop
Expand Up @@ -265,11 +265,21 @@ public static function process(Request $request = null)
}

$response = new BinaryFileResponse($path);
$response->setCache(['public' => true, 'max_age' => 86400]);
$response->setExpires(new \DateTime(gmdate('D, j M Y H:i:s \G\M\T', time() + 10 * 60)));
$response->setLastModified(new \DateTime(gmdate('D, j M Y H:i:s \G\M\T', filemtime($path))));
$cacheSettings = $config->getArray('module.php.cache', ['public' => true, 'max_age' => 86400]);
Copy link
Member

Choose a reason for hiding this comment

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

Here we could just get the caching config group, process it to validate the supported options, and then go ahead acting accordingly for each of them. Those that cannot be passed to setCache() could be processed first and removed from the array, and then the remaining could be passed to setCache() directly. It might be necessary to have some validation in place, although I believe Symfony would handle that for us.

$response->setCache($cacheSettings);
$cacheExpiration = $config->getValue('module.php.expires', 10 * 60);
$expires = $cacheExpiration === null ? null : new \DateTime('@' . (time() + $cacheExpiration));
$response->setExpires($expires);
$cacheEtag = $config->getBoolean('module.php.etag', false);
if ($cacheEtag) {
$response->setAutoEtag();
jaimeperez marked this conversation as resolved.
Show resolved Hide resolved
}
$cacheDirectives = $config->getArray('module.php.directives', []);
foreach ($cacheDirectives as $directiveName => $directiveValue) {
$response->headers->addCacheControlDirective($directiveName, $directiveValue);
}
$response->isNotModified($request);
$response->headers->set('Content-Type', $contentType);
$response->headers->set('Content-Length', sprintf('%u', filesize($path))); // force file size to an unsigned
$response->setContentDisposition(ResponseHeaderBag::DISPOSITION_INLINE);
$response->prepare($request);
return $response;
Expand Down