dynamic / silverstripe-maintenance-mode

Maintainance/Offline Mode Module for SilverStripe
BSD 2-Clause "Simplified" License
21 stars 18 forks source link

Should have the ability to display error page without redirecting users. #4

Closed patricknelson closed 9 years ago

patricknelson commented 9 years ago

I noticed that when we enabled maintenance mode, this module would redirect users to a new page URL (the URL of the UtilityPage instance in the database). In many cases this may be fine, but in my case I'd prefer the ability to display this page regardless of the URL without redirecting them. This is easily accomplished in a few lines of code.

For progressive enhancement, and because the current functionality is to perform the redirect, I've setup a new configuration option:

UtilityPage:
  DisableRedirect: true

This allows you to display the maintenance page on any current URL instead of redirecting to /offline/ (for example). I also added some basic documentation covering this new functionality, which you can preview here:

https://github.com/patricknelson/silverstripe-maintenance-mode

patricknelson commented 9 years ago

Quick note: I noticed a bug in my code since apparently SilverStripe will act unpredictably if you go to invalid URL's (404 pages), since the current page won't have a model assocated with it, so passing the current request object into ->handleRequest() without calling some esoteric methods on the $request object (namely $request->shiftAllParams() and $request->shift();) so, for better separation of concerns, and really just because I want to generate the content for UtilityPage without a ton of uber-specific convolutions, I decided to just instantiate a new SS_HTTPRequest object and call it a day :D

patricknelson commented 9 years ago

Also FYI, tested in SilverStripe 3.1.x

dljoseph commented 9 years ago

Hi @patricknelson , I like the solution that you have proposed. I've tested it and found it to work in 3.1.x, but to get it to work in 3.2 beta1, I had to change line 50 of MaintenanceModeExtension.php to

$response = $controller->handleRequest($this->owner->request, $this->owner->model);

When I originally wrote this extension, I made it work exactly as you have done, which is, ultimately, the desired result. I will merge your pull request... I think the next enhancement, as you pointed out, will be to handle 404 ErrorPages :)

Thanks for your contribution.

dljoseph commented 9 years ago

@patricknelson - release v0.5 uses your handleRequest snippet in an onAfterInit method to render the UtilityPage template on 404 errors. It can be enhanced further to respond to other error codes by removing the 404-specific error code check. Also, the code could probably do with some minor refactoring to move your snippet into a separate method as I've violated the DRY (Dont' Repeat Yourself) principle by duplicating your code to add to the onAfterInit method.

Tested in 3.1.x and 3.2 beta1

patricknelson commented 9 years ago

I don't understand; my original code manufactured a new instance of SS_HTTPRequest precisely because of the issues regarding 404s since recycling the original (and frankly irrelevant) $request was causing the same issues I think you are having. I'm not at a computer to verify this for sure but do you know why it wasn't working in 3.2? It's possible that there may be a much simpler solution.

dljoseph commented 9 years ago

I think you are correct. I have retested on 3.2 with your earlier code and cannot replicate the errors that I encountered the first time, so...not sure what caused it. I will revert the code to your earlier solution.

patricknelson commented 9 years ago

Ok cool. I've installed 3.2 on my laptop and verifying as well. I always say, the simpler the solution which accomplishes the task, the better it is! We'll see.

dljoseph commented 9 years ago

I agree.

patricknelson commented 9 years ago

I'm not very familiar with 3.2 at this point, but the errors I'm getting are related to DataModal not being passed in the second argument of ->handleRequest() which definitely wasn't previously an issue in 3.1. It appears that ContentController and Controller have different signatures for that method, which is bad practice of course (when you override methods like that, you must make the requirements tighter, not looser), so that is stoopid: http://stackoverflow.com/questions/13423494/why-is-overriding-method-parameters-a-violation-of-strict-standards-in-php

patricknelson commented 9 years ago

Since this PR was closed, I'll have to open a new one. Very simple change though. I'll pull your latest changes and basically just... copy/paste my latest version.

patricknelson commented 9 years ago

Ok, try #5