Fix php error

[richardhj]

Fixes #638

[alexander-schranz]

Think the CacheControlListener need also be adopted. I'm getting:

TypeError: FOS\HttpCacheBundle\EventListener\CacheControlListener::__construct(): Argument #2 ($ttlHeader) must be of type string, null given, called in /home/runner/work/sulu/sulu/src/Sulu/Bundle/WebsiteBundle/Tests/Application/var/cache/website/test/ContainerY4qBT6C/Sulu_Bundle_WebsiteBundle_Tests_Application_KernelTestDebugContainer.php on line 530

Update see: #639 (comment)

[dbu]

damn, did not realize how defaults in the configuration work. and looks like we did not add enough tests to discover this issue, sorry.

to avoid side effects like always enabling the cache control listener even when not wanted, lets use the fallback in the FOSHttpCacheExtension class as @94noni originally proposed with the default value if nothing is in the array.

the default will then only matter if people write ttl_header: ~ but afaik it is best practice to do that.

to make things more explicit, lets add a public constant in the src/EventListener/CacheControlListener.php class and reference that both from Configuration and from FOSHttpCacheExtension to avoid the magic string. do you want to do that @richardhj ? then i can tag a patch version with the fix.

[richardhj]

Should be ready to review

[alexander-schranz]

@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.

[dbu]

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?

[alexander-schranz]

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

[dbu]

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

[94noni]

lets do it that way in fine 👍🏻

[dbu]

tagged https://github.com/FriendsOfSymfony/FOSHttpCacheBundle/releases/tag/3.1.1