Silverstripe-framework: RFC-2 Error page simplification

[jonom]

RFC-2 Move ErrorPage in to optional module

Author: @jonom
Status: Pending review
Version: 1.2.2 (2016-03-20)

Purpose and outcome

This RFC aims to address a few issues with the current state of error page responses and error page content management in SilverStripe CMS:

1. Flaws Statically generated error pages are under-utilised and go stale quickly.
2. Inflexibility 'Error content' management is not a requirement for many websites, and a different approach may be preferred where it is.
3. Confusion The ErrorPage content management model leads to a confusing CMS experience.

Since the current ErrorPage implementation is suitable for some sites but not all, this proposal recommends removing the ErrorPage class from core and moving it in to a separate optional module. This would extend upon a simpler error response model in core, and pave the way for developers to create alternate solutions for managing, displaying, and caching error response content that suits their needs better.

Community is welcome to give feedback. The proposal will be submitted to the core-committers for approval.

Motivation

Static error pages easily become stale and broken

They are generated on dev/build but only if they don't already exist. After that they are only regenerated when re-published from the CMS. This means the static pages can easily become very out of date, and even include dev-environment content rendered when the website project was first initiated.

Static error pages are not widely utilised

On a regular SilverStripe installation directives in the .htaccess file suggest that error responses will be handled efficiently by Apache serving up a static error page. This is true for a 500 response, but all 404 responses in fact go through the full SilverStripe request cycle and a dynamic error response is generated, meaning the static 404 file is never used. In the case of a fatal error Apache serves the 500 error page because PHP has aborted the response. Hopefully this situation doesn't happen often, but when it does it is likely that a stale error page is being served.

Inflexible error response model

• The use of ErrorPage content removes the ability to set a custom error message from code using ContentController::httpError($code, $message) ($message is ignored)
• Error responses which don't have a corresponding ErrorPage published in the CMS will show the content for a 500 Server Error which is misleading.

• Static error pages are only generated for the default language (can anyone confirm this?) and primary Subsite.

  ErrorPages confuse regular (non-technical) CMS users

• It doesn't make sense to show error pages in the Site Tree and allow drag and drop re-ordering, since they aren't part of a website's menu structure or information architecture.

• Non-technical CMS users are generally not interested in editing error page content
• Non-technical CMS users read the labels in the Site Tree and think something has gone wrong, not making the connection that there is content there they can edit
  

  Proposal

Remove ErrorPage from core and split off in to an optional module

The ErrorPage class brings with it some UX problems and flaws as outlined above, and is not needed in every CMS installation, so let's separate it out in to an optional module. This will make the CMS experience simpler and cleaner for users who don't need this functionality.

Provide a simple error response mechanism that can be overridden or replaced

RequestHandler::httpError() already provides a plain-text error response with default descriptions for a range of status codes, with extension points that allow you to customise the response to your liking. This means we already have a good base in place that can be built upon with modules. It may make sense to modify this base to provide a branded error page by default, using a site's Page.ss templates.

Don't cache error page content by default

We're not effectively caching error page content at the moment and don't have built-in caching for any other content so let's leave that functionality to dedicated modules.

(Except for the 500 error page)

We can't serve up a dynamically generated response in the event of a fatal error, so let's provide a simple static html page for this purpose. This could be manually replaced or customised if desired to match the look of the website. This may not be ideal for all websites but it is just a starting point that can be expanded upon with modules.

Other considerations

• This RFC does not propose any solutions for fixing existing problems with ErrorPage for developers that choose to go on using it as a module. Upgrades to that module would be a good idea but are out of scope for this RFC.
• Who will be responsible for maintaining the ErrorPage module? (SilverStripe Ltd?)
• Document upgrade path
  
  
  
  • install ErrorPage module or remove ErrorPage instances)
  
  
  • recommend some alternative modules and pros/cons if/when available
  
  
• Update docs
• Should Installer require ErrorPage as a dependancy or a simpler alternative?
  

  (Partial) Alternatives

Fix generation of static error pages

• How do we ensure error pages are regenerated regularly? dev/build is not done regularly and a basic SilverStripe installation doesn't currently require cron jobs to be set up. Generating and caching pages at request time may be an option.
• How will we know we need to serve up a static 404 without going through the request process and hitting the database? If we can't solve this problem any performance gains to be made by saving a static error page will be similar to serving up a 404 page that has been cached using partial caching.
• How to support multi-domains, translation, SSL vs non-SSL...?

These are difficult questions to answer so this doesn't seem like a good solution.

Separate out error pages from SiteTree

