Skip to content

Conversation

@andrerom
Copy link
Contributor

@andrerom andrerom commented Nov 7, 2017

In the need for specifying tags.header config, I copied over logic from CoreBundle.

@bdunogier Is this ok approach? I would assume we need it here anyway once stuff like this is removed from kernel. Order might be open question here tough, this bundle is registered before kernel,. so kernel might overwrite the values it does have (all but tags section) if we attempt to change them here (but we don't, so for now it's fine afaik).

@andrerom andrerom requested a review from bdunogier November 7, 2017 16:39
@bdunogier
Copy link
Contributor

We need that in this package, indeed. Looks good to me.

@Plopix
Copy link
Contributor

Plopix commented Nov 8, 2017

+1

Copy link
Member

@vidarl vidarl left a comment

Choose a reason for hiding this comment

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

Well. Maybe looks good. but doesn't work

For instance, the "fos_http_cache.tags.header" parameter is never set based on the yml config, so #25 does not work even with this PR applied ( I have to explicitly set the fos_http_cache.tags.header parameter )

@andrerom
Copy link
Contributor Author

andrerom commented Nov 8, 2017

Ok, was afraid of that, then we need a pass or custom config for this ala what fos does for tag handler (but on 1.x tag handler seems to only work if varnish is configured, so I think we need something abstract of this): https://github.com/FriendsOfSymfony/FOSHttpCacheBundle/blob/master/src/DependencyInjection/FOSHttpCacheExtension.php#L401

@andrerom
Copy link
Contributor Author

andrerom commented Nov 8, 2017

@vidarl Checked this and this PR works as intended, debugging config shows they have been set.

What is wrong is that in #25 I assumed this would be available as param as well, it's not, so I need to do a pass there to extract this. There is fos_http_cache.tag_handler.header, however it and rest of tag handler is only set if proxy client is "varnish" or "custom" so I can not rely on it: https://github.com/FriendsOfSymfony/FOSHttpCacheBundle/blob/1.3/DependencyInjection/FOSHttpCacheExtension.php#L331-L338

Hence the updated Warning inline here..

Merging this and continuing in #25 to get the right value out.

@andrerom andrerom merged commit 250149c into master Nov 8, 2017
@andrerom andrerom deleted the fos_config branch November 8, 2017 13:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

5 participants