-
-
Notifications
You must be signed in to change notification settings - Fork 3.7k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Single Task Controllers RFC #3503
Changes from 4 commits
45eb8d6
2be4e10
9f7a6d3
8f149bb
a308d43
2addb89
bbb3330
5da5e13
83fc12c
12426a6
4654fd6
ab223dd
486546b
9f5c697
eb88f74
a734bef
8a65be7
00c442f
3c9d4e8
cfd10cf
5176d24
a443c47
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,51 @@ | ||
<?php | ||
/** | ||
* @package Joomla.Libraries | ||
* @subpackage Captcha | ||
* | ||
* @copyright Copyright (C) 2005 - 2013 Open Source Matters, Inc. All rights reserved. | ||
* @license GNU General Public License version 2 or later; see LICENSE.txt | ||
*/ | ||
|
||
defined('JPATH_PLATFORM') or die; | ||
|
||
class JCmsControllerAdd extends JCmsControllerDisplay | ||
{ | ||
/** | ||
* Instantiate the controller. | ||
* | ||
* @param JInput $input The input object. | ||
* @param JApplicationBase $app The application object. | ||
* @param array $config Configuration | ||
* @since 12.1 | ||
*/ | ||
public function __construct(JInput $input, $app = null, $config = array()) | ||
{ | ||
$input->set('layout', 'edit'); | ||
|
||
parent::__construct($input, $app, $config); | ||
} | ||
|
||
/** | ||
* (non-PHPdoc) | ||
* @see JCmsControllerDisplay::execute() | ||
*/ | ||
public function execute() | ||
{ | ||
$config = $this->config; | ||
$prefix = $this->getPrefix(); | ||
$model = $this->getModel($prefix, $config['subject'], $config); | ||
|
||
if (!$model->allowAction('core.create')) | ||
{ | ||
$msg = $this->translate('JLIB_APPLICATION_ERROR_CREATE_RECORD_NOT_PERMITTED'); | ||
$url = 'index.php?option='.$config['option'].'&task=display.'.$config['subject']; | ||
$this->setRedirect($url, $msg, 'error'); | ||
return false; | ||
} | ||
|
||
parent::execute(); | ||
return true; | ||
} | ||
|
||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,242 @@ | ||
<?php | ||
/** | ||
* @package Joomla.Libraries | ||
* @subpackage Captcha | ||
* | ||
* @copyright Copyright (C) 2005 - 2013 Open Source Matters, Inc. All rights reserved. | ||
* @license GNU General Public License version 2 or later; see LICENSE.txt | ||
*/ | ||
|
||
defined('JPATH_PLATFORM') or die; | ||
|
||
abstract class JCmsControllerBase extends JControllerBase | ||
{ | ||
/** | ||
* Configuration variables | ||
* @var array | ||
*/ | ||
protected $config; | ||
|
||
/** | ||
* Associative array of models | ||
* stored as $models[$prefix][$name] used by get models | ||
* @var array | ||
*/ | ||
protected $models = array(); | ||
|
||
/** | ||
* URL for redirection. | ||
* | ||
* @var string | ||
* @since 12.2 | ||
* @note Replaces _redirect. | ||
*/ | ||
protected $redirect; | ||
|
||
/** | ||
* Redirect message. | ||
* | ||
* @var string | ||
* @since 12.2 | ||
* @note Replaces _message. | ||
*/ | ||
protected $message; | ||
|
||
/** | ||
* Redirect message type. | ||
* | ||
* @var string | ||
* @since 12.2 | ||
* @note Replaces _messageType. | ||
*/ | ||
protected $messageType; | ||
|
||
/** | ||
* Instantiate the controller. | ||
* | ||
* @param JInput $input The input object. | ||
* @param JApplicationBase $app The application object. | ||
* @param array $config Configuration | ||
* @since 12.1 | ||
*/ | ||
public function __construct(JInput $input, $app = null, $config = array()) | ||
{ | ||
parent::__construct($input, $app); | ||
|
||
if (!array_key_exists('option', $config)) | ||
{ | ||
$config['option'] = $input->get('option'); | ||
} | ||
|
||
$this->config = $config; | ||
} | ||
|
||
/** | ||
* Method to check the session token | ||
* @return void | ||
*/ | ||
protected function validateSession() | ||
{ | ||
JSession::checkToken() or jexit(JText::_('JINVALID_TOKEN')); | ||
} | ||
|
||
/** | ||
* Method to refresh the session token to prevent the back button | ||
* @return void | ||
*/ | ||
protected function refreshToken() | ||
{ | ||
$session = JFactory::getSession(); | ||
$session->getToken(true); | ||
} | ||
|
||
/** | ||
* Method to translate a string using JText::_() method | ||
* @param string $string | ||
* @return string translation result | ||
*/ | ||
protected function translate($string) | ||
{ | ||
return JText::_($string); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Personally not a fan of this - I'd much rather just see us use JText::_ directly There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I agree that it doesn't make much sense to add a wrapper around JText. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I did this because personally I don't like static functions spread all over the place. Also I'm almost positive it is an anti-pattern. This way if Joomla decides to scrap JText now or in the future there is only one function that has to change. You'll probably notice with the exception of JFactory, I've wrapped all statics in class methods. And to be honest I regret not wrapping it too. Sincerely, Babel-university.com P.S. This message was sent via iPhone, so please forgive any errors
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
I'd say the uses in the controllers are out least concern here. If we ever are going to change There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Actually I would anything touching JSession just because we know the On Friday, April 25, 2014, Thomas Hunziker [email protected] wrote:
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Just because the Framework is doing something doesn't automatically mean On Friday, April 25, 2014, George Wilson [email protected] wrote:
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We don't know yet what will happen and when. And it may well be that this methods are not suiteable for what we will do then and we have to deprecate them as well. Static functions aren't evil by itself. There are perfectly fine useages of those. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good morning. The truth is none of us know what is going to happen in the future, so any discussion about the future is nothing more than an assumption. However if we choose to learn from the past which is absolute, we stand a better chance of improving our assumption. Correct me if I'm wrong, but as I recall we didn't used to use JSession::checkToken(). Forgive me for not recalling the exact class we used to use, but I remember the change because I had to go through my extension and change all the references.(which sucked)
We are going to have to agree to disagree on this one.
For someone who has made it over the JLearning curve these might seem unnecessary, but when I code I like to assume the next person reading my code doesn't know any PHP. Which forces me to be very expressive. Tell me which of these express the intention which is "To refresh the session token"
or
Again which expresses the true intention which is "To make sure the current session is a valid"
or
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Actually, I don't want to refresh an existing token. I just want to issue a new token. So
It may come down to personal preferences of course. However I'm a big fan of keeping PRs on its topic and adding new methods to wrap static calls doesn't seem to be needed for adding single task contollers. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. refreshToken() doesn't return the token. It creates a new token and replaces the old token that is already in the session. This is why I call it refresh token. I have to admit, I'm surprised that these simple functions that actually only serve the purpose of moving the responsibility for session checks into a reusable function (making building controllers easier) would be so heavily debated, while the fact that the inversion of the controller.task hasn't even gotten a mention. Crazy world we live in. Sincerely, Babel-university.com P.S. This message was sent via iPhone, so please forgive any errors
|
||
} | ||
|
||
/** | ||
* Method to get the option prefix from the input | ||
* @return string ucfirst(substr($this->config['option'], 4)); | ||
*/ | ||
protected function getPrefix() | ||
{ | ||
$prefix = ucfirst(substr($this->config['option'], 4)); | ||
|
||
return $prefix; | ||
} | ||
|
||
/** | ||
* Method to get a model, creating it if it doesn't already exist. | ||
* @param string $prefix | ||
* @param string $name | ||
* @param array $config | ||
* @return JCmsModelBase | ||
* @throws ErrorException | ||
*/ | ||
public function getModel($prefix, $name, $config = array()) | ||
{ | ||
$prefix = ucfirst($prefix); | ||
$name = ucfirst($name); | ||
|
||
if (isset($this->models[$prefix][$name])) | ||
{ | ||
return $this->models[$prefix][$name]; | ||
} | ||
|
||
$class = $prefix.'Model'.$name; | ||
|
||
if (!class_exists($class)) | ||
{ | ||
throw new ErrorException(JText::sprintf('JLIB_APPLICATION_ERROR_MODELCLASS_NOT_FOUND', $class)); | ||
return false; | ||
} | ||
|
||
$config = $this->normalizeConfig($config); | ||
|
||
$this->models[$prefix][$name] = new $class($config); | ||
|
||
return $this->models[$prefix][$name]; | ||
} | ||
|
||
/** | ||
* Method to insure all config variables are are included. | ||
* Intended to be used in getModel, getView and other factory methods | ||
* that can be passed a config array. | ||
* @param array $config to normalize | ||
*/ | ||
protected function normalizeConfig($config) | ||
{ | ||
//Safe merge. will not overwrite existing keys | ||
$config += $this->config; | ||
} | ||
|
||
/** | ||
* Redirects the browser or returns false if no redirect is set. | ||
* @return boolean False if no redirect exists. | ||
* | ||
* @since 12.2 | ||
*/ | ||
public function redirect() | ||
{ | ||
if ($this->hasRedirect()) | ||
{ | ||
$app = $this->app; | ||
|
||
// Enqueue the redirect message | ||
$app->enqueueMessage($this->message, $this->messageType); | ||
|
||
// Execute the redirect | ||
$app->redirect($this->redirect); | ||
} | ||
|
||
return false; | ||
} | ||
|
||
/** | ||
* Method to check if the controller has a redirect | ||
* @return boolean | ||
*/ | ||
public function hasRedirect() | ||
{ | ||
if (!empty($this->redirect)) | ||
{ | ||
return true; | ||
} | ||
return false; | ||
} | ||
|
||
/** | ||
* Set a URL for browser redirection. | ||
* | ||
* @param string $url URL to redirect to. | ||
* @param string $msg Message to display on redirect. Optional. | ||
* @param string $type Message type. Optional, defaults to 'message'. | ||
* @param bool $useJRoute should we phrase the url with JRoute? | ||
* | ||
* @return $this Object to support chaining. | ||
* | ||
* @since 12.2 | ||
*/ | ||
public function setRedirect($url, $msg = null, $type = 'message', $useJRoute = true) | ||
{ | ||
if ($useJRoute) | ||
{ | ||
$this->redirect = JRoute::_($url, false); | ||
} | ||
else | ||
{ | ||
$this->redirect = $url; | ||
} | ||
|
||
if ($msg !== null) | ||
{ | ||
$this->message = $msg; | ||
} | ||
|
||
$this->messageType = $type; | ||
|
||
return $this; | ||
} | ||
|
||
/** | ||
* Method to cast all values in a cid array to integer values | ||
* @param array $cid | ||
* @return array $cleanCid | ||
*/ | ||
protected function cleanCid($cid) | ||
{ | ||
$cleanCid = array(); | ||
foreach ((array)$cid AS $id) | ||
{ | ||
$cleanCid[] = (int)$id; | ||
} | ||
return $cleanCid; | ||
} | ||
|
||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,49 @@ | ||
<?php | ||
/** | ||
* @package Joomla.Libraries | ||
* @subpackage Captcha | ||
* | ||
* @copyright Copyright (C) 2005 - 2013 Open Source Matters, Inc. All rights reserved. | ||
* @license GNU General Public License version 2 or later; see LICENSE.txt | ||
*/ | ||
|
||
defined('JPATH_PLATFORM') or die; | ||
|
||
class JCmsControllerCancel extends JCmsControllerBase | ||
{ | ||
/** | ||
* (non-PHPdoc) | ||
* @see JController::execute() | ||
*/ | ||
public function execute() | ||
{ | ||
$config = $this->config; | ||
$url = 'index.php?option='.$config['option'].'&task=display.'.$config['subject']; | ||
|
||
$prefix = $this->getPrefix(); | ||
$model = $this->getModel($prefix, $config['subject'], $config); | ||
$keyName = $model->getKeyName(); | ||
|
||
$input = $this->input; | ||
$pk = $input->getInt($keyName, 0); | ||
|
||
if ($pk != 0) | ||
{ | ||
try | ||
{ | ||
$model->checkin($pk); | ||
} | ||
catch (Exception $e) | ||
{ | ||
$msg = $e->getMessage(); | ||
$this->setRedirect($url, $msg, 'warning'); | ||
return false; | ||
} | ||
} | ||
|
||
$this->setRedirect($url); | ||
return true; | ||
} | ||
|
||
|
||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd much rather see us return parent::execute in situations like this - because that can return false or true as well
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree. I'll make the change when I wake up tomorrow.
Sincerely,
Mathew Lenning
Babel-university.com
P.S. This message was sent via iPhone, so please forgive any errors