ErrorPages could be visually separated at the bottom of the SiteTree, or moved to the Settings section of the CMS, to reduce confusion and clutter. However if most websites don't actually need the ability to edit error page content in the CMS, and we're adhering to 'Convention over Configuration' then this functionality should be optional in the form of a separate module.

Impact

• The ErrorPage module will need to be installed on websites where that functionality is required, including:
  
  
  
  • Websites that rely on a fully-themed 500 error response
  
  
  • Websites that are making use of the static 404 page (non-standard configurations)
  
  
• If the ErrorPage module is not installed, developers will likely want to install an alternative module or implement their own solution to enhance error responses beyond plain-text.

[jonom]

If I'm wrong about anything here or left anything out please point it out. Thanks

[camfindlay]

awesome effort on this @jonom - I'll look at raising this with the core committers at the next hangout session. We can take a preliminary vote from them to see whether you'll want to do further community discussion to reach an acceptable solution to this :+1:

[jonom]

Sounds good!

[jonom]

By the way, I should note that while I think porting the ErrorPage functionality to a new module is important for backwards-compatibility, I'm not interested in maintaining this module myself as I don't want to use it in my own projects. So would be looking for SilverStripe Ltd or a developer who values this functionality to take ownership of it. I'm happy to otherwise take the development effort as far as I can but may need some help with the deeper elements (like the Controller error-handling chain).

[camfindlay]

@jonom there was some good points raised around your RFC in the core committers hangout if you want to check it out and perhaps address a few things. https://youtu.be/d6ADget_PfY?t=21m9s

[jonom]

Hey Cam, I tuned in for the hangout but I didn't catch anything in particular I haven't already addressed in the RFC. I think the main objections coming from Sam were that:

1. Being able to customise 404 content in the CMS is really important
2. Serving a statically cached 404 page is really important for performance
3. Serving a static pre-rendered (branded) 500 server error page is important

In response:

1. I just don't think this is true for the majority of users - in my experience most CMS users ignore the ability to edit the content of the 404 page. I'm sure it's useful in some cases but if it's not important to the _majority_ of users then I think it should be moved to an optional module.
2. I totally agree. But as far as I can tell SilverStripe simply doesn't do this. A static 404 file is generated but it doesn't ever get served up - even for requests in the assets directory. I'm dying to be proved wrong about this so please tell me if I'm mistaken :)
3. Agree - a static file is the only option here. But I don't think it's _critical_ for all websites that this page is rendered the same way as any other page on the site. As Hamish pointed out, fixing this won't be trivial as there are issues around translation, compatibility with the subsites module, and performance overhead of generating the static files. I'm not convinced there's a one-size-fits-all solution here, which is why I think it would be better to start with something simple, and any more complex behaviour could be added with a module.

Part of the goal here is flexibility, if we simplify the approach and ensure it can be overloaded then it opens the door for people to develop and choose between different solutions - including restoring the current approach if they wish. I'm not against static caching, I'm very much for it - I just think it should be addressed outside of core, or if it's left in core then it needs some serious TLC and should be available to be used for more than just error pages.

[stevie-mayhew]

I think that the error page _should_ be editable via the CMS. The need for dynamically generated 404 pages with some information from the CMS content field is also something which we rely on heavily - "Did you mean ..." is something useful for a 404 page.

I believe there is a way to compromise all of this, even if it means retaining some of the functionality that is current but pulling them out of the Site tree. If the content isn't customisable then it should be easy to set up and instantiate / generate / whatever all the error page conditions which you wish to use. An example of this is that we heavily use 403 forbidden pages, and almost always have this content customisable for the client as they need to change the information as to why people cannot access certain parts of the site based on rules which they can set via "access levels" for each page type. Losing this sort of functionality and having to rely on programatically generated responses diminishes the functionality of these page types dramatically and would force us to choose another approach.

[jonom]

Hi @stevie-mayhew, this RFC is proposing that ErrorPage becomes an optional module, so you shouldn't have to lose any functionality if you don't want to - just install the module.

I figure if this RFC is accepted it could lead to multiple options for developers when rolling out a site:

1. Accept the default error page title+content for each error status code (no customisation)
2. Override some or all of the default error page titles and content in a yml config file (basic customisation)
3. Bypass the config system and administer error page content in the CMS (advanced customisation, would require some custom code or installation of a module).

Whichever approach is used, it should be possible to override any of this content in code by calling httpError($code, $message, $title). I added the $title argument.

I think option No. 1 would be a fine starting point and suitable for many basic websites. For those sites it will be nice to have the CMS a bit less cluttered and confusing.

The point is that not everyone needs or wants error page content to be administered in the CMS so it shouldn't be forced on us - but for the developers that do want the functionality it should be available as an add-on.

