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.

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

Fix example in expr.xml #485

Open
wants to merge 2 commits into
base: trunk
Choose a base branch
from

Conversation

nisbet-hubbard
Copy link

The original example doesn’t actually work. This PR replaces it with a functional snippet along the lines of ex. 4 from mod_headers.

@@ -761,7 +761,8 @@ Require expr replace(%{REQUEST_METHOD}, 'E', 'O') == 'GET'"
Header set foo-checksum "expr=%{md5:foo}"

# This delays the evaluation of the condition clause compared to <If>
Header always set CustomHeader my-value "expr=%{REQUEST_URI} =~ m#^/special_path\.php$#"
SetEnvIf REQUEST_URI ^/special_path\.php$ special
Header always set CustomHeader my-value env=special

Copy link
Member

@covener covener Oct 1, 2024

Choose a reason for hiding this comment

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

The original works fine for me and the replacement doesn't use expressions

$ curl -vk http://localhost/special_path.php 2>&1 |grep CustomHeader
< CustomHeader: my-value
$ curl -vk http://localhost/other_path.php 2>&1 |grep CustomHeader
$

Copy link
Member

Choose a reason for hiding this comment

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

What I find surprising about the existing example and comment is that delaying the evaluation of REQUEST_URI doesn't really make much sense. it should really check something like the CONTENT_TYPE or REQUEST_STATUS.

Copy link
Author

Choose a reason for hiding this comment

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

The original works fine for me

Yes, sometimes it does. A case where it doesn’t is when an existing header is set by the application which one would like to override

@header( 'Cache-Control: no-cache' );

Copy link
Member

Choose a reason for hiding this comment

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

There's a different result from the replacement directives? It seems like the override case is more likely about "onsuccess" vs "always" then about using an expression or an env condition

Copy link
Author

Choose a reason for hiding this comment

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

That was my first thought. So I tested each version with always and implicit onsuccess

Header always set Cache-Control "max-age=604800" "expr=%{REQUEST_URI} =~ m#^/wp\.serviceworker$#"
SetEnvIf Request_URI ^/wp\.serviceworker$ worker
Header always set Cache-Control "max-age=604800" env=worker

Only the second works.

Copy link
Author

Choose a reason for hiding this comment

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

@covener I’ve identified the condition under which REQUEST_URI doesn’t work in expr=.

It’s when the path refers to something that’s not a static file, but dynamically generated by an application. This also applies to <If >. Previously, we observed the same limitation with <FilesMatch >, though I didn’t connect the dots at the time.

If you think this behaviour should be preserved, I suppose we can highlight the rule whenever it applies.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants