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

Allow to set response headers on error pages #39

Merged
merged 4 commits into from
Dec 5, 2017

Conversation

pkamps
Copy link
Member

@pkamps pkamps commented Feb 14, 2017

In ezp it's not possible to set custom response headers for error pages. ezp only allows you to set response headers by requested URLs (see HTTPHeaderSettings in site.ini).

This pull request allows you to specify custom response header for error pages on ezp. That's an important feature in case your site is behind a reverse proxy (CDN akamai, CloudFront, Varnish) and you control the caching TTLs with response headers.

You can test this pull request by accessing a URL that does not exists. From that request you should get a response with this header: 'Cache-Control:public, must-revalidate, max-age=300'

The only small complexity in this pull request is the fact that we re-use the 'Status' header in order to build the response status code - for example '404 Not Found'.

@pkamps
Copy link
Member Author

pkamps commented Feb 15, 2017

Pull request for ez systems:
ezsystems#1279

@benkmugo
Copy link
Member

benkmugo commented May 11, 2017

@pkamps If we want to avoid the HTTPName BC break Gaetano mentioned in the original PR, we could just retain HTTPName it and restructure the code.

Move it in place as a fallback so that HeaderList takes precedence if available and rework the old HTTPName logic so it uses the $Result['responseHeaders'] array the HeaderList coded uses.

That way installations using the old setting would continue to work and other installations could use the new setting as it takes priority.

The PR also has a ToDo still in place if that's not too involved maybe that can be done as part of this PR too?

@pkamps
Copy link
Member Author

pkamps commented Jun 23, 2017

I changed the approach so that it is not breaking backwards compatibility. It will use all HeaderList settings and in case there still is a deprecated _ HTTPName_ setting, it will that HTTPName for the response 'Status'. The rest of the code is like before.

I decided not to implement the TODO note I added in this pull request. The variable $Result['responseHeaders'] is maybe used later in the code execution - that's why I don't want to replace it with the PHP function http_response_code(). There is a risk of regression issues if I would do it.

@pkamps
Copy link
Member Author

pkamps commented Jul 7, 2017

@benkmugo Could you recheck please? See my previous comment for updates.

@benkmugo
Copy link
Member

Tested the new setting as well as the fallback for the now removed HTTPName setting.
Both working as expected.

Good to go from my end.

@pkamps pkamps merged commit 97690e9 into master Dec 5, 2017
@pkamps pkamps deleted the error_page_custom_response_headers branch December 5, 2017 22:27
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