FEATURE: Add Setting queryParams|cookieParams.respect to invert the control
#22
Conversation
…t add cookies and arguments which affect the caching. which makes configuration much easier.
| case (in_array($key, $this->ignoredQueryParams) && empty($this->respectedQueryParams)): | ||
| $ignoredQueryParams[$key] = $value; | ||
| break; | ||
| case (!in_array($key, $this->respectedQueryParams) && empty($this->ignoredQueryParams)): |
There was a problem hiding this comment.
I think a respected queryParameter should be added to $allowedQueryParams[$key] as i would assume this to be part of the cache identity.
That imho would also mean that allowedQueryParams should only be checked when $this->respectedQueryParams is empty.
There was a problem hiding this comment.
@mficzel How would you configure FullPageCache to not cache a search query in this case?
mficzel
changed the title
queryParams|cookieParams.respect to invert the control
|
@sbruggmann I really like this change. Have you seen the comment by Martin? Would you be willing to finish this PR? |
| # => If the "search" argument is present in the request, it will not be cached. | ||
| # But if a "utm_source" argument is present, the request will be cached as if the argument wouldn't be there. | ||
| # Important: Only evaluated if "ignore" is empty! | ||
| respect: [] |
There was a problem hiding this comment.
Setting empty Arrays is a bit nasty, as this will override already set configurations in packages loaded before 🤔
There was a problem hiding this comment.
Why should another package write this config before this one?
There was a problem hiding this comment.
Happens all the time when dependencies between packages are not properly declared. Not actually declaring this key but only documenting it helps in those cases.
There was a problem hiding this comment.
But it's not the business of a package to deal with incorrect dependencies.
There was a problem hiding this comment.
But it's not the business of a package to deal with incorrect dependencies.
We have a development package for example, we load in require-dev.
Loading a package there, will reset settings already set in another package.
So i personally prefer to not have empty configurations in configs @Sebobo
There was a problem hiding this comment.
@paavo but if that package has a "suggest" or "require" on this package, then then merge order is also fine and you won't have problems.
Or is the actual problem using arrays instead of objects?
There was a problem hiding this comment.
Let me make an example @Sebobo with Sitegeist.MagicWand:
We have our Site-Package where we define MagicWand Settings clonePresets: ....
Then we have a development package we load in require-dev (to only load this on DEV Environments).
This development package requires MagicWand.
And MagicWand sets default clonePresets: [] and overrides the Configuration in the Site-Package.
Or is the actual problem using arrays instead of objects?
The Problem it, that configurations get overridden.
Yes, FullpageCache is probably the opposite of a Dev-Package 😆
But i really prefer not overriding configurations with empty values.
There was a problem hiding this comment.
Then your site package needs also an require-dev or suggest entry for your dev package.
There was a problem hiding this comment.
I am undecided wether we should help here. On the one hand it is an easy fix to not actually write the empty config array. On the other hand projects with basically random loading order between interdepending packages will run into issues eventually that are hard to debug. So we may actually do those a favor ...
added new options
cookieParams.respectandqueryParams.respectwhich work the opposite way the optionscookieParams.ignoreandqueryParams.ignoredo.this way we define in the settings which cookies and arguments really matter to our code.
all others don't affect the caching.
especially with tag managers and different tracking we have a lot of changing cookies and arguments which are only meant for the frontend and should never affect the code generation and caching.
we as developers should have the option to just allow known cookies and arguments, and ignore the rest.