Skip to content

Fix php error #639

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

Merged
merged 2 commits into from
Jan 9, 2025
Merged

Fix php error #639

Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
5cfbee5
Fix php error
richardhj Jan 9, 2025
a13756b
Introduce DEFAULT_TTL_HEADER_NAME constant
richardhj Jan 9, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/DependencyInjection/FOSHttpCacheExtension.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,7 @@ public function load(array $configs, ContainerBuilder $container): void

if ($config['debug']['enabled'] || (!empty($config['cache_control']))) {
Copy link
Contributor

@alexander-schranz alexander-schranz Jan 9, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

@dbu Do we want to have the CacheControlListener registered if only the new ttl_header config is defined, for maybe just the debug ENVs?

If not we would need to go with: count($config['cache_control']) > 1 and addDefaultsIfNotSet in the config. The ?? would not be required then also.

Copy link
Contributor

Choose a reason for hiding this comment

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

i can't see a use case for configuring the ttl_header but not configuring any reverse_proxy_ttl at all.

if we merge as is now, my understanding is that the behaviour is the same as before, except that the ttl_header configuration is respected if overwritten, but does not cause errors if not explicitly mentioned by the user. am i missing something?

Copy link
Contributor

Choose a reason for hiding this comment

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

yes it will not break, it just register a listener which does then nothing as no rules are configured.

Copy link
Contributor

Choose a reason for hiding this comment

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

i clarified with alexander, we leave it like this. the cleanest way would be to count if there are any cache_control.rules, but this is not the bug and could have edge case side effects if somebody relies on the cache control service to exist even without rules if there is any configuration.

could be a cleanup for 4.x. when we count the rules, we can also addDefaultsIfNotSet on the ttl_header

$debugHeader = $config['debug']['enabled'] ? $config['debug']['header'] : false;
$ttlHeader = $config['cache_control']['ttl_header'];
$ttlHeader = $config['cache_control']['ttl_header'] ?? null;
$container->setParameter('fos_http_cache.debug_header', $debugHeader);
$container->setParameter('fos_http_cache.ttl_header', $ttlHeader);
$loader->load('cache_control_listener.xml');
Expand Down
Loading