sminnee /
silverstripe-framework
Duplicate code is one of the most pungent code smells. A rule that is often used is to re-structure code once it is duplicated in three or more places.
Common duplication problems, and corresponding solutions are:
Complex classes like RequestHandler often do a lot of different things. To break such a class down, we need to identify a cohesive component within that class. A common approach to find such a component is to look for fields/methods that share the same prefixes, or suffixes. You can also have a look at the cohesion graph to spot any un-connected, or weakly-connected components.
Once you have determined the fields that belong together, you can apply the Extract Class refactoring. If the component makes sense as a sub-class, Extract Subclass is also a candidate, and is often faster.
While breaking up the class, it is a good idea to analyze how other classes use RequestHandler, and based on these observations, apply Extract Interface, too.
| 1 | <?php |
||
| 57 | class RequestHandler extends ViewableData { |
||
| 58 | |||
| 59 | /** |
||
| 60 | * @var SS_HTTPRequest $request The request object that the controller was called with. |
||
| 61 | * Set in {@link handleRequest()}. Useful to generate the {} |
||
| 62 | */ |
||
| 63 | protected $request = null; |
||
| 64 | |||
| 65 | /** |
||
| 66 | * The DataModel for this request |
||
| 67 | */ |
||
| 68 | protected $model = null; |
||
| 69 | |||
| 70 | /** |
||
| 71 | * This variable records whether RequestHandler::__construct() |
||
| 72 | * was called or not. Useful for checking if subclasses have |
||
| 73 | * called parent::__construct() |
||
| 74 | * |
||
| 75 | * @var boolean |
||
| 76 | */ |
||
| 77 | protected $brokenOnConstruct = true; |
||
| 78 | |||
| 79 | /** |
||
| 80 | * The default URL handling rules. This specifies that the next component of the URL corresponds to a method to |
||
| 81 | * be called on this RequestHandlingData object. |
||
| 82 | * |
||
| 83 | * The keys of this array are parse rules. See {@link SS_HTTPRequest::match()} for a description of the rules |
||
| 84 | * available. |
||
| 85 | * |
||
| 86 | * The values of the array are the method to be called if the rule matches. If this value starts with a '$', then |
||
| 87 | * the named parameter of the parsed URL wil be used to determine the method name. |
||
| 88 | * @config |
||
| 89 | */ |
||
| 90 | private static $url_handlers = array( |
||
| 91 | '$Action' => '$Action', |
||
| 92 | ); |
||
| 93 | |||
| 94 | |||
| 95 | /** |
||
| 96 | * Define a list of action handling methods that are allowed to be called directly by URLs. |
||
| 97 | * The variable should be an array of action names. This sample shows the different values that it can contain: |
||
| 98 | * |
||
| 99 | * <code> |
||
| 100 | * array( |
||
| 101 | * // someaction can be accessed by anyone, any time |
||
| 102 | * 'someaction', |
||
| 103 | * // So can otheraction |
||
| 104 | * 'otheraction' => true, |
||
| 105 | * // restrictedaction can only be people with ADMIN privilege |
||
| 106 | * 'restrictedaction' => 'ADMIN', |
||
| 107 | * // complexaction can only be accessed if $this->canComplexAction() returns true |
||
| 108 | * 'complexaction' '->canComplexAction' |
||
| 109 | * ); |
||
| 110 | * </code> |
||
| 111 | * |
||
| 112 | * Form getters count as URL actions as well, and should be included in allowed_actions. |
||
| 113 | * Form actions on the other handed (first argument to {@link FormAction()} should NOT be included, |
||
| 114 | * these are handled separately through {@link Form->httpSubmission}. You can control access on form actions |
||
| 115 | * either by conditionally removing {@link FormAction} in the form construction, |
||
| 116 | * or by defining $allowed_actions in your {@link Form} class. |
||
| 117 | * @config |
||
| 118 | */ |
||
| 119 | private static $allowed_actions = null; |
||
| 120 | |||
| 121 | /** |
||
| 122 | * @config |
||
| 123 | * @var boolean Enforce presence of $allowed_actions when checking acccess. |
||
| 124 | * Defaults to TRUE, meaning all URL actions will be denied. |
||
| 125 | * When set to FALSE, the controller will allow *all* public methods to be called. |
||
| 126 | * In most cases this isn't desireable, and in fact a security risk, |
||
| 127 | * since some helper methods can cause side effects which shouldn't be exposed through URLs. |
||
| 128 | */ |
||
| 129 | private static $require_allowed_actions = true; |
||
| 130 | |||
| 131 | public function __construct() { |
||
| 142 | |||
| 143 | /** |
||
| 144 | * Set the DataModel for this request. |
||
| 145 | */ |
||
| 146 | public function setDataModel($model) { |
||
| 149 | |||
| 150 | /** |
||
| 151 | * Handles URL requests. |
||
| 152 | * |
||
| 153 | * - ViewableData::handleRequest() iterates through each rule in {@link self::$url_handlers}. |
||
| 154 | * - If the rule matches, the named method will be called. |
||
| 155 | * - If there is still more URL to be processed, then handleRequest() |
||
| 156 | * is called on the object that that method returns. |
||
| 157 | * |
||
| 158 | * Once all of the URL has been processed, the final result is returned. |
||
| 159 | * However, if the final result is an array, this |
||
| 160 | * array is interpreted as being additional template data to customise the |
||
| 161 | * 2nd to last result with, rather than an object |
||
| 162 | * in its own right. This is most frequently used when a Controller's |
||
| 163 | * action will return an array of data with which to |
||
| 164 | * customise the controller. |
||
| 165 | * |
||
| 166 | * @param $request The {@link SS_HTTPRequest} object that is reponsible for distributing URL parsing |
||
| 167 | * @uses SS_HTTPRequest |
||
| 168 | * @uses SS_HTTPRequest->match() |
||
| 169 | * @return HTTPResponse|RequestHandler|string|array |
||
| 170 | */ |
||
| 171 | public function handleRequest(HTTPRequest $request, DataModel $model) { |
||
| 261 | |||
| 262 | protected function findAction($request) { |
||
| 289 | |||
| 290 | /** |
||
| 291 | * Given a request, and an action name, call that action name on this RequestHandler |
||
| 292 | * |
||
| 293 | * Must not raise SS_HTTPResponse_Exceptions - instead it should return |
||
| 294 | * |
||
| 295 | * @param $request |
||
| 296 | * @param $action |
||
| 297 | * @return HTTPResponse |
||
| 298 | */ |
||
| 299 | protected function handleAction($request, $action) { |
||
| 316 | |||
| 317 | /** |
||
| 318 | * Get a array of allowed actions defined on this controller, |
||
| 319 | * any parent classes or extensions. |
||
| 320 | * |
||
| 321 | * Caution: Since 3.1, allowed_actions definitions only apply |
||
| 322 | * to methods on the controller they're defined on, |
||
| 323 | * so it is recommended to use the $class argument |
||
| 324 | * when invoking this method. |
||
| 325 | * |
||
| 326 | * @param string $limitToClass |
||
| 327 | * @return array|null |
||
| 328 | */ |
||
| 329 | public function allowedActions($limitToClass = null) { |
||
| 362 | |||
| 363 | /** |
||
| 364 | * Checks if this request handler has a specific action, |
||
| 365 | * even if the current user cannot access it. |
||
| 366 | * Includes class ancestry and extensions in the checks. |
||
| 367 | * |
||
| 368 | * @param string $action |
||
| 369 | * @return bool |
||
| 370 | */ |
||
| 371 | public function hasAction($action) { |
||
| 405 | |||
| 406 | /** |
||
| 407 | * Return the class that defines the given action, so that we know where to check allowed_actions. |
||
| 408 | */ |
||
| 409 | protected function definingClassForAction($actionOrigCasing) { |
||
| 421 | |||
| 422 | /** |
||
| 423 | * Check that the given action is allowed to be called from a URL. |
||
| 424 | * It will interrogate {@link self::$allowed_actions} to determine this. |
||
| 425 | */ |
||
| 426 | public function checkAccessAction($action) { |
||
| 476 | |||
| 477 | /** |
||
| 478 | * Throws a HTTP error response encased in a {@link SS_HTTPResponse_Exception}, which is later caught in |
||
| 479 | * {@link RequestHandler::handleAction()} and returned to the user. |
||
| 480 | * |
||
| 481 | * @param int $errorCode |
||
| 482 | * @param string $errorMessage Plaintext error message |
||
| 483 | * @uses SilverStripe\Control\HTTPResponse_Exception |
||
| 484 | * @throws SilverStripe\Control\HTTPResponse_Exception |
||
| 485 | */ |
||
| 486 | public function httpError($errorCode, $errorMessage = null) { |
||
| 499 | |||
| 500 | /** |
||
| 501 | * Returns the SS_HTTPRequest object that this controller is using. |
||
| 502 | * Returns a placeholder {@link NullHTTPRequest} object unless |
||
| 503 | * {@link handleAction()} or {@link handleRequest()} have been called, |
||
| 504 | * which adds a reference to an actual {@link SS_HTTPRequest} object. |
||
| 505 | * |
||
| 506 | * @return SS_HTTPRequest|NullHTTPRequest |
||
| 507 | */ |
||
| 508 | public function getRequest() { |
||
| 511 | |||
| 512 | /** |
||
| 513 | * Typically the request is set through {@link handleAction()} |
||
| 514 | * or {@link handleRequest()}, but in some based we want to set it manually. |
||
| 515 | * |
||
| 516 | * @param SS_HTTPRequest |
||
| 517 | */ |
||
| 518 | public function setRequest($request) { |
||
| 521 | } |
||
| 522 |
Loading history...
Duplicated code is one of the most pungent code smells. If you need to duplicate the same code in three or more different places, we strongly encourage you to look into extracting the code into a single class or operation.
You can also find more detailed suggestions in the “Code” section of your repository.