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

FEATURE: Add Setting queryParams|cookieParams.respect to invert the control #22

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
+76 −4
Open

FEATURE: Add Setting queryParams|cookieParams.respect to invert the control #22

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
22 changes: 20 additions & 2 deletions Classes/Middleware/RequestCacheMiddleware.php
View file
Original file line number Diff line number Diff line change
@@ -46,12 +46,24 @@ class RequestCacheMiddleware implements MiddlewareInterface
*/
protected $ignoredQueryParams;

/**
* @var array
* @Flow\InjectConfiguration(path="request.queryParams.respect")
*/
protected $respectedQueryParams;

/**
* @var array
* @Flow\InjectConfiguration(path="request.cookieParams.ignore")
*/
protected $ignoredCookieParams;

/**
* @var array
* @Flow\InjectConfiguration(path="request.cookieParams.respect")
*/
protected $respectedCookieParams;

/**
* @var boolean
* @Flow\InjectConfiguration(path="maxPublicCacheTime")
@@ -127,13 +139,17 @@ protected function getCacheIdentifierForRequestIfCacheable(ServerRequestInterfac
$requestQueryParams = $request->getQueryParams();
$allowedQueryParams = [];
$ignoredQueryParams = [];
$respectedQueryParams = [];
$disallowedQueryParams = [];
foreach ($requestQueryParams as $key => $value) {
switch (true) {
case (in_array($key, $this->allowedQueryParams)):
$allowedQueryParams[$key] = $value;
break;
case (in_array($key, $this->ignoredQueryParams)):
case (in_array($key, $this->ignoredQueryParams) && empty($this->respectedQueryParams)):
$ignoredQueryParams[$key] = $value;
break;
case (!in_array($key, $this->respectedQueryParams) && empty($this->ignoredQueryParams)):
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 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.

Copy link

Choose a reason for hiding this comment

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

@mficzel How would you configure FullPageCache to not cache a search query in this case?

$ignoredQueryParams[$key] = $value;
break;
default:
@@ -148,7 +164,9 @@ protected function getCacheIdentifierForRequestIfCacheable(ServerRequestInterfac
$requestCookieParams = $request->getCookieParams();
$disallowedCookieParams = [];
foreach ($requestCookieParams as $key => $value) {
if (!in_array($key, $this->ignoredCookieParams)) {
if (!in_array($key, $this->ignoredCookieParams) && !empty($this->ignoredCookieParams) && empty($this->respectedCookieParams)) {
$disallowedCookieParams[$key] = $value;
} else if (in_array($key, $this->respectedCookieParams) && empty($this->ignoredCookieParams)) {
$disallowedCookieParams[$key] = $value;
}
}
29 changes: 28 additions & 1 deletion Configuration/Settings.yaml
View file
Original file line number Diff line number Diff line change
@@ -16,9 +16,22 @@ Flowpack:
cookieParams:
# ignored cookie params exclude cookies that are handled by the frontend
# and are not relevant for the backend. A usecase would be gdpr consent cookies
# if they are only used on the client side
# if they are only used on the client side.
# Example:
# - 'consent_settings'
# => If the cookie "consent_settings" is present, it will be cached.
# But if a "_ga" cookie is present, the request will not be cached.
# Important: Only evaluated if "respect" is empty!
ignore: []

# respected cookie params are relevant for the backend and will skip the caching.
# Example:
# - 'Neos_Session'
# => If the "Neos_Session" cookie is present in the request, it will not be cached.
# But if a "_ga" cookie is present, the request will be cached.
# Important: Only evaluated if "ignore" is empty!
respect: []

# a request will only qualify for caching if it only contains queryParams that
# are allowed or ignored. All other arguments will prevent caching.
queryParams:
@@ -29,8 +42,22 @@ Flowpack:
# ignored arguments are not part of the cache identifier but do not
# prevent caching either. Use this for arguments that are meaningless for
# the backend like utm_campaign
# Example:
# - 'utm_campaign'
# => If the "utm_campaign" argument is present in the request, it will be cached.
# But if a "utm_source" argument is present, the request will not be cached.
# Important: Only evaluated if "respect" is empty!
ignore: []

# respected arguments are relevant for the backend and will skip the caching.
# All other arguments will be ignored and cached as if they weren't there.
# Example:
# - 'search'
# => 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: []
Copy link
Contributor

Choose a reason for hiding this comment

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

Setting empty Arrays is a bit nasty, as this will override already set configurations in packages loaded before 🤔

Copy link
Member

Choose a reason for hiding this comment

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

Why should another package write this config before this one?

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

But it's not the business of a package to deal with incorrect dependencies.

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

paavo reviewed yesterday

Oh, i just wanted to drop a comment 🙈 😆
Nothing reviewed so far 😆

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

Then your site package needs also an require-dev or suggest entry for your dev package.

Copy link
Member

Choose a reason for hiding this comment

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

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


Neos:
Flow:
http:
29 changes: 28 additions & 1 deletion README.md
View file
Original file line number Diff line number Diff line change
@@ -29,9 +29,22 @@ Flowpack:
cookieParams:
# ignored cookie params exclude cookies that are handled by the frontend
# and are not relevant for the backend. A usecase would be gdpr consent cookies
# if they are only used on the client side
# if they are only used on the client side.
# Example:
# - 'consent_settings'
# => If the cookie "consent_settings" is present, it will be cached.
# But if a "_ga" cookie is present, the request will not be cached.
# Important: Only evaluated if "respect" is empty!
ignore: []

# respected cookie params are relevant for the backend and will skip the caching.
# Example:
# - 'Neos_Session'
# => If the "Neos_Session" cookie is present in the request, it will not be cached.
# But if a "_ga" cookie is present, the request will be cached.
# Important: Only evaluated if "ignore" is empty!
respect: []

# a request will only qualify for caching if it only contains queryParams that
# are allowed or ignored. All other arguments will prevent caching.
queryParams:
@@ -42,7 +55,21 @@ Flowpack:
# ignored arguments are not part of the cache identifier but do not
# prevent caching either. Use this for arguments that are meaningless for
# the backend like utm_campaign
# Example:
# - 'utm_campaign'
# => If the "utm_campaign" argument is present in the request, it will be cached.
# But if a "utm_source" argument is present, the request will not be cached.
# Important: Only evaluated if "respect" is empty!
ignore: []

# respected arguments are relevant for the backend and will skip the caching.
# All other arguments will be ignored and cached as if they weren't there.
# Example:
# - 'search'
# => 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: []
```

You can also move the cache backend to something faster if available, to improve performance even more.