API Error page support for Security controller errors #1007

Merged
merged 1 commit into from May 5, 2014

Projects

None yet

3 participants

Contributor

[ref: CWPBUG-157]

@tractorcow tractorcow added this to the 3.1.5 milestone May 2, 2014
Contributor

cc @mateusz ; Should this go into 3.1, or should we move this into master? We can patch this feature in ourselves if we want in the mean time.

Owner
mateusz commented May 5, 2014

Yeah, that looks good, but I wonder why the extension point is in the httpError only... Would make a lot of sense to me to introduce a new extension point in Director::handleRequest catch block for that Exception, and hook in there - we would then get rid of all plaintext errors.

It's not an urgent change, so we could put it in 3.2

@hafriedlander hafriedlander commented on an outdated diff May 5, 2014
code/controllers/ErrorPageControllerExtension.php
@@ -0,0 +1,16 @@
+<?php
+
+/**
+ * Enhances error handling for a controller with ErrorPage generated output
+ *
+ * @package cms
+ * @subpackage controller
+ */
+class ErrorPageControllerExtension extends Extension {
+
+ public function onBeforeHTTPError($statusCode, $request) {
+ if($response = ErrorPage::response_for($statusCode)) {
hafriedlander
hafriedlander May 5, 2014 Owner

Better to split this into two lines, rather than putting = in a conditional. Otherwise you end up having to check to see if this was deliberate or a mistake every time you read the code.

Contributor

@mateusz Hamish and I had a go at implementing it the way you suggested, but there are A FEW places across the codebase that catch(SS_HTTPResponse_Exception) is used to generate a result; It could potentially be breaking to change this in 3.1, so let's aim for 3.2 for such a rewrite.

I've updated the assignment as your suggestion @hafriedlander.

Owner
mateusz commented May 5, 2014

Yep, aim at 3.2 is a good idea! It's not something that we need now so I think it can wait for "proper" fix! Thanks for battling with that one.

@hafriedlander hafriedlander merged commit 435a5ee into silverstripe:3.1 May 5, 2014
@tractorcow tractorcow deleted the tractorcow:pulls/3.1-security-errorpage branch May 6, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment