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 EZP-21547: headers not sent for cached errors #758

Merged
merged 1 commit into from
Sep 17, 2013

Conversation

bdunogier
Copy link
Member

Fixes https://jira.ez.no/browse/EZP-21547.

Maybe not the right way to do it, but it at least highlights what needs to be fixed.

  1. Makes sure error headers are stored in cache data (in addition to the error code & message)
  2. Sends those headers again when using the cached version

Testing

Without the patch, if you go to http://ez/Media/ as anonymous user, you get a 401. If you reload, you will get a 200 OK.
With the patch, you will consistently get a 401.

@gggeek
Copy link
Contributor

gggeek commented Sep 16, 2013

Maybe the correct way would be not to cache error page at all?
It is true that by never sending an http 200ok we make downstream caching proxies behave better, but in the end isn't the view-cache just another internal-http-cache in the end? (thinking the way that Sf does brings lots of clarity to the process ;-) )

@bdunogier
Copy link
Member Author

Well, except that we don't have Symfony here.

I haven't traced back the whole history, but the use-case seems quite clear to me: you have a members section, and point people to an entry page of this section, to show them a login screen OR a registration form. If the page is heavily requested and uncached, the load on the system might be quite high.

It isn't ideal from an HTTP cache perspective, but after all it is not HTTP cache :-)

@lolautruche
Copy link
Contributor

+1

1 similar comment
@yannickroger
Copy link
Contributor

+1

@bdunogier
Copy link
Member Author

I'm wondering if @dpobel isn't -1 on the heade( $headers[] = ... ) part... :-)

@patrickallaert
Copy link
Contributor

+1

@dpobel
Copy link
Contributor

dpobel commented Sep 17, 2013

I'm not fan of this syntax, but I can live with that anyway :)

@bdunogier
Copy link
Member Author

Syntax changed, merging.

The headers are added to the module result, and stored
in cache. They are then sent to the browser when rendering
the result, cached or not.
@bdunogier bdunogier merged commit 57171cd into master Sep 17, 2013
@bdunogier
Copy link
Member Author

Merged to master.

@bdunogier bdunogier deleted the fix-EZP-21547-401_cached_error branch September 17, 2013 13:49
@bdunogier
Copy link
Member Author

Extra commit: 1f86c9a.

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.

6 participants