[jonom]

p.s. I would draw a comparison with the Lost Password page. You can't administer the content for that page in the CMS but no one seems to mind. If you wanted to you could create a LostPassword.ss template and come up with your own way of customising that content. As a baseline behaviour I think this is fine, something to be built upon if people want to.

[nathanbrauer]

Has anyone looked into the possibility/ability of having SilverStripe serve the default Apache error pages instead? I wonder if there's a way to have PHP just tell Apache, "serve your 404 page," or a way to retrieve Apache's ErrorDocument settings which PHP can process from there....

[nathanbrauer]

Also need to keep multiple domains into consideration. As it stands, www.marketo.com is loaded through a cache proxy and the error pages use the domain name of our origin.

[jonom]

Yeah this is why I want SilverStripe to ship with a more basic approach to error handling that can be built upon - I think there is more than one solution and SilverStripe shouldn't push developers in to doing it one way but allow them to choose between different modules or implement their own approach.

I'm picturing perhaps a simple error response class in core that can be completely replaced through dependancy injection. I haven't delved in to the core code yet to see what's possible though.

[jonom]

@hafriedlander I think you were going to initiate some private discussion with other core devs who couldn't make it to the core dev hangout when this was raised - did anything come of that? If there's any specific things I need to address or revise in the RFC I look forward to hearing your thoughts - or if I'm on the right track here I look forward to receiving the go-ahead to start some work on it ;)

[chillu]

Jono, I think you've addressed Sam's concerns well, and I agree with your assessments.

You're right about 404 errors not using the static cached file. ModelAsController->getNestedController() calls ErrorPage::response_for(404). But that's defaulting to generating the page dynamically: https://github.com/silverstripe/silverstripe-cms/blob/3.1/code/model/ErrorPage.php#L53

I think @sminnee's performance concerns stem from the fact that the default .htaccess passes on ALL requests which aren't mapping to an existing file to SilverStripe (see https://github.com/silverstripe/silverstripe-installer/blob/3.1/.htaccess). So if you have a broken URL to an image on your server, these all run through the standard SilverStripe request cycle. I was under the impression that the ErrorDocument 404 /assets/error-404.html catches some of that, but that doesn't seem to be the case.

Overall, we're doing expensive SilverStripe 404 error page generation already in pretty much all cases, which means there's no performance regressions. Sam, am I missing anything here? That being said, I think we should address this issue. I would suggest caching the generated 404 template through SS_Cache for an hour or so, which should have roughly the same effect: It smooths out traffic spikes from bots. dev/build would clear the cache. Tradeoff between showing possibly outdated template data if it changed in the last hour, and allowing specific control via dev/build on deployments etc.

[jonom]

Thanks for the feedback and confirming my conclusions @chillu! Caching the generated 404 for an hour sounds like a good idea to me but I just think it's important that if a solution like this is added to core, it doesn't block other caching solutions being implemented. Some devs might want a dynamic response at all times or need a more complex caching solution.

I think caching in general is in it's infancy with SilverStripe and I'm really excited to see how modules like dynamiccache, cacheinclude and staticpublishqueue continue to develop. I want to be able to install a module like this on _every_ site I develop and have it cache my 404 responses along with all other content so I can get that instant responsiveness we all love, on all websites great and small.

I know caching of 404 responses is important in terms of server load but I think if we can nail the best way to do that, it should be able to applied to other content as well - so overall I feel like it should be handled by a module rather than core. Sam suggested that 'ErrorPage the module' is included with the Installer as a dependancy - perhaps we could also bundle a caching module and use it for 404s as a starting point? Then people could remove the module if they don't want it or switch it out with something else depending on their needs.

[chillu]

I'd keep any "pluggable error page caching" API small, since its such a special case. In the end, error pages are just another page which requires caching - they just happen to be requested more often on an average SilverStripe installation. In order to get this RFC across the line, I think any discussion about bundling caching modules are distracting (although I agree we need to do better in this regard).

@silverstripe/core-team Are you OK with Jono to start coding on this?

Jono, can you please update the RFC with the outcomes of the discussion above? Makes it easier for the core team to review the proposal without going through long comment threads (which for me was the whole point of RFCs heh).

[jonom]

@chillu I thought that everything I've written in my comments on this thread was already in the original RFC, so I obviously didn't explain myself very clearly in the beginning :) I've revised the RFC to hopefully be a lot clearer now.

[flashbackzoo]

Nice work on the RFC @jonom - especially for taking the time to consider the user experience.

@clarkepaul and I just had a chat about the UX aspect of your RFC. We agree that removing the error pages from the SiteTree is a good idea. But we're not sold on moving the ability to edit them into a module.

