U4-6631 - A physically nonexistent template results to an empty ContentResult and http status 200

Created by Meixger 18 May 2015, 09:37:48 Updated by Stephan 15 Jun 2015, 15:37:15

When the associated template is not found, a 404/503 should be thrown.

Actually an empty ActionResult will be returned: return Content(""); Also the http status code is 200

@Shandem https://github.com/umbraco/Umbraco-CMS/blob/a57aa0772111a1c281e6c9ceb199164cf7a4b0fa/src/Umbraco.Web/Mvc/RenderMvcController.cs#L99

Comments

Shannon Deminick 18 May 2015, 10:14:22

Pretty sure this has been discussed a few times previously... maybe here? https://groups.google.com/forum/?fromgroups#!searchin/umbraco-dev/template$20404/umbraco-dev/AfTxEQGwWH0/QtymyqIWBukJ

Pretty sure @zpqrtbnk will have some opinions on when/where this discussion ended up


Meixger 18 May 2015, 10:56:46

The default 404 behavior looks pretty fine to me. Still, an empty ContentResult doesn't never make any sense to me.

In this specific case (missing template file) i would expect an error or 404 page.

ftr https://groups.google.com/forum/?fromgroups#!searchin/umbraco-dev/template$20blank/umbraco-dev/-Cn1v1ydZo8/ZFZU4XV9WgsJ


Shannon Deminick 20 May 2015, 01:31:39

Have assigned to stephen to get his feedback, if we make this change we'll have to indicate that it is a breaking change.


Stephan 20 May 2015, 06:08:39

So... this is when we're rendering a content and we have been able to identify a template to use (from the content & template in database) yet at the time of rendering, the actual template file is nowhere to be found, correct?

I guess at the moment we return an empty content "by default", there is no strong reason to do so. What else could we do?

  • 4xx code: these are "client errors" according to standards, and this is not really a client error, as the content exists and the request is valid, only we cannot render it.
  • 5xx code: "server errors" make more sense, if the template exists in the DB then it's an error that the corresponding file does not exist. 503 is really a "the server is down" message... so to me the most appropriate would be "500 Internal Server Error - The template is missing".

And then... maybe that's a bit extreme. If no template is found by PublishedContentRequestEngine, the request is passed on to MVC and RenderRouteHandler.GetHandlerOnMissingTemplate transfers to PublishedContentNotFoundHandler - so we display a warning message but not a 500 error.

So in my mind we need to pick one of:

  • just throw, will cause err 500, because a missing physical file is bad
  • or (better?) don't return Content("") but reuse PublishedContentNotFoundHandler with a message eg "No physical file exists for template ... to render the document at url ...".

Let me know what you'd like.


Meixger 20 May 2015, 06:53:18

Upon reflection i see this specific case really as an error 503.

  • 4xx code: I agree, if the editor's intention was to create a working page and the template exists in the DB but not on disk, then a 4xx would be inappropriate. Even with a 'The template is missing' message.

  • 5xx code: If the template exists in the DB but not on disk [anymore], some admin/developer should step in an fix it. But without an exception the error would simple be missed.

Regarding the error code, 503 is really a temporary error. You tell search engine crawlers that the downtime is temporary.

I would show crawlers a 503 and catch it in my global error handler where i log it and show the user a nice [temporary] error page.


Stephan 20 May 2015, 09:59:52

If you have a global error handler that logs errors and show the user a nice error page, then crawlers will not see the 503, right? Then we might as well throw an exception in the controller, which will be handled by the error handler.

We'd just invoke EnsurePhysicalViewExists(template) which would return void, and throw instead of logging.

Thoughts?


Meixger 20 May 2015, 10:12:23

Oh no, my global error handler doesn't alter the http status code! So the crawler will see a 503 and the user will see a nice error page.

Yes, throw instead of logging would be fine for me.


Shannon Deminick 05 Jun 2015, 16:25:26

Changing this to a 'breaking change' because it's changed behavior


Stephan 15 Jun 2015, 13:04:32

Have we reached an agreement on that one? Proposing to replace the line

return Content("");

with

throw new Exception("No physical file template file was found for template " + template);

And then the site should have a proper catch-all page for 500 errors. Thoughts?


Shannon Deminick 15 Jun 2015, 13:25:24

I think that is correct, but we will mark this issue as 'breaking' since some people might be relying on that behavior and will need to change it.


Shannon Deminick 15 Jun 2015, 13:25:36

(ah, it's already marked as breaking)


Stephan 15 Jun 2015, 15:37:11

Fixed: ee0297bb69e86a499fe75aa9ee2a6305d18e3b68


Priority: Normal

Type: Bug

State: Fixed

Assignee: Stephan

Difficulty: Normal

Category:

Backwards Compatible: False

Fix Submitted:

Affected versions:

Due in version: 7.3.0

Sprint:

Story Points:

Cycle: