tractorcow / silverstripe-dynamiccache

Simple on the fly caching of dynamic content for Silverstripe
39 stars 27 forks source link

Cached 404 pages returns 200 header #23

Closed keskistok closed 8 years ago

keskistok commented 9 years ago

I'm using version 3.1 of the module. I noticed that 404 "not found" pages gets cached and the second time the same url is requested it returns a 200 header. This is a problem since it could lead a lot of "not found" pages getting indexed in google. Is there an easy way to avoid this behaviour?

tractorcow commented 9 years ago

This should be fixed... the http response code in general should be cached as well.

jonom commented 8 years ago

Hey @tractorcow finally trying out this module for the first time and it's sweet. Thanks for making it! I noticed that for me error pages aren't being cached at all - I assume because ErrorPage is excluded in the module's config:

# Specify page types to skip caching for
  ignoredPages:
    - ErrorPage
    - UserDefinedForm

Any tips for caching 404 responses? Definitely would like to be caching these at my end.

tractorcow commented 8 years ago

Remove the page from ignoredPages if you like. :D

jonom commented 8 years ago

Hehe should have seen that one coming. Just wondering if there's a good reason for having ErrorPage excluded out of the box? Makes sense to enable caching of 404s doesn't it and a 500 response wouldn't get as far as cache generation so I would think a better default is to cache them. Also wonder if UserDefinedForm still needs to be excluded now that you've got SecurityIDs being inserted?

tractorcow commented 8 years ago

Well, if you add a page, I suppose you don't want it continuing to be reported as 404. That would be the exception wouldn't it? It seems a bit silly now that I think about it. You'd just flush to clear.

Shall we just dump these exceptions?

tractorcow commented 8 years ago

UserDefinedForm is excluded because of validation messages not because of SecurityID. You need session-specific messages to appear on redirect.

jonom commented 8 years ago

Well, if you add a page, I suppose you don't want it continuing to be reported as 404. That would be the exception wouldn't it? It seems a bit silly now that I think about it. You'd just flush to clear.

Yeah out of the box the whole cache is purged every time you publish a page or write a DataObject so that should cover the new page argument.

UserDefinedForm is excluded because of validation messages not because of SecurityID

Is that covered by this: https://github.com/tractorcow/silverstripe-dynamiccache/blob/3.1/code/DynamicCache.php#L73-L81 or could we expand that code to cover UserForms so the initial page load can be cached?

jonom commented 8 years ago

I'm not sure that form error stuff works actually - when I trigger a validation error on a form I get a 200 ok response at /Form and a white screen, the redirection back to the form page doesn't happen. I'll see if I can work out what's happening.

tractorcow commented 8 years ago

Sooner or later I'll have to test this module out again.

I'm ashamed I haven't done anything on this module for years. =(

jonom commented 8 years ago

You're a busy man! If I can fix any little issues I'll open a PR.

jonom commented 8 years ago

Never mind about that form issue - worked out it's only happening when I have a port number in the URL which causes Director::isSiteURL() to fail. parse_url() is stuffing up on the output of Director::protocolAndHost() for some reason and including the port when it shouldn't. screen shot 2015-11-20 at 4 56 47 pm Doing my head in. Edit: CodeKit app changing my browser output on the fly and messing with my head :D

tractorcow commented 8 years ago

Fixed with #30