We have work planned for our current sprint, based on the UX research from November, which involves re-factoring the 'Filters' panel. An alternative solution to removing error pages entirely, might be to incorporate them in the re-factored menu.

Simply moving the current bad UX into a module doesn't solve the problem. It will resurface for anyone who installs the module.

We'll put together some wireframes of what the error pages might look, if relocated to the new menu, and post them here by the end of the week.

[jonom]

If you can provide a better interface for managing ErrorPage content that will be brilliant! I did hope that would be addressed as part of the separation in to a separate module. I still would prefer to see this be an optional module as for all of my clients the ability to edit error page content is just clutter in the CMS, and I'm sure I'm not the only one. Is this something we could do a poll on maybe to see how many website owners have actually taken advantage of the ability to edit their error messages? I can tell you that for all of my clients, the error pages have only ever been published by me, so I would have rather set the content in the config system.

[clarkepaul]

Hey, we are starting to work on splitting out the options for how you view the site tree from the search as this was the worst performing task from our tested audience. We asked them to find the deleted pages and not one user could perform the task from memory.

This could be a possible option to remove the error pages from the site tree but still allowing users to access them if needed, it is still a wireframe so likely to change. I still need to think how the error pages overlap with the pagetype select dropdown in the search area.

Anyways, let us know what you think.

[flashbackzoo]

@jonom The reason I'm against ErrorPage content editing being a module, is it goes against a CMS's core competency, enabling non-technical users to update their site's content. Although ErrorPages are not always customised, their content is publicly available by default, so that content should be manageable by default too. I don't think you should be required to install a module to manage out-of-the-box content.

But yeah a poll is definitely something we can look at doing.

[chillu]

In theory, everything should be content editable. In practice, its a tradeoff between flexibility and usability. There's lots of similar content which you can't edit in the CMS, e.g. "lost password" labels. email content and feedback. So while we might make all of that "secondary content" editable at some point, I don't think it should block moving the error page type feature into a module. We could achieve the "content editing abilities" by having a HTML field in /admin/settings as well.

@flashbackzoo That being said, I like new filtering UX. It could also make it easier to hide pages which CMS authors usually shouldn't touch, like "search results". When are you expecting to get some user testing done on that? Interested to see if CMS users would actually figure out how to edit error page content. Its such a rarely used feature anyway, I think pushing it into a filter will take it from "rarely used" to "never used".

@jonom Do you think its feasible to implement your proposal without removing ErrorPage from core? We could still remove the static error page generation, make the "server error" a generic static template, and make the error handling in the controller layer more pluggable, right?

[flashbackzoo]

@chillu Yeah being able to edit labels, email content etc is a step too far. But I think having the ability to edit all page content blocks is important. Having an HTML field in Settings is a reasonable solution.

We have user testing planned into our current sprint. We're developing the above wireframes a bit more and will test those.

[jonom]

@chillu yes, if we do decide that ErrorPage should stay in core we can still complete most of the proposal and make a lot of improvements. But...

Yeah being able to edit labels, email content etc is a step too far. But I think having the ability to edit all page content blocks is important.

@flashbackzoo for me, the login and lost password 'pages' are public-facing content too and are no less important than error messages – but they're not represented in the CMS. If we don't make this content editable I don't see why we should make a special exception for error messages. These pieces of content are developer centric and if most users aren't interested in editing them, we don't need to clutter the CMS with it out of the box. This is what 'convention over configuration' is all about. I definitely acknowledge that _some_ people want to do this and that's why I think it would be a good fit for a module.

The wireframes are looking good but the error pages are still represented in a SiteTree which is one of the issues raised in this RFC. Really ErrorPage shouldn't extend SiteTree as it shouldn't need to implement the Hierarchy class, nor should it need URLSegment,ShowInMenus,ShowInSearch etc. It's misleading to represent them as Pages so like @chillu I would prefer to see this type of content administered in the settings section of the CMS. If the utility 'pages' were visually separated somehow and not allowed to be rearranged with the other pages this might help though.

This is getting a bit off the RFC topic now :) but @clarkepaul I don't think it is intuitive to search out a filter to restore an archived page, usually filters show everything by default so it's not where I would go to look to restore a page. Perhaps a fourth icon in the row that is a kind if 'rewind/undo' icon dedicated to letting you restore a page? Or what if when you click 'Add a Page' that interface gives you two options 'Add a new page' and 'Restore an archived page'? I think that could work pretty nice actually, and people would be reminded it's a possible action every time they create a page. Also (they're probably planned but) some tooltips for those icons would be needed I think.

[flashbackzoo]

@jonom Good point - I hadn't considered the login and reset password pages. I think you've just about convinced me.

One other question. Would sites that currently have custom error pages break if they upgrade without installing the module?

[jonom]

We'd have to make sure they don't break, not sure exactly how that will look yet. May need to be an upgrade script to archive any existing ErrorPages on build if the module isn't installed, otherwise I think they might be changed to regular Pages during a build if the ErrorPage class is missing? But yeah, would certainly need to make sure that upgrading without the module is smooth. I'll add a note about that to the RFC.

[flashbackzoo]

Cool - I'm on board with it.

[jonom]

:clap: Nice!

[clarkepaul]

@jonom You made a good case for them not showing in the tree - I was thinking the error pages would show separately from the site tree (divided by a line or similar).
The area where you can select what you see in the tree (currently eye icon), I don't see it as a filter but more like "view options".

[jonom]

@clarkepaul Hmmm if I unchecked the Modified box though... would you say the Modified pages would be filtered out? :)

[clarkepaul]

Yup they are essentially the same thing, I'm making the assumption users don't think of it as "filtering" as that is quite a technical term. It's like the way we use the settings area for a pagetype to change the view of that page.

[jonom]

In any case I think it does make Archived content easier to access, but I feel like a lot of users probably won't use that view options box, so there may be a visibility issue there, plus there's still a bit of a jump to work out that you can filter archived content in to the site tree, then find the page you want, then restore it - it's a bit abstract. Adding it to the Add New Page interface and explicitly spelling out the option might help educate users that the action is available and hopefully they would then be able to recall where to find it.

[clarkepaul]

Yeah obviously that is a wireframe so we have some tough decisions to make whether text is better to use, or what iconography to use. What you cant see in those wireframes is that when you select "Pages" you are presented with both icons and the supporting text but as with any UI some exploration is needed for tasks you don't do regularly.

FYI We are also trying to find a solution that will match other areas of the CMS that will also have an actions bar eg. the files area

[jonom]

@silverstripe/core-team I don't expect to hear anything more from you guys until after 3.2 ships but just FYI this is on pause for me until I get the go ahead or some more direction about anything that needs to change. I haven't started any development work on this yet but I assume 4.0 is a long way off (a year?) so I'm not in a rush.

[tractorcow]

Sorry, rfc label was renamed to "rfc/accepted" after it was added to this issue. I've applied the correct label now.

[KINKCreative]

Guys, what about an ErrorPage controller, which would render a static file from the theme on default. The controller would still take the dynamic content as passed by e.g. httpError(). It would have the standard routes of /404, /400 etc. The default would serve just a default Page.ss template file. If an ErrorPage_404.ss is added to the theme, THAT is used when rendering.

As far as editing of the error page (our clients have never needed this, btw), here's an approach: The default routes would be setup with the module and populate defaults / passed parameters (e.g. Title / Content), but the controller could also check for an ErrorPage having been inserted onto the Stage, the ErrorPage would need a dropdown to select the error type. e.g. ErrorPage with ErrorType being 404. In this case, /404 would serve the custom template, but populated from the stage.

[jonom]

@KINKCreative that could be a good option but my opinion is that we should start with some really simple functionality (plain text response), and allow people to extend it to make it more complex - so an approach like you've outlined could be achieved with a module that extends httpError(). That way people can apply whatever caching system and error content management approach makes the most sense for their site. My main gripe with the current model is that a) it's broken, and b) it's inflexible. Rather than implement another opinionated solution I'd like to strip this functionality back to basics and then developers will be free to upgrade it however they like.

[KINKCreative]

Agreed @jonom. As long as no Error Pages are added to the stage on /dev/build - that would be rad :) I'll try playing around with my approach (the top part is what I see could be part of the core / extension as you set it up now) and if it's worthy of a PR, I'll hit you up.

[jonom]

@KINKCreative I've started looking at separating out ErrorPage so it can be removed and looks pretty straightforward - see https://github.com/silverstripe/silverstripe-cms/pull/1352. If you use that as a starting point you just need to delete the ErrorPage class and remove the ErrorPageControllerExtension extensions in _cms/_config/config.yml_ to be free of ErrorPage, including having any added on build.

I need to reduce the scope of this RFC a bit accordingly.

[jonom]

Have updated this RFC to reduce the scope a bit - now is centred squarely on making ErrorPage optional, as there is a reasonable base available already to allow implementing alternative solutions. This reduced scope would mean changes are limited to the CMS project.

[chillu]

RequestHandler::httpError() already provides a plain-text error response with default descriptions for a range of status codes, with extension points that allow you to customise the response to your liking. This means we already have a good base in place that can be built upon with modules.

I don't think a plain text response for "page not found" is a good default in terms of user experience - it'll erode trust in visitors if you don't show them a branded page with further navigation options. In my opinion, we should generate this dynamically from the default theme - but with non-editable content by default. Given the number of bots hitting 404s, this increases the importance of partial caching and optimised code in the average SS installation (more work than passing through assets/page-not-found.html via SilverStripe).

Another factor here: SS4 comes with a new filesystem implementation, which maps unpublished file paths. For example, a request to assets/<hash>/secret.pdf will actually route through assets/.protected/<hash>/secret.pdf via SilverStripe if you have permissions. Which means we can no longer assume that because a file doesn't exist in assets/, we can return a (fast) Apache 404. We don't do that by default, but this optimisation path is now closed for higher performance use cases.

[jonom]

I don't think a plain text response for "page not found" is a good default in terms of user experience

@chillu That's true, and I was imagining that we would add a new dependency to Installer to account for that. I was thinking rather than making ErrorPage-the-module a dependancy (as has been suggested previously) we could have a very simple module that renders the default message using a standard page template - just like what you described. (I would make a module that does this as part of carrying out this work and happy to transfer ownership to SilverStripe Ltd.)

We could instead make that a core feature, I just wondered if some performance zealots would have an issue with that - since the default response would cover static resources and ajax requests as well. As long as that behaviour is replaceable and doesn't leave behind any overhead I agree it would be a good default.

[chillu]

I was thinking rather than making ErrorPage-the-module a dependancy (as has been suggested previously) we could have a very simple module that renders the default message using a standard page template

Given the relatively small amount of code, I'd just make this core behaviour - but ensure you can customise it. If we treat every customiseable feature as a module which you can opt-out of, we can go on holidays while composer install runs ;)

So as long as the default user experience is considered, it's a +1 from me.

[dhensby]

I agree that we should remove ErrorPage - I'm not ever sure a module makes sense - does anyone want to maintain it? The current way error pages are managed and generated is broken.

The times I've wanted a custom error page, it's been important to know where I am in the site heirachy (eg: is this a 404 on what _should_ be a blog page? if so, I could show other blogs) but because you are in this floating.

I think as long as there's a way to hook into the error response (when someone does Controller::httpError()) and they can render their own template, then that's all a dev needs to get started. I'm not sure a SiteTree object adds any value to rendering error pages.

[wilr]

I think we need to find a way to make it content manageable. We use it for most clients to provide custom content (i.e they put links to major sections or recent changes). Whether this content is stored in a unique admin tab for Error Module or Settings tab for Error Page management.

[sminnee]

I think we need to find a way to make it content manageable.

That would be part of the errorpage module, surely? Otherwise we're not splitting this functionality into a module, we're just rewriting the functionality.

[jonom]

Yeah exactly - a single solution will not suit everyone. Some people will want to go on using the ErrorPage module but splitting it out means devs are not locked in to it and are free to implement any alternative solution they like.

[dhensby]

That would be part of the errorpage module, surely? Otherwise we're not splitting this functionality into a module, we're just rewriting the functionality.

Exactly.

[jonom]

@sminnee @chillu I'm just watching the last cc hangout and noticed you discussing performance degradation as a result of this RFC being implemented. Please note that there would be no performance degradation. The static 404 file is generated but never utilised - that is a key piece of information in this RFC.

Take a look at the code in ErrorPage::response_for() and tell me in what scenario the static file would be served? The static file is only served as a fallback, and the only way I can see you could get to that point is if you publish a 404 page in the CMS then _delete_ that page from the CMS. But it will be re-generated the next time you build the DB at which point you're back to fully dynamic 404 responses again. Likewise the 404 rule in the installer's .htaccess file will always be ignored.

Am I wrong about any of this? As far as I can see, we only have an illusion of static caching of 404s in SilverStripe. If I'm right, I think there's just no good reason for ErrorPage to live in core.

[dhensby]

You're not wrong. See http://www.silverstripe.org/blog/route-whitelist/

[jonom]

@sminnee could I get your thoughts on above performance implications? Keen to get this to accepted stage and think the perceived performance limitation is holding this up.

[tractorcow]

Take a look at the code in ErrorPage::response_for() and tell me in what scenario the static file would be served? The static file is only served as a fallback, and the only way I can see you could get to that point is if you publish a 404 page in the CMS then delete that page from the CMS. But it will be re-generated the next time you build the DB at which point you're back to fully dynamic 404 responses again. Likewise the 404 rule in the installer's .htaccess file will always be ignored.

Look at https://github.com/silverstripe/silverstripe-cms/blob/master/code/logging/ErrorPageErrorFormatter.php; This is the error handler which relies on the static error pages, through ErrorPage::get_content_for_errorcode.

This handler intentionally avoids doing any SQL queries or relying on dataobjects in case it's handling a database error.

So...

The static 404 file is generated but never utilised - that is a key piece of information in this RFC.

Isn't quite true. :D

[dhensby]

If the DB isn't available, it shouldn't be blindly serving a 404, though?

It's mostly true. I don't think this rfc says don't have simple 404s, its just saying this current solution is fragile and should be reconsidered

[jonom]

@tractorcow I don't debate the intention of ErrorPage::get_content_for_errorcode but I just can't seem to produce a scenario where it used. Can you give me some direction here on how to get ErrorPageErrorFormatter to produce some output? I've manually adjusted the content of the static error pages and tried setting up errors all kinds of different ways but have never been successful in seeing the static content on screen. The one exception to this is the static 500 error page, when it gets served up directly by Apache after a fatal error.

I don't think this rfc says don't have simple 404s, its just saying this current solution is fragile and should be reconsidered

That's right. Current approach is flawed and very opinionated, and I don't see a good reason to force this one method of managing error content on all SilverStripe users. I intend for all the existing functionality (however broken it is) to be moved to a module, so I don't see why anyone should be against this being moved out of core? If you like the things the way are just install the ErrorPage module :)

I would really like someone to prove that static error page content is utilised if they can. I've tried and failed, and it seems I really need it confirmed either way before this RFC can get approved as it seems to be a key point of friction. I think it's irrelevant because the module should replicate the current behaviour, but if we can clear up this point maybe I can get a green label and invest time in working on this. Then we can debate the finer details when I've got an implementation ready to share.

The main proposal here is to make ErrorPage optional, so all we should need for this RFC to be accepted is for everyone to agree that it makes sense for it to be moved out of core. I'm not talking about killing ErrorPage. I just want it to be optional.

[tractorcow]

If the DB isn't available, it shouldn't be blindly serving a 404, though?

Yeah it's mostly for 500 errors, not so much 404s.

If the database crashes mid way through a query, we don't want to re-query it to generate a 404 page. This could be due to high load on the database cluster (e.g. out of memory).

[tractorcow]

The main proposal here is to make ErrorPage optional, so all we should need for this RFC to be accepted is for everyone to agree that it makes sense for it to be moved out of core. I'm not talking about killing ErrorPage. I just want it to be optional.

It's not the only page I'd like out of core... VirtualPage as well please!

[tractorcow]

To get to the point, I'm not against moving out of the core, but I still think we need a static file.

[dhensby]

Another related edgecase https://github.com/silverstripe/silverstripe-framework/issues/5518

[nathanbrauer]

Wow, it's been almost a year! :)

Any movement here?

As a side note, we had a module built that sort-of helps the situation:
https://github.com/Marketo/SilverStripe-UniversalErrorPage

(But it'd be better if it didn't have to override ErrorPage's behavior.)

cc: @stevie-mayhew

[jonom]

@nathanbrauer I haven't had time to dive back in to this and sort of ran out of energy too to be honest. From memory it should be very easy to split off the built in error page functionality out to a separate module, so maybe this could still be done in time for SS4? I haven't been following along too closely on SS4 dev lately so the situation might have changed a bit. If you feel like picking this up please feel free :)

[tractorcow]

@jonom we are doing module splitting now; See the new https://github.com/silverstripe/silverstripe-admin module we just created. We haven't prioritised errorpage though.

[tractorcow]

@jonom if you want to do the work yourself I could setup a repo for you and give you admin.

[jonom]

@tractorcow I know I should but I can't fit it in right at the moment sorry :( I could probably do it in a couple of weeks if that won't be too late? Just getting back on deck this week after some time off.

[patricknelson]

For anyone interested, I've already addressed the speed concerns in my own custom implementation of the ErrorPage class by doing a few very specific things:

1. Generate static pages as a task: Setup an ErrorPageTask which runs as part of my deployment workflow. This is automated such that, along with a dev/build, it will also run this task to build out the static pages (will show sample code below). It's roughly as simple as iterating through each ErrorPage instance in the database and calling ->doPublish() on each one.
2. Reference static files via my CustomErrorPage class when error response is needed. But in order to do so...
3. Refactor those pesky static calls to point to my custom class via CustomErrorPage::response_for(...) instead of ErrorPage::response_for(...). Something I've brought up in the silverstripe-dev mailing list, since unfortunately core modifications like these have to be made in order to reduce 404 page load overhead (which can be _quite_ considerable for large sites like mine).

Code samples:

CustomErrorPage.php

<?php

class CustomErrorPage extends ErrorPage {

    public static function response_for($statusCode) {
        // Try to get cached version *FIRST* before anything else.
        $cachedPath = self::get_filepath_for_errorcode(
            $statusCode,
            class_exists('Translatable') ? Translatable::get_current_locale() : null
        );

        if(file_exists($cachedPath)) {
            $response = new SS_HTTPResponse();

            $response->setStatusCode($statusCode);
            $response->setBody(file_get_contents($cachedPath));

            return $response;
        }

        // Continue with existing functionality.
        return parent::response_for($statusCode);
    }
}

ErrorPageTask.php

NOTE: Here I have special functions in my code base. For example, withExceptions(...) which converts notices/warnings from trigger_error/user_error to catchable exceptions (which have the benefit of also halting execution at the current stack location up to the try/catch unlike PHP warning messages). The whileAdmin(...) helper function can be found in my SilverStripe database migrations module (specifically here) which facilitates the unfortunate need to escalate to admin permissions (modifying global state at the session level) in order to perform some necessary Versioned object tasks (on SiteTree or Page children).

Also note: In the code below, SSL is forcibly enabled manually since our site is 100% SSL all the time so this ensures links are properly prefixed with https://.

<?php

class ErrorPageTask extends BuildTask {

    protected $title = 'Cache Error Pages';

    protected $description = 'Re-publishes error pages to ensure the latest version of the page is saved to the static cache (e.g. assets/error-404.html).';

    protected $enabled = true;


    /**
     * @param   SS_HTTPRequest $request
     */
    public function run($request) {
        // Command line only...
        if (!Director::is_cli()) {
            echo "This can only be run from the CLI.<br>";
            exit();
        }

        withExceptions(function() {
            whileAdmin(function() {
                // Before starting, if we're in production, trick the server into thinking we're currently on a secure
                // connection. This way we can at least be sure that references will point to the correct protocol when done.
                if (Director::isLive()) {
                    $_SERVER['SSL'] = true;
                    $_SERVER['HTTPS'] = 'on';
                }

                echo "Rebuilding static error pages... ";
                foreach(ErrorPage::get()->toArray() as $page) {
                    try {
                        /** @var $page ErrorPage  */
                        $page->doPublish();
                    } catch(Exception $e) {
                        // Output and log this message, then move on.
                        $message = "\n\nError processing page '{$page->getTitle()}': {$e->getMessage()}";
                        dump($message);
                        logger($message);
                    }
                }
                echo "Done.\n\n";
            });
        });
    }
}

[patricknelson]

p.s. Along with my implementation suggestions above, I'd suggest we adopt @chillu's recommendation from the mailing list about using the Injector to access the static methods at least for the ::response_for(...) method:

// Call static method on injected class.
$obj = Injector::inst()->get('ErrorPage');
$obj::response_for(404);

Digression: Perhaps someday that could be reduced to a simpler API somewhere in core, e.g. injector('ErrorPage')::response_for(404), but alas PHP doesn't allow scope resolution operators tacked immediately onto the end of function calls 😔 In that case you'd be better off with Laravel-esque Facades which would allow for static calls on both instance-level and static level methods (utilizing reflection to determine the correct approach once the injector has determine the correct class to use).

[jonom]

@tractorcow I've taken a first pass at removing ErrorPage from CMS. I realise I'm late to the party here but thought it might be possible to remove ErrorPage from core in time for beta 1, even if 'ErrorPage: the module' isn't available at the same time.

Do you want to create a repo like you offered so I can start setting that up?

CMS PR: https://github.com/silverstripe/silverstripe-cms/pull/1849
Framework PR: https://github.com/silverstripe/silverstripe-framework/pull/7017

[tractorcow]

@jonom I've setup https://github.com/silverstripe/silverstripe-errorpage and given the core team admin permissions there. Let me know before you push anything to packagist though ok?

[jonom]

@tractorcow will do!

[jonom]

Tracking progress at https://github.com/silverstripe/silverstripe-errorpage/issues/1

[jonom]

Well look, this came together just in time for beta 1! If anyone wants to submit improvements for or is interested in maintaining ErrorPage-the-module head on over to https://github.com/silverstripe/silverstripe-errorpage. Cooking up your own alternative module is an option now too of course. @patricknelson @nathanbrauer @KINKCreative

[chillu]

@jonom Great job! Can you please review/merge my improved upgrading guide? https://github.com/silverstripe/silverstripe-framework/pull/7066. Would be good to add alternative approaches there as they pop up in the community.

[jonom]

I published an alternative to ErrorPage today, if anyone is interested in testing it out: https://github.com/jonom/silverstripe-custom-errors